From f7f4bff9367306d3824e388c4eecc9c966be368b Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Tue, 24 Sep 2024 16:07:35 +0300 Subject: [PATCH 01/11] fix[Op#57989]: allow pagination links to cofigure nagivation visit via `data-turbo-action` Navigating within a frame can be configured as either an application visit ("advance" or "replace") or as a _restoration_ visit See [Ref](https://turbo.hotwired.dev/handbook/drive#page-navigation-basics) Set the turbo action to "advance" within the project list turbo frame such that clicking pagination links (ex. ?page=1) also updates the history state, and the URL updates accordingly. This allows for any further actions to retain the current page. Also see: https://turbo.hotwired.dev/handbook/frames#promoting-a-frame-navigation-to-a-page-visit --- .../custom_field_projects/table_component.rb | 7 ++++--- app/helpers/pagination_helper.rb | 9 +++++++++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/app/components/admin/custom_fields/custom_field_projects/table_component.rb b/app/components/admin/custom_fields/custom_field_projects/table_component.rb index 9ef7650dab1a..970301255d14 100644 --- a/app/components/admin/custom_fields/custom_field_projects/table_component.rb +++ b/app/components/admin/custom_fields/custom_field_projects/table_component.rb @@ -52,9 +52,10 @@ def sortable? # the controller action that link_to should use. # def optional_pagination_options - return super unless params[:url_for_action] - - super.merge(params: { action: params[:url_for_action] }) + super.tap do |options| + options[:params] = { action: params[:url_for_action] } if params[:url_for_action] + options[:turbo_action] = "advance" + end end end end diff --git a/app/helpers/pagination_helper.rb b/app/helpers/pagination_helper.rb index c8a53aaadc42..81bf2516eb31 100644 --- a/app/helpers/pagination_helper.rb +++ b/app/helpers/pagination_helper.rb @@ -202,6 +202,7 @@ def previous_or_next_page(page, text, class_suffix) def link(text, target, attributes) new_attributes = attributes.dup new_attributes["data-turbo-stream"] = true if turbo? + new_attributes["data-turbo-action"] = turbo_action if turbo_action.present? super(text, target, new_attributes) end @@ -210,6 +211,14 @@ def allowed_params @options[:allowed_params] # rubocop:disable Rails/HelperInstanceVariable end + # Customize the Turbo visit action for pagination links. Can be set to "advance" or "replace". + # "advance" - push a new entry onto the history stack. + # "replace" - replace the current history entry. + # See: https://turbo.hotwired.dev/reference/attributes + def turbo_action + @options[:turbo_action] # rubocop:disable Rails/HelperInstanceVariable + end + def turbo? @options[:turbo] # rubocop:disable Rails/HelperInstanceVariable end From 368ec4da83d793401bf12e1304bcfdbbbddb35fb Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Tue, 24 Sep 2024 16:34:17 +0300 Subject: [PATCH 02/11] tests[Op#57989]: pagination links should have page param --- .../custom_fields_project_spec.rb | 20 ++----------------- .../project_mappings_spec.rb | 20 ++----------------- .../custom_field_projects_index.rb | 19 ++++++++++++++++-- .../project_custom_field_mappings_index.rb | 12 +++-------- 4 files changed, 24 insertions(+), 47 deletions(-) diff --git a/spec/features/admin/custom_fields/custom_fields_project_spec.rb b/spec/features/admin/custom_fields/custom_fields_project_spec.rb index 8081ae4c1534..d77540491baa 100644 --- a/spec/features/admin/custom_fields/custom_fields_project_spec.rb +++ b/spec/features/admin/custom_fields/custom_fields_project_spec.rb @@ -115,15 +115,7 @@ expect(page).to have_text(subproject.name) aggregate_failures "pagination links maintain the correct url" do - within ".op-pagination" do - pagination_links = page.all(".op-pagination--item-link") - expect(pagination_links.size).to be_positive - - pagination_links.each do |pagination_link| - uri = URI.parse(pagination_link["href"]) - expect(uri.path).to eq(custom_field_projects_path(custom_field)) - end - end + custom_field_projects_page.expect_correct_pagination_links(model: custom_field) end end @@ -138,15 +130,7 @@ expect(page).to have_no_text(project.name) aggregate_failures "pagination links maintain the correct url after unlinking is done" do - within ".op-pagination" do - pagination_links = page.all(".op-pagination--item-link") - expect(pagination_links.size).to be_positive - - pagination_links.each do |pagination_link| - uri = URI.parse(pagination_link["href"]) - expect(uri.path).to eq(custom_field_projects_path(custom_field)) - end - end + custom_field_projects_page.expect_correct_pagination_links(model: custom_field) end end diff --git a/spec/features/admin/project_custom_fields/project_mappings_spec.rb b/spec/features/admin/project_custom_fields/project_mappings_spec.rb index ea0eee6e21e6..9742080eee5f 100644 --- a/spec/features/admin/project_custom_fields/project_mappings_spec.rb +++ b/spec/features/admin/project_custom_fields/project_mappings_spec.rb @@ -119,15 +119,7 @@ expect(page).to have_text(subproject.name) aggregate_failures "pagination links maintain the correct url" do - within ".op-pagination" do - pagination_links = page.all(".op-pagination--item-link") - expect(pagination_links.size).to be_positive - - pagination_links.each do |pagination_link| - uri = URI.parse(pagination_link["href"]) - expect(uri.path).to eq(project_mappings_admin_settings_project_custom_field_path(project_custom_field)) - end - end + project_custom_field_mappings_page.expect_correct_pagination_links(model: project_custom_field) end end @@ -142,15 +134,7 @@ expect(page).to have_no_text(project.name) aggregate_failures "pagination links maintain the correct url after unlinking is done" do - within ".op-pagination" do - pagination_links = page.all(".op-pagination--item-link") - expect(pagination_links.size).to be_positive - - pagination_links.each do |pagination_link| - uri = URI.parse(pagination_link["href"]) - expect(uri.path).to eq(project_mappings_admin_settings_project_custom_field_path(project_custom_field)) - end - end + project_custom_field_mappings_page.expect_correct_pagination_links(model: project_custom_field) end end diff --git a/spec/support/pages/admin/custom_fields/custom_fields_projects/custom_field_projects_index.rb b/spec/support/pages/admin/custom_fields/custom_fields_projects/custom_field_projects_index.rb index a170b17e58ef..1b692f34369c 100644 --- a/spec/support/pages/admin/custom_fields/custom_fields_projects/custom_field_projects_index.rb +++ b/spec/support/pages/admin/custom_fields/custom_fields_projects/custom_field_projects_index.rb @@ -33,17 +33,32 @@ module Admin module CustomFields module CustomFieldsProjects class CustomFieldProjectsIndex < ::Pages::Projects::Index - def path + def path(custom_field) "/custom_fields/#{custom_field.id}/projects" end def within_row(project) - row = page.find("#admin-custom-fields-custom-field-projects-row-component-project-#{project.id}") + row = page.find("#{row_id_prefix}-#{project.id}") row.hover within row do yield row end end + + def expect_correct_pagination_links(model:) + within ".op-pagination" do + pagination_links = page.all(".op-pagination--item-link") + expect(pagination_links.size).to be_positive + + pagination_links.each.with_index(1) do |pagination_link, page_number| + uri = URI.parse(pagination_link["href"]) + expect(uri.path).to eq(path(model)) + expect(uri.query).to include("page=#{page_number}") + end + end + end + + def row_id_prefix = "#admin-custom-fields-custom-field-projects-row-component-project" end end end diff --git a/spec/support/pages/admin/settings/project_custom_fields/project_custom_field_mappings_index.rb b/spec/support/pages/admin/settings/project_custom_fields/project_custom_field_mappings_index.rb index ab41fabafb8f..136762591a4c 100644 --- a/spec/support/pages/admin/settings/project_custom_fields/project_custom_field_mappings_index.rb +++ b/spec/support/pages/admin/settings/project_custom_fields/project_custom_field_mappings_index.rb @@ -32,18 +32,12 @@ module Pages module Admin module Settings module ProjectCustomFields - class ProjectCustomFieldMappingsIndex < ::Pages::Projects::Index - def path + class ProjectCustomFieldMappingsIndex < ::Pages::Admin::CustomFields::CustomFieldsProjects::CustomFieldProjectsIndex + def path(project_custom_field) "/admin/settings/project_custom_fields/#{project_custom_field.id}/project_mappings" end - def within_row(project) - row = page.find("#settings-project-custom-fields-project-custom-field-mapping-row-component-project-#{project.id}") - row.hover - within row do - yield row - end - end + def row_id_prefix = "#settings-project-custom-fields-project-custom-field-mapping-row-component-project" end end end From 8eb6c98e51b826fea183553d95798145ed9c392f Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Tue, 24 Sep 2024 16:34:42 +0300 Subject: [PATCH 03/11] chore[Op#57516]: remove redundant turbo stream mixin --- .../custom_fields/custom_field_projects/table_component.rb | 2 -- .../storages/project_storages/projects/table_component.rb | 2 -- 2 files changed, 4 deletions(-) diff --git a/app/components/admin/custom_fields/custom_field_projects/table_component.rb b/app/components/admin/custom_fields/custom_field_projects/table_component.rb index 970301255d14..1e9f55a9d927 100644 --- a/app/components/admin/custom_fields/custom_field_projects/table_component.rb +++ b/app/components/admin/custom_fields/custom_field_projects/table_component.rb @@ -30,8 +30,6 @@ module Admin module CustomFields module CustomFieldProjects class TableComponent < Projects::TableComponent - include OpTurbo::Streamable - def columns @columns ||= query.selects.reject { |select| select.is_a?(Queries::Selects::NotExistingSelect) } end diff --git a/modules/storages/app/components/storages/project_storages/projects/table_component.rb b/modules/storages/app/components/storages/project_storages/projects/table_component.rb index 243ccbe46ab1..0ccf7c9497bf 100644 --- a/modules/storages/app/components/storages/project_storages/projects/table_component.rb +++ b/modules/storages/app/components/storages/project_storages/projects/table_component.rb @@ -32,8 +32,6 @@ # for every "column" defined below. module Storages::ProjectStorages::Projects class TableComponent < Projects::TableComponent - include OpTurbo::Streamable - options :storage def columns From df6bfc5280bb985540c3ca552d7d0fa5bd68160d Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Tue, 24 Sep 2024 17:17:47 +0300 Subject: [PATCH 04/11] tests[Op#57989]: elevate project pagination link assertions to base projects index --- .../project_storages/project_storages_index.rb | 2 +- .../custom_field_projects_index.rb | 13 ------------- spec/support/pages/projects/index.rb | 15 ++++++++++++++- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/modules/storages/spec/support/pages/admin/storages/project_storages/project_storages_index.rb b/modules/storages/spec/support/pages/admin/storages/project_storages/project_storages_index.rb index b1008cb411ab..ebd99db68b2d 100644 --- a/modules/storages/spec/support/pages/admin/storages/project_storages/project_storages_index.rb +++ b/modules/storages/spec/support/pages/admin/storages/project_storages/project_storages_index.rb @@ -33,7 +33,7 @@ module Admin module Storages module ProjectStorages class Index < ::Pages::Projects::Index - def path + def path(storage) "/admin/settings/storages/#{storage.id}/project_storages" end diff --git a/spec/support/pages/admin/custom_fields/custom_fields_projects/custom_field_projects_index.rb b/spec/support/pages/admin/custom_fields/custom_fields_projects/custom_field_projects_index.rb index 1b692f34369c..78fb409497e0 100644 --- a/spec/support/pages/admin/custom_fields/custom_fields_projects/custom_field_projects_index.rb +++ b/spec/support/pages/admin/custom_fields/custom_fields_projects/custom_field_projects_index.rb @@ -45,19 +45,6 @@ def within_row(project) end end - def expect_correct_pagination_links(model:) - within ".op-pagination" do - pagination_links = page.all(".op-pagination--item-link") - expect(pagination_links.size).to be_positive - - pagination_links.each.with_index(1) do |pagination_link, page_number| - uri = URI.parse(pagination_link["href"]) - expect(uri.path).to eq(path(model)) - expect(uri.query).to include("page=#{page_number}") - end - end - end - def row_id_prefix = "#admin-custom-fields-custom-field-projects-row-component-project" end end diff --git a/spec/support/pages/projects/index.rb b/spec/support/pages/projects/index.rb index 66810eb38d40..9b504a335268 100644 --- a/spec/support/pages/projects/index.rb +++ b/spec/support/pages/projects/index.rb @@ -33,7 +33,7 @@ module Projects class Index < ::Pages::Page include ::Components::Autocompleter::NgSelectAutocompleteHelpers - def path + def path(*) "/projects" end @@ -127,6 +127,19 @@ def expect_page_link(text) end end + def expect_correct_pagination_links(model:) + within ".op-pagination" do + pagination_links = page.all(".op-pagination--item-link") + expect(pagination_links.size).to be_positive + + pagination_links.each.with_index(1) do |pagination_link, page_number| + uri = URI.parse(pagination_link["href"]) + expect(uri.path).to eq(path(model)) + expect(uri.query).to include("page=#{page_number}") + end + end + end + def expect_filters_container_toggled expect(page).to have_css(".op-filters-form") end From bdb67efe80d3ac007774705faca74182d93c0371 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Tue, 24 Sep 2024 17:18:06 +0300 Subject: [PATCH 05/11] fix[Op#57989]: Specify action in non-index turbo stream requests storage projects list Follows: https://github.com/opf/openproject/pull/15860 ```ruby Failure/Error: expect(uri.path).to eq(path(model)) expected: "/admin/settings/storages/178/project_storages" got: "/admin/settings/storages/178/project_storages/338" ``` --- .../custom_field_projects/table_component.rb | 20 +------ ...streamable_pagination_links_constraints.rb | 55 +++++++++++++++++++ .../projects/table_component.rb | 2 + .../storages/admin/project_storages_spec.rb | 12 ++++ 4 files changed, 71 insertions(+), 18 deletions(-) create mode 100644 app/components/projects/concerns/table_component/streamable_pagination_links_constraints.rb diff --git a/app/components/admin/custom_fields/custom_field_projects/table_component.rb b/app/components/admin/custom_fields/custom_field_projects/table_component.rb index 1e9f55a9d927..5eb0f19a4cd5 100644 --- a/app/components/admin/custom_fields/custom_field_projects/table_component.rb +++ b/app/components/admin/custom_fields/custom_field_projects/table_component.rb @@ -30,6 +30,8 @@ module Admin module CustomFields module CustomFieldProjects class TableComponent < Projects::TableComponent + include ::Projects::Concerns::TableComponent::StreamablePaginationLinksConstraints + def columns @columns ||= query.selects.reject { |select| select.is_a?(Queries::Selects::NotExistingSelect) } end @@ -37,24 +39,6 @@ def columns def sortable? false end - - # @override optional_pagination_options are passed to the pagination_options - # which are passed to #pagination_links_full in pagination_helper.rb - # - # In Turbo streamable components, we need to be able to specify the url_for(action:) so that links are - # generated in the context of the component index action, instead of any turbo stream actions performing - # partial updates on the page. - # - # params[:url_for_action] is passed to the pagination_options making it's way down to any pagination links - # that are generated via link_to which calls url_for which uses the params[:url_for_action] to specify - # the controller action that link_to should use. - # - def optional_pagination_options - super.tap do |options| - options[:params] = { action: params[:url_for_action] } if params[:url_for_action] - options[:turbo_action] = "advance" - end - end end end end diff --git a/app/components/projects/concerns/table_component/streamable_pagination_links_constraints.rb b/app/components/projects/concerns/table_component/streamable_pagination_links_constraints.rb new file mode 100644 index 000000000000..ddaedeaa11a7 --- /dev/null +++ b/app/components/projects/concerns/table_component/streamable_pagination_links_constraints.rb @@ -0,0 +1,55 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +module Projects + module Concerns + module TableComponent + module StreamablePaginationLinksConstraints + # @override optional_pagination_options are passed to the pagination_options + # which are passed to #pagination_links_full in pagination_helper.rb + # + # In Turbo streamable components, we need to be able to specify the url_for(action:) so that links are + # generated in the context of the component index action, instead of any turbo stream actions performing + # partial updates on the page. + # + # params[:url_for_action] is passed to the pagination_options making it's way down to any pagination links + # that are generated via link_to which calls url_for which uses the params[:url_for_action] to specify + # the controller action that link_to should use. + # + # data-turbo-action="advance" is added to the pagination links ensure links pushState to the browser + # + def optional_pagination_options + super.tap do |options| + options[:params] = { action: params[:url_for_action] } if params[:url_for_action] + options[:turbo_action] = "advance" + end + end + end + end + end +end diff --git a/modules/storages/app/components/storages/project_storages/projects/table_component.rb b/modules/storages/app/components/storages/project_storages/projects/table_component.rb index 0ccf7c9497bf..ea1db72e0516 100644 --- a/modules/storages/app/components/storages/project_storages/projects/table_component.rb +++ b/modules/storages/app/components/storages/project_storages/projects/table_component.rb @@ -32,6 +32,8 @@ # for every "column" defined below. module Storages::ProjectStorages::Projects class TableComponent < Projects::TableComponent + include ::Projects::Concerns::TableComponent::StreamablePaginationLinksConstraints + options :storage def columns diff --git a/modules/storages/spec/features/storages/admin/project_storages_spec.rb b/modules/storages/spec/features/storages/admin/project_storages_spec.rb index f4ae5ee03f8c..fa0d54eaacc4 100644 --- a/modules/storages/spec/features/storages/admin/project_storages_spec.rb +++ b/modules/storages/spec/features/storages/admin/project_storages_spec.rb @@ -228,6 +228,10 @@ expect(page).to have_text(project.name) expect(page).to have_text(subproject.name) + + aggregate_failures "pagination links maintain the correct url" do + project_storages_index_page.expect_correct_pagination_links(model: storage) + end end context "when the user does not select a folder" do @@ -297,6 +301,10 @@ project_storages_index_page.within_the_table_row_containing(project.name) do expect(page).to have_text("No specific folder") end + + aggregate_failures "pagination links maintain the correct url" do + project_storages_index_page.expect_correct_pagination_links(model: storage) + end end context "when oauth access has not been granted and manual selection" do @@ -388,6 +396,10 @@ def call expect(page).to have_no_selector("dialog") expect(page).to have_text("Successful deletion.") expect(page).to have_no_text(project.name) + + aggregate_failures "pagination links maintain the correct url" do + project_storages_index_page.expect_correct_pagination_links(model: storage) + end end end end From 0c33b93c84ceba6c80fd971460353fbf9df83066 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Thu, 26 Sep 2024 11:53:31 +0300 Subject: [PATCH 06/11] fix[Op#57989]: Pass the `page` parameter in Delete requests so the current page is maintained --- .../custom_field_projects/row_component.rb | 5 ++--- app/components/projects/row_component.rb | 4 ++++ .../row_component.rb | 3 ++- .../custom_field_projects_controller.rb | 2 +- .../project_custom_fields_controller.rb | 2 +- ...troy_confirmation_dialog_component.html.erb | 2 +- .../destroy_confirmation_dialog_component.rb | 7 ++++++- .../project_storages/projects/row_component.rb | 3 ++- .../storages/project_storages_controller.rb | 5 +++-- .../custom_fields_project_spec.rb | 13 ++++++++----- .../project_mappings_spec.rb | 13 ++++++++----- spec/support/pages/projects/index.rb | 18 +++++++++++++++--- 12 files changed, 53 insertions(+), 24 deletions(-) diff --git a/app/components/admin/custom_fields/custom_field_projects/row_component.rb b/app/components/admin/custom_fields/custom_field_projects/row_component.rb index 51ce950bbac0..d7b6ac1d62ee 100644 --- a/app/components/admin/custom_fields/custom_field_projects/row_component.rb +++ b/app/components/admin/custom_fields/custom_field_projects/row_component.rb @@ -57,11 +57,10 @@ def more_menu_detach_project def detach_from_project_url url_helpers.custom_field_project_path( custom_field_id: @table.params[:custom_field].id, - custom_fields_project: { project_id: project.id } + custom_fields_project: { project_id: project.id }, + page: current_page ) end - - def project = model.first end end end diff --git a/app/components/projects/row_component.rb b/app/components/projects/row_component.rb index 70adc2566d7d..3f540eac0ecd 100644 --- a/app/components/projects/row_component.rb +++ b/app/components/projects/row_component.rb @@ -373,5 +373,9 @@ def user_can_view_project? def custom_field_column?(column) column.is_a?(::Queries::Projects::Selects::CustomField) end + + def current_page + table.model.current_page.to_s + end end end diff --git a/app/components/settings/project_custom_fields/project_custom_field_mapping/row_component.rb b/app/components/settings/project_custom_fields/project_custom_field_mapping/row_component.rb index f4645d2a23ff..5ee09b6baf3e 100644 --- a/app/components/settings/project_custom_fields/project_custom_field_mapping/row_component.rb +++ b/app/components/settings/project_custom_fields/project_custom_field_mapping/row_component.rb @@ -35,7 +35,8 @@ class RowComponent < Admin::CustomFields::CustomFieldProjects::RowComponent def detach_from_project_url url_helpers.unlink_admin_settings_project_custom_field_path( id: @table.params[:custom_field].id, - project_custom_field_project_mapping: { project_id: project.id } + project_custom_field_project_mapping: { project_id: project.id }, + page: current_page ) end end diff --git a/app/controllers/admin/custom_fields/custom_field_projects_controller.rb b/app/controllers/admin/custom_fields/custom_field_projects_controller.rb index 8f69e323e74a..e21966dd67b9 100644 --- a/app/controllers/admin/custom_fields/custom_field_projects_controller.rb +++ b/app/controllers/admin/custom_fields/custom_field_projects_controller.rb @@ -103,7 +103,7 @@ def render_project_list(url_for_action: action_name) update_via_turbo_stream( component: Admin::CustomFields::CustomFieldProjects::TableComponent.new( query: available_custom_fields_projects_query, - params: { custom_field: @custom_field, url_for_action: } + params: params.merge({ custom_field: @custom_field, url_for_action: }) ) ) end diff --git a/app/controllers/admin/settings/project_custom_fields_controller.rb b/app/controllers/admin/settings/project_custom_fields_controller.rb index b532962b12f5..5bccb8501fc7 100644 --- a/app/controllers/admin/settings/project_custom_fields_controller.rb +++ b/app/controllers/admin/settings/project_custom_fields_controller.rb @@ -158,7 +158,7 @@ def render_project_list(url_for_action: action_name) update_via_turbo_stream( component: Settings::ProjectCustomFields::ProjectCustomFieldMapping::TableComponent.new( query: project_custom_field_mappings_query, - params: { custom_field: @custom_field, url_for_action: } + params: params.merge({ custom_field: @custom_field, url_for_action: }) ) ) end diff --git a/modules/storages/app/components/storages/project_storages/destroy_confirmation_dialog_component.html.erb b/modules/storages/app/components/storages/project_storages/destroy_confirmation_dialog_component.html.erb index 956475512f2a..cef571b12e07 100644 --- a/modules/storages/app/components/storages/project_storages/destroy_confirmation_dialog_component.html.erb +++ b/modules/storages/app/components/storages/project_storages/destroy_confirmation_dialog_component.html.erb @@ -31,7 +31,7 @@ See COPYRIGHT and LICENSE files for more details. component_wrapper do primer_form_with( model: @project_storage, - url: admin_settings_storage_project_storage_path(id: @project_storage), + url: admin_settings_storage_project_storage_path(id: @project_storage, page: current_page), data: { turbo: true, controller: 'disable-when-checked', diff --git a/modules/storages/app/components/storages/project_storages/destroy_confirmation_dialog_component.rb b/modules/storages/app/components/storages/project_storages/destroy_confirmation_dialog_component.rb index 228e6125fb47..0db817199e7b 100644 --- a/modules/storages/app/components/storages/project_storages/destroy_confirmation_dialog_component.rb +++ b/modules/storages/app/components/storages/project_storages/destroy_confirmation_dialog_component.rb @@ -32,11 +32,12 @@ class DestroyConfirmationDialogComponent < ApplicationComponent include OpTurbo::Streamable include OpPrimer::ComponentHelpers - def initialize(storage:, project_storage:) + def initialize(storage:, project_storage:, params:) super @storage = storage @project_storage = project_storage + @params = params end def id @@ -56,6 +57,10 @@ def text text end + def current_page + @params[:page] + end + def confirmation_text I18n.t("project_storages.remove_project.dialog.confirmation_text") end diff --git a/modules/storages/app/components/storages/project_storages/projects/row_component.rb b/modules/storages/app/components/storages/project_storages/projects/row_component.rb index b144b519edd1..e1995faaeec4 100644 --- a/modules/storages/app/components/storages/project_storages/projects/row_component.rb +++ b/modules/storages/app/components/storages/project_storages/projects/row_component.rb @@ -68,7 +68,8 @@ def more_menu_detach_project icon: :trash, label: I18n.t("project_storages.remove_project.label"), href: destroy_confirmation_dialog_admin_settings_storage_project_storage_path( - id: project_storage.id + id: project_storage.id, + page: current_page ), data: { controller: "async-dialog" diff --git a/modules/storages/app/controllers/storages/admin/storages/project_storages_controller.rb b/modules/storages/app/controllers/storages/admin/storages/project_storages_controller.rb index 3e842e26be0f..f83ffe11d00b 100644 --- a/modules/storages/app/controllers/storages/admin/storages/project_storages_controller.rb +++ b/modules/storages/app/controllers/storages/admin/storages/project_storages_controller.rb @@ -117,7 +117,8 @@ def update def destroy_confirmation_dialog respond_with_dialog Storages::ProjectStorages::DestroyConfirmationDialogComponent.new( storage: @storage, - project_storage: @project_storage + project_storage: @project_storage, + params: ) end @@ -186,7 +187,7 @@ def update_project_list_via_turbo_stream(url_for_action: action_name) component: Storages::ProjectStorages::Projects::TableComponent.new( query: storage_projects_query, storage: @storage, - params: { url_for_action: } + params: params.merge({ url_for_action: }) ) ) end diff --git a/spec/features/admin/custom_fields/custom_fields_project_spec.rb b/spec/features/admin/custom_fields/custom_fields_project_spec.rb index d77540491baa..ad769c064fbf 100644 --- a/spec/features/admin/custom_fields/custom_fields_project_spec.rb +++ b/spec/features/admin/custom_fields/custom_fields_project_spec.rb @@ -119,18 +119,21 @@ end end - it "allows unlinking a project from a custom field" do - project = create(:project) - create(:custom_fields_project, custom_field:, project:) + it "allows unlinking a project from a custom field", with_settings: { per_page_options: "2,5" } do + projects = create_list(:project, 4) + projects.each { |project| create(:custom_fields_project, custom_field:, project:) } - visit custom_field_projects_path(custom_field) + current_page = 3 + visit custom_field_projects_path(custom_field, page: current_page) + project = custom_field_projects_page.project_in_first_row custom_field_projects_page.click_menu_item_of("Remove from project", project) expect(page).to have_no_text(project.name) aggregate_failures "pagination links maintain the correct url after unlinking is done" do - custom_field_projects_page.expect_correct_pagination_links(model: custom_field) + custom_field_projects_page.expect_correct_pagination_links(model: custom_field, current_page:) + custom_field_projects_page.expect_current_page_number(current_page) end end diff --git a/spec/features/admin/project_custom_fields/project_mappings_spec.rb b/spec/features/admin/project_custom_fields/project_mappings_spec.rb index 9742080eee5f..9aaeaa5e48ea 100644 --- a/spec/features/admin/project_custom_fields/project_mappings_spec.rb +++ b/spec/features/admin/project_custom_fields/project_mappings_spec.rb @@ -123,18 +123,21 @@ end end - it "allows unlinking a project from a custom field" do - project = create(:project) - create(:project_custom_field_project_mapping, project_custom_field:, project:) + it "allows unlinking a project from a custom field", with_settings: { per_page_options: "2,5" } do + projects = create_list(:project, 4) + projects.each { |project| create(:project_custom_field_project_mapping, project_custom_field:, project:) } - visit project_mappings_admin_settings_project_custom_field_path(project_custom_field) + current_page = 3 + visit project_mappings_admin_settings_project_custom_field_path(project_custom_field, page: current_page) + project = project_custom_field_mappings_page.project_in_first_row project_custom_field_mappings_page.click_menu_item_of("Remove from project", project) expect(page).to have_no_text(project.name) aggregate_failures "pagination links maintain the correct url after unlinking is done" do - project_custom_field_mappings_page.expect_correct_pagination_links(model: project_custom_field) + project_custom_field_mappings_page.expect_correct_pagination_links(model: project_custom_field, current_page:) + project_custom_field_mappings_page.expect_current_page_number(current_page) end end diff --git a/spec/support/pages/projects/index.rb b/spec/support/pages/projects/index.rb index 9b504a335268..8aa1d8434774 100644 --- a/spec/support/pages/projects/index.rb +++ b/spec/support/pages/projects/index.rb @@ -127,16 +127,23 @@ def expect_page_link(text) end end - def expect_correct_pagination_links(model:) - within ".op-pagination" do + def expect_correct_pagination_links(model:, current_page: 1) + within ".op-pagination--pages" do pagination_links = page.all(".op-pagination--item-link") expect(pagination_links.size).to be_positive - pagination_links.each.with_index(1) do |pagination_link, page_number| + page_number_links = pagination_links.reject { |link| link.text =~ /previous|next/i } + page_number_links.each.with_index(1) do |pagination_link, page_number| uri = URI.parse(pagination_link["href"]) expect(uri.path).to eq(path(model)) expect(uri.query).to include("page=#{page_number}") end + + if current_page > 1 + expect(page).to have_link("Previous", href: "#{path(model)}?#{{ page: current_page - 1 }.to_query}") + else + expect(page).to have_link("Next", href: "#{path(model)}?#{{ page: current_page + 1 }.to_query}") + end end end @@ -544,6 +551,11 @@ def within_table(&) within "#project-table", & end + def project_in_first_row + first_row = within("#projects-table") { find(".op-project-row-component", match: :first) } + Project.find_by!(name: first_row.text.split("\n").first) + end + def within_row(project) row = page.find("#project-#{project.id}") row.hover From 02dd9e1440c8fff06a34795d6a25fbebdf8384c3 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Thu, 26 Sep 2024 16:32:14 +0300 Subject: [PATCH 07/11] tests[Op#57989]: project storage removal should also maintain current page --- .../storages/admin/project_storages_spec.rb | 13 ++++++++++--- spec/support/pages/projects/index.rb | 4 ++-- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/modules/storages/spec/features/storages/admin/project_storages_spec.rb b/modules/storages/spec/features/storages/admin/project_storages_spec.rb index fa0d54eaacc4..20a2d7f97aea 100644 --- a/modules/storages/spec/features/storages/admin/project_storages_spec.rb +++ b/modules/storages/spec/features/storages/admin/project_storages_spec.rb @@ -371,8 +371,15 @@ def call expect(page).to have_text(project.name) end - it "is possible to remove the project after checking the confirmation checkbox in the dialog" do - expect(page).to have_text(project.name) + it "is possible to remove the project after checking the confirmation checkbox in the dialog", + with_settings: { per_page_options: "2,5" } do + projects = create_list(:project, 4) + projects.each { |project| create(:project_storage, storage:, project:) } + + current_page = 3 + visit admin_settings_storage_project_storages_path(storage, page: current_page) + + project = project_storages_index_page.project_in_first_row(column_text_separator: "\t") project_storages_index_page.click_menu_item_of("Remove project", project) # The original DeleteService would try to remove actual files from actual storages, @@ -398,7 +405,7 @@ def call expect(page).to have_no_text(project.name) aggregate_failures "pagination links maintain the correct url" do - project_storages_index_page.expect_correct_pagination_links(model: storage) + project_storages_index_page.expect_correct_pagination_links(model: storage, current_page:) end end end diff --git a/spec/support/pages/projects/index.rb b/spec/support/pages/projects/index.rb index 8aa1d8434774..c33a469d8752 100644 --- a/spec/support/pages/projects/index.rb +++ b/spec/support/pages/projects/index.rb @@ -551,9 +551,9 @@ def within_table(&) within "#project-table", & end - def project_in_first_row + def project_in_first_row(column_text_separator: "\n") first_row = within("#projects-table") { find(".op-project-row-component", match: :first) } - Project.find_by!(name: first_row.text.split("\n").first) + Project.find_by!(name: first_row.text.split(column_text_separator).first) end def within_row(project) From dd5571015002d4b8e6d71dbf202c206ecc443746 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Thu, 26 Sep 2024 16:36:41 +0300 Subject: [PATCH 08/11] fix[Op#57989]: Let params default to empty has in confirmation dialog component Reduce setup fatigue in unit testing. Optional arg --- .../project_storages/destroy_confirmation_dialog_component.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/storages/app/components/storages/project_storages/destroy_confirmation_dialog_component.rb b/modules/storages/app/components/storages/project_storages/destroy_confirmation_dialog_component.rb index 0db817199e7b..ecd4b986e0d4 100644 --- a/modules/storages/app/components/storages/project_storages/destroy_confirmation_dialog_component.rb +++ b/modules/storages/app/components/storages/project_storages/destroy_confirmation_dialog_component.rb @@ -32,7 +32,7 @@ class DestroyConfirmationDialogComponent < ApplicationComponent include OpTurbo::Streamable include OpPrimer::ComponentHelpers - def initialize(storage:, project_storage:, params:) + def initialize(storage:, project_storage:, params: {}) super @storage = storage From 7d9a4e45d7186da58bef1c4617aa793b78bb6a4d Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 27 Sep 2024 11:23:53 +0300 Subject: [PATCH 09/11] tests[Op#57989]: With single records, assert against pagination options Activating multiple-projects returns a turbo stream, and since those can hit different actions, incorrect urls can be sent back. See: https://github.com/opf/openproject/pull/15860 --- .../features/storages/admin/project_storages_spec.rb | 4 ++-- .../custom_fields/custom_fields_project_spec.rb | 2 +- .../project_custom_fields/project_mappings_spec.rb | 2 +- spec/support/pages/projects/index.rb | 12 ++++++++++++ 4 files changed, 16 insertions(+), 4 deletions(-) diff --git a/modules/storages/spec/features/storages/admin/project_storages_spec.rb b/modules/storages/spec/features/storages/admin/project_storages_spec.rb index 20a2d7f97aea..4f7d8f98ffe5 100644 --- a/modules/storages/spec/features/storages/admin/project_storages_spec.rb +++ b/modules/storages/spec/features/storages/admin/project_storages_spec.rb @@ -230,7 +230,7 @@ expect(page).to have_text(subproject.name) aggregate_failures "pagination links maintain the correct url" do - project_storages_index_page.expect_correct_pagination_links(model: storage) + project_storages_index_page.expect_correct_pagination_options(model: storage) end end @@ -303,7 +303,7 @@ end aggregate_failures "pagination links maintain the correct url" do - project_storages_index_page.expect_correct_pagination_links(model: storage) + project_storages_index_page.expect_correct_pagination_options(model: storage) end end diff --git a/spec/features/admin/custom_fields/custom_fields_project_spec.rb b/spec/features/admin/custom_fields/custom_fields_project_spec.rb index ad769c064fbf..a12a4d901812 100644 --- a/spec/features/admin/custom_fields/custom_fields_project_spec.rb +++ b/spec/features/admin/custom_fields/custom_fields_project_spec.rb @@ -115,7 +115,7 @@ expect(page).to have_text(subproject.name) aggregate_failures "pagination links maintain the correct url" do - custom_field_projects_page.expect_correct_pagination_links(model: custom_field) + custom_field_projects_page.expect_correct_pagination_options(model: custom_field) end end diff --git a/spec/features/admin/project_custom_fields/project_mappings_spec.rb b/spec/features/admin/project_custom_fields/project_mappings_spec.rb index 9aaeaa5e48ea..880222955d46 100644 --- a/spec/features/admin/project_custom_fields/project_mappings_spec.rb +++ b/spec/features/admin/project_custom_fields/project_mappings_spec.rb @@ -119,7 +119,7 @@ expect(page).to have_text(subproject.name) aggregate_failures "pagination links maintain the correct url" do - project_custom_field_mappings_page.expect_correct_pagination_links(model: project_custom_field) + project_custom_field_mappings_page.expect_correct_pagination_options(model: project_custom_field) end end diff --git a/spec/support/pages/projects/index.rb b/spec/support/pages/projects/index.rb index c33a469d8752..8a2fe2aa999d 100644 --- a/spec/support/pages/projects/index.rb +++ b/spec/support/pages/projects/index.rb @@ -147,6 +147,18 @@ def expect_correct_pagination_links(model:, current_page: 1) end end + def expect_correct_pagination_options(model:) + within ".op-pagination--options" do + pagination_links = page.all(".op-pagination--item-link") + expect(pagination_links.size).to be_positive + + pagination_links.each do |pagination_link| + uri = URI.parse(pagination_link["href"]) + expect(uri.path).to eq(path(model)) + end + end + end + def expect_filters_container_toggled expect(page).to have_css(".op-filters-form") end From f0f8bf247a23c6798765fd98e1c7546cfe7bb9bb Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 27 Sep 2024 12:00:49 +0300 Subject: [PATCH 10/11] chore[Op#57989]: use consistent naming with existing methods Existing methods: `#expect_page_link` -> follow this naming convention, just introduce collection assertions -> `#expect_page_links` -> `#expect_page_sizes` --- .../spec/features/storages/admin/project_storages_spec.rb | 6 +++--- .../admin/custom_fields/custom_fields_project_spec.rb | 4 ++-- .../admin/project_custom_fields/project_mappings_spec.rb | 4 ++-- spec/support/pages/projects/index.rb | 5 +++-- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/modules/storages/spec/features/storages/admin/project_storages_spec.rb b/modules/storages/spec/features/storages/admin/project_storages_spec.rb index 4f7d8f98ffe5..9057b15def07 100644 --- a/modules/storages/spec/features/storages/admin/project_storages_spec.rb +++ b/modules/storages/spec/features/storages/admin/project_storages_spec.rb @@ -230,7 +230,7 @@ expect(page).to have_text(subproject.name) aggregate_failures "pagination links maintain the correct url" do - project_storages_index_page.expect_correct_pagination_options(model: storage) + project_storages_index_page.expect_page_sizes(model: storage) end end @@ -303,7 +303,7 @@ end aggregate_failures "pagination links maintain the correct url" do - project_storages_index_page.expect_correct_pagination_options(model: storage) + project_storages_index_page.expect_page_sizes(model: storage) end end @@ -405,7 +405,7 @@ def call expect(page).to have_no_text(project.name) aggregate_failures "pagination links maintain the correct url" do - project_storages_index_page.expect_correct_pagination_links(model: storage, current_page:) + project_storages_index_page.expect_page_links(model: storage, current_page:) end end end diff --git a/spec/features/admin/custom_fields/custom_fields_project_spec.rb b/spec/features/admin/custom_fields/custom_fields_project_spec.rb index a12a4d901812..7726caf0d324 100644 --- a/spec/features/admin/custom_fields/custom_fields_project_spec.rb +++ b/spec/features/admin/custom_fields/custom_fields_project_spec.rb @@ -115,7 +115,7 @@ expect(page).to have_text(subproject.name) aggregate_failures "pagination links maintain the correct url" do - custom_field_projects_page.expect_correct_pagination_options(model: custom_field) + custom_field_projects_page.expect_page_sizes(model: custom_field) end end @@ -132,7 +132,7 @@ expect(page).to have_no_text(project.name) aggregate_failures "pagination links maintain the correct url after unlinking is done" do - custom_field_projects_page.expect_correct_pagination_links(model: custom_field, current_page:) + custom_field_projects_page.expect_page_links(model: custom_field, current_page:) custom_field_projects_page.expect_current_page_number(current_page) end end diff --git a/spec/features/admin/project_custom_fields/project_mappings_spec.rb b/spec/features/admin/project_custom_fields/project_mappings_spec.rb index 880222955d46..97769b0e80a9 100644 --- a/spec/features/admin/project_custom_fields/project_mappings_spec.rb +++ b/spec/features/admin/project_custom_fields/project_mappings_spec.rb @@ -119,7 +119,7 @@ expect(page).to have_text(subproject.name) aggregate_failures "pagination links maintain the correct url" do - project_custom_field_mappings_page.expect_correct_pagination_options(model: project_custom_field) + project_custom_field_mappings_page.expect_page_sizes(model: project_custom_field) end end @@ -136,7 +136,7 @@ expect(page).to have_no_text(project.name) aggregate_failures "pagination links maintain the correct url after unlinking is done" do - project_custom_field_mappings_page.expect_correct_pagination_links(model: project_custom_field, current_page:) + project_custom_field_mappings_page.expect_page_links(model: project_custom_field, current_page:) project_custom_field_mappings_page.expect_current_page_number(current_page) end end diff --git a/spec/support/pages/projects/index.rb b/spec/support/pages/projects/index.rb index 8a2fe2aa999d..d83f6f04ac43 100644 --- a/spec/support/pages/projects/index.rb +++ b/spec/support/pages/projects/index.rb @@ -127,7 +127,7 @@ def expect_page_link(text) end end - def expect_correct_pagination_links(model:, current_page: 1) + def expect_page_links(model:, current_page: 1) within ".op-pagination--pages" do pagination_links = page.all(".op-pagination--item-link") expect(pagination_links.size).to be_positive @@ -147,10 +147,11 @@ def expect_correct_pagination_links(model:, current_page: 1) end end - def expect_correct_pagination_options(model:) + def expect_page_sizes(model:) within ".op-pagination--options" do pagination_links = page.all(".op-pagination--item-link") expect(pagination_links.size).to be_positive + expect(page).to have_css(".op-pagination--item_current") pagination_links.each do |pagination_link| uri = URI.parse(pagination_link["href"]) From 998eb22dd3ee3b5db016c00c0c6a317272558925 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 27 Sep 2024 14:07:37 +0300 Subject: [PATCH 11/11] docs[Op#57989]: Improve `PaginationHelper#turbo_action` dev helper doc --- app/helpers/pagination_helper.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/helpers/pagination_helper.rb b/app/helpers/pagination_helper.rb index 81bf2516eb31..28e9f79f3c80 100644 --- a/app/helpers/pagination_helper.rb +++ b/app/helpers/pagination_helper.rb @@ -215,6 +215,12 @@ def allowed_params # "advance" - push a new entry onto the history stack. # "replace" - replace the current history entry. # See: https://turbo.hotwired.dev/reference/attributes + # + # Example: Promoting a Frame Navigation to a Page Visit + # By default navigation within a turbo frame does not change the rest of the browser's state, + # but you can promote a frame navigation a "Visit" by setting the turbo-action attribute to "advance". + # See: https://turbo.hotwired.dev/handbook/frames#promoting-a-frame-navigation-to-a-page-visit + # def turbo_action @options[:turbo_action] # rubocop:disable Rails/HelperInstanceVariable end