From 1e69dc42ee27afca472cdcaf0fa3403080f57782 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Wed, 30 Sep 2026 20:43:11 +0100 Subject: [PATCH 1/3] [DREAM-791] Forward drag handle arguments Adds a drag_handle_arguments option to the Border Box list header and forwards it to the Primer drag handle, so sortable lists can mark the header handle as their drag target without wrapping the header. https://community.openproject.org/wp/DREAM-791 --- .../open_project/common/border_box_list_component.rb | 3 ++- .../common/border_box_list_component/header.html.erb | 2 +- .../common/border_box_list_component/header.rb | 6 +++++- .../with_header_drag_handle.html.erb | 7 ++++++- .../common/border_box_list_component_spec.rb | 11 +++++++++++ 5 files changed, 25 insertions(+), 4 deletions(-) diff --git a/app/components/open_project/common/border_box_list_component.rb b/app/components/open_project/common/border_box_list_component.rb index d9fddea0e292..36a405d078d4 100644 --- a/app/components/open_project/common/border_box_list_component.rb +++ b/app/components/open_project/common/border_box_list_component.rb @@ -59,10 +59,11 @@ class BorderBoxListComponent < ApplicationComponent # # @param title [String] header title. # # @param show_drag_handle [Boolean] whether the header renders a # # leading drag handle. + # # @param drag_handle_arguments [Hash] forwarded to `Primer::OpenProject::DragHandle`. # # @param system_arguments [Hash] forwarded to {Header}. List wiring # # arguments are supplied internally. # # @return [ViewComponent::Slot] - # def with_header(title: nil, show_drag_handle: false, **system_arguments, &block) + # def with_header(title: nil, show_drag_handle: false, drag_handle_arguments: {}, **system_arguments, &block) # end renders_one :header, ->(**system_arguments) { system_arguments = system_arguments.except(:id, :list_id) diff --git a/app/components/open_project/common/border_box_list_component/header.html.erb b/app/components/open_project/common/border_box_list_component/header.html.erb index c20234d914ac..a9dc6936b85e 100644 --- a/app/components/open_project/common/border_box_list_component/header.html.erb +++ b/app/components/open_project/common/border_box_list_component/header.html.erb @@ -37,7 +37,7 @@ See COPYRIGHT and LICENSE files for more details. ) do |grid| %> <% if show_drag_handle? %> <% grid.with_area(:drag_handle, classes: "hide-when-print") do %> - <%= render(Primer::OpenProject::DragHandle.new) %> + <%= render(Primer::OpenProject::DragHandle.new(**drag_handle_arguments)) %> <% end %> <% end %> diff --git a/app/components/open_project/common/border_box_list_component/header.rb b/app/components/open_project/common/border_box_list_component/header.rb index 1ef4a1e948d7..9b1382fc9a4d 100644 --- a/app/components/open_project/common/border_box_list_component/header.rb +++ b/app/components/open_project/common/border_box_list_component/header.rb @@ -150,7 +150,8 @@ class Header < ApplicationComponent :interactive, :collapsed, :collapsible, - :show_drag_handle + :show_drag_handle, + :drag_handle_arguments alias_method :show_drag_handle?, :show_drag_handle @@ -177,6 +178,7 @@ class Header < ApplicationComponent # with a toggle button. # @param show_drag_handle [Boolean] whether the header renders a leading # drag handle. Defaults to `false`. + # @param drag_handle_arguments [Hash] forwarded to `Primer::OpenProject::DragHandle`. # @param system_arguments [Hash] forwarded to `Primer::Beta::BorderBox#with_header`. def initialize( title: nil, @@ -190,6 +192,7 @@ def initialize( collapsed: false, collapsible: false, show_drag_handle: false, + drag_handle_arguments: {}, **system_arguments ) super() @@ -206,6 +209,7 @@ def initialize( @collapsed = collapsed @collapsible = collapsible @show_drag_handle = show_drag_handle + @drag_handle_arguments = drag_handle_arguments @system_arguments = system_arguments end diff --git a/lookbook/previews/open_project/common/border_box_list_component_preview/with_header_drag_handle.html.erb b/lookbook/previews/open_project/common/border_box_list_component_preview/with_header_drag_handle.html.erb index 4f6c495bd335..10c66eed5999 100644 --- a/lookbook/previews/open_project/common/border_box_list_component_preview/with_header_drag_handle.html.erb +++ b/lookbook/previews/open_project/common/border_box_list_component_preview/with_header_drag_handle.html.erb @@ -4,7 +4,12 @@ header_padding:, collapsible: ) do |list| %> - <% list.with_header(title: "Reorderable section", count: true, show_drag_handle: true) %> + <% list.with_header( + title: "Reorderable section", + count: true, + show_drag_handle: true, + drag_handle_arguments: { data: { sortable_lists__item_target: "handle" } } + ) %> <% list.with_item do %> <%= render(Primer::OpenProject::FlexLayout.new(align_items: :center, ml: -2)) do |row| %> <% row.with_column(style: "width: 1.5rem") { render(Primer::OpenProject::DragHandle.new) } %> diff --git a/spec/components/open_project/common/border_box_list_component_spec.rb b/spec/components/open_project/common/border_box_list_component_spec.rb index 3ef267a27c0e..2d9fc49c2515 100644 --- a/spec/components/open_project/common/border_box_list_component_spec.rb +++ b/spec/components/open_project/common/border_box_list_component_spec.rb @@ -1323,4 +1323,15 @@ def template_placeholder(rendered) expect(page).to have_no_css("ul [data-empty-list-item]") end end + + it "forwards drag handle arguments to the handle only" do + rendered = render_inline(described_class.new(container: "sortable-header")) do |list| + list.with_header(title: "Types", show_drag_handle: true, + drag_handle_arguments: { data: { sortable_lists__item_target: "handle" } }) + end + + expect(rendered).to have_css(".DragHandle[data-sortable-lists--item-target='handle']") + expect(rendered).to have_no_css(".Box-header[data-sortable-lists--item-target]") + expect(rendered).to have_no_css("[drag_handle_arguments]") + end end From 7f30c1b04a9d37b937abc062eeb4e8d9bcead8d0 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Wed, 30 Sep 2026 20:50:28 +0100 Subject: [PATCH 2/3] [DREAM-791] Migrate type drag to sortable lists Replaces the generic drag-and-drop wiring on the type index with sortable lists. Drops are validated relative anchors; a blank anchor places the type at the start of the current page, so dragging on later pages stays on that page. The list morphs in place and keeps page and expansion context in its drop URL and pagination links. A lone type is fixed in place. Moves the type index feature specs onto the page object, which finds groups by exact title. https://community.openproject.org/wp/DREAM-791 --- .../types/grouped_list_component.html.erb | 22 ++- .../types/grouped_list_component.rb | 35 +++- .../work_package_types/types_controller.rb | 61 ++++++- app/models/type.rb | 2 + .../work_package_types/types/index.html.erb | 6 +- .../types/grouped_list_component_spec.rb | 73 ++++++++ .../types_controller_spec.rb | 4 +- spec/features/types/deletion_spec.rb | 6 +- spec/features/types/type_ordering_spec.rb | 81 +++++++++ .../features/types/variant_comparison_spec.rb | 13 +- spec/features/types/variants_index_spec.rb | 52 +++--- .../work_package_types/type_ordering_spec.rb | 172 ++++++++++++++++++ spec/support/pages/types/index.rb | 71 ++++++-- 13 files changed, 515 insertions(+), 83 deletions(-) create mode 100644 spec/features/types/type_ordering_spec.rb create mode 100644 spec/requests/work_package_types/type_ordering_spec.rb diff --git a/app/components/work_package_types/types/grouped_list_component.html.erb b/app/components/work_package_types/types/grouped_list_component.html.erb index b6fb54845e36..ca36c289e3b5 100644 --- a/app/components/work_package_types/types/grouped_list_component.html.erb +++ b/app/components/work_package_types/types/grouped_list_component.html.erb @@ -27,10 +27,19 @@ See COPYRIGHT and LICENSE files for more details. ++#%> -<%= component_wrapper(data: { controller: "generic-drag-and-drop" }) do %> - <%= flex_layout(data: drop_target_config) do |container| %> +<%= component_wrapper(data: root_data) do %> + <%= flex_layout( + role: :list, + aria: { label: t(:label_type_plural) }, + data: drop_target_config + ) do |container| %> <% types.each do |root| %> - <% container.with_row(mt: 3, data: draggable_item_config(root)) do %> + <% container.with_row( + id: "sortable-type-#{root.id}", + role: :listitem, + mt: 3, + data: draggable_item_config(root) + ) do %> <%= render( OpenProject::Common::BorderBoxListComponent.new( container: "op-types-group-#{root.id}", @@ -41,6 +50,7 @@ See COPYRIGHT and LICENSE files for more details. <% list.with_header( collapsed: collapsed?(root), show_drag_handle: reorderable?(root), + drag_handle_arguments: { data: { sortable_lists__item_target: "handle" } }, count: variants_count(root) ) do |header| %> <% header.with_title do %> @@ -97,6 +107,8 @@ See COPYRIGHT and LICENSE files for more details. <% end %> <% end %> <% end %> + <%= helpers.pagination_links_full( + types, + params: { controller: "/work_package_types/types", action: "index", id: nil, **context_args } + ) %> <% end %> - -<%= helpers.pagination_links_full(types) %> diff --git a/app/components/work_package_types/types/grouped_list_component.rb b/app/components/work_package_types/types/grouped_list_component.rb index 26408b3670f2..6b05dafd4b94 100644 --- a/app/components/work_package_types/types/grouped_list_component.rb +++ b/app/components/work_package_types/types/grouped_list_component.rb @@ -35,16 +35,17 @@ class GroupedListComponent < ApplicationComponent include OpTurbo::Streamable include WorkPackageTypes::VariantRoutes - def initialize(types:, expanded_type_id: nil) + def initialize(types:, expanded_type_id: nil, page_args: {}) super() @types = types @expanded_type_id = expanded_type_id + @page_args = page_args.presence || { page: types.current_page, per_page: types.per_page } end private - attr_reader :types, :expanded_type_id + attr_reader :types, :expanded_type_id, :page_args def collapsed?(root) root.id != expanded_type_id @@ -104,19 +105,37 @@ def reorderable?(type) !(type.first? && type.last?) end + def context_args + page_args.merge(expand: expanded_type_id).compact + end + + def root_data + { + controller: "sortable-lists", + sortable_lists_move_url_template_value: drop_type_path("__id__", **context_args).sub("__id__", "{id}"), + 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 { - generic_drag_and_drop_target: "container", - "target-allowed-drag-type": "work-package-type" + controller: "sortable-lists--list", + sortable_lists__list_type_value: ::Type.model_name.param_key, + sortable_lists__list_accepted_type_value: ::Type.model_name.param_key, + sortable_lists__list_name_value: t(:label_type_plural) } end def draggable_item_config(root) { - "draggable-type": "work-package-type", - "draggable-id": root.id, - "drop-url": drop_type_path(root) - } + controller: "sortable-lists--item", + sortable_lists__item_target: "preview", + sortable_lists__item_id_value: root.id, + sortable_lists__item_type_value: ::Type.model_name.param_key, + sortable_lists__item_label_value: root.name, + sortable_lists__item_mobility_value: ("fixed" unless reorderable?(root)) + }.compact end end end diff --git a/app/controllers/work_package_types/types_controller.rb b/app/controllers/work_package_types/types_controller.rb index 1b5e87d94d3d..8a72112b73bd 100644 --- a/app/controllers/work_package_types/types_controller.rb +++ b/app/controllers/work_package_types/types_controller.rb @@ -43,7 +43,8 @@ class TypesController < ApplicationController end def index - @expanded_type_id = params[:expand].presence&.to_i + @expanded_type_id = expanded_type_id + @page_args = page_args @types = types_for_index end @@ -93,12 +94,7 @@ def duplicate end def drop - unless @type.update(params.permit(:position)) - render_error_flash_message_via_turbo_stream(message: @type.errors.full_messages.to_sentence) - end - - update_via_turbo_stream(component: Types::GroupedListComponent.new(types: types_for_index)) - respond_to_with_turbo_streams + render_ordering_result(move_after_anchor, error_message: I18n.t(:error_invalid_list_move_anchor)) end def menu @@ -108,7 +104,7 @@ def menu protected def find_type - @type = ::Type.find(params[:id]) + @type = ::Type.find(params.expect(:id)) end def types_for_index @@ -158,5 +154,54 @@ def belonging_wps_url(type_id) def archived_projects @archived_projects ||= @type.projects.archived end + + private + + def page_args + { page: page_param, per_page: per_page_param } + end + + def expanded_type_id + Integer(params[:expand].to_s, exception: false) + end + + def ordering_component + Types::GroupedListComponent.new(types: types_for_index, page_args:, expanded_type_id:) + end + + def render_ordering_result(moved, error_message:) + if moved + update_via_turbo_stream(component: ordering_component, method: :morph) + render_success_flash_message_via_turbo_stream(message: I18n.t(:notice_successful_update)) + else + render_error_flash_message_via_turbo_stream(message: error_message) + end + respond_with_turbo_streams(status: moved ? :ok : :unprocessable_entity) + end + + def valid_drop_request? + params[:list_type] == ::Type.model_name.param_key && + (params[:list_id].nil? || params[:list_id] == "") && + params.key?(:prev_id) + end + + def move_after_anchor + return false unless valid_drop_request? + + predecessor = params[:prev_id] + if predecessor.nil? || predecessor == "" + move_to_page_start + else + @type.move_after_anchor(predecessor, scope: ::Type.all) + end + end + + def move_to_page_start + current_page = ::Type.page(page_param).per_page(per_page_param) + return false if current_page.empty? + + predecessor = ::Type.offset(current_page.offset - 1).pick(:id) if current_page.offset.positive? + @type.move_after_anchor(predecessor, scope: ::Type.all) + end end end diff --git a/app/models/type.rb b/app/models/type.rb index e07ff0623e6f..d720b5f610bb 100644 --- a/app/models/type.rb +++ b/app/models/type.rb @@ -45,6 +45,8 @@ class Type < ApplicationRecord has_many :project_types, dependent: :delete_all has_many :projects, through: :project_types + include Lists::MoveAfterAnchor + acts_as_list validates :name, diff --git a/app/views/work_package_types/types/index.html.erb b/app/views/work_package_types/types/index.html.erb index b398452c9a0f..f1512d095172 100644 --- a/app/views/work_package_types/types/index.html.erb +++ b/app/views/work_package_types/types/index.html.erb @@ -70,7 +70,11 @@ See COPYRIGHT and LICENSE files for more details. %> <% if @types.any? %> - <%= render WorkPackageTypes::Types::GroupedListComponent.new(types: @types, expanded_type_id: @expanded_type_id) %> + <%= render WorkPackageTypes::Types::GroupedListComponent.new( + types: @types, + expanded_type_id: @expanded_type_id, + page_args: @page_args + ) %> <% else %> <%= no_results_box( diff --git a/spec/components/work_package_types/types/grouped_list_component_spec.rb b/spec/components/work_package_types/types/grouped_list_component_spec.rb index 977a2623f621..e233b7c54a0b 100644 --- a/spec/components/work_package_types/types/grouped_list_component_spec.rb +++ b/spec/components/work_package_types/types/grouped_list_component_spec.rb @@ -164,4 +164,77 @@ end end end + + describe "sortable groups", with_settings: { per_page_options: "2,100" } do + let!(:types) { %w[A B C D E].map { |name| create(:type, name:) } } + let(:page_two) { Type.order(:position).page(2).per_page(2) } + let(:expanded) { page_two.first } + let!(:variant) { create(:type_variant, type: expanded, variant_name: "Variant") } + let(:wrapper) { described_class.wrapper_key } + + subject(:rendered_component) do + with_request_url "/types?page=2&per_page=2" do + render_inline(described_class.new(types: page_two, expanded_type_id: expanded.id)) + end + end + + it_behaves_like "a sortable-lists list", list_type: "type", name: "Types" + + it "wires the component wrapper as the sortable-lists root", :aggregate_failures do + expect(rendered_component).to have_element(id: wrapper) do |root| + expect(root["data-controller"]).to eq("sortable-lists") + expect(root["data-sortable-lists-sortable-lists--list-outlet"]) + .to eq("##{wrapper} [data-controller~='sortable-lists--list']") + expect(root["data-sortable-lists-sortable-lists--item-outlet"]) + .to eq("##{wrapper} [data-controller~='sortable-lists--item']") + end + end + + it "lists the types of the page by name" do + expect(rendered_component).to have_selector(:list, "Types") do |list| + expect(list.all(:heading).map { it.text.squish }).to eq(page_two.map(&:name)) + end + end + + it "registers type groups, not variant rows, as sortable items", :aggregate_failures do + expect(rendered_component) + .to have_element(role: "listitem", "data-controller": "sortable-lists--item", count: 2) + expect(rendered_component).to have_no_element(:li, "data-controller": "sortable-lists--item") + end + + it "gives each type group a single drag handle as its item handle" do + expect(rendered_component).to have_button(accessible_name: "Drag to reorder", count: 2) do |handle| + handle["data-sortable-lists--item-target"] == "handle" + end + end + + it "carries page context through the drop URL" do + expected = drop_type_path("__id__", page: 2, per_page: 2, expand: expanded.id).sub("__id__", "{id}") + + expect(rendered_component).to have_element(id: wrapper) do |root| + expect(root["data-sortable-lists-move-url-template-value"]).to eq(expected) + end + end + + it "carries page context through the pagination links" do + expect(rendered_component).to have_link("1", href: types_path(page: 1, per_page: 2, expand: expanded.id)) + end + end + + describe "a lone type" do + let!(:lone) { create(:type, name: "Only") } + + subject(:rendered_component) do + with_request_url "/types" do + render_inline(described_class.new(types: Type.page(1).per_page(10))) + end + end + + it "fixes a lone type in place without a drag handle", :aggregate_failures do + expect(Type.count).to eq(1) + expect(rendered_component) + .to have_element(role: "listitem", "data-sortable-lists--item-mobility-value": "fixed", count: 1) + expect(rendered_component).to have_no_button(accessible_name: "Drag to reorder") + end + end end diff --git a/spec/controllers/work_package_types/types_controller_spec.rb b/spec/controllers/work_package_types/types_controller_spec.rb index 4100dcae9946..4bd86fe8d37e 100644 --- a/spec/controllers/work_package_types/types_controller_spec.rb +++ b/spec/controllers/work_package_types/types_controller_spec.rb @@ -228,10 +228,10 @@ let!(:first_type) { create(:type, name: "First") } let!(:second_type) { create(:type, name: "Second") } - it "reorders the dropped type to the given position" do + it "reorders the dropped type before the first item" do expect(first_type.position).to be < second_type.position - put :drop, params: { id: second_type.id, position: 1 }, format: :turbo_stream + put :drop, params: { id: second_type.id, list_type: "type", list_id: "", prev_id: "" }, format: :turbo_stream expect(response).to have_http_status(:ok) expect(second_type.reload.position).to eq(1) diff --git a/spec/features/types/deletion_spec.rb b/spec/features/types/deletion_spec.rb index 08bf3bf3e2cc..ff397bd2ec53 100644 --- a/spec/features/types/deletion_spec.rb +++ b/spec/features/types/deletion_spec.rb @@ -34,16 +34,14 @@ shared_let(:admin) { create(:admin) } let(:dialog_id) { WorkPackageTypes::Types::TypeDeletionDialogComponent::DIALOG_ID } + let(:index_page) { Pages::Types::Index.new } before { login_as(admin) } def click_delete(type) visit types_path - within("[data-draggable-id='#{type.id}'] .Box-header") do - find("action-menu > button").click - click_on I18n.t(:button_delete) - end + index_page.within_actions_menu(type) { |menu| menu.find(:menuitem, "Delete").click } end # The dialog arrives over a turbo stream, and a click lands nowhere until the native diff --git a/spec/features/types/type_ordering_spec.rb b/spec/features/types/type_ordering_spec.rb new file mode 100644 index 000000000000..857197856427 --- /dev/null +++ b/spec/features/types/type_ordering_spec.rb @@ -0,0 +1,81 @@ +# 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. +#++ + +require "spec_helper" + +RSpec.describe "Paginated type ordering", :js, :selenium, + with_settings: { per_page_options: "2,100" } do + shared_let(:admin) { create(:admin) } + shared_let(:types) { %w[A B C D E].map { |name| create(:type, name:) } } + + let(:index_page) { Pages::Types::Index.new } + + before { login_as(admin) } + + def drag(name, before:) + wait_for_turbo_stream { index_page.drag(name, before:) } + end + + it "reorders twice on page two across morphs and persists on reload" do + visit types_path(page: 2, per_page: 2) + index_page.expect_page_order("C", "D") + + drag("D", before: "C") + index_page.expect_page_order("D", "C") + index_page.expect_db_order("A", "B", "D", "C", "E") + + drag("C", before: "D") + index_page.expect_page_order("C", "D") + index_page.expect_db_order("A", "B", "C", "D", "E") + + page.refresh + index_page.expect_page_order("C", "D") + expect(page).to have_current_path(types_path(page: 2, per_page: 2)) + end + + it "moves expanded groups without mixing their variants or matching nested rows" do + Type.find_by!(name: "C").update!(name: "Bug") + Type.find_by!(name: "D").update!(name: "Bugfix") + bug = Type.find_by!(name: "Bug") + zeta = create(:type_variant, type: bug, variant_name: "Zeta") + alpha = create(:type_variant, type: bug, variant_name: "Alpha") + visit types_path(page: 2, per_page: 2, expand: bug.id) + index_page.expect_page_order("Bug", "Bugfix") + + drag("Bugfix", before: "Bug") + index_page.expect_page_order("Bugfix", "Bug") + index_page.expect_db_order("A", "B", "Bugfix", "Bug", "E") + within(index_page.type_group("Bug")) do + expect(page).to have_link("Alpha") + expect(page).to have_link("Zeta") + end + expect(bug.variants.non_default_variants.in_display_order).to eq([alpha, zeta]) + end +end diff --git a/spec/features/types/variant_comparison_spec.rb b/spec/features/types/variant_comparison_spec.rb index 3874892fea01..a843c79a6520 100644 --- a/spec/features/types/variant_comparison_spec.rb +++ b/spec/features/types/variant_comparison_spec.rb @@ -61,6 +61,7 @@ shared_let(:twin) { create(:type_variant, type:, variant_name: "Twin") } let(:form) { TypeVariant::FORM_CONFIGURATION } + let(:index_page) { Pages::Types::Index.new } def within_row(section, key, &) = within("#comparison-#{section}-#{key}", &) @@ -85,21 +86,15 @@ def within_column(variant, &) = within_test_selector("comparison-cell-#{variant. it "opens from the type's action menu, and only where there is something to compare" do visit types_path - within("[data-draggable-id='#{type.id}'] .Box-header") do - find("action-menu > button").click - - click_on I18n.t("types.comparison.action") - end + index_page.within_actions_menu(type) { |menu| menu.find(:menuitem, "Compare variants").click } expect(page).to have_current_path(comparison_type_variants_path(type_id: type.id)) expect(page).to have_test_selector("variant-comparison") visit types_path - within("[data-draggable-id='#{plain_type.id}'] .Box-header") do - find("action-menu > button").click - - expect(page).to have_no_link(I18n.t("types.comparison.action")) + index_page.within_actions_menu(plain_type) do |menu| + expect(menu).to have_no_selector(:menuitem, "Compare variants") end end diff --git a/spec/features/types/variants_index_spec.rb b/spec/features/types/variants_index_spec.rb index 6bcdac835fa5..eed3fa1ae14e 100644 --- a/spec/features/types/variants_index_spec.rb +++ b/spec/features/types/variants_index_spec.rb @@ -37,6 +37,8 @@ shared_let(:zeta_variant) { create(:type_variant, type: bug_type, variant_name: "Zeta variant") } shared_let(:alfa_variant) { create(:type_variant, type: bug_type, variant_name: "Alpha variant") } + let(:index_page) { Pages::Types::Index.new } + before { login_as(admin) } it "makes only types draggable via a drag handle" do @@ -45,11 +47,10 @@ expect(page).to have_text(bug_type.name) expect(page).to have_text(feature_type.name) - expect(page).to have_css("[data-draggable-id='#{bug_type.id}'] .DragHandle", visible: :all) - expect(page).to have_css("[data-draggable-id='#{feature_type.id}'] .DragHandle", visible: :all) - - variant_row = page.find(".Box-row", text: alfa_variant.variant_name, visible: :all) - expect(variant_row).to have_no_css(".DragHandle", visible: :all) + [bug_type, feature_type].each do |type| + expect(index_page.type_group(type)) + .to have_button(accessible_name: "Drag to reorder", count: 1, visible: :all) + end end it "links a type's header to its settings page" do @@ -70,11 +71,11 @@ it "counts a type's named variants in a badge on its header" do visit types_path - within("[data-draggable-id='#{bug_type.id}'] .Box-header") do + index_page.within_type_header(bug_type) do expect(page).to have_css(".Counter", text: "2") end - within("[data-draggable-id='#{feature_type.id}'] .Box-header") do + index_page.within_type_header(feature_type) do expect(page).to have_no_css(".Counter") end end @@ -84,11 +85,11 @@ visit types_path - within("[data-draggable-id='#{feature_type.id}'] .Box-header") do + index_page.within_type_header(feature_type) do expect(page).to have_css(".Label", text: I18n.t("types.index.enabled_in_new_projects")) end - within("[data-draggable-id='#{bug_type.id}'] .Box-header") do + index_page.within_type_header(bug_type) do expect(page).to have_no_css(".Label", text: I18n.t("types.index.enabled_in_new_projects")) end end @@ -96,11 +97,10 @@ it "offers configure, move and delete on a type" do visit types_path - within("[data-draggable-id='#{bug_type.id}'] .Box-header") do - find("action-menu > button").click - expect(page).to have_link(I18n.t(:button_configure)) - expect(page).to have_button(I18n.t(:button_move)) - expect(page).to have_link(I18n.t(:button_delete)) + index_page.within_actions_menu(bug_type) do |menu| + expect(menu).to have_selector(:menuitem, "Configure") + expect(menu).to have_selector(:menuitem, "Move", exact: true) + expect(menu).to have_selector(:menuitem, "Delete") end end @@ -136,7 +136,7 @@ it "adds a variant to a type from the group's add-variant row" do visit types_path(expand: bug_type.id) - within("[data-draggable-id='#{bug_type.id}']") do + within(index_page.type_group(bug_type)) do click_on I18n.t("types.index.add_variant", name: bug_type.name) end @@ -146,14 +146,14 @@ fill_in TypeVariant.human_attribute_name(:variant_name), with: "Hardware" click_on I18n.t(:button_continue) - expect(bug_type.reload.variants.non_default_variants.pluck(:variant_name)) + wait_for { bug_type.reload.variants.non_default_variants.pluck(:variant_name) } .to contain_exactly("Alpha variant", "Zeta variant", "Hardware") end it "returns to the index when the add-variant wizard is cancelled" do visit types_path(expand: bug_type.id) - within("[data-draggable-id='#{bug_type.id}']") do + within(index_page.type_group(bug_type)) do click_on I18n.t("types.index.add_variant", name: bug_type.name) end @@ -167,10 +167,7 @@ it "duplicates a type from its action menu" do visit types_path - within("[data-draggable-id='#{bug_type.id}'] .Box-header") do - find("action-menu > button").click - click_on I18n.t(:button_duplicate) - end + index_page.within_actions_menu(bug_type) { |menu| menu.find(:menuitem, "Duplicate").click } expect(page).to have_text(I18n.t("types.index.duplicate_notice", name: bug_type.name)) expect(page).to have_text(I18n.t("types.index.duplicate_name", name: bug_type.name)) @@ -182,10 +179,7 @@ expect(bug_type.position).to be < feature_type.position - drag_handle = page.find("[data-draggable-id='#{feature_type.id}'] .DragHandle") - target = page.find("[data-draggable-id='#{bug_type.id}']") - - drag_n_drop_element(from: drag_handle, to: target) + wait_for_turbo_stream { index_page.drag(feature_type, before: bug_type) } wait_for { feature_type.reload.position }.to be < bug_type.reload.position end @@ -195,13 +189,9 @@ expect(bug_type.position).to be < feature_type.position - within("[data-draggable-id='#{feature_type.id}'] .Box-header") do - find("action-menu > button").click - click_on I18n.t(:button_move) - click_on I18n.t(:label_sort_highest) - end + index_page.move(feature_type, "Move to top") - expect(page).to have_text(I18n.t(:notice_successful_update)) + expect(page).to have_text("Successful update.") expect(feature_type.reload.position).to be < bug_type.reload.position end diff --git a/spec/requests/work_package_types/type_ordering_spec.rb b/spec/requests/work_package_types/type_ordering_spec.rb new file mode 100644 index 000000000000..edb3553d654f --- /dev/null +++ b/spec/requests/work_package_types/type_ordering_spec.rb @@ -0,0 +1,172 @@ +# 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. +#++ + +require "spec_helper" + +RSpec.describe "Type ordering", :skip_csrf, + type: :rails_request, + with_settings: { per_page_options: "2,100" } do + current_user { create(:admin) } + + let!(:types) { %w[A B C D E].map { |name| create(:type, name:) } } + let(:drag_params) { { list_type: Type.model_name.param_key, list_id: "", prev_id: "" } } + + def type_named(name) = Type.find_by!(name:) + + def expect_order(*names) + expect(Type.order(:position).pluck(:name)).to eq(names) + end + + def turbo_fragment + Capybara.string( + Nokogiri::HTML(response.parsed_body).css("turbo-stream[action=update] template").map(&:inner_html).join + ) + end + + def drop(name, request_params = drag_params, page: 2, **context) + put drop_type_path(type_named(name), page:, per_page: 2, **context), + params: request_params, + as: :json, + headers: { "Accept" => "text/vnd.turbo-stream.html" } + end + + def expect_refused(*names) + aggregate_failures do + expect(response).to have_http_status(:unprocessable_entity) + expect_order(*names) + end + end + + [nil, ""].each do |anchor| + it "moves to page two's beginning for #{anchor.inspect}", :aggregate_failures do + drop("D", drag_params.merge(prev_id: anchor), expand: type_named("D").id) + + expect(response).to have_http_status(:ok) + expect_order("A", "B", "D", "C", "E") + expect(response).to have_turbo_stream(action: "update", method: "morph", + target: WorkPackageTypes::Types::GroupedListComponent.wrapper_key) + expand = type_named("D").id + expect(turbo_fragment).to have_link("1", href: types_path(page: 1, per_page: 2, expand:)) + expect(turbo_fragment).to have_link("3", href: types_path(page: 3, per_page: 2, expand:)) + expect(turbo_fragment).to have_no_css("[href*='/drop']") + end + + it "moves to the global beginning on page one for #{anchor.inspect}" do + drop("B", drag_params.merge(prev_id: anchor), page: 1) + + expect(response).to have_http_status(:ok) + expect_order("B", "A", "C", "D", "E") + end + end + + it "moves below an explicit predecessor on page two" do + drop("C", drag_params.merge(prev_id: type_named("D").id)) + + expect(response).to have_http_status(:ok) + expect_order("A", "B", "D", "C", "E") + end + + [false, true, 1.5, 0, -1, "01", "1junk", " ", [], {}].each do |anchor| + it "rejects malformed anchor #{anchor.inspect}" do + drop("D", drag_params.merge(prev_id: anchor)) + + expect_refused("A", "B", "C", "D", "E") + end + end + + [false, true, "1", [], {}].each do |list_id| + it "rejects destination #{list_id.inspect}" do + drop("D", drag_params.merge(list_id:)) + + expect_refused("A", "B", "C", "D", "E") + end + end + + { + "missing anchor" => { list_type: "type" }, + "missing type" => { prev_id: "" }, + "wrong type" => { list_type: "status", prev_id: "" }, + "unknown anchor" => { list_type: "type", prev_id: "99999999" }, + "absolute position" => { position: 1 } + }.each do |description, request_params| + it "rejects #{description}" do + drop("D", request_params) + + expect_refused("A", "B", "C", "D", "E") + end + end + + it "rejects a self anchor" do + drop("D", drag_params.merge(prev_id: type_named("D").id)) + + expect_refused("A", "B", "C", "D", "E") + end + + it "rejects a page boundary that resolves to the moved type" do + drop("B") + + expect_refused("A", "B", "C", "D", "E") + end + + it "rejects an empty page" do + type_named("E").destroy! + drop("C", page: 3) + + expect_refused("A", "B", "C", "D") + end + + it "ignores a non-scalar expansion parameter when dropping" do + drop("D", expand: ["1"]) + + expect(response).to have_http_status(:ok) + expect_order("A", "B", "D", "C", "E") + end + + it "preserves variant membership and alphabetical order" do + zeta = create(:type_variant, type: type_named("D"), variant_name: "Zeta") + alpha = create(:type_variant, type: type_named("D"), variant_name: "Alpha") + drop("D") + + expect(response).to have_http_status(:ok) + expect(type_named("D").variants.non_default_variants.in_display_order).to eq([alpha, zeta]) + expect([alpha.reload.type_id, zeta.reload.type_id]).to eq([type_named("D").id] * 2) + end + + context "without admin permission" do + current_user { create(:user) } + + it "rejects dragging" do + drop("D") + + expect(response).to have_http_status(:forbidden) + expect_order("A", "B", "C", "D", "E") + end + end +end diff --git a/spec/support/pages/types/index.rb b/spec/support/pages/types/index.rb index caff923b23dd..08240f77d2e9 100644 --- a/spec/support/pages/types/index.rb +++ b/spec/support/pages/types/index.rb @@ -32,11 +32,55 @@ module Pages module Types + # Drives the type index; type groups are addressed by name. class Index < ::Pages::Page def path "/types" end + def type_list + page.find(:list, accessible_name: I18n.t(:label_type_plural)) + end + + def type_group(type) + type_list.find(:heading, canonical_name(type), exact: true).ancestor(:list_item) + end + + def within_type_header(type, &) + within(type_group(type).find(".Box-header"), &) + end + + def within_actions_menu(type, &) + within_type_header(type) do + within(open_controlled_menu(find(:button, accessible_name: I18n.t(:label_actions))), &) + end + end + + def move(type, direction_label) + within_actions_menu(type) do |menu| + within(open_controlled_menu(menu.find(:menuitem, I18n.t(:button_move), exact: true))) do |submenu| + submenu.find(:menuitem, direction_label, exact: true).click + end + end + end + + def drag(type, before:) + target = type_group(before) + + perform_native_drag(source: drag_handle(type), target:, offset_y: -(target.native.rect.height / 4)) + end + + def expect_page_order(*names) + page.document.synchronize do + found = type_list.all(:heading).map { it.text.squish } + raise Capybara::ExpectationNotMet, "Expected #{names}, got #{found}" unless found == names + end + end + + def expect_db_order(*names) + expect(::Type.order(:position).pluck(:name)).to eq(names) + end + def expect_listed(*types) headers = page.all(".Box-header .Button-label, .Box-header a") @@ -48,9 +92,7 @@ def click_new end def delete(type) - open_actions(type) - - click_link I18n.t(:button_delete) + click_delete(type) expect(page).to have_css("##{deletion_dialog_id}[open]") @@ -58,27 +100,26 @@ def delete(type) end def delete_expecting_refusal(type) - open_actions(type) - - click_link I18n.t(:button_delete) + click_delete(type) end private - def open_actions(type) - within_header(type) { find("action-menu > button").click } + def click_delete(type) + within_actions_menu(type) { |menu| menu.find(:menuitem, I18n.t(:button_delete)).click } end - def deletion_dialog_id - WorkPackageTypes::Types::TypeDeletionDialogComponent::DIALOG_ID + def drag_handle(type) + type_group(type).find(:button, accessible_name: I18n.t("drag_handle.button_drag")) end - def within_header(type) - header = page.find(".Box-header", text: canonical_name(type)) + def open_controlled_menu(button) + button.click + page.find(:menu, id: button["aria-controls"]) + end - within header do - yield header - end + def deletion_dialog_id + WorkPackageTypes::Types::TypeDeletionDialogComponent::DIALOG_ID end def canonical_name(type) From 91023d027224967b340d7cc6001448d88214afc7 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Wed, 30 Sep 2026 21:02:29 +0100 Subject: [PATCH 3/3] [DREAM-791] Keep page context in type move menus Carries page and expansion context into the lazy type menus and posts menu moves with it. Moves stay global, and the server now answers with a morph of the current page instead of redirecting to the first page. Removes the unused type_move permitted parameter. https://community.openproject.org/wp/DREAM-791 --- .../types/grouped_list_component.rb | 2 +- .../types/type_actions_component.rb | 11 +++-- .../work_package_types/types_controller.rb | 17 ++++--- app/models/permitted_params.rb | 4 -- .../types/grouped_list_component_spec.rb | 5 ++ .../types/type_actions_component_spec.rb | 49 +++++++++++++++++++ .../types_controller_spec.rb | 11 ++--- spec/features/types/type_ordering_spec.rb | 44 +++++++++++++++++ .../work_package_types/type_ordering_spec.rb | 45 +++++++++++++++++ spec/support/pages/types/index.rb | 2 +- 10 files changed, 167 insertions(+), 23 deletions(-) diff --git a/app/components/work_package_types/types/grouped_list_component.rb b/app/components/work_package_types/types/grouped_list_component.rb index 6b05dafd4b94..7537ffbc8f80 100644 --- a/app/components/work_package_types/types/grouped_list_component.rb +++ b/app/components/work_package_types/types/grouped_list_component.rb @@ -90,7 +90,7 @@ def menu_id(type) end def menu_src(type) - menu_type_path(type) + menu_type_path(type, **context_args) end def variant_menu_id(variant) diff --git a/app/components/work_package_types/types/type_actions_component.rb b/app/components/work_package_types/types/type_actions_component.rb index d5fc3d56e8a8..292d6ca96baa 100644 --- a/app/components/work_package_types/types/type_actions_component.rb +++ b/app/components/work_package_types/types/type_actions_component.rb @@ -38,10 +38,12 @@ def self.menu_id(type) "type-#{type.id}-action-menu" end - def initialize(type:) + def initialize(type:, page_args: {}, expanded_type_id: nil) super() @type = type + @page_args = page_args + @expanded_type_id = expanded_type_id end def menu_id @@ -50,7 +52,7 @@ def menu_id private - attr_reader :type + attr_reader :type, :page_args, :expanded_type_id def type_actions(menu) configure_action(menu) @@ -175,8 +177,9 @@ def move_action(menu) def move_item(submenu, move_to, label, icon) submenu.with_item( label:, - href: move_types_path(type, type: { move_to: }), - form_arguments: { method: :post } + tag: :button, + href: move_types_path(type, **page_args, expand: expanded_type_id), + form_arguments: { method: :post, inputs: [{ name: "type[move_to]", value: move_to.to_s }] } ) do |item| item.with_leading_visual_icon(icon:) end diff --git a/app/controllers/work_package_types/types_controller.rb b/app/controllers/work_package_types/types_controller.rb index 8a72112b73bd..eec99e1d938b 100644 --- a/app/controllers/work_package_types/types_controller.rb +++ b/app/controllers/work_package_types/types_controller.rb @@ -53,12 +53,7 @@ def type end def move - if @type.update(permitted_params.type_move) - flash[:notice] = I18n.t(:notice_successful_update) - else - flash.now[:error] = I18n.t(:error_type_could_not_be_saved) - end - redirect_to types_path + render_ordering_result(move_in_direction, error_message: I18n.t(:error_type_could_not_be_saved)) end def destroy @@ -98,7 +93,7 @@ def drop end def menu - render Types::TypeActionsComponent.new(type: @type), layout: false + render Types::TypeActionsComponent.new(type: @type, page_args:, expanded_type_id:), layout: false end protected @@ -179,6 +174,14 @@ def render_ordering_result(moved, error_message:) respond_with_turbo_streams(status: moved ? :ok : :unprocessable_entity) end + def move_in_direction + type_params = params[:type] + return false unless type_params.is_a?(ActionController::Parameters) + + direction = type_params[:move_to] + direction.in?(%w[highest higher lower lowest]) && @type.update(move_to: direction) + end + def valid_drop_request? params[:list_type] == ::Type.model_name.param_key && (params[:list_id].nil? || params[:list_id] == "") && diff --git a/app/models/permitted_params.rb b/app/models/permitted_params.rb index 152c47fe95af..87371b0158d1 100644 --- a/app/models/permitted_params.rb +++ b/app/models/permitted_params.rb @@ -241,10 +241,6 @@ def type(args = {}) whitelisted end - def type_move - params.require(:type).permit(*self.class.permitted_attributes[:move_to]) - end - def enumerations_move params.require(:enumeration).permit(*self.class.permitted_attributes[:move_to]) end diff --git a/spec/components/work_package_types/types/grouped_list_component_spec.rb b/spec/components/work_package_types/types/grouped_list_component_spec.rb index e233b7c54a0b..e29bfbc94211 100644 --- a/spec/components/work_package_types/types/grouped_list_component_spec.rb +++ b/spec/components/work_package_types/types/grouped_list_component_spec.rb @@ -219,6 +219,11 @@ it "carries page context through the pagination links" do expect(rendered_component).to have_link("1", href: types_path(page: 1, per_page: 2, expand: expanded.id)) end + + it "carries page context into the lazy menus" do + expect(rendered_component) + .to have_element(:"include-fragment", src: menu_type_path(expanded, page: 2, per_page: 2, expand: expanded.id)) + end end describe "a lone type" do diff --git a/spec/components/work_package_types/types/type_actions_component_spec.rb b/spec/components/work_package_types/types/type_actions_component_spec.rb index 693c1e7c749c..454aec331135 100644 --- a/spec/components/work_package_types/types/type_actions_component_spec.rb +++ b/spec/components/work_package_types/types/type_actions_component_spec.rb @@ -75,4 +75,53 @@ end end end + + describe "paginated moves" do + let!(:types) { %w[A B C D E].map { |name| create(:type, name:) } } + let(:ordered) { Type.order(:position) } + let(:page_two) { ordered.page(2).per_page(2).to_a } + let(:page_args) { { page: 2, per_page: 2 } } + + def render_for(type, **args) + render_inline(described_class.new(type:, **args)) + end + + def expect_directions(rendered, present: [], absent: []) + aggregate_failures do + present.each { |label| expect(rendered).to have_selector(:menuitem, text: I18n.t(label)) } + absent.each { |label| expect(rendered).to have_no_selector(:menuitem, text: I18n.t(label)) } + end + end + + it "keeps all directions for a page's first type and posts with page context" do + first_on_page = page_two.first + expect(first_on_page).not_to eq(ordered.first) + + rendered = render_for(first_on_page, page_args:) + + expect_directions(rendered, present: %i[label_sort_highest label_sort_higher label_sort_lower label_sort_lowest]) + expect(rendered).to have_element(:form, action: move_types_path(first_on_page, **page_args), method: "post") + end + + it "keeps downward moves for a page's last type" do + last_on_page = page_two.last + expect(last_on_page).not_to eq(ordered.last) + + rendered = render_for(last_on_page, page_args:) + + expect_directions(rendered, present: %i[label_sort_lower label_sort_lowest]) + end + + it "omits upward directions at the global start" do + expect_directions(render_for(ordered.first), + present: %i[label_sort_lower label_sort_lowest], + absent: %i[label_sort_highest label_sort_higher]) + end + + it "omits downward directions at the global end" do + expect_directions(render_for(ordered.last), + present: %i[label_sort_highest label_sort_higher], + absent: %i[label_sort_lower label_sort_lowest]) + end + end end diff --git a/spec/controllers/work_package_types/types_controller_spec.rb b/spec/controllers/work_package_types/types_controller_spec.rb index 4bd86fe8d37e..3807c9f7ce99 100644 --- a/spec/controllers/work_package_types/types_controller_spec.rb +++ b/spec/controllers/work_package_types/types_controller_spec.rb @@ -97,11 +97,10 @@ let(:params) { { "id" => type.id, "type" => { move_to: "lower" } } } before do - post :move, params: + post :move, params:, format: :turbo_stream end - it { expect(response).to be_redirect } - it { expect(response).to redirect_to(types_path) } + it { expect(response).to have_http_status(:ok) } it "has the position updated" do expect(Type.find_by(name: "My type").position).to eq(2) @@ -117,13 +116,13 @@ allow(Type).to receive(:find).and_return(type) allow(type).to receive(:update).and_return false - post :move, params: + post :move, params:, format: :turbo_stream end - it { expect(response).to redirect_to(types_path) } + it { expect(response).to have_http_status(:unprocessable_entity) } it "has an unsuccessful move flash" do - expect(flash[:error]).to eq(I18n.t(:error_type_could_not_be_saved)) + expect(response.body).to include(I18n.t(:error_type_could_not_be_saved)) end it "doesn't update the position" do diff --git a/spec/features/types/type_ordering_spec.rb b/spec/features/types/type_ordering_spec.rb index 857197856427..684a8a86ad60 100644 --- a/spec/features/types/type_ordering_spec.rb +++ b/spec/features/types/type_ordering_spec.rb @@ -78,4 +78,48 @@ def drag(name, before:) end expect(bug.variants.non_default_variants.in_display_order).to eq([alpha, zeta]) end + + it "moves up across pages and refreshes the next lazy menu" do + visit types_path(page: 2, per_page: 2) + index_page.move("C", "Move up") + + index_page.expect_page_order("B", "D") + index_page.expect_db_order("A", "C", "B", "D", "E") + expect(page).to have_text("Successful update.") + expect(page).to have_current_path(types_path(page: 2, per_page: 2)) + + index_page.move("B", "Move up") + index_page.expect_page_order("C", "D") + index_page.expect_db_order("A", "B", "C", "D", "E") + end + + it "moves to the global top while keeping index pagination links" do + visit types_path(page: 2, per_page: 2) + index_page.move("D", "Move to top") + + index_page.expect_page_order("B", "C") + index_page.expect_db_order("D", "A", "B", "C", "E") + page.refresh + index_page.expect_page_order("B", "C") + + within(".op-pagination--pages") { click_on "1" } + index_page.expect_page_order("D", "A") + expect(page).to have_current_path(types_path(page: 1, per_page: 2)) + end + + it "moves down into the next page" do + visit types_path(page: 2, per_page: 2) + index_page.move("D", "Move down") + + index_page.expect_page_order("C", "E") + index_page.expect_db_order("A", "B", "C", "E", "D") + end + + it "moves to the global bottom" do + visit types_path(page: 2, per_page: 2) + index_page.move("C", "Move to bottom") + + index_page.expect_page_order("D", "E") + index_page.expect_db_order("A", "B", "D", "E", "C") + end end diff --git a/spec/requests/work_package_types/type_ordering_spec.rb b/spec/requests/work_package_types/type_ordering_spec.rb index edb3553d654f..aff7e9b364c7 100644 --- a/spec/requests/work_package_types/type_ordering_spec.rb +++ b/spec/requests/work_package_types/type_ordering_spec.rb @@ -159,6 +159,44 @@ def expect_refused(*names) expect([alpha.reload.type_id, zeta.reload.type_id]).to eq([type_named("D").id] * 2) end + { + highest: ["D", %w[D A B C E]], + higher: ["C", %w[A C B D E]], + lower: ["D", %w[A B C E D]], + lowest: ["C", %w[A B D E C]] + }.each do |direction, (name, names)| + it "moves #{direction} globally and refreshes the current page", :aggregate_failures do + post move_types_path(type_named(name), page: 2, per_page: 2), + params: { type: { move_to: direction } }, as: :turbo_stream + + expect(response).to have_http_status(:ok) + expect_order(*names) + expect(response.body).to include(I18n.t(:notice_successful_update)) + expect(turbo_fragment.all(:heading).map { it.text.squish }).to eq(names[2, 2]) + expect(turbo_fragment).to have_link("1", href: types_path(page: 1, per_page: 2)) + expect(turbo_fragment).to have_link("3", href: types_path(page: 3, per_page: 2)) + end + end + + [nil, "sideways", false, [], {}].each do |direction| + it "rejects invalid direction #{direction.inspect}" do + post move_types_path(type_named("D"), page: 2, per_page: 2), + params: { type: { move_to: direction } }, as: :json, + headers: { "Accept" => "text/vnd.turbo-stream.html" } + + expect_refused("A", "B", "C", "D", "E") + end + end + + it "keeps page context in the lazy menu forms", :aggregate_failures do + get menu_type_path(type_named("C"), page: 2, per_page: 2, expand: type_named("C").id) + + expect(response).to have_http_status(:ok) + form_action = move_types_path(type_named("C"), page: 2, per_page: 2, expand: type_named("C").id) + expect(response.body).to have_element(:form, action: form_action, count: 4) + expect(response.body).to have_field("type[move_to]", type: :hidden, with: "higher") + end + context "without admin permission" do current_user { create(:user) } @@ -168,5 +206,12 @@ def expect_refused(*names) expect(response).to have_http_status(:forbidden) expect_order("A", "B", "C", "D", "E") end + + it "rejects menu moves" do + post move_types_path(type_named("D")), params: { type: { move_to: "highest" } }, as: :turbo_stream + + expect(response).to have_http_status(:forbidden) + expect_order("A", "B", "C", "D", "E") + end end end diff --git a/spec/support/pages/types/index.rb b/spec/support/pages/types/index.rb index 08240f77d2e9..f3b3c6e3652b 100644 --- a/spec/support/pages/types/index.rb +++ b/spec/support/pages/types/index.rb @@ -59,7 +59,7 @@ def within_actions_menu(type, &) def move(type, direction_label) within_actions_menu(type) do |menu| within(open_controlled_menu(menu.find(:menuitem, I18n.t(:button_move), exact: true))) do |submenu| - submenu.find(:menuitem, direction_label, exact: true).click + wait_for_turbo_stream { submenu.find(:menuitem, direction_label, exact: true).click } end end end