[DREAM-786] Extend sortable-lists for admin surfaces and migrate project attributes by myabc · Pull Request #24504 · opf/openproject

myabc · 2026-07-29T19:05:27Z

Ticket

https://community.openproject.org/wp/DREAM-786

What are you trying to accomplish?

Extends the shared sortable-lists Stimulus suite so admin surfaces can adopt it, and proves the extended contract by migrating Administration → Projects → Project attributes (sections + fields) off Dragula (generic-drag-and-drop).

Suite extensions (all additive, Backlogs keeps working unmodified): rows may be any element — an item's owning list is the innermost list outlet containing it (replaces the closest('li') assumption); a per-type move-URL template map so one root can host item types with different endpoints; the optimistic=true flag is now opt-in (Backlogs opts in to keep its event-only success contract, admin surfaces reconcile through morph streams).
Backend: anchor-based (prev_id) ordering primitives with strict validation — a blank anchor means top; an unknown, out-of-scope, self, or omitted anchor is a 422 with zero mutation. Both project-attribute drop endpoints convert in-place to the list_type/list_id/prev_id wire (the page migrates in the same PR, so no dual-wire period).
Server responses switch to method="morph" turbo streams; field rows gain unique per-field wrapper IDs so the morph matcher associates rows correctly on reorder.
Sections and fields render a custom drag preview and grab/grabbing handle cursors (the native drag snapshot painted margin whitespace and squared-off corners).
Also fixes a suite bug the new feature spec exposed: the row lookup climbed ancestors unbounded, so dropping into an empty section resolved the enclosing section itself as the prev_id anchor; the lookup is now bounded to the row's own list.

Note

Visually this is more or less a like-for-like migration. The newer Backlogs-style styling improvements (e.g. drop indicators) land when this page migrates to BorderBoxListComponent: https://community.openproject.org/wp/DREAM-697

Covered by vitest suite specs (nested dual-role topology), model/service/request/component specs, and a Selenium feature spec (same-section, cross-section and empty-section drags, menu moves, drag-after-morph).

What approach did you choose and why?

Ordering primitives are model concerns, not services. Lists::MoveAfterAnchor (for acts_as_list models) and CustomFieldSection#insert_after_key (for the attribute_order array) live next to the state they mutate. ProjectCustomFieldSections::UpdateService is no longer in the drop path (it carried no validation or side effects relevant to positioning).
CustomFields::DropService keeps a legacy adapter path. The user-custom-fields surface still sends the old target_id/position wire; the path is pinned by a regression spec and dies with https://community.openproject.org/wp/DREAM-790.
Concurrency: cross-section field moves are transactional and lock both section rows in deterministic id order before reading attribute_order (a read-modify-write array). The acts_as_list side needs no equivalent lock — position shifts are single SQL statements, so the worst concurrent case is a surprising order, not corruption.
Move-URL templates come from route helpers with a sentinel substitution (__id__ → {id}), never hardcoded paths, so relative-URL-root installations keep working.
The existing move_to menu actions and CustomFields::MoveService are untouched (shared four-direction menu descriptor is https://community.openproject.org/wp/DREAM-775).

Screen recording

Screen.Recording.2026-08-06.at.13.34.24.mov

Follow-up notes:

Auto-scroll while dragging is not wired on this page (it never had it under Dragula); wiring sortable-lists--scrollable needs a scroll-container decision.
The sortable-lists consumer contract documentation lands with https://community.openproject.org/wp/DREAM-774.
The Selenium drag helpers (selenium_target_point, honey-pot cleanup) are duplicated between the backlogs and admin specs — worth extracting when the next surface migrates.

Merge checklist

Added/updated tests
Added/updated documentation in Lookbook (patterns, previews, etc)
Tested major browsers (Chrome, Firefox, Edge, ...)

Copilot

Pull request overview

This PR updates the Admin → Settings → Project custom fields UI to use the sortable-lists Stimulus subsystem (including nested lists), adds anchor-based drop endpoints for reordering/moving fields and sections, and switches the Turbo Stream updates for the settings components to method="morph" to reconcile server-rendered order.

Changes:

Backend: add anchor-based reordering (list_id/prev_id) for project custom fields and sections, including a reusable Lists::MoveAfterAnchor concern.
Frontend: extend sortable-lists root to support nested list topologies, per-item-type move URL templates, and an opt-in optimistic request flag (kept for Backlogs).
Tests: add/extend model, service, request, and component specs covering reorder/move success and “no mutation on invalid anchor” cases.

Reviewed changes

Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
app/services/custom_fields/drop_service.rb	Adds anchor-based (list_id/prev_id) move logic with transactional section updates.
app/controllers/admin/settings/project_custom_fields_controller.rb	Switches drop endpoint to sortable-lists params + validates request; returns 422 on invalid anchors.
app/controllers/admin/settings/project_custom_field_sections_controller.rb	Replaces position-based drop with anchor-based `move_after_anchor`.
app/models/concerns/lists/move_after_anchor.rb	New concern to reorder `acts_as_list` models relative to an anchor id.
app/models/custom_field_section.rb	Includes `MoveAfterAnchor` and adds `insert_after_key` for per-section attribute ordering.
app/controllers/concerns/admin/settings/project_custom_fields/component_streams.rb	Uses Turbo Stream `method: :morph` for section(s) updates.
app/components/settings/project_custom_field_sections/index_component.rb	Wires sortable-lists root with per-type move URL templates and outlets.
app/components/settings/project_custom_field_sections/index_component.html.erb	Replaces dragula/generic-dnd wiring with sortable-lists list + items.
app/components/settings/project_custom_field_sections/show_component.rb	Wires each section’s field list and field rows for sortable-lists.
app/components/settings/project_custom_field_sections/show_component.html.erb	Adds handle target wiring and sortable-lists item/list data attributes.
app/components/settings/project_custom_field_sections/custom_field_row_component.rb	Adds stable wrapper identity for morphing.
app/components/settings/project_custom_field_sections/custom_field_row_component.html.erb	Wires the drag handle as sortable-lists item handle target.
frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts	Adds nested-owner list resolution, per-type URL mapping, and optimistic flag gating.
frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts	Adds coverage for nested topology, URL template map, and optimistic behavior.
frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts	Uses root-resolved rows container for sticky bounds (nested DOM support).
frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts	Tests root-resolved rows container behavior.
frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts	Adds `resolveItemType` helper and exported attribute constant.
frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts	Adds unit tests for `resolveItemType`.
frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.ts	Extends `SortableListsRoot` contract with `ownerRowsContainer`.
frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.spec.ts	Updates stubs to satisfy the extended root contract.
frontend/src/stimulus/controllers/dynamic/sortable-lists/scrollable.controller.spec.ts	Updates stubs to satisfy the extended root contract.
modules/backlogs/app/views/backlogs/backlog/show.html.erb	Opts Backlogs into optimistic move requests via data attribute.
config/locales/en.yml	Adds `error_invalid_list_move_anchor` message.
spec/services/custom_fields/drop_service_spec.rb	Adds service-level coverage for anchor-based reorders and rollback behavior.
spec/models/project_custom_field_section_spec.rb	Adds unit coverage for `insert_after_key`.
spec/models/concerns/lists/move_after_anchor_spec.rb	Adds unit coverage for `MoveAfterAnchor`.
spec/requests/admin/settings/project_custom_fields_drop_spec.rb	Adds request specs for custom field drop behavior + authorization.
spec/requests/admin/settings/project_custom_field_sections_drop_spec.rb	Adds request specs for section drop behavior and morph response.
spec/components/settings/project_custom_field_sections/index_component_spec.rb	Verifies sortable-lists root wiring and absence of dragula remnants.
spec/components/settings/project_custom_field_sections/show_component_spec.rb	Verifies sortable-lists wiring for nested field lists and handle target.
spec/components/settings/project_custom_field_sections/custom_field_row_component_spec.rb	Verifies stable row keys and handle wiring.

github-actions · 2026-07-29T21:00:46Z

Warning

Flaky specs

rspec ./spec/features/users/non_working_times_spec.rb[1:2:1]
rspec ./spec/features/users/non_working_times_spec.rb[1:2:2]
rspec ./spec/features/users/non_working_times_spec.rb[1:3:1]
rspec ./spec/features/users/non_working_times_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 #24504, linked for reference only):

- `rspec ./spec/features/users/non_working_times_spec.rb[1:2:1]`
- `rspec ./spec/features/users/non_working_times_spec.rb[1:2:2]`
- `rspec ./spec/features/users/non_working_times_spec.rb[1:3:1]`
- `rspec ./spec/features/users/non_working_times_spec.rb[1:6:1]`

Treat this as a standalone task, unrelated to PR #24504. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24504 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.

Copilot

Pull request overview

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

dombesz · 2026-07-30T10:14:46Z

+    #
+    # Returns false without mutating when the anchor is unknown, outside
+    # the scope, or the record itself.
+    def move_after_anchor(prev_id, scope:) # rubocop:disable Naming/PredicateMethod -- verb command, not a query


Instead of implementing a new move_after method, I would prefer reusing the existing move_after method from:

openproject/modules/backlogs/lib/open_project/backlogs/list.rb

Lines 45 to 62 in a9f0bda

def move_after(position: nil, prev_id: nil)

# Remove so the potential 'prev' has a correct position

remove_from_list

reload

cast_position = ActiveRecord::Type::Integer.new.cast(position)

id_or_position = cast_position ? { position: cast_position - 1 } : { id: prev_id }

prev = acts_as_list_list.find_by(**id_or_position)

if prev.blank?

# If prev is blank, it means the element should be inserted at the first position.

insert_at

move_to_top

else

# There's a valid predecessor

insert_at(prev.position + 1)

end

end

@dombesz I considered this, but the Backlogs move_after has semantics this endpoint can't adopt: it mutates first (remove_from_list) and treats an unknown prev_id as "move to top", while these drop endpoints must reject an invalid or stale anchor with 422 and no mutation — that guarantee is also what keeps the cross-section move race-safe under the section row locks.

It's also coupled to the Backlogs domain (list scopes, update_columns position override), so it isn't reusable as-is.

So I would actually propose we do the opposite: keep Lists::MoveAfterAnchor as the canonical concern and then migrate Backlogs back onto it (albeit in a follow-up). It would also give us a chance to reevaluate its lenient fallback and support for the legacy position: param deserve their own consideration). Happy to create a work package.

added https://community.openproject.org/wp/AGILE-370

Copilot

Pull request overview

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

Copilot

Pull request overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.

Replaces the drag path's closest('li') source-row lookup and the item's parentElement assumption with one invariant: an item's owning list is the innermost list outlet containing it. Rows now work as any element, which admin BorderBox rows and table rows require. https://community.openproject.org/wp/DREAM-786

Adds a type-to-template map on the root so one root can host item types with different move endpoints, falling back to the single template. Menu moves read the type off the item element, so map-only roots serve the directional path too. https://community.openproject.org/wp/DREAM-786

The optimistic=true query flag suppresses the server's frame reload and belongs to Backlogs' event-only success contract. Admin surfaces reconcile through morph streams instead, so the root now sends the flag only when a consumer declares it; Backlogs opts in. https://community.openproject.org/wp/DREAM-786

Sortable-list moves arrive as a prev_id anchor rather than an absolute position. Adds the shared move-after-anchor primitive with strict validation: blank means top, and an unknown, out-of-scope, or self anchor is rejected without mutation. https://community.openproject.org/wp/DREAM-786

Custom fields inside a section are ordered by the attribute_order array rather than acts_as_list. Adds the anchor-based counterpart to add_to_order with the same strict semantics: blank anchors the head, an unresolvable anchor is rejected without mutation. https://community.openproject.org/wp/DREAM-786

Accepts the sortable-lists list_id/prev_id wire alongside the legacy target_id/position wire, which the user custom fields surface still sends until its own migration. Anchor moves validate before mutating and lock both section rows in id order against concurrent reorders. Reuses the single locked section object for the same-row case so every read and write goes through it, closing a lost-update window where a fresh, unlocked lookup of the target section could be read from and written to before the lock took effect. Runs the anchor move in a savepoint transaction so its rollback guard undoes a partially applied cross-section reparent even when nested inside a caller's own transaction. https://community.openproject.org/wp/DREAM-786

Converts both project-attribute drop endpoints from absolute positions to the sortable-lists list_type/list_id/prev_id wire with a strict contract: exact list type, list id only where a section is addressed, and a required scalar prev_id filtered through permit, so a collection id cannot reach the anchor lookup as an IN match. Violations and rejected anchors answer 422 without mutation. https://community.openproject.org/wp/DREAM-786

The optimistic client reorder must survive the server response, so the section update and sections replace streams switch to morph. Field row wrappers gain a per-field id: with every row sharing one wrapper id the morph matcher can associate the wrong rows on reorder. https://community.openproject.org/wp/DREAM-786

Replaces the page's Dragula wiring with the shared suite: one root with per-type move URL templates, the sections flex container and each BorderBox as typed lists, sections and fields as typed items with their existing drag handles. Server order reconciles through morph streams instead of the optimistic flag. https://community.openproject.org/wp/DREAM-786

The row lookup climbed ancestors unbounded, so an empty section's placeholder row resolved the enclosing section as its anchor instead of resolving to none. Dropping the first field into an empty section then sent the section's id as prev_id, and the server rejected it as an invalid anchor. Bounds the ancestor search to the row's own rows container, and adds a nested-topology regression test. https://community.openproject.org/wp/DREAM-786

Adds the Selenium feature spec for drag and menu reorders of sections and fields, including cross-section, empty-section, and post-morph drags. frontend/CONTEXT.md is not touched: it was deliberately removed from the tree pending a team decision (11d9a5c) that has not since been revisited, so the consumer-contract notes the task called for stay out of scope here. https://community.openproject.org/wp/DREAM-786

The optimistic anchor lookup queried all descendants, so an item in a nested inner list could shadow the intended anchor when ids from different tables collide. Matches the anchor against the target list's direct rows through the bounded row resolvers instead. https://community.openproject.org/wp/DREAM-786

Section rows previously fell back to the browser's native drag snapshot, which paints the box's top margin as whitespace and squares off the rounded corners on a white backing. Marks the BorderBox as the item's preview target and zeroes the clone's margin so spacing utilities never bleed into the preview.

Primer's DragHandle ships cursor: move. Scopes a grab/grabbing override to sortable-lists handles until the DragHandle itself adopts it upstream.

Copilot

Pull request overview

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

Suppressed comments (1)

spec/services/custom_fields/drop_service_spec.rb:148

Typo in comment: “un-reachable-in-practice” should be “unreachable-in-practice” (or similar).

bsatarnejad

LGTM 👍🏼

myabc requested a review from Copilot July 29, 2026 19:05

myabc added the DO NOT MERGE label Jul 29, 2026

Copilot started reviewing on behalf of myabc July 29, 2026 19:06 View session

Copilot AI reviewed Jul 29, 2026

View reviewed changes

Comment thread app/services/custom_fields/drop_service.rb

myabc force-pushed the implementation/DREAM-786-sortable-lists-admin-project-attributes branch from d4cb4b4 to 707d2f1 Compare July 29, 2026 20:13

myabc changed the title ~~Implementation/dream 786 sortable lists admin project attributes~~ [DREAM-786] Extend sortable-lists for admin surfaces and migrate project attributes Jul 29, 2026

myabc requested a review from Copilot July 29, 2026 22:16

Copilot started reviewing on behalf of myabc July 29, 2026 22:17 View session

Copilot AI reviewed Jul 29, 2026

View reviewed changes

dombesz reviewed Jul 30, 2026

View reviewed changes

myabc requested a review from Copilot August 1, 2026 12:00

Copilot started reviewing on behalf of myabc August 1, 2026 12:01 View session

Copilot AI reviewed Aug 1, 2026

View reviewed changes

myabc mentioned this pull request Aug 5, 2026

Model a scroll container in the drag-and-drop spec fixtures #24622

Open

3 tasks

myabc force-pushed the implementation/DREAM-786-sortable-lists-admin-project-attributes branch from e5c2971 to 58b8108 Compare August 5, 2026 19:58

myabc requested a lite review from Copilot August 5, 2026 19:58

Copilot started reviewing on behalf of myabc August 5, 2026 19:58 View session

Copilot AI reviewed Aug 5, 2026

View reviewed changes

Comment thread spec/components/settings/project_custom_field_sections/custom_field_row_component_spec.rb

myabc marked this pull request as ready for review August 5, 2026 23:05

myabc requested a review from bsatarnejad August 6, 2026 10:20

myabc added this to the 17.8.x milestone Aug 6, 2026

myabc force-pushed the implementation/DREAM-786-sortable-lists-admin-project-attributes branch from 58b8108 to 9118b5a Compare August 6, 2026 10:57

myabc added 7 commits August 6, 2026 13:51

Show grab cursors on sortable-lists drag handles

cef98a4

Primer's DragHandle ships cursor: move. Scopes a grab/grabbing override to sortable-lists handles until the DragHandle itself adopts it upstream.

myabc force-pushed the implementation/DREAM-786-sortable-lists-admin-project-attributes branch from 75afce9 to cef98a4 Compare August 6, 2026 11:51

myabc added maintenance styling javascript Pull requests that update Javascript code ruby Pull requests that update Ruby code needs review and removed DO NOT MERGE labels Aug 6, 2026

myabc requested review from dombesz and a lite review from Copilot August 6, 2026 12:16

Copilot started reviewing on behalf of myabc August 6, 2026 12:16 View session

Copilot AI reviewed Aug 6, 2026

View reviewed changes

bsatarnejad approved these changes Aug 6, 2026

View reviewed changes

myabc merged commit f055c63 into dev Aug 6, 2026
33 of 34 checks passed

myabc deleted the implementation/DREAM-786-sortable-lists-admin-project-attributes branch August 6, 2026 16:14

Navigation Menu

Uh oh!

[DREAM-786] Extend sortable-lists for admin surfaces and migrate project attributes - #24504

[DREAM-786] Extend sortable-lists for admin surfaces and migrate project attributes#24504
myabc merged 14 commits into
devfrom
implementation/DREAM-786-sortable-lists-admin-project-attributes

myabc commented Jul 29, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

github-actions Bot commented Jul 29, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

dombesz Jul 30, 2026

Uh oh!

myabc Aug 6, 2026

Uh oh!

myabc Aug 6, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

bsatarnejad left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

	def move_after(position: nil, prev_id: nil)
	# Remove so the potential 'prev' has a correct position
	remove_from_list
	reload
	cast_position = ActiveRecord::Type::Integer.new.cast(position)
	id_or_position = cast_position ? { position: cast_position - 1 } : { id: prev_id }

	prev = acts_as_list_list.find_by(**id_or_position)

	if prev.blank?
	# If prev is blank, it means the element should be inserted at the first position.
	insert_at
	move_to_top
	else
	# There's a valid predecessor
	insert_at(prev.position + 1)
	end
	end

Uh oh!

Conversation

myabc commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

What are you trying to accomplish?

What approach did you choose and why?

Screen recording

Merge checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

github-actions Bot commented Jul 29, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

dombesz Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

myabc Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

myabc Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

bsatarnejad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

myabc commented Jul 29, 2026 •

edited

Loading