Merge pull request #24504 from opf/implementation/DREAM-786-sortable-lists-admin-project-attributes

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

Author: myabc

app/components/settings/project_custom_field_sections/custom_field_row_component.html.erb

@@ -3,7 +3,7 @@
     flex_layout(justify_content: :space_between, align_items: :center) do |main_container|
       main_container.with_column(flex_layout: true, align_items: :center) do |content_container|
         content_container.with_column(mr: 2) do
-          render(Primer::OpenProject::DragHandle.new(classes: "handle"))
+          render(Primer::OpenProject::DragHandle.new(classes: "handle", data: { sortable_lists__item_target: "handle" }))
         end
         content_container.with_column(mr: 2) do
           render(

app/components/settings/project_custom_field_sections/custom_field_row_component.rb

@@ -45,6 +45,10 @@ def initialize(project_custom_field:, first:, last:)
 
       private
 
+      def wrapper_uniq_by
+        @project_custom_field.id
+      end
+
       def edit_action_item(menu)
         menu.with_item(label: t("label_edit"),
                        href: edit_admin_settings_project_custom_field_path(@project_custom_field),

app/components/settings/project_custom_field_sections/index_component.html.erb

@@ -1,10 +1,10 @@
 <%=
   component_wrapper(data: wrapper_data_attributes) do
     if @project_custom_field_sections.any?
-      flex_layout(classes: "dragula-container", data: { "allowed-drop-type": "section" }.merge(drop_target_config)) do |flex|
+      flex_layout(data: sections_list_data) do |flex|
         @project_custom_field_sections.each do |section|
           flex.with_row(
-            data: draggable_item_config(section)
+            data: section_item_data(section)
           ) do
             render(row_component_class.new(project_custom_field_section: section, first_and_last:))
           end

app/components/settings/project_custom_field_sections/index_component.rb

@@ -53,22 +53,37 @@ def first_and_last
 
       def wrapper_data_attributes
         {
-          controller: "generic-drag-and-drop"
+          controller: "sortable-lists",
+          sortable_lists_move_url_templates_value: move_url_templates.to_json,
+          sortable_lists_sortable_lists__list_outlet: "##{wrapper_key} [data-controller~='sortable-lists--list']",
+          sortable_lists_sortable_lists__item_outlet: "##{wrapper_key} [data-controller~='sortable-lists--item']"
         }
       end
 
-      def drop_target_config
+      # Built from route helpers with a sentinel so relative-URL-root
+      # installations keep working; {id} is expanded client-side.
+      def move_url_templates
+        id_placeholder = "__id__"
         {
-          generic_drag_and_drop_target: "container",
-          "target-allowed-drag-type": "section" # the type of dragged items which are allowed to be dropped in this target
+          section: drop_admin_settings_project_custom_field_section_path(id_placeholder).sub(id_placeholder, "{id}"),
+          custom_field: drop_admin_settings_project_custom_field_path(id_placeholder).sub(id_placeholder, "{id}")
         }
       end
 
-      def draggable_item_config(section)
+      def sections_list_data
         {
-          "draggable-id": section.id,
-          "draggable-type": "section",
-          "drop-url": drop_admin_settings_project_custom_field_section_path(section)
+          controller: "sortable-lists--list",
+          sortable_lists__list_type_value: "section",
+          sortable_lists__list_accepted_type_value: "section"
+        }
+      end
+
+      def section_item_data(section)
+        {
+          controller: "sortable-lists--item",
+          sortable_lists__item_id_value: section.id,
+          sortable_lists__item_type_value: "section",
+          sortable_lists__item_label_value: section.name
         }
       end
     end

app/components/settings/project_custom_field_sections/show_component.html.erb

@@ -1,11 +1,11 @@
 <%=
   component_wrapper(class: "op-project-custom-field-section-container", data: { test_selector: "project-custom-field-section-container-#{@project_custom_field_section.id}" }) do
-    render(border_box_container(mt: 3, data: drag_and_drop_target_config)) do |component|
+    render(border_box_container(mt: 3, data: field_list_data)) do |component|
       component.with_header(font_weight: :bold) do
         flex_layout(justify_content: :space_between, align_items: :center) do |section_header_container|
           section_header_container.with_column(flex_layout: true, align_items: :center) do |content_container|
             content_container.with_column(mr: 2) do
-              render(Primer::OpenProject::DragHandle.new(classes: "handle"))
+              render(Primer::OpenProject::DragHandle.new(classes: "handle", data: { sortable_lists__item_target: "handle" }))
             end
             content_container.with_column do
               render(Primer::Beta::Text.new(font_weight: :bold)) do
@@ -120,7 +120,7 @@
         end
       else
         @ordered_cfs.each_with_index do |cf, index|
-          component.with_row(data: draggable_item_config(cf)) do
+          component.with_row(data: field_item_data(cf)) do
             render(
               custom_field_row_component_class.new(
                 project_custom_field: cf,

app/components/settings/project_custom_field_sections/show_component.rb

@@ -54,20 +54,26 @@ def wrapper_uniq_by
         @project_custom_field_section.id
       end
 
-      def drag_and_drop_target_config
+      def field_list_data
         {
-          generic_drag_and_drop_target: "container",
-          "target-container-accessor": ".Box > ul",
-          "target-id": @project_custom_field_section.id,
-          "target-allowed-drag-type": "custom-field"
+          controller: "sortable-lists--list",
+          sortable_lists__list_type_value: "custom_field",
+          sortable_lists__list_accepted_type_value: "custom_field",
+          sortable_lists__list_id_value: @project_custom_field_section.id,
+          sortable_lists__list_name_value: @project_custom_field_section.name,
+          # The section's drag preview snapshots the box itself rather than
+          # the browser's native capture of the row wrapper, which paints the
+          # box's top margin and squares off the rounded corners.
+          sortable_lists__item_target: "preview"
         }
       end
 
-      def draggable_item_config(project_custom_field)
+      def field_item_data(project_custom_field)
         {
-          "draggable-id": project_custom_field.id,
-          "draggable-type": "custom-field",
-          "drop-url": drop_admin_settings_project_custom_field_path(project_custom_field)
+          controller: "sortable-lists--item",
+          sortable_lists__item_id_value: project_custom_field.id,
+          sortable_lists__item_type_value: "custom_field",
+          sortable_lists__item_label_value: project_custom_field.name
         }
       end
 

app/controllers/admin/settings/project_custom_field_sections_controller.rb

@@ -95,17 +95,17 @@ def move
     end
 
     def drop
-      call = ::ProjectCustomFieldSections::UpdateService.new(user: current_user, model: @project_custom_field_section).call(
-        position: params[:position].to_i
-      )
+      moved = valid_drop_request? &&
+        @project_custom_field_section.move_after_anchor(drop_params[:prev_id], scope: ProjectCustomFieldSection.all)
 
-      if call.success?
+      if moved
         update_header_via_turbo_stream(allow_custom_field_creation: allow_custom_field_creation?)
         update_sections_via_turbo_stream(project_custom_field_sections: ProjectCustomFieldSection.all)
+        respond_with_turbo_streams
       else
-        render_section_error_via_turbo_stream(call)
+        render_error_flash_message_via_turbo_stream(message: I18n.t(:error_invalid_list_move_anchor))
+        respond_with_turbo_streams(status: :unprocessable_entity)
       end
-      respond_with_turbo_streams
     end
 
     def new_link
@@ -126,6 +126,26 @@ def set_project_custom_field_section
       @project_custom_field_section = ProjectCustomFieldSection.find(params[:id])
     end
 
+    # The sortable-lists wire for the one global sections list: the type must
+    # match and no list id may be addressed. prev_id must be present as a
+    # scalar parameter (blank means top): an accidentally omitted anchor
+    # cannot read as a move-to-top request, and a collection-valued id
+    # (prev_id[]=...) cannot reach the anchor lookup as an IN list. The
+    # raw list_id check stays deliberately: permit cannot distinguish an
+    # absent list_id from a filtered-out collection one, and both a
+    # nonblank and a collection value are contract violations here.
+    def valid_drop_request?
+      drop_params[:list_type] == "section" &&
+        params[:list_id].blank? &&
+        drop_params.key?(:prev_id)
+    end
+
+    # permit's scalar filter drops collection-valued parameters, so a
+    # missing and a non-scalar prev_id both fail the key check above.
+    def drop_params
+      @drop_params ||= params.permit(:list_type, :list_id, :prev_id)
+    end
+
     def allow_custom_field_creation?
       ProjectCustomFieldSection.any?
     end

app/controllers/admin/settings/project_custom_fields_controller.rb

@@ -159,20 +159,20 @@ def move
     end
 
     def drop
+      return render_invalid_drop_request unless valid_drop_request?
+
       result = CustomFields::DropService.new(user: current_user, custom_field: @custom_field).call(
-        target_id: params[:target_id],
-        position: params[:position]
+        list_id: drop_params[:list_id],
+        prev_id: drop_params[:prev_id]
       )
 
       if result.success?
         drop_success_streams(result)
+        respond_with_turbo_streams
       else
-        render_error_flash_message_via_turbo_stream(
-          message: join_flash_messages(result.errors)
-        )
+        render_error_flash_message_via_turbo_stream(message: join_flash_messages(result.errors))
+        respond_with_turbo_streams(status: :unprocessable_entity)
       end
-
-      respond_with_turbo_streams
     end
 
     def destroy
@@ -265,6 +265,26 @@ def find_custom_field
       @custom_field = ProjectCustomField.find(params.expect(:id))
     end
 
+    # Ids must be scalar strings: a collection-valued list_id would pick an
+    # arbitrary target section out of an IN lookup, and a collection-valued
+    # prev_id would 500 instead of answering the promised 422. permit's
+    # scalar filter drops collection values, so the presence checks below
+    # reject them alongside genuinely missing parameters.
+    def valid_drop_request?
+      drop_params[:list_type] == "custom_field" &&
+        drop_params[:list_id].present? &&
+        drop_params.key?(:prev_id)
+    end
+
+    def drop_params
+      @drop_params ||= params.permit(:list_type, :list_id, :prev_id)
+    end
+
+    def render_invalid_drop_request
+      render_error_flash_message_via_turbo_stream(message: I18n.t(:error_invalid_list_move_anchor))
+      respond_with_turbo_streams(status: :unprocessable_entity)
+    end
+
     def drop_success_streams(call)
       update_section_via_turbo_stream(project_custom_field_section: call.result[:current_section])
       if call.result[:section_changed]

app/controllers/concerns/admin/settings/project_custom_fields/component_streams.rb

@@ -50,7 +50,8 @@ def update_section_via_turbo_stream(project_custom_field_section:)
                 # a single custom field section, and not a list of sections. Calling first?
                 # and last? method in the component will not result in an N+1 in this case.
                 project_custom_field_section:
-              )
+              ),
+              method: :morph
             )
           end
 
@@ -66,7 +67,8 @@ def update_sections_via_turbo_stream(project_custom_field_sections:)
             replace_via_turbo_stream(
               component: ::Settings::ProjectCustomFieldSections::IndexComponent.new(
                 project_custom_field_sections:
-              )
+              ),
+              method: :morph
             )
           end
         end

app/models/concerns/lists/move_after_anchor.rb

@@ -0,0 +1,57 @@
+# frozen_string_literal: true
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See COPYRIGHT and LICENSE files for more details.
+#++
+
+module Lists
+  # Anchor-based reordering for acts_as_list models: moves the record
+  # directly below another record of the same list, addressed by id.
+  module MoveAfterAnchor
+    # Moves the record below the record identified by `prev_id` within
+    # `scope` (a relation over the same acts_as_list list). A blank
+    # `prev_id` moves the record to the top.
+    #
+    # 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
+      if prev_id.blank?
+        move_to_top
+        return true
+      end
+
+      anchor = scope.find_by(id: prev_id)
+      return false if anchor.nil? || anchor.id == id
+
+      # Removing the record first shifts the anchor up by one when the
+      # record currently sits above it, so the target slot differs by
+      # direction of travel.
+      insert_at(position > anchor.position ? anchor.position + 1 : anchor.position)
+      true
+    end
+  end
+end