From aa909c2299a11e7c98ec1e6c1d93168755215b35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eduardo=20Mart=C3=ADnez?= Date: Thu, 27 Jun 2024 07:57:02 +0200 Subject: [PATCH] Improve navigation and filters in proposals (#13004) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Store filters selections in session and allow removing all filters * Implement next and prev links from proposal show * Inspect session * Use with_indifferent_access with session admin_filters * Revert "Inspect session" This reverts commit 495a33be31d73a7087b6f353d60a47af9686224e. * Refactor adjacents navigation links * Use spans and adjust margins * Fix wrong spelling * Fix spelling * Ensure the session filtered collection query returns an id * style filters bar * better space handling for containers * Add tests * Change remove all filters text * Remove font-normal class remove all filters tag * Fix expected text in tests * Add comment to adjacent_items method * Move method call to before_action filter * Define index method explicitly in controller * Change translation Co-authored-by: Andrés Pereira de Lucena * Remove unnecessary method call Co-authored-by: Andrés Pereira de Lucena --------- Co-authored-by: Hugoren Martinako Co-authored-by: Andrés Pereira de Lucena (cherry picked from commit 49bd0287a8e85af8b8d1a752cd4262a353919b69) --- .../concerns/decidim/admin/filterable.rb | 81 ++++++++++++++- .../decidim/admin/filterable_helper.rb | 20 +++- .../stylesheets/decidim/admin/_cards.scss | 2 +- .../shared/_adjacent_navigation.html.erb | 30 ++++++ .../decidim/admin/shared/_filters.html.erb | 2 +- decidim-admin/config/locales/en.yml | 6 ++ .../proposals/admin/proposals_controller.rb | 4 + .../proposals/admin/proposals/index.html.erb | 4 +- .../proposals/admin/proposals/show.html.erb | 1 + .../system/admin/filter_proposals_spec.rb | 98 +++++++++++++++++++ 10 files changed, 239 insertions(+), 9 deletions(-) create mode 100644 decidim-admin/app/views/decidim/admin/shared/_adjacent_navigation.html.erb diff --git a/decidim-admin/app/controllers/concerns/decidim/admin/filterable.rb b/decidim-admin/app/controllers/concerns/decidim/admin/filterable.rb index f34a2f0a21a07..d3412478782e7 100644 --- a/decidim-admin/app/controllers/concerns/decidim/admin/filterable.rb +++ b/decidim-admin/app/controllers/concerns/decidim/admin/filterable.rb @@ -20,12 +20,15 @@ module Filterable :filters, :filters_with_values, :find_dynamic_translation, + :filter_prefix_key, :query, :query_params, :query_params_with, :query_params_without, + :blank_query_params, :ransack_params, - :search_field_predicate + :search_field_predicate, + :adjacent_items delegate :categories, to: :current_component delegate :scopes, to: :current_organization @@ -36,10 +39,70 @@ def query private + def check_admin_session_filters + if (current_filters = ransack_params).present? + admin_session_filters = session["admin_filters"] || {} + return if admin_session_filters[filter_prefix_key] == current_filters + + current_filters = {} if current_filters[:reset_filters] == "true" + + admin_session_filters[filter_prefix_key] = current_filters + session["admin_filters"] = admin_session_filters + + redirect_to url_for(query_params.merge(q: {})) if current_filters.blank? + else + @session_filter_params = {} unless session_filter_params.is_a?(Hash) + redirect_to url_for(query_params_with(session_filter_params)) if session_filter_params.present? + end + end + def filtered_collection paginate(query.result) end + def session_filtered_collection + @session_filtered_collection ||= begin + query = base_query.ransack(session_filter_params, search_context: :admin, auth_object: current_user).result + # The limit reorders as pagination does + query.limit(query.count) + end + end + + # This method takes the query used by filter and selects the id of + # each item of the filtered collection (this extra select id avoids + # some errors where the SQL of the filtered collection query uses + # aliases and the id is not available in the result) and uses the lag + # and lead window functions which returns the previous and next ids in + # the query + def adjacent_items(item) + query = + <<-SQL.squish + WITH + collection AS (#{session_filtered_collection.select(:id).to_sql}), + successors AS ( + SELECT + id, + Lag(id, 1) OVER () prev_item, + Lead(id, 1) OVER () next_item + FROM + collection + ) + SELECT + prev_item, + next_item + FROM + successors + WHERE + successors.id = #{item.id} + SQL + + (ActiveRecord::Base.connection.exec_query(query).first || {}).compact_blank.transform_values { |id| collection.find_by(id:) } + end + + def filter_prefix_key + @filter_prefix_key ||= controller_name.to_sym + end + def base_query raise NotImplementedError, "A base query is needed to filter admin resources" end @@ -63,14 +126,26 @@ def ransack_params query_params[:q] || {} end + def session_filter_params + @session_filter_params ||= (session["admin_filters"] || {}).with_indifferent_access.fetch(filter_prefix_key, {}) + end + # For injecting ransack params while keeping query params in links. def query_params_with(hash) query_params.merge(q: ransack_params.merge(hash)) end # For rejecting ransack params while keeping query params in links. - def query_params_without(*filters) - query_params.merge(q: ransack_params.except(*filters)) + def query_params_without(*) + q = ransack_params.except(*) + + return blank_query_params if q.blank? + + query_params.merge(q:) + end + + def blank_query_params + query_params.merge(q: { reset_filters: true }) end # Ransack predicate to use in the search_form_for. diff --git a/decidim-admin/app/helpers/decidim/admin/filterable_helper.rb b/decidim-admin/app/helpers/decidim/admin/filterable_helper.rb index 35eeb8e63d266..21e53401f53f9 100644 --- a/decidim-admin/app/helpers/decidim/admin/filterable_helper.rb +++ b/decidim-admin/app/helpers/decidim/admin/filterable_helper.rb @@ -98,9 +98,14 @@ def applied_filters_hidden_field_tags end def applied_filters_tags(i18n_ctx) - ransack_params.slice(*filters).map do |filter, value| + tags = ransack_params.slice(*filters).map do |filter, value| applied_filter_tag(filter, value, filterable_i18n_scope_from_ctx(i18n_ctx)) - end.join.html_safe + end + return if tags.blank? + + tags << remove_all_filters_tag if tags.count > 1 + + tags.join.html_safe end def applied_filter_tag(filter, value, i18n_scope) @@ -111,6 +116,13 @@ def applied_filter_tag(filter, value, i18n_scope) end end + def remove_all_filters_tag + link_to(url_for(blank_query_params), class: "label bg-transparent") do + concat t("decidim.admin.filters.remove_all") + concat icon("delete-bin-line", aria_label: t("decidim.admin.filters.remove_all"), role: "img") + end + end + def remove_filter_icon_link(filter) icon_link_to( "delete-bin-line", @@ -125,6 +137,10 @@ def filterable_i18n_scope_from_ctx(i18n_ctx) i18n_scope += ".#{i18n_ctx}" if i18n_ctx i18n_scope end + + def filtered_adjacent_paths(item, path_method) + adjacent_items(item).transform_values(&method(path_method)) + end end end end diff --git a/decidim-admin/app/packs/stylesheets/decidim/admin/_cards.scss b/decidim-admin/app/packs/stylesheets/decidim/admin/_cards.scss index 16066695ae4f0..eab273bf743dd 100644 --- a/decidim-admin/app/packs/stylesheets/decidim/admin/_cards.scss +++ b/decidim-admin/app/packs/stylesheets/decidim/admin/_cards.scss @@ -107,7 +107,7 @@ } .fcell .label { - @apply p-2 mb-2; + @apply p-2; } .card-section-draggable-list { diff --git a/decidim-admin/app/views/decidim/admin/shared/_adjacent_navigation.html.erb b/decidim-admin/app/views/decidim/admin/shared/_adjacent_navigation.html.erb new file mode 100644 index 0000000000000..d915726e56112 --- /dev/null +++ b/decidim-admin/app/views/decidim/admin/shared/_adjacent_navigation.html.erb @@ -0,0 +1,30 @@ +<% adjacent_paths ||= {} %> +
+ + <% if adjacent_paths["prev_item"].present? %> + <%= link_to( + adjacent_paths["prev_item"], + class: "button button__sm button__text-secondary", + title: t("previous_title", scope: "decidim.admin.shared.adjacent_navigation"), + rel: "prev" + ) do %> + <%= icon "arrow-left-s-line", class: "fill-current" %> + <%= t("previous", scope: "decidim.admin.shared.adjacent_navigation") %> + <% end %> + <% end %> + + + + <% if adjacent_paths["next_item"].present? %> + <%= link_to( + adjacent_paths["next_item"], + class: "button button__sm button__text-secondary", + title: t("next_title", scope: "decidim.admin.shared.adjacent_navigation"), + rel: "next" + ) do %> + <%= t("next", scope: "decidim.admin.shared.adjacent_navigation") %> + <%= icon "arrow-right-s-line", class: "fill-current" %> + <% end %> + <% end %> + +
diff --git a/decidim-admin/app/views/decidim/admin/shared/_filters.html.erb b/decidim-admin/app/views/decidim/admin/shared/_filters.html.erb index 1767e85dc6ddb..37517f09f19d2 100644 --- a/decidim-admin/app/views/decidim/admin/shared/_filters.html.erb +++ b/decidim-admin/app/views/decidim/admin/shared/_filters.html.erb @@ -31,6 +31,6 @@ <% end %> -
+
<%= applied_filters_tags(i18n_ctx) %>
diff --git a/decidim-admin/config/locales/en.yml b/decidim-admin/config/locales/en.yml index 769aa583b4730..134b775a90a6c 100644 --- a/decidim-admin/config/locales/en.yml +++ b/decidim-admin/config/locales/en.yml @@ -453,6 +453,7 @@ en: values: 'false': Published 'true': Unpublished + remove_all: Remove all scope_id_eq: label: Scope search_label: Search @@ -968,6 +969,11 @@ en: help: These tokens are used to publicly share this unpublished resource to any user. They will be hidden when the resource is published. Click on the token's share icon to visit the shareable URL. title: Share tokens shared: + adjacent_navigation: + next: Next + next_title: Next item + previous: Previous + previous_title: Previous item gallery: add_images: Add images edit_images: Edit images diff --git a/decidim-proposals/app/controllers/decidim/proposals/admin/proposals_controller.rb b/decidim-proposals/app/controllers/decidim/proposals/admin/proposals_controller.rb index 481fc6ee6be1b..17047b6755d41 100644 --- a/decidim-proposals/app/controllers/decidim/proposals/admin/proposals_controller.rb +++ b/decidim-proposals/app/controllers/decidim/proposals/admin/proposals_controller.rb @@ -14,6 +14,10 @@ class ProposalsController < Admin::ApplicationController helper_method :proposals, :query, :form_presenter, :proposal, :proposal_ids helper Proposals::Admin::ProposalBulkActionsHelper + before_action :check_admin_session_filters, only: [:index] + + def index; end + def show @notes_form = form(ProposalNoteForm).instance @answer_form = form(Admin::ProposalAnswerForm).from_model(proposal) diff --git a/decidim-proposals/app/views/decidim/proposals/admin/proposals/index.html.erb b/decidim-proposals/app/views/decidim/proposals/admin/proposals/index.html.erb index 3bb958657e977..c2fe6caa3eb58 100644 --- a/decidim-proposals/app/views/decidim/proposals/admin/proposals/index.html.erb +++ b/decidim-proposals/app/views/decidim/proposals/admin/proposals/index.html.erb @@ -16,8 +16,8 @@ <%= render partial: "decidim/proposals/admin/proposals/bulk_actions/assign_to_valuator" %> <%= render partial: "decidim/proposals/admin/proposals/bulk_actions/unassign_from_valuator" %>
- <%= admin_filter_selector(:proposals) %> -
+ <%= admin_filter_selector(filter_prefix_key) %> +
diff --git a/decidim-proposals/app/views/decidim/proposals/admin/proposals/show.html.erb b/decidim-proposals/app/views/decidim/proposals/admin/proposals/show.html.erb index 9c24832227d9d..752ab30fc76f9 100644 --- a/decidim-proposals/app/views/decidim/proposals/admin/proposals/show.html.erb +++ b/decidim-proposals/app/views/decidim/proposals/admin/proposals/show.html.erb @@ -1,6 +1,7 @@ <% add_decidim_page_title(translated_attribute(proposal.title)) %>
+ <%= render partial: "decidim/admin/shared/adjacent_navigation", locals: { adjacent_paths: filtered_adjacent_paths(proposal, :proposal_path) } %>

<%= decidim_html_escape(present(proposal).title).html_safe %> diff --git a/decidim-proposals/spec/system/admin/filter_proposals_spec.rb b/decidim-proposals/spec/system/admin/filter_proposals_spec.rb index 5204f626492fe..6e1a5221b4d5e 100644 --- a/decidim-proposals/spec/system/admin/filter_proposals_spec.rb +++ b/decidim-proposals/spec/system/admin/filter_proposals_spec.rb @@ -115,6 +115,104 @@ def proposal_without_state(token) end end + context "when filtering by multiple filters" do + let!(:scope1) { create(:scope, organization:, name: { "en" => "Scope1" }) } + let!(:scope2) { create(:scope, organization:, name: { "en" => "Scope2" }) } + let!(:answered_proposal_with_scope1) { create(:proposal, :with_answer, component:, scope: scope1) } + let!(:unanswered_proposal_with_scope1) { create(:proposal, component:, scope: scope1) } + let!(:answered_proposal_with_scope2) { create(:proposal, :with_answer, component:, scope: scope2) } + let!(:unanswered_proposal_with_scope2) { create(:proposal, component:, scope: scope2) } + + before do + apply_filter("Answered", "Answered") + visit_component_admin + end + + it_behaves_like "a filtered collection", options: "Scope", filter: "Scope1" do + let(:in_filter) { translated(answered_proposal_with_scope1.title) } + let(:not_in_filter) { translated(answered_proposal_with_scope2.title) } + end + + context "when multiple filters are checked" do + before do + apply_filter("Scope", "Scope1") + end + + it "has a link to remove all filters" do + expect(page).to have_content(translated(answered_proposal_with_scope1.title)) + expect(page).to have_no_content(translated(unanswered_proposal_with_scope1.title)) + expect(page).to have_no_content(translated(answered_proposal_with_scope2.title)) + expect(page).to have_no_content(translated(unanswered_proposal_with_scope2.title)) + + within("[data-applied-filters-tags]") do + expect(page).to have_content("Remove all") + end + + within("[data-applied-filters-tags]") do + click_on("Remove all") + end + + expect(page).to have_content(translated(answered_proposal_with_scope1.title)) + expect(page).to have_content(translated(unanswered_proposal_with_scope1.title)) + expect(page).to have_content(translated(answered_proposal_with_scope2.title)) + expect(page).to have_content(translated(unanswered_proposal_with_scope2.title)) + expect(page).to have_css("[data-applied-filters-tags]", exact_text: "") + end + + it "stores the filters in the session and recovers it when visiting the component page" do + within("tr[data-id='#{answered_proposal_with_scope1.id}']") do + click_on("Answer proposal") + end + + within "form.edit_proposal_answer" do + fill_in_i18n_editor( + :proposal_answer_answer, + "#proposal_answer-answer-tabs", + en: "An answer does not change the filters" + ) + click_on "Answer" + end + + within("[data-applied-filters-tags]") do + expect(page).to have_content("Answered: Answered") + expect(page).to have_content("Scope: Scope1") + expect(page).to have_content("Remove all") + end + + filter_params = CGI.parse(URI.parse(page.current_url).query) + expect(filter_params["q[scope_id_eq]"]).to eq([scope1.id.to_s]) + expect(filter_params["q[with_any_state]"]).to eq(["state_published"]) + end + end + + context "when the admin visits a proposal from a filtered list" do + let!(:other_proposal_with_scope2) { create(:proposal, :with_answer, component:, scope: scope2) } + + before { visit_component_admin } + + it "can navigate through the proposals of the filtered list" do + ids = find_all("tr[data-id]").map { |node| node["data-id"].to_i } + + within("tr[data-id='#{ids[0]}']") do + click_on("Answer proposal") + end + + expect(page).to have_no_link("Previous") + expect(page).to have_link("Next", href: %r{/manage/proposals/#{ids[1]}$}) + + click_on("Next") + + expect(page).to have_link("Previous", href: %r{/manage/proposals/#{ids[0]}$}) + expect(page).to have_link("Next", href: %r{/manage/proposals/#{ids[2]}$}) + + click_on("Next") + + expect(page).to have_link("Previous", href: %r{/manage/proposals/#{ids[1]}$}) + expect(page).to have_no_link("Next") + end + end + end + context "when searching by ID or title" do let!(:proposal1) { create(:proposal, component:) } let!(:proposal2) { create(:proposal, component:) }