Extract organiser grant/revoke diffing into WorkshopOrganiserAssigner service (#2941) by davidmillen50 · Pull Request #3049 · codebar/planner

davidmillen50 · 2026-10-19T13:52:45Z

This refactor relates to issue #2941 and extracts workshop organiser role assignment into WorkshopOrganiserAssigner.

Organiser selection behavior:

On create, an empty selection (including the blank value submitted by the form) defaults to the chapter organisers.
On update, selecting no organisers clears the workshop organiser roster. The edit form submits a blank sentinel for this case.
If an update request omits the organisers parameter entirely, the existing roster is left unchanged.
Invalid organiser values are rejected with a validation response before workshop changes are saved; only organisers of the workshop chapter may be selected.

Controller and service specs cover these cases, including empty selection, omitted parameters, and invalid values.

mroderick

One behaviour change on the update path and two assertion gaps in the new specs - details in the line comments.

The blocker for me is the absent-param handling in WorkshopOrganiserAssigner: nil on update now silently clears all organisers where it previously raised, and nil means chapter fallback on create but full revocation on update. The rest of the extraction looks faithful.

mroderick · 2026-10-24T07:57:07Z

+    if @default_to_chapter && @organiser_ids.blank?
+      grant(@workshop.chapter.organisers.pluck(:id))
+    else
+      desired_ids = Array(@organiser_ids).reject(&:empty?).map(&:to_s)


If the organisers param is absent, @organiser_ids is nil and Array(nil) gives [], so revoke_other_organisers([]) strips every current organiser and the update redirects with a success notice. Before this change the same request raised NoMethodError on nil.reject! and left the roles untouched. The edit form's multi-select omits the key entirely when nothing is selected, so deselecting all organisers and saving hits exactly this path.

Nil also means opposite things on the two routes now: chapter fallback on create (default_to_chapter: true), full strip on update.

If the old 500 was simply broken and a silent clear is the intended behaviour for #2941, could you note that in the PR description and add a spec for the absent-param case? If not, guarding the else branch reads like:

raise ArgumentError, 'organiser_ids is required on the update path' if @organiser_ids.nil?

or returning without revoking, so only a submitted list can clear the roster.

I've left the error as it was previously implemented

mroderick · 2026-10-24T07:57:08Z

+            host: nil, organisers: [organiser.id.to_s]
+          }
+        }
+      end.not_to raise_error


.not_to raise_error is the only assertion in both new create specs, so they stay green even if the grant never happens: a mistyped service call, or a wrong params key falling back to the chapter default, passes just the same. Could the provided-ids spec also assert the effect?

Suggested change

end.not_to raise_error

end.not_to raise_error

expect(Workshop.unscoped.order(:id).last.organisers).to include(organiser)

(created is assigned on the next line, so the suggestion queries inline; swap the order if you prefer to reuse created.)

The PATCH #update spec below has real value assertions, so this is only about the two create cases.

this should now be resolved

I have updated the comment above into a suggested change: the added line asserts that the provided organiser id reaches the created workshop, so the spec fails if the params key is dropped and the chapter fallback takes over. Use the "Commit suggestion" button to apply it as-is, or reorder to reuse created if you prefer.

thanks I've applied the suggestion.

…r arg

test: the added line asserts that the provided organiser id reaches the created workshop, so the spec fails if the params key is dropped and the chapter fallback takes over. Co-authored-by: Morgan Roderick <20321+mroderick@users.noreply.github.com>

…g-from-workshops-controller

mroderick

Re-review at 295bb4c. The nil guard, the create-spec assertion and the fallback spec all check out - thanks.

One new blocker: a scalar workshop[organisers]="" now silently clears every organiser, where the old code raised. The rest are non-blocking: the deselect-all 500 and the mismatch between it and the PR description, no transaction around grant plus revoke, the leftover controller methods, and two small spec and style notes. Details are in the line comments.

mroderick · 2026-11-12T09:42:49Z

+    else
+      raise ArgumentError, 'organiser_ids is required' if @organiser_ids.nil?
+
+      desired_ids = Array(@organiser_ids).reject(&:empty?).map(&:to_s)


workshop[organisers] sent as a bare scalar (a crafted or truncated PATCH with organisers=) arrives here as "". Array("") is [""], reject(&:empty?) empties it, so grant is a no-op and revoke_other_organisers([]) removes every current organiser, then the request redirects with the success notice. Before this change the same input raised NoMethodError on "".reject! and left the roles alone.

The nil guard below covers the nil shape but not this one, so a malformed request still clears the roster silently. Something like:

raise ArgumentError, 'organiser_ids must be an array of ids' unless @organiser_ids.is_a?(Array) desired_ids = @organiser_ids.reject(&:blank?).map(&:to_s)

keeps the accident loud and treats whitespace-only entries the same as empty ones.

Fixed: scalar organiser values are rejected before assignment. The update spec now covers the exact blank-string submission and verifies the existing roster is preserved.

mroderick · 2026-11-12T09:42:49Z

+    if @default_to_chapter && @organiser_ids.blank?
+      grant(@workshop.chapter.organisers.pluck(:id))
+    else
+      raise ArgumentError, 'organiser_ids is required' if @organiser_ids.nil?


On the update path a plain form action reaches this raise: _edit_form.html.haml:8 is a collection multi-select with no hidden sentinel, so deselecting every organiser omits workshop[organisers] from the request and organiser_ids is nil. The result is a 500 while the old organisers stay in place.

The PR description says [] from a form deselect clears the roster silently, but a multi-select never sends []; it also says nil raises, which is true here and false on create, where nil falls back to the chapter organisers.

If the 500 is the intended outcome, could the description say so and this case get a controller spec? If deselect-all should clear, a hidden sentinel in the form (or treating nil as [] on update) gets there.

Fixed: an omitted organisers parameter now leaves the existing roster unchanged; the blank sentinel still supports intentional clear. Controller specs cover both cases.

mroderick · 2026-11-12T09:42:49Z

+  private
+
+  def grant(organiser_ids)
+    organiser_ids.each { |id| Member.find(id).add_role(:organiser, @workshop) }


Member.find(id) raises RecordNotFound for the first stale id after the earlier ids have been granted, and revocation never runs. On create that lands after the workshop row is saved, so the admin sees a 500 with a half-assigned roster. The ordering is inherited from the old controller methods, but a service named for the whole grant/revoke diff reads as all-or-nothing.

Member.where(id: desired_ids).index_by(&:id), diffed against the requested ids and wrapped in a transaction, would make it so.

Fixed: all requested members are resolved before role changes, and grant/revoke runs in a transaction. Specs cover stale IDs and rollback.

mroderick · 2026-11-12T09:42:49Z

-    organiser_ids.reject!(&:empty?)
-    grant_organiser_access(organiser_ids)
-    revoke_organiser_access(organiser_ids)
+    WorkshopOrganiserAssigner.new(@workshop, organiser_ids).call


Both call sites now delegate here, which leaves grant_organiser_access (line 246) and revoke_organiser_access (line 250) without callers. They also lack the to_s normalisation the service applies, so anyone who reaches for the leftover copy gets the grant-only path this extraction exists to remove.

Can they go in this PR? Events and meetings keep their own copies; extracting those is a separate change.

Removed the unused workshop grant/revoke helpers. The event and meeting copies are unchanged, as requested.

mroderick · 2026-11-12T09:42:49Z

+            host: nil
+          }
+        }
+      end.not_to raise_error


This example only proves that nothing raised: a service call that granted nothing keeps it green, and created.chapter says nothing about organisers.

Adding a chapter organiser before the request and asserting created.reload.organisers includes them pins the fallback wiring at controller level. The service spec covers the behaviour itself.

mroderick · 2026-11-12T09:42:49Z

@@ -0,0 +1,70 @@
+require 'rails_helper'
+
+RSpec.describe WorkshopOrganiserAssigner, type: :service do


No other file under spec/ uses type: :service; the other service specs rely on the inferred type. Worth dropping for consistency, unless the type is going in repo-wide.

…g-from-workshops-controller

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

davidmillen50 mentioned this pull request Oct 19, 2026

Extract organiser grant/revoke diffing from Admin::WorkshopsController #2941

Open

davidmillen50 force-pushed the 2941-extract-organiser-grant-revoke-diffing-from-workshops-controller branch 3 times, most recently from b897cc5 to 88a8cd0 Compare October 22, 2026 13:18

mroderick requested changes Oct 24, 2026

View reviewed changes

davidmillen50 force-pushed the 2941-extract-organiser-grant-revoke-diffing-from-workshops-controller branch 2 times, most recently from faa1309 to 3a25aec Compare October 25, 2026 14:22

davidmillen50 and others added 4 commits October 30, 2026 10:15

chore: simplify WorkshopOrganiserAssigner and remove redundant chapte…

ad9dd83

…r arg

fix: raise ArgumentError when organiser_ids is nil

b2b9854

fix: strengthen organiser assignment specs to verify grant effects

1c5e3c9

davidmillen50 force-pushed the 2941-extract-organiser-grant-revoke-diffing-from-workshops-controller branch from 05f513a to dd44316 Compare October 30, 2026 10:18

davidmillen50 added 2 commits October 30, 2026 10:52

fix: create organiser test

abbbc73

Merge branch 'master' into 2941-extract-organiser-grant-revoke-diffin…

295bb4c

…g-from-workshops-controller

mroderick requested changes Nov 12, 2026

View reviewed changes

davidmillen50 and others added 7 commits November 12, 2026 10:34

Merge branch 'master' into 2941-extract-organiser-grant-revoke-diffin…

5f70e85

…g-from-workshops-controller

fix: handle empty workshop organiser selection

6630557

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

fix: validate workshop organiser selections

427328e

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

fix: make organiser assignment atomic

0095afb

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

fix: address organiser review feedback

88045ee

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

fix: remove obsolete workshop organiser helpers

adb986e

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

fix: cover blank scalar organiser submissions

9996058

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

	end.not_to raise_error
	end.not_to raise_error
	expect(Workshop.unscoped.order(:id).last.organisers).to include(organiser)

		@@ -0,0 +1,70 @@
		require 'rails_helper'

		RSpec.describe WorkshopOrganiserAssigner, type: :service do

Conversation

davidmillen50 commented Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mroderick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mroderick Oct 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mroderick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

davidmillen50 commented Oct 19, 2026 •

edited

Loading

mroderick Oct 24, 2026 •

edited

Loading