diff --git a/app/controllers/admin/workshops_controller.rb b/app/controllers/admin/workshops_controller.rb index 3f8c6f169..a33be6e50 100644 --- a/app/controllers/admin/workshops_controller.rb +++ b/app/controllers/admin/workshops_controller.rb @@ -29,11 +29,10 @@ def create @workshop.selected_host_id = host_id @workshop.created_by = current_user authorize(@workshop) - if @workshop.save - finish_creation - else - flash[:warning] = @workshop.errors.full_messages; render 'new' - end + return if render_organiser_selection_error('new', default_to_chapter: true) + return finish_creation if @workshop.save + + flash[:warning] = @workshop.errors.full_messages; render 'new' end def edit @@ -55,16 +54,13 @@ def update authorize @workshop @workshop.assign_attributes(workshop_params.merge(selected_host_id: host_id)) + return if params[:workshop].key?(:organisers) && render_organiser_selection_error('edit') - if @workshop.valid? - @workshop.save - update_workshop_details + return render_update_error unless @workshop.valid? - redirect_to admin_workshop_path(@workshop), notice: I18n.t('admin.messages.workshop.updated') - else - flash[:warning] = @workshop.errors.full_messages - render 'edit' - end + @workshop.save + update_workshop_details + redirect_to admin_workshop_path(@workshop), notice: I18n.t('admin.messages.workshop.updated') end def send_invites; end @@ -227,9 +223,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 @@ -241,11 +235,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 @@ -256,19 +246,28 @@ def organiser_ids params[:workshop][:organisers] end - def grant_organiser_access(organiser_ids) - organiser_ids.each { |id| Member.find(id).add_role(:organiser, @workshop) } + def update_workshop_details + assign_organisers(organiser_ids) if params[:workshop].key?(:organisers) + assign_host(host_id) 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 + def render_organiser_selection_error(template, default_to_chapter: false) + error = WorkshopOrganiserAssigner.validation_error(@workshop, organiser_ids, default_to_chapter:) + return false unless error + + render_with_organiser_error(template, error) + true end - def update_workshop_details - assign_organisers(organiser_ids) - assign_host(host_id) + def render_with_organiser_error(template, error) + @workshop.errors.add(:base, error) + flash.now[:warning] = @workshop.errors.full_messages + render template, status: :unprocessable_content + end + + def render_update_error + flash[:warning] = @workshop.errors.full_messages + render 'edit' end def set_sponsor diff --git a/app/services/workshop_organiser_assigner.rb b/app/services/workshop_organiser_assigner.rb new file mode 100644 index 000000000..2d64f2f7e --- /dev/null +++ b/app/services/workshop_organiser_assigner.rb @@ -0,0 +1,83 @@ +class WorkshopOrganiserAssigner + def self.validation_error(workshop, organiser_ids, default_to_chapter: false) + return if default_to_chapter && default_selection?(organiser_ids) + return 'organiser_ids is required' if organiser_ids.nil? + return 'organiser_ids must be an array of ids' unless organiser_ids.is_a?(Array) + + validate_ids(workshop, organiser_ids.reject(&:blank?)) + end + + def self.default_selection?(organiser_ids) + organiser_ids.nil? || organiser_ids == [] || (organiser_ids.is_a?(Array) && organiser_ids.all?(&:blank?)) + end + + def self.validate_ids(workshop, ids) + return if ids.empty? + return 'Select valid chapter organisers.' unless valid_ids?(ids) + return 'Select organisers from this chapter.' unless chapter_organisers?(workshop, ids) + + nil + end + + def self.valid_ids?(ids) + ids.all? { |id| valid_id?(id) } + end + + def self.chapter_organisers?(workshop, ids) + eligible_ids = workshop.chapter&.organisers&.where(id: ids.map(&:to_i))&.pluck(:id).to_a.map(&:to_s) + (ids.map(&:to_s).uniq - eligible_ids).empty? + end + + private_class_method :validate_ids, :valid_ids?, :chapter_organisers? + + def self.valid_id?(id) + (id.is_a?(String) || id.is_a?(Integer)) && id.to_s.match?(/\A[1-9]\d*\z/) + end + + private_class_method :valid_id? + + def initialize(workshop, organiser_ids, default_to_chapter: false) + @workshop = workshop + @organiser_ids = organiser_ids + @default_to_chapter = default_to_chapter + end + + def call + error = self.class.validation_error(@workshop, @organiser_ids, default_to_chapter: @default_to_chapter) + raise ArgumentError, error if error + + default_selection = @default_to_chapter && self.class.default_selection?(@organiser_ids) + Member.transaction { assign_roles(default_selection) } + end + + private + + def assign_roles(default_selection) + desired_ids = organiser_ids_for(default_selection) + grant(resolve_members(desired_ids)) + revoke_other_organisers(desired_ids) unless default_selection + end + + def organiser_ids_for(default_selection) + return @workshop.chapter.organisers.pluck(:id).map(&:to_s) if default_selection + + @organiser_ids.reject(&:blank?).map(&:to_s) + end + + def resolve_members(desired_ids) + members = Member.where(id: desired_ids.map(&:to_i)).index_by { |member| member.id.to_s } + missing_ids = desired_ids.uniq - members.keys + raise ArgumentError, "Unknown organiser IDs: #{missing_ids.join(', ')}" if missing_ids.any? + + members.values + end + + def grant(organisers) + organisers.each { |organiser| organiser.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 b0a781508..01c77c73d 100644 --- a/spec/controllers/admin/workshops_controller_spec.rb +++ b/spec/controllers/admin/workshops_controller_spec.rb @@ -334,6 +334,167 @@ 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 + organiser.add_role(:organiser, workshop.chapter) + 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', slack_channel: 'workshop-channel', + slack_channel_link: 'https://codebar.slack.link/workshop-channel', + 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.organisers).to include(organiser) + expect(created.chapter).to eq(workshop.chapter) + end + + it 'does not raise when organiser_ids are blank' do + chapter_organiser = Fabricate(:member) + chapter_organiser.add_role(:organiser, workshop.chapter) + + 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', slack_channel: 'workshop-channel', + slack_channel_link: 'https://codebar.slack.link/workshop-channel', + coach_spaces: 5, student_spaces: 15, + host: nil + } + } + end.not_to raise_error + created = Workshop.unscoped.order(:id).last + expect(created.reload.organisers).to include(chapter_organiser) + end + + it 'defaults to chapter organisers when the form sends only the blank sentinel' do + chapter_organiser = Fabricate(:member) + chapter_organiser.add_role(:organiser, workshop.chapter) + + 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', slack_channel: 'workshop-channel', + slack_channel_link: 'https://codebar.slack.link/workshop-channel', + coach_spaces: 5, student_spaces: 15, + host: nil, organisers: [''] + } + } + + created = Workshop.unscoped.order(:id).last + expect(flash[:warning]).to be_nil + expect(response).to redirect_to(admin_workshop_path(created)) + expect(created.organisers).to include(chapter_organiser) + end + + it 'rejects a scalar organiser value before creating the workshop' 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', slack_channel: 'workshop-channel', + slack_channel_link: 'https://codebar.slack.link/workshop-channel', + coach_spaces: 5, student_spaces: 15, + host: nil, organisers: 'not-an-array' + } + } + end.not_to change(Workshop, :count) + + expect(response).to have_http_status(:unprocessable_content) + expect(flash[:warning]).to include('organiser_ids must be an array of ids') + end + end + + describe 'PATCH #update organiser revocation' do + let(:existing_organiser) { Fabricate(:member) } + let(:new_organiser) { Fabricate(:member) } + + def update_workshop(organisers: :omitted, **attributes) + submitted_organisers = organisers == :omitted ? {} : { organisers: } + patch :update, params: { id: workshop.id, workshop: workshop_update_attributes.merge(attributes, submitted_organisers) } + end + + def workshop_update_attributes + date = workshop.date_and_time + { + local_date: date.strftime('%d/%m/%Y'), + local_time: date.strftime('%H:%M'), + local_end_time: (date + 2.hours).strftime('%H:%M'), + host: workshop.host&.id || '' + } + end + + it 'revokes removed organisers after update' do + existing_organiser.add_role(:organiser, workshop) + new_organiser.add_role(:organiser, workshop.chapter) + update_workshop(organisers: [new_organiser.id.to_s]) + workshop.reload + expect(workshop.organisers).to include(new_organiser) + expect(workshop.organisers).not_to include(existing_organiser) + end + + it 'clears organisers when the form submits only its blank sentinel' do + chapter_organiser = Fabricate(:member) + chapter_organiser.add_role(:organiser, workshop.chapter) + existing_organiser.add_role(:organiser, workshop) + + update_workshop(organisers: ['']) + + expect(response).to redirect_to(admin_workshop_path(workshop)) + expect(workshop.reload.organisers).to be_empty + expect(workshop.organisers).not_to include(chapter_organiser) + end + + it 'preserves organisers when the request omits the organisers parameter' do + existing_organiser.add_role(:organiser, workshop) + + update_workshop + + expect(response).to redirect_to(admin_workshop_path(workshop)) + expect(workshop.reload.organisers).to include(existing_organiser) + end + + it 'rejects a blank scalar organiser value before saving workshop changes' do + existing_organiser.add_role(:organiser, workshop) + update_workshop(organisers: '', description: 'Should not save') + + expect(response).to have_http_status(:unprocessable_content) + expect(workshop.reload.description).not_to eq('Should not save') + expect(workshop.organisers).to include(existing_organiser) + end + + it 'rejects an unknown organiser ID before saving workshop changes' do + update_workshop(organisers: ['999999999'], description: 'Should not save') + + expect(response).to have_http_status(:unprocessable_content) + expect(workshop.reload.description).not_to eq('Should not save') + end + + it 'rejects a non-chapter organiser before saving workshop changes' do + outsider = Fabricate(:member) + update_workshop(organisers: [outsider.id.to_s], description: 'Should not save') + + expect(response).to have_http_status(:unprocessable_content) + expect(workshop.reload.description).not_to eq('Should not save') + expect(workshop.organisers).to be_empty + end + end + describe 'DELETE #destroy' do context 'when workshop invitations have been sent' do before do @@ -432,6 +593,13 @@ def create_workshop describe 'GET #edit' do render_views + it 'includes a blank organiser value so deselecting all submits an empty selection' do + get :edit, params: { id: workshop.id } + + html = Nokogiri::HTML(response.body) + expect(html.css('input[type="hidden"][name="workshop[organisers][]"][value=""]')).to be_present + end + it 'pre-fills main date and time values in ISO format' do get :edit, params: { id: workshop.id } diff --git a/spec/services/workshop_organiser_assigner_spec.rb b/spec/services/workshop_organiser_assigner_spec.rb new file mode 100644 index 000000000..2eec7d86f --- /dev/null +++ b/spec/services/workshop_organiser_assigner_spec.rb @@ -0,0 +1,117 @@ +require 'rails_helper' + +RSpec.describe WorkshopOrganiserAssigner 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 + organiser.add_role(:organiser, chapter) + 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) + organiser.add_role(:organiser, chapter) + 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 'defaults to the chapter organisers when only the form sentinel is submitted' do + chapter_organiser = Fabricate(:member) + chapter_organiser.add_role(:organiser, chapter) + described_class.new(workshop, [''], default_to_chapter: true).call + + expect(workshop.reload.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 + + context 'when organiser_ids contain a non-chapter organiser' do + it 'rejects the selection before changing workshop roles' do + organiser = Fabricate(:member) + + expect do + described_class.new(workshop, [organiser.id.to_s]).call + end.to raise_error(ArgumentError, 'Select organisers from this chapter.') + expect(workshop.reload.organisers).to be_empty + end + end + + context 'when organiser_ids include a stale member ID' do + it 'rejects all requested IDs before granting any roles' do + chapter_organiser = Fabricate(:member) + chapter_organiser.add_role(:organiser, chapter) + allow(described_class).to receive(:validation_error).and_return(nil) + + expect do + described_class.new(workshop, [chapter_organiser.id.to_s, '999999999']).call + end.to raise_error(ArgumentError, 'Unknown organiser IDs: 999999999') + expect(workshop.reload.organisers).to be_empty + end + end + + it 'rolls back grants if a later roster change fails' do + chapter_organiser = Fabricate(:member) + chapter_organiser.add_role(:organiser, chapter) + existing_organiser = Fabricate(:member) + existing_organiser.add_role(:organiser, workshop) + assigner = described_class.new(workshop, [chapter_organiser.id.to_s]) + allow(assigner).to receive(:revoke_other_organisers).and_raise(StandardError, 'revoke failed') + + expect { assigner.call }.to raise_error(StandardError, 'revoke failed') + expect(workshop.reload.organisers).to include(existing_organiser) + expect(workshop.organisers).not_to include(chapter_organiser) + end + end +end