Conversation
The sortable-lists drop endpoint addresses a target by the id of the record it lands behind, so the three enumeration lists need the shared anchor concern and a list type that cannot be confused across classes. https://community.openproject.org/wp/DREAM-789
The enumeration lists kept their own drag controller, position-based drops and server-rendered move forms. They join the shared stack: one anchor-only endpoint with strict parameter guards serves drag and menu moves, and its response morphs the list. The endpoint changes together with its consumers so that no commit leaves the lists unable to reorder. https://community.openproject.org/wp/DREAM-789
A second drag after a completed morph is what catches stale Pragmatic registrations; a second menu move alone passes while they are broken. The single-item examples guard the menu grouping against a duplicate. https://community.openproject.org/wp/DREAM-789
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The cross-cutting changes span multiple modules and require final human review.
Pull request overview
Migrates work package priorities, time-entry activities, and document types to the shared sortable-list reordering system.
Changes:
- Adds anchor-based moves with strict validation and Turbo morph responses.
- Replaces legacy drag-and-drop and move forms.
- Adds model, request, component, and feature coverage.
File summaries
| File | Description |
|---|---|
spec/support/shared/enumeration_anchor_move.rb |
Shared endpoint validation coverage |
spec/requests/admin/settings/work_package_priorities_spec.rb |
Priority move request coverage |
spec/models/enumeration/enumeration_sortable_list_type_spec.rb |
Sortable type and anchor behavior |
spec/features/admin/settings/work_package_priorities_spec.rb |
Priority drag/menu flows |
spec/components/admin/enumerations/item_component_spec.rb |
Enumeration move menu coverage |
spec/components/admin/enumerations/index_component_spec.rb |
Sortable wiring coverage |
modules/documents/spec/requests/documents/admin/settings/document_types_spec.rb |
Document type request coverage |
modules/documents/spec/features/documents/admin/settings/document_types_spec.rb |
Document type UI flows |
modules/documents/spec/components/documents/admin/document_types/index_component_spec.rb |
Document sortable wiring |
modules/documents/app/models/document_type.rb |
Enables anchor movement and sortable type |
modules/documents/app/components/documents/admin/document_types/item_component.html.erb |
Adds sortable handle |
modules/documents/app/components/documents/admin/document_types/index_component.html.erb |
Adds sortable list data |
modules/costs/spec/requests/admin/settings/time_entry_activities_spec.rb |
Activity move request coverage |
modules/costs/spec/features/admin/settings/time_entry_activities_spec.rb |
Activity menu flow |
modules/costs/app/models/time_entry_activity.rb |
Adds sortable list type |
app/models/issue_priority.rb |
Adds sortable list type |
app/models/enumeration.rb |
Enables anchor movement |
app/controllers/admin/settings/enumerations_controller_base.rb |
Validates moves and renders morph responses |
app/components/admin/enumerations/item_component.rb |
Provides the shared move menu |
app/components/admin/enumerations/item_component.html.erb |
Adds sortable handle |
app/components/admin/enumerations/index_component.rb |
Configures sortable controllers |
app/components/admin/enumerations/index_component.html.erb |
Provides sortable list and item markup |
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
3 tasks
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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/DREAM-789
What are you trying to accomplish?
Enumeration slice of DREAM-789: work package priorities, time-entry activities and document types move from the legacy
generic-drag-and-dropcontroller and server-renderedmove_toforms onto the shared sortable-lists stack.Those three lists were the last admin surfaces posting a direction or a raw position to reorder. That endpoint could not express a drop anchor, re-rendered the whole list on every move, and let array or hash parameters slip past
blank?and read as "move to top". The menu also rendered its available directions server-side, so it went stale the moment the user reordered anything.Stacked on #25464 (DREAM-775), which adds the shared move-menu helper these lists now use; this PR should be reviewed and merged after that one.
What approach did you choose and why?
EnumerationandDocumentTypeincludeLists::MoveAfterAnchor, and each concrete class carries its ownSORTABLE_LIST_TYPEderived frommodel_name.param_key. A constant on the STI base would be visible from every subclass and would let a priority anchor against an activity, so the value is defined per concrete class.Admin::Settings::EnumerationsControllerBase#moveis now anchor-only, guarded exactly asStatusesController#valid_drop_request?is: the permitted parameters must carry the rightlist_typeand aprev_idkey, the rawlist_idmust be blank, and a suppliedlist_idmust have survived scalar permitting. An empty array or an empty hash sent as a JSONlist_idpassesblank?but is dropped entirely bypermit, the same trap the statuses endpoint guards against, so that case is rejected too rather than silently read as "move to top". A rejected request answers 422 witherror_invalid_list_move_anchorand mutates nothing. A successful one morphs the index component rather than replacing it, which is what DREAM-789 asks for and is deliberately not Roles'204 No Content. It is also deliberately not whatStatusesController#movedoes: statuses kept their server-sidemove_tobranch because their list is paginated and a direction can cross a page boundary, whereas these lists are unpaginated, so the work package's "retire the move-menu persistence paths" applies andmove_toandpositionare deleted outright. The consequence is that reordering now needs JavaScript, as it already does for roles.A successful move renders the list morph stream before the success flash stream, so a visible flash is a reliable signal that the list has already been reconciled with the new order. The feature specs wait on that flash and on the busy attribute clearing before acting again, instead of querying the database, since optimistic DOM order and persisted order can both look right before the response has actually landed.
Cross-class anchors are exercised in the shared request example only between priorities and time-entry activities, because those two share the
enumerationstable and therefore one ID space. Document types have their own table and their own ID space, so a priority's numeric ID can coincide with a perfectly valid document-type anchor; a cross-class rejection test would be non-deterministic there, and document types rely on the absent-ID and wrong-list-type cases instead.The index component mirrors
Admin::TextTransformActions::IndexComponent: root controller and move-URL template on the wrapper, list data on the border box, item data per row. Hosting the root controller on the component wrapper is the non-obvious part: anupdatestream renders only the wrapper's inner HTML, so the root element and its controller instance survive every morph by construction, while the list and item elements inside are morphed and re-registered by the root'sturbo:morph-elementlistener. The URL template keeps the__id__sentinel so relative-URL-root installations keep working, and stays controller-relative because three controllers share the component. The rows container needs no override: both the border box list andborder_box_containerrender a Primer BorderBox, whose rows sit in a directulchild, which is the list controller's default.In the item menu, Move shares Edit's group instead of getting one of its own.
with_item_grouponly drops an empty group at server render, while the item controller hides the Move submenu client-side from live DOM position, so a separate group would leave two adjacent separators on a single-item list. With this grouping exactly one separator renders in every state, and nomoveDividertarget is needed.Document types keep their two-column grid and its sass; only the data attributes change. The table migration is DREAM-840, which stacks on this.
DREAM-789 stays open. Covered here: priorities, time-entry activities and document types reorder by drag and by menu; the drop endpoint is anchor-only and rejects malformed parameters; the update boundary morphs. Not covered: user custom field sections, project phase definitions and PDF export templates.
AI involvement
Merge checklist