From 49bd0287a8e85af8b8d1a752cd4262a353919b69 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 --- .../concerns/decidim/admin/filterable.rb | 79 ++++++++++++++- .../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, 238 insertions(+), 8 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 9471b35ce0646..e04c659b8573d 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,6 +126,10 @@ 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)) @@ -70,7 +137,15 @@ def query_params_with(hash) # For rejecting ransack params while keeping query params in links. def query_params_without(*) - query_params.merge(q: ransack_params.except(*)) + 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 a01809821f7a6..4221d813d95b0 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 09175b5b03447..b0cf812736638 100644 --- a/decidim-admin/config/locales/en.yml +++ b/decidim-admin/config/locales/en.yml @@ -451,6 +451,7 @@ en: values: 'false': Published 'true': Unpublished + remove_all: Remove all scope_id_eq: label: Scope search_label: Search @@ -965,6 +966,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 b958081f14792..cc3b0b79a4be8 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 @@ -13,8 +13,8 @@
- <%= 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 66c3e64c18b41..d11ee2f88d8e2 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:) }