Merge pull request #16741 from opf/bug/55392-page-number-not-reset-wh…

…en-changing-the-sort-order-via-the-configure-view-modal

[#55392] page number should reset when changing the sort order via the configure view modal

app/components/projects/configure_view_modal_component.html.erb

@@ -14,7 +14,7 @@
           method: :get
         ) do |form| %>
       <% helpers.projects_query_params.except(:columns, :sortBy).each do |name, value| %>
-        <%= hidden_field_tag name, value %>
+        <%= hidden_field_tag name, value, data: {"sort-by-config-target" => name} %>
       <% end %>
       <%= render(Primer::Alpha::TabPanels.new(label: "label")) do |tab_panel| %>
         <% tab_panel.with_tab(selected: true, id: "tab-selects--columns") do |tab| %>
@@ -47,6 +47,9 @@
   <% end %>
   <%= render(Primer::Alpha::Dialog::Footer.new) do %>
     <%= render(Primer::ButtonComponent.new(data: { "close-dialog-id": MODAL_ID })) { I18n.t(:button_cancel) } %>
-    <%= render(Primer::ButtonComponent.new(scheme: :primary, type: :submit, form: QUERY_FORM_ID)) { I18n.t(:button_apply) } %>
+    <%= render(Primer::ButtonComponent.new(scheme: :primary,
+                                           type: :submit,
+                                           data: { "test-selector": "#{MODAL_ID}-submit"},
+                                           form: QUERY_FORM_ID)) { I18n.t(:button_apply) } %>
   <% end %>
 <% end %>

app/controllers/projects_controller.rb

@@ -74,7 +74,7 @@ def index # rubocop:disable Format/AbcSize
         )
         replace_via_turbo_stream(component: Projects::TableComponent.new(query: @query, current_user:, params:))
 
-        current_url = url_for(params.permit(:conroller, :action, :query_id, :filters, :columns, :sortBy, :page, :per_page))
+        current_url = url_for(params.permit(:controller, :action, :query_id, :filters, :columns, :sortBy, :page, :per_page))
         turbo_streams << turbo_stream.push_state(current_url)
         turbo_streams << turbo_stream.turbo_frame_set_src(
           "projects_sidemenu",

app/helpers/pagination_helper.rb

@@ -111,25 +111,18 @@ def per_page_links(paginator, options)
   #  Prefers page over the other two and
   #  calculates page in it's absence based on limit and offset.
   #  Return 1 if all else fails.
-
   def page_param(options = params)
     page = if options[:page]
-
              options[:page].to_i
-
            elsif options[:offset] && options[:limit]
-
              begin
                # + 1 as page is not 0 but 1 based
                (options[:offset].to_i / per_page_param(options)) + 1
              rescue ZeroDivisionError
                1
              end
-
            else
-
              1
-
            end
 
     if page > 0
@@ -141,12 +134,11 @@ def page_param(options = params)
 
   # Returns per_page option used for pagination
   # based on:
-  #  * per_page session value
   #  * per_page options value
+  #  * per_page session value
   #  * limit options value
   #  in that order
   #  Return smallest possible setting if all else fails.
-
   def per_page_param(options = params)
     per_page_candidates = [options[:per_page].to_i, session[:per_page].to_i, options[:limit].to_i]
 

frontend/src/stimulus/controllers/dynamic/sort-by-config.controller.ts

@@ -50,13 +50,21 @@ export default class SortByConfigController extends Controller {
   declare readonly inputRowTargets:HTMLElement[];
   declare readonly inputRowContainerTarget:HTMLElement;
 
+  declare parentForm:HTMLFormElement | null;
+  declare pageTarget:HTMLInputElement;
+
+  declare initialSortBy:string;
+
   connect():void {
     this.inputRowTargets.forEach((row) => {
       this.manageRow(row);
     });
 
     this.displayNewFieldSelectorIfNeeded();
     this.disableSelectedFieldsForOtherSelects();
+
+    this.initialSortBy = this.sortByFieldTarget.value;
+    this.registerPaginationResetHandler();
   }
 
   buildSortJson():string {
@@ -69,6 +77,43 @@ export default class SortByConfigController extends Controller {
     return JSON.stringify(compact(filters));
   }
 
+  // Tries to find the parent form in the DOM. If present and the form contains a `page` field marked
+  // with the proper target, will register a handler to trigger a pagination reset when the sorting
+  // changes.
+  registerPaginationResetHandler():void {
+    this.parentForm = this.sortByFieldTarget.closest('form');
+
+    if (this.parentForm) {
+      this.pageTarget = this.parentForm.querySelector('input[data-sort-by-config-target="page"]') as HTMLInputElement;
+
+      if (this.pageTarget) {
+        this.parentForm.addEventListener('submit', this.onFormSubmit.bind(this));
+      }
+    }
+  }
+
+  onFormSubmit(event:SubmitEvent):void {
+    if (!this.parentForm || !this.pageTarget) { return; }
+    event.preventDefault();
+
+    this.resetPaginationIfSortingChanged();
+
+    this.parentForm.submit();
+  }
+
+  // When the sorting criteria changes, reset the pagination to the first page.
+  // This leads to a better UX since the previous position in the pagination becomes
+  // obsolete when the underlying collection changes due to sorting adjustments. It is better to start
+  // from a fresh perspective on the data.
+  resetPaginationIfSortingChanged():void {
+    const currentSelection = this.sortByFieldTarget.value;
+
+    if (this.initialSortBy !== currentSelection) {
+      // Reset the pagination:
+      this.pageTarget.value = '';
+    }
+  }
+
   fieldChanged(event:Event):void {
     const target = event.target as HTMLElement;
     const row = target.closest('div[data-sort-by-config-target="inputRow"]') as HTMLElement;

spec/features/projects/projects_index_spec.rb

@@ -1190,6 +1190,45 @@ def load_and_open_filters(user)
         projects_page.expect_number_of_sort_fields(1)
       end
 
+      it "resets the pagination when sorting (bug #55392)" do
+        # We need pagination, so reduce the page size to enable it
+        allow(Setting).to receive(:per_page_options_array).and_return([1, 5])
+        projects_page.set_page_size(1)
+        wait_for_reload
+        projects_page.expect_page_size(1)
+        projects_page.go_to_page(2) # Go to another page that is not the first one
+        wait_for_reload
+        projects_page.expect_current_page_number(2)
+
+        # Open config dialog and make changes to the sorting
+        projects_page.open_configure_view
+        projects_page.switch_configure_view_tab(I18n.t("label_sort"))
+        projects_page.within_sort_row(0) do
+          projects_page.change_sort_order(column_identifier: :name, direction: :desc)
+        end
+
+        # Save and close the dialog
+        projects_page.submit_config_view_dialog
+        wait_for_reload
+
+        # Changing the sorting resets the pagination to the first page
+        projects_page.expect_current_page_number(1)
+
+        # Go to another page again
+        projects_page.go_to_page(2)
+        wait_for_reload
+        projects_page.expect_current_page_number(2)
+
+        # Open dialog, do not change anything and save
+        projects_page.open_configure_view
+        projects_page.switch_configure_view_tab(I18n.t("label_sort"))
+        projects_page.submit_config_view_dialog
+        wait_for_reload
+
+        # An unchanged sorting will keep the current position in the pagination
+        projects_page.expect_current_page_number(2)
+      end
+
       it "does not allow to sort via long text custom fields" do
         long_text_custom_field = create(:text_project_custom_field)
         Setting.enabled_projects_columns += [long_text_custom_field.column_name]

spec/support/pages/projects/index.rb

@@ -523,6 +523,10 @@ def within_sort_row(index, &)
         within(field_component, &)
       end
 
+      def submit_config_view_dialog
+        page.find('[data-test-selector="op-project-list-configure-dialog-submit"]').click
+      end
+
       def within_table(&)
         within "#project-table", &
       end