[COMMS-879] Restore semantic identifier in-memory after a failed project move#24203
Open
judithroth wants to merge 1 commit into
Conversation
…ect move https://community.openproject.org/wp/COMMS-879 WorkPackages::SetAttributesService#clear_semantic_identifier nils out identifier/sequence_number in memory before contract validation runs, so a fresh one can be allocated after a successful move without violating the (project_id, sequence_number) unique index. If the move then fails contract validation, nothing is ever saved -- the DB row's real identifier is untouched -- but the work package object returned to the caller kept the cleared identifier, so anything reading formatted_id/display_id off it (error messages, to_s, ...) showed a bare numeric id instead. Restore identifier/sequence_number from their pre-clear values via dirty tracking once SetAttributesService knows the contract rejected the change, so every consumer of the returned work package sees accurate data instead of only patching the one view that renders it. An AR-level after_validation callback was considered but doesn't work: the model's own validations run before the contract's own validators add their errors (see ModelContract#valid?), so an after_validation hook can't see contract-only failures like an invalid assignee. Restoring once SetAttributesService's full validation result is known avoids that timing issue and the risk of resurrecting the same unique-constraint hazard on the success path.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR ensures work packages keep their semantic identifier in-memory when a project move fails contract validation, so downstream consumers (e.g., controller flash/error messages) don’t fall back to showing the bare numeric ID.
Changes:
- Restore
identifier/sequence_numberafter a failed validation when they were cleared as part of a project move attempt. - Add service-level spec coverage to ensure the semantic identifier is restored on the returned work package.
- Add controller spec coverage to ensure the error flash shows the semantic identifier (not
#<id>) when the move fails.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| app/services/work_packages/set_attributes_service.rb | Restores semantic identifier attributes after unsuccessful contract validation. |
| app/models/work_package/semantic_identifier.rb | Adds restore_identifier_after_failed_move to rehydrate cleared identifier fields using dirty tracking. |
| spec/services/work_packages/set_attributes_service_spec.rb | Verifies identifier/sequence_number/formatted_id are restored after a failed move validation. |
| spec/controllers/work_packages/moves_controller_spec.rb | Verifies failure flash message contains semantic identifier and not the numeric #id. |
|
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. |
|
Caution The provided work package version does not match the core version Details:
Please make sure that:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://community.openproject.org/wp/COMMS-879
What are you trying to accomplish?
Use the correct (semantic) identifier in the error message after failed project move.
What approach did you choose and why?
WorkPackages::SetAttributesService#clear_semantic_identifier nils out identifier/sequence_number in memory before contract validation runs, so a fresh one can be allocated after a successful move without violating the (project_id, sequence_number) unique index. If the move then fails contract validation, nothing is ever saved -- the DB row's real identifier is untouched -- but the work package object returned to the caller kept the cleared identifier, so anything reading formatted_id/display_id off it (error messages, to_s, ...) showed a bare numeric id instead.
Restore identifier/sequence_number from their pre-clear values via dirty tracking once SetAttributesService knows the contract rejected the change, so every consumer of the returned work package sees accurate data instead of only patching the one view that renders it.
An AR-level after_validation callback was considered but doesn't work: the model's own validations run before the contract's own validators add their errors (see ModelContract#valid?), so an after_validation hook can't see contract-only failures like an invalid assignee. Restoring once SetAttributesService's full validation result is known avoids that timing issue and the risk of resurrecting the same unique-constraint hazard on the success path.
Merge checklist