Merge pull request #16793 from opf/bug/57989-removing-a-custom-field-…

…from-a-project-within-nested-pagination-does-not-retain-the-current-page

Bug[Op#57989]: unlinking a project within a multi-activation page does not retain the current page

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

app/components/admin/custom_fields/custom_field_projects/table_component.rb

@@ -30,7 +30,7 @@ module Admin
   module CustomFields
     module CustomFieldProjects
       class TableComponent < Projects::TableComponent
-        include OpTurbo::Streamable
+        include ::Projects::Concerns::TableComponent::StreamablePaginationLinksConstraints
 
         def columns
           @columns ||= query.selects.reject { |select| select.is_a?(Queries::Selects::NotExistingSelect) }
@@ -39,23 +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
-          return super unless params[:url_for_action]
-
-          super.merge(params: { action: params[:url_for_action] })
-        end
       end
     end
   end

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

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

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

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

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

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,20 @@ 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
+    #
+    # 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
+
     def turbo?
       @options[:turbo] # rubocop:disable Rails/HelperInstanceVariable
     end

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',

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

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"

modules/storages/app/components/storages/project_storages/projects/table_component.rb

@@ -32,7 +32,7 @@
 # for every "column" defined below.
 module Storages::ProjectStorages::Projects
   class TableComponent < Projects::TableComponent
-    include OpTurbo::Streamable
+    include ::Projects::Concerns::TableComponent::StreamablePaginationLinksConstraints
 
     options :storage
 

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

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_page_sizes(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_page_sizes(model: storage)
+        end
       end
 
       context "when oauth access has not been granted and manual selection" do
@@ -363,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,
@@ -388,6 +403,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_page_links(model: storage, current_page:)
+        end
       end
     end
   end

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
 

spec/features/admin/custom_fields/custom_fields_project_spec.rb

@@ -115,38 +115,25 @@
       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_page_sizes(model: custom_field)
       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
-        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_page_links(model: custom_field, current_page:)
+        custom_field_projects_page.expect_current_page_number(current_page)
       end
     end
 

spec/features/admin/project_custom_fields/project_mappings_spec.rb

@@ -119,38 +119,25 @@
       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_page_sizes(model: project_custom_field)
       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
-        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_page_links(model: project_custom_field, current_page:)
+        project_custom_field_mappings_page.expect_current_page_number(current_page)
       end
     end