Repository navigation
Extract organiser grant/revoke diffing into WorkshopOrganiserAssigner service (#2941) - #3049
Conversation
b897cc5 to
88a8cd0
Compare
mroderick
left a comment
mroderick
left a comment
There was a problem hiding this comment.
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.
| if @default_to_chapter && @organiser_ids.blank? | ||
| grant(@workshop.chapter.organisers.pluck(:id)) | ||
| else | ||
| desired_ids = Array(@organiser_ids).reject(&:empty?).map(&:to_s) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I've left the error as it was previously implemented
| host: nil, organisers: [organiser.id.to_s] | ||
| } | ||
| } | ||
| end.not_to raise_error |
There was a problem hiding this comment.
.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?
workshop.reload
expect(workshop.organisers).to include(organiser)The PATCH #update spec below has real value assertions, so this is only about the two create cases.
There was a problem hiding this comment.
this should now be resolved
| chapter_organiser.add_role(:organiser, chapter) | ||
| described_class.new(workshop, [], default_to_chapter: true).call | ||
| workshop.reload | ||
| expect(workshop.organisers).to include(chapter_organiser) |
There was a problem hiding this comment.
This pins the chapter-fallback grant but not that the fallback is grant-only. If the branch ever picked up revocation (or the condition on line 9 drifted), pre-existing workshop organisers would lose their roles with this spec still green. Could it also assert a workshop-only organiser keeps the role?
existing = Fabricate(:member)
existing.add_role(:organiser, workshop)
described_class.new(workshop, [], default_to_chapter: true).call
workshop.reload
expect(workshop.organisers).to include(existing)There was a problem hiding this comment.
this should now be resolved
b2a2928 to
faa1309
Compare
faa1309 to
3a25aec
Compare
this refactor relates to issue 2941
Before: missing organiser_ids raised NoMethodError (nil.reject!) and left roles untouched.
After: