[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 now be any element — an item's owning list is the innermost list outlet containing it (replaces the closest('li') assumption, unblocks <tr>/BorderBox rows); a per-type move-URL template map on the root 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 instead).
Backend: anchor-based (prev_id) ordering primitives with strict validation — blank anchor means top, an unknown/out-of-scope/self anchor is rejected with zero mutation.
Both project-attribute drop endpoints convert in-place to the list_type/list_id/prev_id wire with a uniform 422 contract (an omitted prev_id parameter is a 422, never an implicit move-to-top).
Server responses switch to method="morph" turbo streams, and field rows gain unique per-field wrapper IDs so the morph matcher can associate rows correctly on reorder.
En route, the new feature spec exposed a real suite bug: the row lookup climbed ancestors unbounded, so dropping into an empty section resolved the enclosing section itself as the prev_id anchor (server 422). The lookup is now bounded to the row's own list.

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. The sections controller calls move_after_anchor directly — ProjectCustomFieldSections::UpdateService is no longer in the drop path (it carried no validation or side effects relevant to positioning; worth knowing if validation is ever added there).
CustomFields::DropService keeps a legacy adapter path. The user-custom-fields surface still sends the old target_id/position wire until https://community.openproject.org/wp/DREAM-790, so the service accepts either wire; the legacy path is pinned by a regression spec and dies with DREAM-790.
Concurrency: cross-section field moves are transactional and lock both section rows in deterministic id order before reading attribute_order (it is a read-modify-write array, so the lock prevents concurrent admins losing each other's updates). The acts_as_list side has no equivalent lock — position shifts happen in single SQL statements, so the worst concurrent case is a surprising order, not corruption.
In-place endpoint conversion, no dual-wire period — the page migrates in the same PR, so the old absolute-position wire has no remaining callers.
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).

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 (frontend/CONTEXT.md was removed repo-wide pending a wider discussion).
The Selenium drag helpers (selenium_target_point, honey-pot cleanup) are now 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, ...)

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.

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.

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.

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.

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.

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.

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 prev_id parameter. Violations and rejected anchors answer 422 without mutation.

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.

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.

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.

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.

Blankness and key checks let array or hash ids through to the anchor lookup, where a collection prev_id reordered against an arbitrary IN match on sections and raised on fields, breaking the uniform-422 contract. Filters the ids through permit, whose scalar filter drops collection values alongside missing ones.

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.

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

Copilot

Pull request overview

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

myabc added 3 commits July 29, 2026 19:18

[DREAM-786] Resolve move URLs per item type

96adcf2

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.

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 added 8 commits July 29, 2026 22:04

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 added 2 commits July 29, 2026 22:26

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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 wants to merge 13 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!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 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?

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

myabc commented Jul 29, 2026 •

edited

Loading