Merge pull request opf#15680 from opf/bug/53329-query-lost-when-sorti…

…ng-the-project-table-quickly

Bug/53329 query lost when sorting the project table quickly

app/components/projects/configure_view_modal_component.html.erb

@@ -10,12 +10,11 @@
     <%= primer_form_with(
           url: projects_path,
           id: QUERY_FORM_ID,
-          method: :get,
-          data: {
-            controller: "params-from-query",
-            'application-target': "dynamic",
-            'params-from-query-allowed-value': '["filters", "query_id", "page", "per_page"]'
-          }) do |form| %>
+          method: :get
+        ) do |form| %>
+      <% helpers.projects_query_params.except(:columns, :sortBy).each do |name, value| %>
+        <%= hidden_field_tag name, value %>
+      <% end %>
       <%= render(Primer::Alpha::TabPanels.new(label: "label")) do |tab_panel| %>
         <% tab_panel.with_tab(selected: true, id: "tab-selects") do |tab| %>
           <% tab.with_text { I18n.t("label_columns") } %>

app/components/projects/export_list_modal_component.html.erb

@@ -2,14 +2,10 @@
                                      id: MODAL_ID)) do |d| %>
   <% d.with_header(variant: :large) %>
   <% d.with_body do %>
-    <ul class="op-export-options"
-        data-controller="params-from-query"
-        data-application-target="dynamic"
-        data-params-from-query-all-anchors-value="true"
-        data-params-from-query-allowed-value='["query_id", "filters", "sortBy", "columns"]'>
+    <ul class="op-export-options">
       <% helpers.supported_export_formats.each do |key| %>
         <li class="op-export-options--option">
-          <%= link_to url_for(action: 'index', format: key),
+          <%= link_to projects_path(format: key, **helpers.projects_query_params.except(:page, :per_page)),
                       class: 'op-export-options--option-link' do %>
             <%= helpers.op_icon("icon-big icon-export-#{key}") %>
             <span class="op-export-options--option-label"><%= t("export.format.#{key}") %></span>

app/components/projects/index_page_header_component.html.erb

@@ -9,15 +9,15 @@
           header:,
           message: t("lists.can_be_saved"),
           label: t("button_save"),
-          href: projects_query_path(query),
+          href: projects_query_path(query, projects_query_params),
           method: :patch
         )
       elsif can_save_as?
         header_save_action(
           header:,
           message: t("lists.can_be_saved_as"),
           label: t("button_save_as"),
-          href: new_projects_query_path
+          href: new_projects_query_path(projects_query_params)
         )
       end
 
@@ -59,11 +59,20 @@
         end
 
         if can_save?
-          menu_save_item(menu:, label: t('button_save'), href: projects_query_path(query), method: :patch)
+          menu_save_item(
+            menu:,
+            label: t('button_save'),
+            href: projects_query_path(query, projects_query_params),
+            method: :patch
+          )
         end
 
         if may_save_as?
-          menu_save_item(menu:, label: t('button_save_as'), href: new_projects_query_path)
+          menu_save_item(
+            menu:,
+            label: t('button_save_as'),
+            href: new_projects_query_path(projects_query_params)
+          )
         end
 
         menu.with_item(
@@ -123,15 +132,15 @@
     render(Primer::OpenProject::PageHeader.new) do |header|
       header.with_title(data: { 'test-selector': 'project-query-name'}) do
         primer_form_with(model: query,
-                         url: @query.new_record? ? projects_queries_path : projects_query_path(@query),
+                         url: @query.new_record? ? projects_queries_path(projects_query_params) : projects_query_path(@query, projects_query_params),
                          scope: 'query',
-                         data: {
-                           controller: "params-from-query",
-                           'application-target': "dynamic",
-                           'params-from-query-allowed-value': '["filters", "columns", "query_id", "sortBy"]'
-                         },
                          id: 'project-save-form') do |f|
-          render(Queries::Projects::Form.new(f, query:))
+          render(
+            Queries::Projects::Form.new(
+              f,
+              cancel_url: projects_path(**projects_query_params, **{ query_id: query.id }.compact)
+            )
+          )
         end
       end
       header.with_breadcrumbs(breadcrumb_items)

app/components/projects/index_page_header_component.rb

@@ -39,6 +39,8 @@ class Projects::IndexPageHeaderComponent < ApplicationComponent
 
   STATE_OPTIONS = %i[show edit rename].freeze
 
+  delegate :projects_query_params, to: :helpers
+
   def initialize(current_user:, query:, params:, state: :show)
     super
 
@@ -136,12 +138,7 @@ def header_save_action(header:, message:, label:, href:, method: nil)
       mobile_icon: nil, # Do not show on mobile as it is already part of the menu
       mobile_label: nil,
       href:,
-      data: {
-        method:,
-        controller: "params-from-query",
-        "application-target": "dynamic",
-        "params-from-query-allowed-value": '["filters", "columns", "sortBy", "query_id"]'
-      }.compact
+      data: { method: }
     ) do
       render(
         Primer::Beta::Octicon.new(
@@ -159,12 +156,7 @@ def menu_save_item(menu:, label:, href:, method: nil)
       label:,
       href:,
       content_arguments: {
-        data: {
-          method:,
-          controller: "params-from-query",
-          "application-target": "dynamic",
-          "params-from-query-allowed-value": '["filters", "columns", "sortBy", "query_id"]'
-        }.compact
+        data: { method: }
       }
     ) do |item|
       item.with_leading_visual_icon(icon: :"op-save")

app/components/projects/table_component.html.erb

@@ -44,11 +44,7 @@ See COPYRIGHT and LICENSE files for more details.
               <% if column.attribute == :hierarchy %>
                 <th id="project-table--hierarchy-header">
                   <div class="generic-table--sort-header-outer generic-table--sort-header-outer_no-highlighting">
-                    <div class="generic-table--sort-header"
-                         data-controller="params-from-query"
-                         data-application-target="dynamic"
-                         data-params-from-query-all-anchors-value="true"
-                         data-params-from-query-allowed-value='["query_id", "per_page", "filters", "columns"]'>
+                    <div class="generic-table--sort-header">
                       <%= content_tag :a,
                                       helpers.op_icon("icon-hierarchy"),
                                       href: href_only_when_not_sort_lft,
@@ -102,10 +98,5 @@ See COPYRIGHT and LICENSE files for more details.
 </div>
 
 <% if paginated? %>
-  <div data-controller="params-from-query"
-       data-application-target="dynamic"
-       data-params-from-query-all-anchors-value="true"
-       data-params-from-query-allowed-value='["query_id", "columns"]'>
-    <%= helpers.pagination_links_full model, { blocked_url_params: [:query_id, :columns] } %>
-  </div>
+  <%= helpers.pagination_links_full model, allowed_params: %i[query_id filters columns sortBy] %>
 <% end %>

app/components/projects/table_component.rb

@@ -58,7 +58,7 @@ def container_class
     ##
     # The project sort by is handled differently
     def build_sort_header(column, options)
-      helpers.projects_sort_header_tag(column, options.merge(param: :json))
+      helpers.projects_sort_header_tag(column, **options, param: :json)
     end
 
     # We don't return the project row
@@ -90,20 +90,16 @@ def deactivate_class_on_lft_sort
 
     def href_only_when_not_sort_lft
       unless sorted_by_lft?
-        projects_path(sortBy: JSON::dump([["lft", "asc"]]))
+        projects_path(
+          sortBy: JSON.dump([%w[lft asc]]),
+          **helpers.projects_query_params.slice(*helpers.projects_query_param_names_for_sort)
+        )
       end
     end
 
     def order_options(select)
       {
-        caption: select.caption,
-        data:
-          {
-            controller: "params-from-query",
-            "application-target": "dynamic",
-            "params-from-query-allowed-value": '["query_id"]',
-            "params-from-query-all-anchors-value": "true"
-          }
+        caption: select.caption
       }
     end
 

app/components/table_component.html.erb

@@ -40,7 +40,7 @@ See COPYRIGHT and LICENSE files for more details.
         <tr>
           <% headers.each do |name, options| %>
             <% if sortable_column?(name) %>
-              <%= helpers.sort_header_tag name, options %>
+              <%= helpers.sort_header_tag(name, **options) %>
             <% else %>
               <th>
                 <div class="generic-table--sort-header-outer">

app/forms/queries/projects/form.rb

@@ -51,14 +51,13 @@ class Queries::Projects::Form < ApplicationForm
         scheme: :secondary,
         label: I18n.t(:button_cancel),
         tag: :a,
-        data: { "params-from-query-target": "anchor" },
-        href: projects_path(query_id: @query)
+        href: @cancel_url
       )
     end
   end
 
-  def initialize(query:)
+  def initialize(cancel_url:)
     super()
-    @query = query
+    @cancel_url = cancel_url
   end
 end

app/helpers/pagination_helper.rb

@@ -48,9 +48,11 @@ def pagination_links_full(paginator, options = {})
   end
 
   def pagination_option_links(paginator, pagination_options)
+    allowed_params = pagination_options[:allowed_params] || %w[filters sortBy]
+
     option_links = pagination_settings(paginator,
                                        pagination_options[:params]
-                                        .merge(safe_query_params(%w{filters sortBy expand})))
+                                        .merge(safe_query_params(allowed_params)))
 
     content_tag(:div, option_links, class: "op-pagination--options")
   end
@@ -168,7 +170,7 @@ def to_html
 
     def merge_get_params(url_params)
       params = super
-      params.except(*blocked_url_params)
+      allowed_params ? params.slice(*allowed_params) : params
     end
 
     def page_number(page)
@@ -203,8 +205,8 @@ def previous_or_next_page(page, text, class_suffix)
       end
     end
 
-    def blocked_url_params
-      @options[:blocked_url_params] || [] # rubocop:disable Rails/HelperInstanceVariable
+    def allowed_params
+      @options[:allowed_params] # rubocop:disable Rails/HelperInstanceVariable
     end
   end
 

app/helpers/projects_helper.rb

@@ -29,15 +29,17 @@
 module ProjectsHelper
   include WorkPackagesFilterHelper
 
+  PROJECTS_QUERY_PARAM_NAMES = %i[query_id filters columns sortBy per_page page].freeze
+
   # Just like sort_header tag but removes sorting by
   # lft from the sort criteria as lft is mutually exclusive with
   # the other criteria.
-  def projects_sort_header_tag(*)
+  def projects_sort_header_tag(column, **)
     former_criteria = @sort_criteria.criteria.dup
 
     @sort_criteria.criteria.reject! { |a, _| a == "lft" }
 
-    sort_header_tag(*)
+    sort_header_tag(column, **, allowed_params: projects_query_param_names_for_sort)
   ensure
     @sort_criteria.criteria = former_criteria
   end
@@ -70,4 +72,10 @@ def protected_projects_columns_options
     projects_columns_options
       .select { |c| c[:id] == :name }
   end
+
+  def projects_query_param_names_for_sort = PROJECTS_QUERY_PARAM_NAMES - %i[sortBy page]
+
+  def projects_query_params
+    safe_query_params(PROJECTS_QUERY_PARAM_NAMES)
+  end
 end

app/helpers/sort_helper.rb

@@ -272,7 +272,7 @@ def default_sort_order?
   # - the optional caption explicitly specifies the displayed link text.
   # - 2 CSS classes reflect the state of the link: sort and asc or desc
   #
-  def sort_link(column, caption, default_order, html_options = {})
+  def sort_link(column, caption, default_order, allowed_params: nil, **html_options)
     order = order_string(column, inverted: true) || default_order
     caption ||= column.to_s.humanize
 
@@ -283,8 +283,10 @@ def sort_link(column, caption, default_order, html_options = {})
 
     sort_options = { sort_key => sort_param }
 
+    allowed_params ||= %w[filters per_page expand columns]
+
     # Don't lose other params.
-    link_to_content_update(h(caption), safe_query_params(%w{filters per_page expand columns}).merge(sort_options), html_options)
+    link_to_content_update(h(caption), safe_query_params(allowed_params).merge(sort_options), html_options)
   end
 
   # Returns a table header <th> tag with a sort link for the named column
@@ -311,7 +313,7 @@ def sort_link(column, caption, default_order, html_options = {})
   #       </div>
   #     </th>
   #
-  def sort_header_tag(column, options = {})
+  def sort_header_tag(column, allowed_params: nil, **options)
     caption = get_caption(column, options)
 
     default_order = options.delete(:default_order) || "asc"
@@ -321,7 +323,7 @@ def sort_header_tag(column, options = {})
     options[:title] = sort_header_title(column, caption, options)
 
     within_sort_header_tag_hierarchy(options, sort_class(column)) do
-      sort_link(column, caption, default_order, param:, lang:, title: options[:title])
+      sort_link(column, caption, default_order, allowed_params:, param:, lang:, title: options[:title])
     end
   end