Backlogs target#23852
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces Backlogs::Target as a single place to construct and parse backlog “target_id” values (e.g., sprint:42, backlog_bucket:13, inbox) and begins migrating existing code paths to use it for safer parsing and consistent serialization.
Changes:
- Added
Backlogs::Targetdiscriminated-union style value objects (SprintId,BucketId,InboxId) plus.forand.parse. - Updated
Backlogs::WorkPackages::UpdateServiceto parse targets viaBacklogs::Target.parseand pattern-match on the resulting target object. - Updated backlog UI components to generate
target_idviaBacklogs::Target(with explicitto_s) and added unit specs forBacklogs::Target.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| modules/backlogs/spec/models/backlogs/target_spec.rb | Adds specs covering construction, parsing, and round-trip serialization of target ids. |
| modules/backlogs/app/services/backlogs/work_packages/update_service.rb | Switches target parsing to Backlogs::Target.parse and matches on target objects. |
| modules/backlogs/app/models/backlogs/target.rb | Introduces the Backlogs::Target value objects and parsing/constructor helpers. |
| modules/backlogs/app/components/backlogs/work_package_card_menu_component.rb | Uses Backlogs::Target objects when building menu move requests (needs string serialization fix). |
| modules/backlogs/app/components/backlogs/sprint_component.html.erb | Uses Backlogs::Target.for(sprint).to_s for drag-and-drop target_id. |
| modules/backlogs/app/components/backlogs/inbox_component.html.erb | Uses Backlogs::Target::InboxId.to_s for drag-and-drop target_id. |
| modules/backlogs/app/components/backlogs/bucket_component.html.erb | Uses Backlogs::Target.for(backlog_bucket).to_s for drag-and-drop target_id. |
| BucketId[container.id] | ||
| end |
There was a problem hiding this comment.
there is no model for inbox, using nil or Project instance feels odd
|
|
||
| def to_s = "#{type}:#{id}" | ||
|
|
||
| def to_h = { type:, id: } |
There was a problem hiding this comment.
nitpick: you could extract this into a module.
| def to_h = { type:, id: } | |
| def to_s = "#{type}:#{id}" | |
| def to_h = { type:, id: } |
There was a problem hiding this comment.
I thought about it, but it didn't feel justifiable
|
|
||
| def to_s = "#{type}:#{id}" | ||
|
|
||
| def to_h = { type:, id: } |
There was a problem hiding this comment.
I guess you could also write
| def to_h = { type:, id: } | |
| def to_h = super.merge(type:) |
There was a problem hiding this comment.
yes, but here explicit is just much more straightforward
| SprintId = Data.define(:id) do | ||
| def type = :sprint | ||
|
|
||
| def to_s = "#{type}:#{id}" |
There was a problem hiding this comment.
to_param might be a better name here.
There was a problem hiding this comment.
I'll check if it breaks implicit usage
0852131 to
c3b60c7
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
Ticket
Extracted from https://community.openproject.org/wp/74386
What are you trying to accomplish?
Have a class responsible for constructing and parsing target id
Merge checklist