[AGILE-392] Cover unrestricted drag destinations - #24858
Conversation
e994e8c to
f8c47f1
Compare
bf063b8 to
9a400ca
Compare
There was a problem hiding this comment.
Pull request overview
This PR aligns Backlogs batch-drag behavior with the same per-item destination policy used by batch destination menus, ensuring drags don’t offer drops the server will reject (and don’t refuse drops that should be allowed) when the selection contains confined work packages across lists.
Changes:
- Centralizes per-item destination acceptance into
itemAcceptsDestination, and rebuilds destination intersections on that policy. - Reworks drag payload gating from “confined + source list” to “permitted list elements” resolved across the whole batch at drag start.
- Updates Backlogs feature specs and DnD helpers to more reliably observe refusal feedback (animation-frame timing / event stream).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/support/shared/drag_and_drop_helper_spec.rb | Adds dwell: option to stabilize native drag event streams for assertions. |
| modules/backlogs/spec/support/pages/backlog.rb | Updates Backlogs drag helpers and refusal assertions to track container state across events; threads dwell: through Selenium drag helper. |
| modules/backlogs/spec/features/work_packages/batch_move_spec.rb | Adds feature specs covering batch moves with confined members across lists. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/scrollable.controller.spec.ts | Updates mocks to the new root API (dragPermittedLists). |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.ts | Switches list indicator gating to permittedListsAllowDrop. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.spec.ts | Updates test payloads to use permittedListElements. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts | Introduces itemAcceptsDestination and uses it to compute permittedDestinations. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts | Adds unit coverage for itemAcceptsDestination. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts | Emits permittedListElements on the drag payload instead of confinement/source-list fields. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts | Updates DnD payload and acceptance tests to the new permitted-lists model. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.ts | Replaces confinement checks with permittedListsAllowDrop and updates payload shape. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.spec.ts | Updates helper specs for the new payload shape. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts | Adds dragPermittedLists to compute permitted lists across batch members using the shared policy. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts | Extends root controller specs to validate permitted-lists behavior across multi-list batches. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
9a400ca to
078374e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (1)
frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts:315
- This confinement-specific early return bypasses
itemAcceptsDestination, so the drag is not actually driven solely by the shared per-item policy. In particular, the PR description says AGILE-357 can add a workflow restriction as another policy arm without changing either consumer, but a batch with no confined members will still returnnullhere and accept every drag destination. Please evaluate the registered destinations throughitemAcceptsDestinationfor every batch, and derivenullonly when that evaluation leaves all destinations unrestricted.
const members = [itemElement, ...this.prospectiveDragMateElements(itemElement)];
if (members.every((member) => !isConfinedItem(member))) {
return null;
078374e to
fe829fb
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. |
EinLama
left a comment
There was a problem hiding this comment.
Works like a charm! Tested multiple scenarios and could never break it. Code looks great. I had one particular function that made me pause for a bit. Please have a look 🔍
Approving since even if this was a valid point, it's not a blocker.
|
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. |
26d44ad to
8d11d33
Compare
09d4c77 to
c1d4132
Compare
c1d4132 to
1d788ce
Compare
0503d7b to
94a4008
Compare
1d788ce to
1510c7c
Compare
94a4008 to
d65431a
Compare
3ea8b0a to
d76d601
Compare
c51a846 to
e531ddc
Compare
b4f06f9 to
3f387d1
Compare
e531ddc to
717e0c1
Compare
3f387d1 to
b249809
Compare
8dac70e to
f8ee525
Compare
b249809 to
5282243
Compare
f8ee525 to
1376650
Compare
7fa8153 to
cbc6435
Compare
1376650 to
0b88d01
Compare
cbc6435 to
f1c768e
Compare
0b88d01 to
c05cfa5
Compare
c05cfa5 to
c0760a7
Compare
c0760a7 to
e2f3488
Compare
A confined item's own mobility must not narrow a root's unrestricted permitted-destinations answer, as the batch it carries may reach every list. https://community.openproject.org/wp/AGILE-392
e2f3488 to
6b8ede0
Compare
Ticket
https://community.openproject.org/wp/AGILE-392
What are you trying to accomplish?
Preserve coverage of the batch drag destination policy after the core confinement fix landed through AGILE-278 (#24778). This PR now targets dev independently of the destination and action-menu stack.
What approach did you choose and why?
The remaining regression test ensures an item's own confined mobility does not override an unrestricted permitted-destinations answer from the root. The equivalent per-item policy test is already present on dev and is retained there without duplication.
The original runtime confinement changes are already on dev through AGILE-278. This remaining diff introduces no runtime behavior changes.
Documentation added after the approved scope has been extracted unchanged to AGILE-436 / #25359, a separate draft PR based on AGILE-364. That documentation will receive its own review.
Validation