Ensure position is updated on bucket deletion#22956
Merged
ulferts merged 7 commits intorelease/17.4from May 6, 2026
Merged
Conversation
ulferts
force-pushed
the
bug/74369-workpackages-not-moved-to-bottom-of-inbox-upon-backlog-bucket-removal
branch
2 times, most recently
from
cb3da25 to
b4f1646
Compare
ulferts
force-pushed
the
bug/74369-workpackages-not-moved-to-bottom-of-inbox-upon-backlog-bucket-removal
branch
from
b4f1646 to
97f20ce
Compare
This comment was marked as low quality.
This comment was marked as low quality.
ulferts
force-pushed
the
bug/74369-workpackages-not-moved-to-bottom-of-inbox-upon-backlog-bucket-removal
branch
from
97f20ce to
bf71ec4
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves backlog bucket deletion behavior in the Backlogs module by ensuring work package ordering remains correct after a bucket is deleted and by adding an explicit confirmation modal for bucket deletion.
Changes:
- Update
WorkPackages::RebuildPositionsServiceto partition positions bybacklog_bucket_id(in addition toproject_idandsprint_id) so inbox/buckets/sprints don’t share one position sequence. - Add a dedicated “delete backlog bucket” dialog endpoint + Primer DangerDialog component, and wire the bucket menu to open it via
async-dialog. - Extend/adjust specs to cover bucket-related positioning and the new modal-based delete flow.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| modules/backlogs/app/services/backlog_buckets/delete_service.rb | Moves bucket work packages to inbox during bucket deletion (review note about hook ordering). |
| modules/backlogs/app/services/work_packages/rebuild_positions_service.rb | Adds backlog_bucket_id to the SQL window partitioning for position rebuilds. |
| modules/backlogs/app/controllers/backlogs/backlog_buckets_controller.rb | Adds destroy_dialog action to serve the deletion modal. |
| modules/backlogs/app/components/backlogs/backlog_bucket_menu_component.html.erb | Switches “Delete backlog bucket” action to open an async dialog instead of direct DELETE. |
| modules/backlogs/app/components/backlogs/backlog_bucket_destroy_modal_component.rb | New component backing the delete confirmation modal. |
| modules/backlogs/app/components/backlogs/backlog_bucket_destroy_modal_component.html.erb | New Primer DangerDialog modal template for bucket deletion. |
| modules/backlogs/config/routes.rb | Adds destroy_dialog member route for backlog buckets. |
| modules/backlogs/lib/open_project/backlogs/engine.rb | Permits destroy_dialog under create_sprints permission mapping. |
| modules/backlogs/config/locales/en.yml | Adds i18n strings for the new delete modal. |
| modules/backlogs/db/migrate/20260325101553_reposition_work_packages.rb | Updates migration comments to clarify why backlog buckets aren’t reflected there. |
| modules/backlogs/spec/support/pages/backlog.rb | Adds page-object helpers for asserting/confirming the delete modal. |
| modules/backlogs/spec/features/backlogs/pagination_state_spec.rb | Adapts feature spec from browser confirm to modal flow. |
| modules/backlogs/spec/features/backlog_buckets/delete_spec.rb | Adapts delete feature spec to modal flow; asserts moved WPs appear at inbox bottom. |
| modules/backlogs/spec/routing/backlogs/backlog_buckets_routing_spec.rb | New routing spec covering create/update/destroy + dialog endpoints. |
| modules/backlogs/spec/services/backlog_buckets/delete_service_spec.rb | Adds service-level coverage asserting WPs move to inbox and positions update on delete. |
| modules/backlogs/spec/services/work_packages/rebuild_positions_service_integration_spec.rb | Extends integration coverage to include bucket partitions. |
| modules/backlogs/spec/models/work_packages/position_spec.rb | Extends position behavior coverage to buckets + inbox interactions. |
toy
requested changes
May 5, 2026
Contributor
toy
left a comment
toy
left a comment
There was a problem hiding this comment.
More or less nitpicks except the dialog style
| ) | ||
|
|
||
| expect(WorkPackage.where(sprint: nil).to_h { [it.subject, it.position] }) | ||
| expect(WorkPackage.where(sprint: nil, backlog_bucket: nil, project: project1).to_h { [it.subject, it.position] }) |
Contributor
There was a problem hiding this comment.
Relying on subject instead of id makes test failures easier to understand, but suggests doing same in other tests
| shared_let(:bucket2_wp1) { create_work_package(subject: "Bucket 2 WorkPackage 1", backlog_bucket: bucket2, position: nil) } | ||
| shared_let(:bucket2_wp2) { create_work_package(subject: "Bucket 2 WorkPackage 2", backlog_bucket: bucket2, position: nil) } | ||
| shared_let(:bucket2_wp3) { create_work_package(subject: "Bucket 2 WorkPackage 3", backlog_bucket: bucket2, position: nil) } | ||
|
|
Contributor
There was a problem hiding this comment.
Should there also be work packages that are in sprint from other project?
| inbox_wp3.subject => 3 | ||
| ) | ||
|
|
||
| expect(WorkPackage.where(sprint: sprint3).to_h { [it.subject, it.position] }) |
Contributor
There was a problem hiding this comment.
I think it is better to either group together all those that change and all those that don't and/or name buckets and sprints variables to contain project id, alternatively separate tests for every sprint/bucket/project inbox to clearly explain every case
ulferts
deleted the
bug/74369-workpackages-not-moved-to-bottom-of-inbox-upon-backlog-bucket-removal
branch
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.
Ticket
https://community.openproject.org/wp/74369
What are you trying to accomplish?
WorkPackages::RebuildPositionsServiceincludesbacklog_bucket_idin the PARTITION BYMerge checklist