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/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..7537ffbc8f80 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 @@ -89,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) @@ -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/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 1b5e87d94d3d..eec99e1d938b 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 @@ -52,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 @@ -93,22 +89,17 @@ 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 - render Types::TypeActionsComponent.new(type: @type), layout: false + render Types::TypeActionsComponent.new(type: @type, page_args:, expanded_type_id:), layout: false end protected def find_type - @type = ::Type.find(params[:id]) + @type = ::Type.find(params.expect(:id)) end def types_for_index @@ -158,5 +149,62 @@ 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 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] == "") && + 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/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/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/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 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..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 @@ -164,4 +164,82 @@ 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 + + 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 + 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/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 4100dcae9946..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 @@ -228,10 +227,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..684a8a86ad60 --- /dev/null +++ b/spec/features/types/type_ordering_spec.rb @@ -0,0 +1,125 @@ +# 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 + + 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/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..aff7e9b364c7 --- /dev/null +++ b/spec/requests/work_package_types/type_ordering_spec.rb @@ -0,0 +1,217 @@ +# 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 + + { + 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) } + + it "rejects dragging" do + drop("D") + + 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 caff923b23dd..f3b3c6e3652b 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| + wait_for_turbo_stream { 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)