diff --git a/app/controllers/admin/workshops_controller.rb b/app/controllers/admin/workshops_controller.rb index 004ea2e4f..8fdc77653 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).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 diff --git a/app/services/workshop_organiser_assigner.rb b/app/services/workshop_organiser_assigner.rb new file mode 100644 index 000000000..1aeb6632f --- /dev/null +++ b/app/services/workshop_organiser_assigner.rb @@ -0,0 +1,28 @@ +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 + 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 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