A bit of cleanug in backlog module#23464
Conversation
There was a problem hiding this comment.
Pull request overview
This PR cleans up Backlogs helper access checks and reduces repetition in Backlogs specs by introducing a collection matcher for attribute assertions.
Changes:
- Centralizes Backlogs permission checks behind
CommonHelperpredicates. - Adds
have_attrmatcher and uses it for work package position specs. - Simplifies sprint finish specs with
have_attributes.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
spec/support/matchers/have_attr.rb |
Adds a reusable matcher for asserting one attribute across records. |
modules/backlogs/spec/services/work_packages/rebuild_positions_service_integration_spec.rb |
Uses the matcher and simplifies fixture setup with acts_as_list_no_update. |
modules/backlogs/spec/services/sprints/finish_service_spec.rb |
Consolidates repeated record attribute expectations. |
modules/backlogs/spec/models/work_packages/position_spec.rb |
Replaces repeated position hash helpers with relation helpers and have_attr. |
modules/backlogs/app/helpers/backlogs/common_helper.rb |
Renames/refactors Backlogs permission helper methods. |
modules/backlogs/app/components/backlogs/work_package_card_menu_component.rb |
Uses the shared permission helper. |
modules/backlogs/app/components/backlogs/work_package_card_list_item_component.rb |
Includes the shared helper and uses it for drag permission. |
modules/backlogs/app/components/backlogs/sprints_component.rb |
Updates permission checks to the renamed helpers. |
modules/backlogs/app/components/backlogs/sprints_component.html.erb |
Updates sprint creation permission check. |
modules/backlogs/app/components/backlogs/sprint_component.rb |
Removes now-duplicated local permission helper. |
modules/backlogs/app/components/backlogs/inbox_component.rb |
Adds Primer component helper include. |
modules/backlogs/app/components/backlogs/backlog_component.html.erb |
Updates backlog bucket/sprint creation permission checks. |
5802ff7 to
88929c7
Compare
myabc
left a comment
There was a problem hiding this comment.
The cleanup generally looks. Thanks for doing this!
That said I'm not happy with the naming of the have_attr matcher. Although it doesn't directly clash with RSpec's built-in have_attributes, it implies that it might behave similarly when, in fact, it behaves quite differently. have_attribute checks many attributes on one object, while this checks one attribute across many records. tl;dr: Same word family, inverted meaning - it will trip other devs up. The attr abbreviation makes it even less clear.
There's also a subtler mismatch: "have" hints at containing a subset, but this matcher does an exact whole-collection comparison.
Suggestions:
match_attribute- this would be my preferred choice, e.g.match_attribute(:position, identified_by: :subject, ...)have_attribute_values- which might make it more discoverable (since some devs are less aware ofmatch_-prefixed matchers), at the expense of method length.
Alternatively, we could just hold off on introduce have_attr/match_attribute at this point (YAGNI) - and just define have_positions/match_positions for this particular example. WDYT?
|
I don't know of a way to define matcher locally - I would've go with that, as I failed to locate similar tests, so where this could be already reused I've added a commit with a chaining matcher experiment |
88929c7 to
d1ce43b
Compare
Cleanup extracted while working on https://community.openproject.org/wp/74386:
Probably easier to check by commit
I am not sure about
have_attrand the name