Skip to content

[AGILE-318] Assign copied work packages to copied sprints#24129

Open
myabc wants to merge 21 commits into
release/17.6from
bug/agile-318-work-packages-wrong-assigned-sprints
Open

[AGILE-318] Assign copied work packages to copied sprints#24129
myabc wants to merge 21 commits into
release/17.6from
bug/agile-318-work-packages-wrong-assigned-sprints

Conversation

@myabc

@myabc myabc commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Ticket

https://community.openproject.org/wp/AGILE-318

What are you trying to accomplish?

When a project is copied, work packages in the copy kept a sprint_id pointing at the original project's sprint instead of a copy of it. The stray assignment is invisible on the source sprint's board but surfaces in the sprint-completion modal, where the copied work package is listed among the sprint's items. Saved queries and boards filtered on a sprint or backlog bucket had the same problem: their filter values were carried over verbatim and kept matching the source project's records.

Since 17.6 made sprints their own model (a sprints table with a sprint_id FK on work packages, parallel to versions), the project-copy path remapped version_id but had no equivalent for sprint_id / backlog_bucket_id.

Screenshots

image

What approach did you choose and why?

Mirror the existing version-copy path, entirely within modules/backlogs:

  • New SprintsDependentService and BacklogBucketsDependentService copy the source project's owned sprints and backlog buckets, each recording a source→copy id lookup, injected into the copy dependency list immediately before work packages are copied.
  • A patch on the work-packages copy override remaps each copied work package's sprint_id and backlog_bucket_id through those lookups and carries the source position over unchanged (with acts_as_list callbacks suppressed). Owned sprints/buckets are always recreated; a shared assignment (a sprint owned by another project) is kept only when the copied project will still receive that sprint given its placement, and cleared otherwise.
  • A Queries::Copy::FiltersMapper patch remaps sprint_id / backlog_bucket_id query filters through the same lookups, as already done for version_id, so copied queries and boards follow the copied records.
  • Copying runs through a shared copy_collection_with_id_map helper on Projects::Copy::Dependency; the existing version copy reuses it too.

Additional hardening (review follow-ups)

  • Records skipped because their copy fails validation are now merged into the dependency's error set instead of being dropped silently, so a work package no longer loses its assignment unannounced while the copy still reports success.
  • Shared-sprint preservation resolves in a single query against the sprints the target receives, instead of firing the receiving_projects CTE once per sprint; sprint goals are eager-loaded.
  • Owned sprint goals are copied (remapped to the copy); goals owned by other projects on a shared sprint are left alone.
  • CategoriesDependentService now reuses the shared copy_collection_with_id_map helper, which also drops a stray mapping to a nil id when a category fails to persist.

Out of scope / follow-ups

  • Broader foreign-sprint leak — product-confirmed follow-up WP. The same "sprint not receivable by its project" class exists outside copy, e.g. moving a work package to a project that cannot receive its sprint. The plain single move is already cleared today, but the invariant is enforced per write-path by two divergent implementations that carry a self-referential branch, so a leak can persist once introduced. Agreed direction: enforce one central receivable_by? invariant plus a data migration to heal existing rows. Tracked separately.
  • Copy-form checkbox visibility (follow-up to @dombesz review, point 2 + the "module disabled" note). The execution leak is fixed: sprints and backlog buckets are not copied when the source lacks the backlogs module or the acting user cannot view sprints. What remains is cosmetic — the checkboxes are still shown — and hiding them is a framework change affecting every module dependency, so it belongs in a separate change.
  • Narrowing the backlogs position / acts_as_list override is left as a separate cleanup.
  • Task boards are not copied.

Merge checklist

  • Added/updated tests
  • Added/updated documentation in Lookbook (patterns, previews, etc) — N/A (backend, no UI)
  • Tested major browsers (Chrome, Firefox, Edge, ...) — N/A (backend)

Copilot AI review requested due to automatic review settings July 6, 2026 17:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes an issue in the Backlogs project-copy flow where copied work packages could retain a sprint_id pointing to a sprint from the source project. It introduces sprint copying and remaps sprint references on copied work packages so that owned sprints are mapped to their newly created copies, while sprints owned by other projects keep their original IDs.

Changes:

  • Added a Projects::Copy::SprintsDependentService to copy a project’s owned sprints and build a source→target sprint ID lookup.
  • Patched Projects::CopyService.copy_dependencies to run sprint copying before work package copying.
  • Patched Projects::Copy::WorkPackagesDependentService to remap sprint_id during work package copy using the lookup, with integration specs covering owned vs shared sprint behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
modules/backlogs/spec/services/projects/copy_service_integration_spec.rb Adds integration coverage for sprint copying and sprint reassignment on copied work packages.
modules/backlogs/lib/open_project/backlogs/patches/work_packages_dependent_service_patch.rb Remaps copied work packages’ sprint_id via state.sprint_id_lookup.
modules/backlogs/lib/open_project/backlogs/patches/copy_service_patch.rb Inserts SprintsDependentService into the project copy dependency chain before work packages.
modules/backlogs/lib/open_project/backlogs/engine.rb Ensures the new patch for Projects::Copy::WorkPackagesDependentService is applied.
modules/backlogs/config/locales/en.yml Adds the copy UI label for the new sprints dependency.
modules/backlogs/app/services/projects/copy/sprints_dependent_service.rb New dependent service to copy sprints and populate state.sprint_id_lookup.

myabc added 4 commits July 6, 2026 18:57
Adds a `SprintsDependentService` mirroring the version copy path and
injects it before the work-packages dependency so a source->copy sprint
id lookup is available when work packages are copied.

https://community.openproject.org/wp/AGILE-318
Patches the work-packages copy override to translate `sprint_id`
through `state.sprint_id_lookup`: sprints owned by the source project
remap to their copy, shared sprints keep their original id. Fixes copied
work packages appearing in the source project's sprint completion modal.

https://community.openproject.org/wp/AGILE-318
Assigns `state.sprint_id_lookup` after each created sprint (not once
after the loop) so a mid-loop `create!` failure leaves the partial
mapping intact instead of nil, which would otherwise make copied work
packages retain the source project's sprint. Per-iteration assignment
is required because `state` is a `Hashie::Mash`: assigning the empty
hash before the loop converts it into a separate `Mash` instance, so
later mutations of the plain `Hash` are invisible to `state`. Also
strengthens the sprint-copy spec with a source/target decoupling
assertion.
@myabc myabc force-pushed the bug/agile-318-work-packages-wrong-assigned-sprints branch from 3ade633 to f3d85ba Compare July 6, 2026 18:02
@myabc myabc added this to the 17.6.x milestone Jul 6, 2026
@myabc myabc added ruby Pull requests that update Ruby code needs review labels Jul 6, 2026
Drives the copy-project UI end to end and checks the copied work
package lands in a copied sprint, not the source project's sprint,
mirroring the version coverage in spec/features/projects/copy_spec.rb.
@myabc myabc added bugfix and removed DO NOT MERGE labels Jul 6, 2026
Comment thread modules/backlogs/spec/features/projects/copy_backlogs_spec.rb Outdated

@dombesz dombesz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried the feature out and it kind of works, but unfortunately there are a few shortcomings that need to be addressed.

  1. The positions of the work packages are not copied over. Here is a suggested solution . Hope you don't mind, I took the liberty and provided a commit (c79858c) with this fix.
  2. Out of scope: When a member user has the permission to Edit and Copy the project but no permission to View Sprints, the sprints checkbox is still shown on the copy form. The sprints are still copied over alongside the work packages. The expectation is to not show the "Sprints" checkbox, and do not copy the sprints over.
  3. While this might be out of scope of the PR, the Backlog Buckets are copied incorrectly. The work package copies in the new project are assigned with the bucket from the source project, which is incorrect. I think it would make sense to copy the buckets too, alternatively do not copy the wrong bucket ids.
Work packages inside a bucket do not show up in the project copy

Source project: Image

Copy project:
Image

The source project buckets are assigned in the copy project:

Image

Edit: I was also wondering what should be the right behaviour when the Backlogs module is disabled in the source project? Currently the "Sprints" checkbox is still showing up in the copy form, however I'd say it shouldn't show up if the backlogs are disabled and sprints shouldn't be copied.

@myabc myabc force-pushed the bug/agile-318-work-packages-wrong-assigned-sprints branch from baa91a0 to 3512b24 Compare July 7, 2026 13:27
@myabc myabc requested a review from dombesz July 7, 2026 13:33
myabc added 4 commits July 7, 2026 14:43
Copied work packages kept a `backlog_bucket_id` pointing at the source
project's bucket because project copy remapped sprints but not buckets.
Mirror the sprint fix: copy owned buckets, then remap each work package.
Sprints and buckets only need to run before the work packages, so
prepending them is simpler than locating the index and inserting.
@myabc myabc force-pushed the bug/agile-318-work-packages-wrong-assigned-sprints branch from 3512b24 to 02dac87 Compare July 7, 2026 13:46
myabc and others added 2 commits July 7, 2026 16:14
Two feature specs each defined an identical wait_for_copy_to_finish.
The copy is always triggered from the general settings page, so the
helper moves onto that page object and both specs call it there.
…ise preserve the sprint assignment from shared sprints.
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown

Warning

Flaky specs

  • rspec ./modules/bim/spec/features/ifc_models/direct_ifc_upload_spec.rb[1:1:1:1:1]
  • rspec ./modules/bim/spec/features/ifc_models/direct_ifc_upload_spec.rb[1:1:2:1:1]
🤖 Ask Copilot to investigate

Copy 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.

@copilot The following spec(s) are flaky in CI (first seen on PR #24129, linked for reference only):

- `rspec ./modules/bim/spec/features/ifc_models/direct_ifc_upload_spec.rb[1:1:1:1:1]`
- `rspec ./modules/bim/spec/features/ifc_models/direct_ifc_upload_spec.rb[1:1:2:1:1]`

Treat this as a standalone task, unrelated to PR #24129. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24129 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @myabc to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @myabc, and request a review from @myabc.
On every commit, set @myabc as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

@dombesz

dombesz commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Since there are open questions regarding this PR, I would hold off merging until they are cleared.

myabc added 2 commits July 7, 2026 18:02
The sprint and backlog-bucket services duplicated the copy-into-id-map
loop and used `create!`, so a single invalid record aborted the whole
project copy. A shared `copy_collection_with_id_map` on the copy
`Dependency` base uses the non-raising `create` and skips unpersisted
records, matching `VersionsDependentService`, and assigns `state` once
instead of on every iteration.
The WorkPackagesDependentService patch runs for every project copy, so
suppressing `acts_as_list` and forcing the source position also changed
non-backlogs copies. Gate both overrides on `backlogs_enabled?` and
read the id-map lookups nil-safely.
let(:user) { create(:user, member_with_permissions: { project => permissions }) }
module InstanceMethods
def copy_work_package(source_work_package, parent_id, user_cf_ids)
return super unless source.backlogs_enabled?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dombesz I think these guards are needed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily, perhaps the permission check verification would be a more appropriate guard. I logged a separate ticket for this issue https://community.openproject.org/wp/AGILE-328 .

myabc added 2 commits July 7, 2026 18:27
VersionsDependentService duplicated the copy-into-id-map loop that the
new `copy_collection_with_id_map` now provides. Reusing it also skips
unpersisted versions instead of mapping their id to nil.
Copying a sprint reproduced only its scalar columns, silently dropping
its goals. The sprint copy now carries over the source project's own
goals via nested attributes, remapped to the copy; goals owned by other
projects on a shared sprint are left untouched.
current_user { user }
def copy_dependency(*)
if source.receive_shared_sprints?
preserve_sprint_assignments

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown

Warning

Flaky specs

  • rspec ./spec/features/activities/work_package/activities_spec.rb[1:5:2:2]
  • rspec ./spec/features/projects/lists/filters_spec.rb[1:6:1]
🤖 Ask Copilot to investigate

Copy 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.

@copilot The following spec(s) are flaky in CI (first seen on PR #24129, linked for reference only):

- `rspec ./spec/features/activities/work_package/activities_spec.rb[1:5:2:2]`
- `rspec ./spec/features/projects/lists/filters_spec.rb[1:6:1]`

Treat this as a standalone task, unrelated to PR #24129. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24129 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @myabc to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @myabc, and request a review from @myabc.
On every commit, set @myabc as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

The RECEIVE_SHARED branch identity-mapped every received sprint, so a
copy placed outside the sharing subtree kept work packages pointing at
sprints it cannot see, and a receiving project's own (stale) sprints
were never recreated. Copying now always reproduces owned sprints and
preserves a shared assignment only when the copied project will still
receive that sprint, clearing it otherwise.
@myabc myabc force-pushed the bug/agile-318-work-packages-wrong-assigned-sprints branch from 41fe5e6 to 74ad337 Compare July 8, 2026 10:51
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

Warning

Flaky specs

  • rspec ./modules/backlogs/spec/features/work_packages/move_to_backlog_spec.rb[1:3:3:1]
  • rspec ./spec/features/projects/project_autocomplete_spec.rb[1:1]
🤖 Ask Copilot to investigate

Copy 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.

@copilot The following spec(s) are flaky in CI (first seen on PR #24129, linked for reference only):

- `rspec ./modules/backlogs/spec/features/work_packages/move_to_backlog_spec.rb[1:3:3:1]`
- `rspec ./spec/features/projects/project_autocomplete_spec.rb[1:1]`

Treat this as a standalone task, unrelated to PR #24129. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24129 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @myabc to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @myabc, and request a review from @myabc.
On every commit, set @myabc as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

myabc added 3 commits July 8, 2026 13:18
The shared `copy_collection_with_id_map` helper silently dropped any
record whose copy failed validation, so a work package referencing it
lost its assignment with the copy still reporting success. Failed copies
are now merged into the dependency's error set, keeping the copy tolerant
while making the skip visible instead of a silent data loss.
Copied saved queries kept their `sprint_id` and `backlog_bucket_id`
filter values pointing at the source project's records, so a copied
query matched nothing once the sprints and buckets were recreated with
new ids. A `Queries::Copy::FiltersMapper` patch now remaps both through
the copy id-maps, as already done for `version_id`.
`preserved_shared_sprints` filtered every shared sprint through
`Sprint.receiving_projects(...).exists?` in Ruby, firing that CTE once
per sprint and hydrating full records only to read their ids. It now
resolves in a single query against the sprints the target natively
receives. Goals are eager-loaded so `copied_goal_attributes` no longer
queries per sprint.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

Comment on lines +47 to +55
def copy_work_package_attribute_overrides(source_work_package, parent_id, user_cf_ids)
return super unless source.backlogs_enabled?

super.merge(
sprint_id: work_package_sprint_id(source_work_package),
backlog_bucket_id: work_package_backlog_bucket_id(source_work_package),
position: source_work_package.position
)
end
Comment on lines +43 to +45
def copy_dependency(*)
state.sprint_id_lookup = copy_owned_sprints.merge(preserved_shared_sprints)
end
Comment on lines +43 to +45
def copy_dependency(*)
state.backlog_bucket_id_lookup = copy_collection_with_id_map(:backlog_buckets)
end

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dombesz I removed this runtime guard.

`CategoriesDependentService` hand-rolled the same copy loop the shared
`copy_collection_with_id_map` helper now provides. Reusing it drops a
stray mapping to a `nil` id when a category fails to persist and gives
the copy fresh timestamps, matching every other copy dependent.
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

Warning

Flaky specs

  • rspec ./modules/overviews/spec/features/project_description_widget_spec.rb[1:1:1:1:1]
🤖 Ask Copilot to investigate

Copy 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.

@copilot The following spec(s) are flaky in CI (first seen on PR #24129, linked for reference only):

- `rspec ./modules/overviews/spec/features/project_description_widget_spec.rb[1:1:1:1:1]`

Treat this as a standalone task, unrelated to PR #24129. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24129 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @myabc to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @myabc, and request a review from @myabc.
On every commit, set @myabc as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix DO NOT MERGE needs review ruby Pull requests that update Ruby code

Development

Successfully merging this pull request may close these issues.

3 participants