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

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

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

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 }