Skip to content

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

Open
davidmillen50 wants to merge 5 commits into
codebar:masterfrom
davidmillen50:2941-extract-organiser-grant-revoke-diffing-from-workshops-controller
Open

davidmillen50 wants to merge 5 commits into
codebar:masterfrom
davidmillen50:2941-extract-organiser-grant-revoke-diffing-from-workshops-controller

Conversation

@davidmillen50

@davidmillen50 davidmillen50 commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

this refactor relates to issue 2941

Before: missing organiser_ids raised NoMethodError (nil.reject!) and left roles untouched.
After:

  • [] (empty array from form deselect) clears all current organisers (silent, intended)
  • nil (absent param) raises ArgumentError ('organiser_ids is required')
  • Spec added for absent-param case
  • Rubocop clean; controller specs fixed (false positives removed)

@davidmillen50
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 mroderick left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

@mroderick mroderick Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should now be resolved

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks I've applied the suggestion.

@davidmillen50
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
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>
@davidmillen50
davidmillen50 force-pushed the 2941-extract-organiser-grant-revoke-diffing-from-workshops-controller branch from 05f513a to dd44316 Compare October 30, 2026 10:18

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants