From 36d795f69c16affa9d9ef42b45e426dd32332a87 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Thu, 16 Sep 2021 00:19:48 +0200
Subject: [PATCH] Move investment filters to the sidebar
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

As mentioned in commit bc0f04075, a <select> field which submits its
form on change causes many accessibility and usability issues. In this
case there was also an incompatibility with the advanced search filter
which caused a bug solved in commit 541a5fa89.

So the question is where to position the filters and how to display
them. One factor to take into the account is how relevant these filters
are, particularly compared to the links to select the prefered order,
since we don't usually give users the choice of both filters and orders.

Our filters don't really make sense until the valuation phase starts,
since before that phase investments aren't selected nor their
feasibility is decided.

After that phase, the only phase where citizens are really involved
is the final voting; the rest of the phases are done by valuators and
administrators. In the final voting, citizens can only vote on selected
projects, and that's the default filter during that phase.

So these filters are mainly there for information purposes, and not to
help citizens in the phases where they're actually involved (accepting
projects, selecting projects and balloting).

Orders, on the other hand, play a crucial role during the final voting
phase. Since citizens might have already voted for a few projects and
have, let's say, 100,000€ left, ordering by price allows them to find
which projects are within their remaining budget.

In conclusion, orders are more important than filters, and so they
should have a more prominent place.

For consistency with the proposals section, where we've got some links
in the sidebar (bottom part of the page on small screens) providing a
similar funcionality, like accessing selected proposals or archived or
retired proposals, we're moving the investments filters to the sidebar
(bottom part of the page on small screens) as well.
---
 app/assets/javascripts/application.js         |  2 --
 app/assets/javascripts/filter_selector.js     |  8 ------
 app/assets/stylesheets/application.scss       |  1 -
 .../budgets/investmentss/filters.scss         | 19 +++++++++++++
 app/assets/stylesheets/filter_selector.scss   | 14 ----------
 .../investments/filters_component.html.erb    |  4 +++
 .../budgets/investments/filters_component.rb  | 19 +++++++++++++
 .../shared/filter_selector_component.html.erb |  5 ----
 .../shared/filter_selector_component.rb       | 22 ---------------
 app/controllers/concerns/search.rb            |  3 --
 .../budgets/investments/_sidebar.html.erb     |  3 ++
 app/views/budgets/investments/index.html.erb  |  2 --
 .../shared/filter_selector_component_spec.rb  | 14 ----------
 spec/system/budgets/investments_spec.rb       | 28 ++++++++-----------
 14 files changed, 57 insertions(+), 87 deletions(-)
 delete mode 100644 app/assets/javascripts/filter_selector.js
 create mode 100644 app/assets/stylesheets/budgets/investmentss/filters.scss
 delete mode 100644 app/assets/stylesheets/filter_selector.scss
 create mode 100644 app/components/budgets/investments/filters_component.html.erb
 create mode 100644 app/components/budgets/investments/filters_component.rb
 delete mode 100644 app/components/shared/filter_selector_component.html.erb
 delete mode 100644 app/components/shared/filter_selector_component.rb
 delete mode 100644 spec/components/shared/filter_selector_component_spec.rb

diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js
index 8ff6949cb..71b2ac9ce 100644
--- a/app/assets/javascripts/application.js
+++ b/app/assets/javascripts/application.js
@@ -70,7 +70,6 @@
 //= require advanced_search
 //= require registration_form
 //= require suggest
-//= require filter_selector
 //= require forms
 //= require valuation_budget_investment_form
 //= require embed_video
@@ -135,7 +134,6 @@ var initialize_modules = function() {
   App.RegistrationForm.initialize();
   App.Suggest.initialize();
   App.Forms.initialize();
-  App.FilterSelector.initialize();
   App.ValuationBudgetInvestmentForm.initialize();
   App.EmbedVideo.initialize();
   App.FixedBar.initialize();
diff --git a/app/assets/javascripts/filter_selector.js b/app/assets/javascripts/filter_selector.js
deleted file mode 100644
index 5f35db45c..000000000
--- a/app/assets/javascripts/filter_selector.js
+++ /dev/null
@@ -1,8 +0,0 @@
-(function() {
-  "use strict";
-  App.FilterSelector = {
-    initialize: function() {
-      App.Forms.submitOnChange(".filter-selector select");
-    }
-  };
-}).call(this);
diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss
index f438ea3a2..91837158a 100644
--- a/app/assets/stylesheets/application.scss
+++ b/app/assets/stylesheets/application.scss
@@ -27,7 +27,6 @@
 @import "milestones";
 @import "pages";
 @import "dashboard";
-@import "filter_selector";
 @import "legislation";
 @import "legislation_process";
 @import "legislation_process_form";
diff --git a/app/assets/stylesheets/budgets/investmentss/filters.scss b/app/assets/stylesheets/budgets/investmentss/filters.scss
new file mode 100644
index 000000000..a00be0801
--- /dev/null
+++ b/app/assets/stylesheets/budgets/investmentss/filters.scss
@@ -0,0 +1,19 @@
+.investments-filters {
+  border-top: 1px solid $border;
+
+  ul {
+    font-size: $small-font-size;
+    list-style: none;
+    margin-left: 0;
+
+    a {
+      display: inline-block;
+      padding-bottom: $line-height / 4;
+      padding-top: $line-height / 4;
+    }
+
+    [aria-current] {
+      font-weight: bold;
+    }
+  }
+}
diff --git a/app/assets/stylesheets/filter_selector.scss b/app/assets/stylesheets/filter_selector.scss
deleted file mode 100644
index 10de18b25..000000000
--- a/app/assets/stylesheets/filter_selector.scss
+++ /dev/null
@@ -1,14 +0,0 @@
-.filter-selector {
-  text-align: right;
-
-  label {
-    display: inline-block;
-    font-size: $small-font-size;
-    margin-right: $line-height / 2;
-    padding-top: $line-height / 2;
-  }
-
-  select {
-    width: auto;
-  }
-}
diff --git a/app/components/budgets/investments/filters_component.html.erb b/app/components/budgets/investments/filters_component.html.erb
new file mode 100644
index 000000000..7667d5cd4
--- /dev/null
+++ b/app/components/budgets/investments/filters_component.html.erb
@@ -0,0 +1,4 @@
+<div class="investments-filters">
+  <h2 class="sidebar-title" id="investments_filters_header"><%= t("budgets.investments.index.filter") %></h2>
+  <%= link_list(*filters, "aria-labelledby": "investments_filters_header") %>
+</div>
diff --git a/app/components/budgets/investments/filters_component.rb b/app/components/budgets/investments/filters_component.rb
new file mode 100644
index 000000000..12c6ddde8
--- /dev/null
+++ b/app/components/budgets/investments/filters_component.rb
@@ -0,0 +1,19 @@
+class Budgets::Investments::FiltersComponent < ApplicationComponent
+  delegate :valid_filters, :current_filter, :link_list, :current_path_with_query_params, to: :helpers
+
+  def render?
+    valid_filters&.any?
+  end
+
+  private
+
+    def filters
+      valid_filters.map do |filter|
+        [
+          t("budgets.investments.index.filters.#{filter}"),
+          current_path_with_query_params(filter: filter, page: 1),
+          current_filter == filter
+        ]
+      end
+    end
+end
diff --git a/app/components/shared/filter_selector_component.html.erb b/app/components/shared/filter_selector_component.html.erb
deleted file mode 100644
index f8cdd6dee..000000000
--- a/app/components/shared/filter_selector_component.html.erb
+++ /dev/null
@@ -1,5 +0,0 @@
-<%= form_tag({}, method: :get, enforce_utf8: false, class: "filter-selector") do %>
-  <%= query_parameters_tags %>
-  <%= label_tag "filter_selector_filter", t("#{i18n_namespace}.filter") %>
-  <%= select_tag "filter", options_for_select(filter_options, current_filter), id: "filter_selector_filter" %>
-<% end %>
diff --git a/app/components/shared/filter_selector_component.rb b/app/components/shared/filter_selector_component.rb
deleted file mode 100644
index 6ea316a62..000000000
--- a/app/components/shared/filter_selector_component.rb
+++ /dev/null
@@ -1,22 +0,0 @@
-class Shared::FilterSelectorComponent < ApplicationComponent
-  delegate :valid_filters, :current_filter, to: :helpers
-  attr_reader :i18n_namespace
-
-  def initialize(i18n_namespace:)
-    @i18n_namespace = i18n_namespace
-  end
-
-  private
-
-    def query_parameters_tags
-      safe_join(request.query_parameters.reject do |name, _|
-        ["page", "filter"].include?(name)
-      end.map do |name, value|
-        hidden_field_tag name, value, id: "filter_selector_#{name}"
-      end)
-    end
-
-    def filter_options
-      valid_filters.map { |filter| [t("#{i18n_namespace}.filters.#{filter}"), filter] }
-    end
-end
diff --git a/app/controllers/concerns/search.rb b/app/controllers/concerns/search.rb
index 1dd5074f5..d29f5b044 100644
--- a/app/controllers/concerns/search.rb
+++ b/app/controllers/concerns/search.rb
@@ -12,9 +12,6 @@ module Search
   end
 
   def parse_advanced_search_terms
-    if params[:advanced_search].is_a? String
-      params[:advanced_search] = JSON.parse(params[:advanced_search].gsub("=>", ":"))
-    end
     @advanced_search_terms = params[:advanced_search] if params[:advanced_search].present?
     parse_search_date
   end
diff --git a/app/views/budgets/investments/_sidebar.html.erb b/app/views/budgets/investments/_sidebar.html.erb
index 065a73c94..f76cfc224 100644
--- a/app/views/budgets/investments/_sidebar.html.erb
+++ b/app/views/budgets/investments/_sidebar.html.erb
@@ -59,9 +59,12 @@
   </div>
 <% end %>
 
+<%= render Budgets::Investments::FiltersComponent.new %>
+
 <% if @heading && !@heading.content_blocks.where(locale: I18n.locale).empty? %>
   <%= render "budgets/investments/content_blocks" %>
 <% end %>
+
 <% if @map_location&.available? %>
   <%= render "budgets/investments/map" %>
 <% end %>
diff --git a/app/views/budgets/investments/index.html.erb b/app/views/budgets/investments/index.html.erb
index 07e604f1e..d0f813ff0 100644
--- a/app/views/budgets/investments/index.html.erb
+++ b/app/views/budgets/investments/index.html.erb
@@ -70,8 +70,6 @@
         <%= render("shared/order_links", i18n_namespace: "budgets.investments.index") %>
       <% end %>
 
-      <%= render Shared::FilterSelectorComponent.new(i18n_namespace: "budgets.investments.index") %>
-
       <% if investments_default_view? %>
 
         <% @investments.each do |investment| %>
diff --git a/spec/components/shared/filter_selector_component_spec.rb b/spec/components/shared/filter_selector_component_spec.rb
deleted file mode 100644
index 5333fc695..000000000
--- a/spec/components/shared/filter_selector_component_spec.rb
+++ /dev/null
@@ -1,14 +0,0 @@
-require "rails_helper"
-
-describe Shared::FilterSelectorComponent do
-  it "renders a form with a select" do
-    component = Shared::FilterSelectorComponent.new(i18n_namespace: "budgets.investments.index")
-    allow(component).to receive(:valid_filters).and_return(["unfeasible", "winners"])
-    allow(component).to receive(:current_filter).and_return(nil)
-
-    with_request_url("/") { render_inline component }
-
-    expect(page).to have_select "Filtering projects by"
-    expect(page).to have_selector "form[method='get'].filter-selector select"
-  end
-end
diff --git a/spec/system/budgets/investments_spec.rb b/spec/system/budgets/investments_spec.rb
index ddc666256..ae2f3263d 100644
--- a/spec/system/budgets/investments_spec.rb
+++ b/spec/system/budgets/investments_spec.rb
@@ -139,32 +139,31 @@ describe "Budget Investments" do
 
     visit budget_investments_path(budget, heading_id: heading.id)
 
-    expect(page).to have_select "Filtering projects by",
-                                options: ["Not unfeasible", "Unfeasible", "Unselected", "Selected", "Winners"]
+    expect(page).to have_content "FILTERING PROJECTS BY"
 
-    select "Unfeasible", from: "Filtering projects by"
+    click_link "Unfeasible"
 
     expect(page).to have_css ".budget-investment", count: 1
     expect(page).to have_content "Unfeasible investment"
 
-    select "Unselected", from: "Filtering projects by"
+    click_link "Unselected"
 
     expect(page).to have_css ".budget-investment", count: 2
     expect(page).to have_content "Unselected investment"
     expect(page).to have_content "Feasible investment"
 
-    select "Selected", from: "Filtering projects by"
+    click_link "Selected"
 
-    expect(page).to have_css ".budget-investment", count: 2
     expect(page).to have_content "Selected investment"
     expect(page).to have_content "Winner investment"
+    expect(page).to have_css ".budget-investment", count: 2
 
-    select "Winners", from: "Filtering projects by"
+    click_link "Winners"
 
     expect(page).to have_css ".budget-investment", count: 1
     expect(page).to have_content "Winner investment"
 
-    select "Not unfeasible", from: "Filtering projects by"
+    click_link "Not unfeasible"
 
     expect(page).to have_css ".budget-investment", count: 4
     expect(page).to have_content "Selected investment"
@@ -203,13 +202,10 @@ describe "Budget Investments" do
 
       visit budget_investments_path(budget, heading: heading)
 
-      click_on "Advanced search"
-
-      within(".advanced-search-form") do
-        fill_in "With the text", with: "environment"
-        select "Last 24 hours", from: "By date"
-        click_button "Filter"
-      end
+      click_button "Advanced search"
+      fill_in "With the text", with: "environment"
+      select "Last 24 hours", from: "By date"
+      click_button "Filter"
 
       expect(page).to have_content "There is 1 investment containing the term 'environment'"
       expect(page).to have_css ".budget-investment", count: 1
@@ -218,7 +214,7 @@ describe "Budget Investments" do
       expect(page).not_to have_content "Unfeasible environment"
       expect(page).not_to have_content "Unfeasible health"
 
-      select "Unfeasible", from: "Filtering projects by"
+      click_link "Unfeasible"
 
       expect(page).not_to have_content "Feasible environment"
       expect(page).to have_content "There is 1 investment containing the term 'environment'"