From afe4e05b68e694106c4c6f2b0910c30511be4ac8 Mon Sep 17 00:00:00 2001
From: David M <207745043+davidmillen50@users.noreply.github.com>
Date: Mon, 19 Oct 2026 15:07:08 +0100
Subject: [PATCH 1/2] chore: extract organiser grant/revoke diffing into
 service

---
 app/controllers/admin/workshops_controller.rb | 20 ++---------
 app/services/workshop_organiser_assigner.rb   | 34 +++++++++++++++++++
 2 files changed, 36 insertions(+), 18 deletions(-)
 create mode 100644 app/services/workshop_organiser_assigner.rb

diff --git a/app/controllers/admin/workshops_controller.rb b/app/controllers/admin/workshops_controller.rb
index 004ea2e4f..2141065f7 100644
--- a/app/controllers/admin/workshops_controller.rb
+++ b/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, @workshop.chapter).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, @workshop.chapter).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
diff --git a/app/services/workshop_organiser_assigner.rb b/app/services/workshop_organiser_assigner.rb
new file mode 100644
index 000000000..f8d77bc09
--- /dev/null
+++ b/app/services/workshop_organiser_assigner.rb
@@ -0,0 +1,34 @@
+# Encapsulates organiser role management for workshops.
+# Handles granting and revoking organiser roles based on explicit organiser IDs
+# or falling back to the chapter's default organisers.
+
+class WorkshopOrganiserAssigner
+  def initialize(workshop, organiser_ids, chapter)
+    @workshop = workshop
+    @organiser_ids = organiser_ids
+    @chapter = chapter
+  end
+
+  # Executes the organiser role changes.
+  # Adds roles for provided IDs, revokes roles for IDs not present.
+  def call
+    desired_ids = desired_organiser_ids
+    current_ids = @workshop.organisers.pluck(:id).map(&:to_s)
+
+    (desired_ids - current_ids).each { |id| Member.find(id).add_role(:organiser, @workshop) }
+    (current_ids - desired_ids).each { |id| Member.find(id).revoke(:organiser, @workshop) }
+  end
+
+  private
+
+  # Determines which organiser IDs to act on.
+  # If organiser_ids is present and non-empty, use them.
+  # Otherwise, fall back to the chapter's current organisers.
+  def desired_organiser_ids
+    if @organiser_ids.any?
+      @organiser_ids.map(&:to_s)
+    else
+      @chapter.organisers.pluck(:id).map(&:to_s)
+    end
+  end
+end

From 88a8cd0c95c634d15f11aba543e2eeee6421af58 Mon Sep 17 00:00:00 2001
From: David M <207745043+davidmillen50@users.noreply.github.com>
Date: Thu, 22 Oct 2026 14:17:16 +0100
Subject: [PATCH 2/2] chore: simplifiy WorkshopOrganiserAssigner, remove
 redundant chapter arg (#2941)

---
 app/controllers/admin/workshops_controller.rb |  4 +-
 app/services/workshop_organiser_assigner.rb   | 38 ++++++-------
 .../admin/workshops_controller_spec.rb        | 54 +++++++++++++++++++
 .../workshop_organiser_assigner_spec.rb       | 51 ++++++++++++++++++
 4 files changed, 123 insertions(+), 24 deletions(-)
 create mode 100644 spec/services/workshop_organiser_assigner_spec.rb

diff --git a/app/controllers/admin/workshops_controller.rb b/app/controllers/admin/workshops_controller.rb
index 2141065f7..8fdc77653 100644
--- a/app/controllers/admin/workshops_controller.rb
+++ b/app/controllers/admin/workshops_controller.rb
@@ -219,7 +219,7 @@ def assign_host(host_id)
   end
 
   def assign_organisers(organiser_ids)
-    WorkshopOrganiserAssigner.new(@workshop, organiser_ids, @workshop.chapter).call
+    WorkshopOrganiserAssigner.new(@workshop, organiser_ids).call
   end
 
   def finish_creation
@@ -231,7 +231,7 @@ def finish_creation
   end
 
   def assign_organisers_or_default
-    WorkshopOrganiserAssigner.new(@workshop, organiser_ids, @workshop.chapter).call
+    WorkshopOrganiserAssigner.new(@workshop, organiser_ids, default_to_chapter: true).call
   end
 
   def host_id
diff --git a/app/services/workshop_organiser_assigner.rb b/app/services/workshop_organiser_assigner.rb
index f8d77bc09..1aeb6632f 100644
--- a/app/services/workshop_organiser_assigner.rb
+++ b/app/services/workshop_organiser_assigner.rb
@@ -1,34 +1,28 @@
-# Encapsulates organiser role management for workshops.
-# Handles granting and revoking organiser roles based on explicit organiser IDs
-# or falling back to the chapter's default organisers.
-
 class WorkshopOrganiserAssigner
-  def initialize(workshop, organiser_ids, chapter)
+  def initialize(workshop, organiser_ids, default_to_chapter: false)
     @workshop = workshop
     @organiser_ids = organiser_ids
-    @chapter = chapter
+    @default_to_chapter = default_to_chapter
   end
 
-  # Executes the organiser role changes.
-  # Adds roles for provided IDs, revokes roles for IDs not present.
   def call
-    desired_ids = desired_organiser_ids
-    current_ids = @workshop.organisers.pluck(:id).map(&:to_s)
-
-    (desired_ids - current_ids).each { |id| Member.find(id).add_role(:organiser, @workshop) }
-    (current_ids - desired_ids).each { |id| Member.find(id).revoke(:organiser, @workshop) }
+    if @default_to_chapter && @organiser_ids.blank?
+      grant(@workshop.chapter.organisers.pluck(:id))
+    else
+      desired_ids = Array(@organiser_ids).reject(&:empty?).map(&:to_s)
+      grant(desired_ids)
+      revoke_other_organisers(desired_ids)
+    end
   end
 
   private
 
-  # Determines which organiser IDs to act on.
-  # If organiser_ids is present and non-empty, use them.
-  # Otherwise, fall back to the chapter's current organisers.
-  def desired_organiser_ids
-    if @organiser_ids.any?
-      @organiser_ids.map(&:to_s)
-    else
-      @chapter.organisers.pluck(:id).map(&:to_s)
-    end
+  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
diff --git a/spec/controllers/admin/workshops_controller_spec.rb b/spec/controllers/admin/workshops_controller_spec.rb
index 351b6f025..f7bd005ba 100644
--- a/spec/controllers/admin/workshops_controller_spec.rb
+++ b/spec/controllers/admin/workshops_controller_spec.rb
@@ -282,6 +282,60 @@ 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
+    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
+    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
diff --git a/spec/services/workshop_organiser_assigner_spec.rb b/spec/services/workshop_organiser_assigner_spec.rb
new file mode 100644
index 000000000..95482bc9a
--- /dev/null
+++ b/spec/services/workshop_organiser_assigner_spec.rb
@@ -0,0 +1,51 @@
+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
+    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
+  end
+end