From 11f0fd606550bef78f37e98bae42c3e5c27ac4e4 Mon Sep 17 00:00:00 2001 From: Tobias Dillmann Date: Tue, 17 Sep 2024 08:24:09 +0200 Subject: [PATCH 1/5] [#55392] empty line cleanup The empty line between comment and method turned the doc comments into regular comments. Fixed that. #page_param had additional, unneeded empty lines that are inconsistent with the rest of the code base and made the code harder to read. Fixed. --- app/helpers/pagination_helper.rb | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/app/helpers/pagination_helper.rb b/app/helpers/pagination_helper.rb index 7792aca72f6d..c8a53aaadc42 100644 --- a/app/helpers/pagination_helper.rb +++ b/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] From 3ca936a1cdd3802712e6ad1a8781b48f43687ba6 Mon Sep 17 00:00:00 2001 From: Tobias Dillmann Date: Tue, 17 Sep 2024 08:51:18 +0200 Subject: [PATCH 2/5] [#55392] fix typo in params name --- app/controllers/projects_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 277bdfdb2984..cca50ade55b8 100644 --- a/app/controllers/projects_controller.rb +++ b/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", From ceabb80c47da26f841c52271c7590028ef75c359 Mon Sep 17 00:00:00 2001 From: Tobias Dillmann Date: Thu, 19 Sep 2024 09:21:36 +0200 Subject: [PATCH 3/5] [#55392] project list: pagination reset by dedicated controller --- .../configure_view_modal_component.html.erb | 9 ++- .../queries/sort_by_component.html.erb | 4 +- .../pagination-reset-on-sort.controller.ts | 64 +++++++++++++++++++ 3 files changed, 74 insertions(+), 3 deletions(-) create mode 100644 frontend/src/stimulus/controllers/dynamic/pagination-reset-on-sort.controller.ts diff --git a/app/components/projects/configure_view_modal_component.html.erb b/app/components/projects/configure_view_modal_component.html.erb index 691908997ac1..1c954bcf2099 100644 --- a/app/components/projects/configure_view_modal_component.html.erb +++ b/app/components/projects/configure_view_modal_component.html.erb @@ -1,6 +1,7 @@ <%= render(Primer::Alpha::Dialog.new(title: t(:'queries.configure_view.heading'), size: :large, id: MODAL_ID, + data: { controller: "pagination-reset-on-sort", application_target: "dynamic" }, # Hack to give the draggable autcompleter (ng-select) bound to the dialog # enough height to display all options. # This is necessary as long as ng-select does not support popovers. @@ -10,11 +11,15 @@ <%= primer_form_with( url: projects_path, id: QUERY_FORM_ID, - data: { "turbo-stream" => false }, + data: { + "turbo-stream" => false, + "pagination-reset-on-sort-target" => "form", + action: "pagination-reset-on-sort#submit" + }, 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: {"pagination-reset-on-sort-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| %> diff --git a/app/components/queries/sort_by_component.html.erb b/app/components/queries/sort_by_component.html.erb index 041fe0328bf2..ff4548f6a53d 100644 --- a/app/components/queries/sort_by_component.html.erb +++ b/app/components/queries/sort_by_component.html.erb @@ -1,7 +1,9 @@ <%= render(Primer::Beta::Heading.new(tag: :h5)) { I18n.t('queries.configure_view.sort_by.automatic.heading') } %> <%= render(Primer::Beta::Text.new(font_size: :small, color: :subtle)) { I18n.t('queries.configure_view.sort_by.automatic.description', plural: queried_model_name.plural) } %>
- <%= hidden_field_tag :sortBy, current_orders, data: { "sort-by-config-target" => "sortByField" } %> + <%= hidden_field_tag :sortBy, current_orders, data: { + "sort-by-config-target" => "sortByField", + "pagination-reset-on-sort-target" => "sortByField" } %> <%= render(Primer::OpenProject::FlexLayout.new(data: { "sort-by-config-target" => "inputRowContainer" })) do |layout| %> <% order_limit.times do |i| %> <% layout.with_row(mt: 3, data: { "sort-by-config-target" => "inputRow" }) do %> diff --git a/frontend/src/stimulus/controllers/dynamic/pagination-reset-on-sort.controller.ts b/frontend/src/stimulus/controllers/dynamic/pagination-reset-on-sort.controller.ts new file mode 100644 index 000000000000..3654eef6c2cf --- /dev/null +++ b/frontend/src/stimulus/controllers/dynamic/pagination-reset-on-sort.controller.ts @@ -0,0 +1,64 @@ +/* + * -- 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. + * ++ + */ + +import { Controller } from '@hotwired/stimulus'; + +export default class PaginationResetOnSort extends Controller { + static targets = [ + 'sortByField', + 'form', + 'page', + ]; + + declare readonly sortByFieldTarget:HTMLInputElement; + declare readonly formTarget:HTMLFormElement; + declare readonly pageTarget:HTMLInputElement; + + declare sortByFieldInitialSelection:string; + + connect():void { + this.sortByFieldInitialSelection = this.sortByFieldTarget.value; + } + + submit(event:Event):void { + event.preventDefault(); + const currentSelection = this.sortByFieldTarget.value; + + if (this.sortByFieldInitialSelection !== currentSelection) { + // 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. + this.pageTarget.value = ''; + } + + this.formTarget.submit(); + } +} From fba16edef140d682064b70ff5e2e9584222657fc Mon Sep 17 00:00:00 2001 From: Tobias Dillmann Date: Thu, 19 Sep 2024 15:50:09 +0200 Subject: [PATCH 4/5] [#55392] project list: ensure pagination reset on sort change --- .../configure_view_modal_component.html.erb | 9 +-- .../queries/sort_by_component.html.erb | 4 +- .../pagination-reset-on-sort.controller.ts | 64 ------------------- .../dynamic/sort-by-config.controller.ts | 45 +++++++++++++ 4 files changed, 48 insertions(+), 74 deletions(-) delete mode 100644 frontend/src/stimulus/controllers/dynamic/pagination-reset-on-sort.controller.ts diff --git a/app/components/projects/configure_view_modal_component.html.erb b/app/components/projects/configure_view_modal_component.html.erb index 1c954bcf2099..ed3df3f462e0 100644 --- a/app/components/projects/configure_view_modal_component.html.erb +++ b/app/components/projects/configure_view_modal_component.html.erb @@ -1,7 +1,6 @@ <%= render(Primer::Alpha::Dialog.new(title: t(:'queries.configure_view.heading'), size: :large, id: MODAL_ID, - data: { controller: "pagination-reset-on-sort", application_target: "dynamic" }, # Hack to give the draggable autcompleter (ng-select) bound to the dialog # enough height to display all options. # This is necessary as long as ng-select does not support popovers. @@ -11,15 +10,11 @@ <%= primer_form_with( url: projects_path, id: QUERY_FORM_ID, - data: { - "turbo-stream" => false, - "pagination-reset-on-sort-target" => "form", - action: "pagination-reset-on-sort#submit" - }, + data: { "turbo-stream" => false }, method: :get ) do |form| %> <% helpers.projects_query_params.except(:columns, :sortBy).each do |name, value| %> - <%= hidden_field_tag name, value, data: {"pagination-reset-on-sort-target" => name} %> + <%= 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| %> diff --git a/app/components/queries/sort_by_component.html.erb b/app/components/queries/sort_by_component.html.erb index ff4548f6a53d..041fe0328bf2 100644 --- a/app/components/queries/sort_by_component.html.erb +++ b/app/components/queries/sort_by_component.html.erb @@ -1,9 +1,7 @@ <%= render(Primer::Beta::Heading.new(tag: :h5)) { I18n.t('queries.configure_view.sort_by.automatic.heading') } %> <%= render(Primer::Beta::Text.new(font_size: :small, color: :subtle)) { I18n.t('queries.configure_view.sort_by.automatic.description', plural: queried_model_name.plural) } %>
- <%= hidden_field_tag :sortBy, current_orders, data: { - "sort-by-config-target" => "sortByField", - "pagination-reset-on-sort-target" => "sortByField" } %> + <%= hidden_field_tag :sortBy, current_orders, data: { "sort-by-config-target" => "sortByField" } %> <%= render(Primer::OpenProject::FlexLayout.new(data: { "sort-by-config-target" => "inputRowContainer" })) do |layout| %> <% order_limit.times do |i| %> <% layout.with_row(mt: 3, data: { "sort-by-config-target" => "inputRow" }) do %> diff --git a/frontend/src/stimulus/controllers/dynamic/pagination-reset-on-sort.controller.ts b/frontend/src/stimulus/controllers/dynamic/pagination-reset-on-sort.controller.ts deleted file mode 100644 index 3654eef6c2cf..000000000000 --- a/frontend/src/stimulus/controllers/dynamic/pagination-reset-on-sort.controller.ts +++ /dev/null @@ -1,64 +0,0 @@ -/* - * -- 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. - * ++ - */ - -import { Controller } from '@hotwired/stimulus'; - -export default class PaginationResetOnSort extends Controller { - static targets = [ - 'sortByField', - 'form', - 'page', - ]; - - declare readonly sortByFieldTarget:HTMLInputElement; - declare readonly formTarget:HTMLFormElement; - declare readonly pageTarget:HTMLInputElement; - - declare sortByFieldInitialSelection:string; - - connect():void { - this.sortByFieldInitialSelection = this.sortByFieldTarget.value; - } - - submit(event:Event):void { - event.preventDefault(); - const currentSelection = this.sortByFieldTarget.value; - - if (this.sortByFieldInitialSelection !== currentSelection) { - // 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. - this.pageTarget.value = ''; - } - - this.formTarget.submit(); - } -} diff --git a/frontend/src/stimulus/controllers/dynamic/sort-by-config.controller.ts b/frontend/src/stimulus/controllers/dynamic/sort-by-config.controller.ts index 1a8065327216..ae2b4c25ee16 100644 --- a/frontend/src/stimulus/controllers/dynamic/sort-by-config.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sort-by-config.controller.ts @@ -50,6 +50,11 @@ 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); @@ -57,6 +62,9 @@ export default class SortByConfigController extends Controller { 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; From f5e1ecd1c4e9935e4d7760de207da546d984f3cb Mon Sep 17 00:00:00 2001 From: Tobias Dillmann Date: Fri, 20 Sep 2024 09:11:19 +0200 Subject: [PATCH 5/5] [#55392] feature spec for project list pagination --- .../configure_view_modal_component.html.erb | 5 ++- spec/features/projects/projects_index_spec.rb | 39 +++++++++++++++++++ spec/support/pages/projects/index.rb | 4 ++ 3 files changed, 47 insertions(+), 1 deletion(-) diff --git a/app/components/projects/configure_view_modal_component.html.erb b/app/components/projects/configure_view_modal_component.html.erb index ed3df3f462e0..bc7b0a84c24e 100644 --- a/app/components/projects/configure_view_modal_component.html.erb +++ b/app/components/projects/configure_view_modal_component.html.erb @@ -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 %> diff --git a/spec/features/projects/projects_index_spec.rb b/spec/features/projects/projects_index_spec.rb index 76426bfee7ee..9ddb9d5fecbc 100644 --- a/spec/features/projects/projects_index_spec.rb +++ b/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] diff --git a/spec/support/pages/projects/index.rb b/spec/support/pages/projects/index.rb index c5c81901eeea..66810eb38d40 100644 --- a/spec/support/pages/projects/index.rb +++ b/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