Resolve semantic work package id when moving a work package#23016
Merged
Resolve semantic work package id when moving a work package#23016
Conversation
Single-WP routes pass :work_package_id from the URL, which can be a semantic identifier (e.g. "PROJ-42") since commit 4dfdd6e dropped numeric pins on the move and copy HAL action links. The bulk where(id:) lookup in find_work_packages only matches numeric primary keys, so /work_packages/PROJ-42/move/new returned 404. Translate :work_package_id through find_by_display_id first; the bulk :ids branch is untouched since form submissions already send numeric ids.
Project#reserve_semantic_id_block! rewrites work_packages.identifier and sequence_number via a raw SQL UPDATE, leaving the in-memory records with the nil identifier set by SetAttributesService when the project changed. Callers that read the WP straight out of the ServiceResult (HAL action links, the move-and-follow redirect in WorkPackages::BulkJob#redirect_path) then saw an empty display_id and fell back to numeric URLs. Reload after the bulk allocation so the in-memory state matches the database.
The plural counterpart to find_by_display_id resolves a mix of numeric and semantic identifiers in a single composite scope so callers (notably ApplicationController#find_work_packages, used by bulk WP routes) do not have to branch on the input shape or partition into numeric/semantic themselves. Mirrors scope_for_semantic_identifier in using `.or` to union the primary-key, current-identifier, and historical-alias arms; unknown values resolve to no match rather than poisoning the rest of the set.
Both :work_package_id (single-WP routes) and :ids (bulk routes) come from URLs or HTML forms and may carry semantic identifiers, so the prior numeric-only assumption on :ids was wrong. Route both inputs through WorkPackage.where_display_id_in, which returns a chainable scope that matches numeric, current-identifier, and historical-alias forms in a single query — no per-id round trip and no controller-side coupling to WorkPackageSemanticAlias. Tighten the post-move controller spec to assert the redirect URL contains the destination project's semantic prefix when follow: 1 is set, which is the path that exercises the moved WP's display_id.
The bulk SQL UPDATE persists identifier and sequence_number directly,
but callers holding live records have no cheap way to learn what was
written without reloading. Returning the {wp_id => "PROJ-N"} mapping
lets callers (next commit: WorkPackages::UpdateService) refresh
in-memory state without N round trips. Empty input returns an empty
hash for symmetric Hash-typed callers.
Previous fix reloaded each moved work package after the bulk SQL
UPDATE so HAL representers and the move-and-follow redirect saw the
freshly allocated semantic id. That cost one round trip per WP, which
matters when a parent moves with descendants. Now that
reserve_semantic_id_block! returns the {wp_id => identifier}
assignments directly, apply them with assign_attributes +
changes_applied — same observable result, zero extra queries
regardless of bulk size.
The semantic arm reimplemented the union of the identifier column and the alias-table lookup that scope_for_semantic_identifier already encapsulates. Both rely on `where(identifier:)` semantics that accept arrays as IN clauses, so the scalar helper composes correctly for the plural case. Keeps the SQL shape of "match by display id" defined in one place — if the correlated EXISTS ever needs to become a semi-join under bulk pressure, both callers benefit.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes semantic work package identifier handling in the move/copy flow so semantic URLs resolve correctly and redirects stay semantic when semantic mode is enabled.
Changes:
- Refresh in-memory work package identifier/sequence after bulk semantic ID allocation during move (avoids stale
to_param/redirect URLs). - Add
WorkPackage.where_display_id_infor resolving mixed numeric IDs, semantic IDs, and historical aliases as a chainable relation. - Update
ApplicationController#find_work_packagesto accept semantic identifiers (single and bulk) and add specs covering the end-to-end behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| spec/services/work_packages/semantic_ids/integration_spec.rb | Verifies moved WPs have refreshed in-memory identifiers so to_param stays semantic. |
| spec/models/work_package/semantic_identifier_spec.rb | Adds coverage for new where_display_id_in bulk resolver behavior and composability. |
| spec/models/projects/semantic_identifier_spec.rb | Updates expectations for reserve_semantic_id_block! to return assignments (and {} for empty input). |
| spec/controllers/work_packages/moves_controller_spec.rb | Ensures move “new” and “create” work with semantic WP IDs and redirect to semantic URLs. |
| app/services/work_packages/update_service.rb | Applies bulk-allocated semantic ID assignments to in-memory records after raw SQL updates. |
| app/models/work_package/semantic_identifier/finder_methods.rb | Introduces where_display_id_in relation builder for mixed identifier inputs. |
| app/models/projects/semantic_identifier.rb | Makes reserve_semantic_id_block! return { wp_id => semantic_identifier } assignments. |
| app/controllers/application_controller.rb | Switches bulk WP lookup to where_display_id_in to support semantic IDs and aliases. |
clear_changes_information is the established pattern in this codebase for syncing in-memory dirty state after a side-channel persist (queries, historic-attribute eager loading, progress/date-picker controllers). changes_applied has the same effect but no other call site, so prefer the local convention.
akabiru
commented
Apr 30, 2026
3 tasks
judithroth
approved these changes
May 5, 2026
Contributor
judithroth
left a comment
judithroth
left a comment
There was a problem hiding this comment.
Thanks for fixing this! The approach for the custom where is really nice!
I have Just one small remark but I think you'll find a good solution and I don't need to review again.
clear_changes_information would also clobber any unrelated dirty attributes the work package happened to be carrying at this point. The intent is narrower: only the two attributes we just synced from the raw-SQL UPDATE should be marked clean. Use clear_attribute_changes with the explicit attribute list instead.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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/74633
Bug
/work_packages/KSTP-8/move/newreturns 404 in semantic mode while the numeric form works, and the post-move-and-follow redirect lands on a numeric URL.sematic-move-wp-error.mp4
Fix
Two distinct causes that happen to surface together: the bulk-WP controller filter integer-coerces semantic params, and the raw-SQL block allocation leaves the in-memory record holding a stale identifier. Fix each at its layer so move-and-copy honours semantic identifiers end-to-end.