Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for Move to position actions to operate on a contiguous selected block of backlog cards (while preserving existing single-card behavior), using a shared block-move resolver for both menu gating and execution.
Changes:
- Introduces
resolveBlockMove/resolveBlockMoveAvailabilityto resolve predecessor placement for contiguous selections (and to reject sparse/cross-list/truncation-boundary/no-op scopes). - Updates sortable-lists controller/menu plumbing to use action-scope-based availability and to execute batch menu moves through the existing optimistic collection-move path.
- Expands backend + frontend test coverage for contiguous selection moves, gating behavior, and rollback/selection lifecycle.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/backlogs/spec/support/pages/backlog.rb | Adds page helpers for selecting contiguous cards and asserting Move-to-position availability. |
| modules/backlogs/spec/features/work_packages/batch_move_via_menu_spec.rb | New Selenium spec covering contiguous-block positional moves and rollback behavior. |
| modules/backlogs/spec/features/work_packages/batch_destination_menu_spec.rb | Updates destination menu spec to expect Move-to-position availability for contiguous multi-card scopes. |
| modules/backlogs/spec/components/backlogs/work_package_card_menu_component_spec.rb | Tightens expectations around Move-to-position menu rendering (client-driven availability, no legacy form fields). |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts | Updates documentation/comments to distinguish singular-collapse vs batch menu moves. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts | Implements block-move resolution + availability helpers for contiguous selections. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts | Adds unit tests for block-move resolution across many boundary cases. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts | Switches Move-to-position gating to rely on root-provided availability (no longer hides submenu purely for multi-card scope). |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts | Updates/extends menu tests for showing/hiding batch position directions and the submenu. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.ts | Clarifies move availability semantics in the interface comment. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts | Uses block-move availability/execution for selection-backed menu moves. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts | Adds coverage for batch menu moves, rollback behavior, and batch availability reporting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
cbddd59 to
d5e8674
Compare
d5e8674 to
5513fa3
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. |
5513fa3 to
f29fbb1
Compare
f29fbb1 to
05ef05a
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. |
05ef05a to
c3e8e19
Compare
c3e8e19 to
bbe0734
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (1)
frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts:541
selectForActionmutates the current selection before the owner-list, block-resolution, and identity checks below. If a menu opened on an unselected card becomes stale (for example, that direction becomes a no-op before activation), one of those checks returns without moving anything, but the previous batch has already been replaced by the invoker. Resolve and validate the prospective scope withactionScopeForfirst, and only commit the selection change once the move is known to be executable.
const scope = this.selectForAction(itemElement);
9b06e65 to
c88a015
Compare
79b1d26 to
f6848b9
Compare
cd7675b to
fcb8711
Compare
fcb8711 to
ccd6efe
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. |
ccd6efe to
f26b6ad
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. |
|
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. |
Establishes one DOM resolution for availability and execution. Validates contiguous blocks and explicit unavailable reasons while preserving singular placement and sparse-list boundaries. https://community.openproject.org/wp/AGILE-363
Verifies that every resolved row owns its supplied item and prevents nested lists from substituting an outer host row. https://community.openproject.org/wp/AGILE-363
Resolves availability over the invoker's prospective scope and exposes only moves the live block resolver can execute. Preserves destinations for contiguous same-list selections. https://community.openproject.org/wp/AGILE-363
Routes selection roots through the shared block resolver and moves the ordered block through the optimistic collection path. Keeps roots without selection on their singular member endpoint. https://community.openproject.org/wp/AGILE-363
Exercises every direction and gating boundary through Selenium. Covers optimistic order, feedback, cleanup, and 422 rollback while protecting one-card, sparse, cross-list, and confined scopes. https://community.openproject.org/wp/AGILE-363
Resolves collection capability before availability or mutation. Makes incomplete selection roots fail closed while keeping roots without selection on their singular member behavior. https://community.openproject.org/wp/AGILE-363
Restores per-direction availability for plain sortable roots while keeping selection roots on block and capability gating. Protects blank predecessors and removes the obsolete collapse shim. https://community.openproject.org/wp/AGILE-363
Replaces the persisted-order database checks with a reload and a rendered-order assertion, so the spec proves persistence by what the user sees surviving the reload. Payload and ordering semantics remain covered by the controller unit specs and the request specs.
|
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. |
Note
This PR is part of a stack. Please review and merge in this order: #24525 → #24778 → #24779 → this PR.
Ticket
https://community.openproject.org/wp/AGILE-363
What are you trying to accomplish?
Make the existing Backlogs Move to position submenu move either one card or one contiguous selected block in the same list.
The change:
What approach did you choose and why?
Menu availability and activation both call the same resolver. Activation settles the live action scope, resolves again against the current innermost owned list, and passes the ordered rows and IDs to the existing collection
performMovepath. Roots without the selection/collection capability retain their singular member-move behavior.This supersedes the menu-collapse behaviour added in #24778. There, a menu move relocated exactly the card it named, so
collapseForActioncollapsed any wider selection onto that card rather than leaving a batch the move never touched. A positional move here carries the whole block, so collapsing would be wrong: the orchestrator method is removed on this branch, and the selection is committed withselectForActiononly once the move is known to be executable.The resolver validates row ownership so nested sortable lists cannot substitute an outer host row for an inner work package. Fixed rows remain addressable neighbours but cannot enter the selected block, and one-step moves never cross a truncation marker.
Merge checklist
TODO from [AGILE-361] Add batch selection to Backlogs cards #24525: preserve positional-action contiguity after the inherited range-preservation change. For a sparse selection such as A plus D–F: positional actions must remain unavailable until the entire selection is a supported contiguous block. Range discussion.
TODO from [AGILE-278] Move selected work packages atomically #24778: verify Backlogs positional menu moves use the collection contract for both one card and a supported selected block, while other sortable consumers retain their single-item route. Review discussion.
TODO from [AGILE-278] Move selected work packages atomically #24778: audit remaining single-item action/predicate callers and reconcile
position:with the collection contract before removing them; link a follow-up work package for any remaining consolidation. Preserve the single-item service required byadd_existing. Controller discussion, predicate discussion.Added/updated documentation in Lookbook (patterns, previews, etc)