Extract organiser grant/revoke diffing into WorkshopOrganiserAssigner service (#2941)

app/controllers/admin/workshops_controller.rb

@@ -219,9 +219,7 @@ def assign_host(host_id)
   end
 
   def assign_organisers(organiser_ids)
-    organiser_ids.reject!(&:empty?)
-    grant_organiser_access(organiser_ids)
-    revoke_organiser_access(organiser_ids)
+    WorkshopOrganiserAssigner.new(@workshop, organiser_ids).call
   end
 
   def finish_creation
@@ -233,11 +231,7 @@ def finish_creation
   end
 
   def assign_organisers_or_default
-    if organiser_ids.present?
-      assign_organisers(organiser_ids)
-    else
-      grant_organiser_access(@workshop.chapter.organisers.pluck(:id))
-    end
+    WorkshopOrganiserAssigner.new(@workshop, organiser_ids, default_to_chapter: true).call
   end
 
   def host_id
@@ -248,16 +242,6 @@ def organiser_ids
     params[:workshop][:organisers]
   end
 
-  def grant_organiser_access(organiser_ids)
-    organiser_ids.each { |id| Member.find(id).add_role(:organiser, @workshop) }
-  end
-
-  def revoke_organiser_access(organiser_ids)
-    (@workshop.organisers.pluck(:id).map(&:to_s) - organiser_ids).each do |id|
-      Member.find(id).revoke(:organiser, @workshop)
-    end
-  end
-
   def physical_workshop_and_no_host?(workshop)
     !workshop.virtual && host_id.empty?
   end

app/services/workshop_organiser_assigner.rb

@@ -0,0 +1,30 @@
+class WorkshopOrganiserAssigner
+  def initialize(workshop, organiser_ids, default_to_chapter: false)
+    @workshop = workshop
+    @organiser_ids = organiser_ids
+    @default_to_chapter = default_to_chapter
+  end
+
+  def call
+    if @default_to_chapter && @organiser_ids.blank?
+      grant(@workshop.chapter.organisers.pluck(:id))
+    else
+      raise ArgumentError, 'organiser_ids is required' if @organiser_ids.nil?
+
+      desired_ids = Array(@organiser_ids).reject(&:empty?).map(&:to_s)
+      grant(desired_ids)
+      revoke_other_organisers(desired_ids)
+    end
+  end
+
+  private
+
+  def grant(organiser_ids)
+    organiser_ids.each { |id| Member.find(id).add_role(:organiser, @workshop) }
+  end
+
+  def revoke_other_organisers(desired_ids)
+    current_ids = @workshop.organisers.pluck(:id).map(&:to_s)
+    (current_ids - desired_ids).each { |id| Member.find(id).revoke(:organiser, @workshop) }
+  end
+end

spec/controllers/admin/workshops_controller_spec.rb

@@ -282,6 +282,64 @@ def create_workshop
     end
   end
 
+  describe 'POST #create organiser assignment' do
+    let(:organiser) { Fabricate(:member) }
+
+    it 'does not raise when organiser_ids are provided' do
+      expect do
+        post :create, params: {
+          workshop: {
+            chapter_id: workshop.chapter.id,
+            local_date: (Time.zone.now + 1.week).strftime('%d/%m/%Y'),
+            local_time: '15:00', local_end_time: '17:00',
+            virtual: '1', coach_spaces: 5, student_spaces: 15,
+            host: nil, organisers: [organiser.id.to_s]
+          }
+        }
+      end.not_to raise_error
+      created = Workshop.unscoped.order(:id).last
+      expect(created.chapter).to eq(workshop.chapter)
+    end
+
+    it 'does not raise when organiser_ids are blank' do
+      expect do
+        post :create, params: {
+          workshop: {
+            chapter_id: workshop.chapter.id,
+            local_date: (Time.zone.now + 1.week).strftime('%d/%m/%Y'),
+            local_time: '15:00', local_end_time: '17:00',
+            virtual: '1', coach_spaces: 5, student_spaces: 15,
+            host: nil
+          }
+        }
+      end.not_to raise_error
+      created = Workshop.unscoped.order(:id).last
+      expect(created.chapter).to eq(workshop.chapter)
+    end
+  end
+
+  describe 'PATCH #update organiser revocation' do
+    let(:existing_organiser) { Fabricate(:member) }
+    let(:new_organiser) { Fabricate(:member) }
+
+    it 'revokes removed organisers after update' do
+      existing_organiser.add_role(:organiser, workshop)
+      patch :update, params: {
+        id: workshop.id,
+        workshop: {
+          local_date: workshop.date_and_time.strftime('%d/%m/%Y'),
+          local_time: workshop.date_and_time.strftime('%H:%M'),
+          local_end_time: (workshop.date_and_time + 2.hours).strftime('%H:%M'),
+          host: workshop.host&.id || '',
+          organisers: [new_organiser.id.to_s]
+        }
+      }
+      workshop.reload
+      expect(workshop.organisers).to include(new_organiser)
+      expect(workshop.organisers).not_to include(existing_organiser)
+    end
+  end
+
   describe 'DELETE #destroy' do
     context 'when workshop invitations have been sent' do
       before do

spec/services/workshop_organiser_assigner_spec.rb

@@ -0,0 +1,70 @@
+require 'rails_helper'
+
+RSpec.describe WorkshopOrganiserAssigner, type: :service do
+  let(:workshop) { Fabricate(:workshop) }
+  let(:chapter) { workshop.chapter }
+
+  describe '#call' do
+    context 'when organiser_ids are provided' do
+      let(:organiser) { Fabricate(:member) }
+
+      it 'grants organiser role to members in the list' do
+        expect do
+          described_class.new(workshop, [organiser.id.to_s], default_to_chapter: false).call
+          workshop.reload
+        end.to change { workshop.organisers.include?(organiser) }.from(false).to(true)
+      end
+
+      it 'revokes organiser role from members not in the list' do
+        existing_organiser = Fabricate(:member)
+        existing_organiser.add_role(:organiser, workshop)
+        workshop.reload
+        described_class.new(workshop, [organiser.id.to_s], default_to_chapter: false).call
+        workshop.reload
+        expect(workshop.organisers).not_to include(existing_organiser)
+      end
+    end
+
+    context 'when organiser_ids are blank with default_to_chapter' do
+      it 'defaults to the chapter organisers' do
+        chapter_organiser = Fabricate(:member)
+        chapter_organiser.add_role(:organiser, chapter)
+        described_class.new(workshop, [], default_to_chapter: true).call
+        workshop.reload
+        expect(workshop.organisers).to include(chapter_organiser)
+      end
+
+      it 'preserves existing workshop organisers when falling back to chapter' do
+        chapter_organiser = Fabricate(:member)
+        chapter_organiser.add_role(:organiser, chapter)
+        existing_organiser = Fabricate(:member)
+        existing_organiser.add_role(:organiser, workshop)
+        described_class.new(workshop, [], default_to_chapter: true).call
+        workshop.reload
+        expect(workshop.organisers).to include(existing_organiser)
+        expect(workshop.organisers).to include(chapter_organiser)
+      end
+    end
+
+    context 'when organiser_ids contain only blank values (update path)' do
+      it 'revokes all and does not fall back to chapter' do
+        chapter_organiser = Fabricate(:member)
+        chapter_organiser.add_role(:organiser, chapter)
+        existing_organiser = Fabricate(:member)
+        existing_organiser.add_role(:organiser, workshop)
+        described_class.new(workshop, [''], default_to_chapter: false).call
+        workshop.reload
+        expect(workshop.organisers).not_to include(existing_organiser)
+        expect(workshop.organisers).to eq([])
+      end
+    end
+
+    context 'when organiser_ids is nil' do
+      it 'raises ArgumentError' do
+        expect do
+          described_class.new(workshop, nil, default_to_chapter: false).call
+        end.to raise_error(ArgumentError, 'organiser_ids is required')
+      end
+    end
+  end
+end