From 9a765a6415b7f7ce74021870db30e8d119dd8e5d Mon Sep 17 00:00:00 2001
From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com>
Date: Tue, 12 Apr 2022 12:13:05 +0100
Subject: [PATCH] Add year filter (#464)

* add year filter

* update text and filter order

* fix typo

* extract partials

* add tests with 2 filters

* Allow OR filters within scope categories and AND filters across them

* Rubocop

* Remove some of the N+1 query madness by preloading required organisations

* Refactor scopes

* Fix new specs

* unticked checkboxes by deault

* fix test

Co-authored-by: baarkerlounger <db@slothlife.xyz>
Co-authored-by: baarkerlounger <5101747+baarkerlounger@users.noreply.github.com>
---
 app/controllers/case_logs_controller.rb     |  19 +++-
 app/helpers/filters_helper.rb               |   8 +-
 app/models/case_log.rb                      |   9 ++
 app/views/case_logs/_log_filters.erb        |  20 +---
 app/views/filters/_checkbox_filter.html.erb |  10 ++
 spec/helpers/filters_helper_spec.rb         |  10 +-
 spec/models/case_log_spec.rb                |  32 ++++++
 spec/requests/case_logs_controller_spec.rb  | 117 +++++++++++++++-----
 8 files changed, 172 insertions(+), 53 deletions(-)
 create mode 100644 app/views/filters/_checkbox_filter.html.erb

diff --git a/app/controllers/case_logs_controller.rb b/app/controllers/case_logs_controller.rb
index ca9804056..b4b4f4851 100644
--- a/app/controllers/case_logs_controller.rb
+++ b/app/controllers/case_logs_controller.rb
@@ -7,7 +7,7 @@ class CaseLogsController < ApplicationController
   before_action :find_resource, except: %i[create index edit]
 
   def index
-    set_session_filters if params[:status].present?
+    set_session_filters
 
     @pagy, @case_logs = pagy(filtered_case_logs)
 
@@ -121,14 +121,21 @@ private
   end
 
   def filtered_case_logs
-    user_case_logs = current_user.case_logs
-    status_filter = JSON.parse(session[:case_logs_filters])["status"] if session[:case_logs_filters].present?
-    return user_case_logs unless status_filter
+    query = CaseLog.for_organisation(current_user.organisation)
+    if session[:case_logs_filters].present?
+      filters = JSON.parse(session[:case_logs_filters])
+      filters.each do |category, values|
+        next if values.reject(&:empty?).blank?
 
-    user_case_logs.filter_by_status(status_filter)
+        query = query.public_send("filter_by_#{category}", values)
+      end
+    end
+    query.all.includes(:owning_organisation, :managing_organisation)
   end
 
   def set_session_filters
-    session[:case_logs_filters] = { status: params[:status] }.to_json
+    new_filters = session[:case_logs_filters].present? ? JSON.parse(session[:case_logs_filters]) : {}
+    %i[status years].each { |filter| new_filters[filter] = params[filter] if params[filter].present? }
+    session[:case_logs_filters] = new_filters.to_json
   end
 end
diff --git a/app/helpers/filters_helper.rb b/app/helpers/filters_helper.rb
index aaee74fb6..f448bbeb5 100644
--- a/app/helpers/filters_helper.rb
+++ b/app/helpers/filters_helper.rb
@@ -1,9 +1,11 @@
 module FiltersHelper
-  def filter_selected?(filter)
-    return true unless session[:case_logs_filters]
+  def filter_selected?(filter, value)
+    return false unless session[:case_logs_filters]
 
     selected_filters = JSON.parse(session[:case_logs_filters])
-    selected_filters["status"].present? && selected_filters["status"].include?(filter.to_s)
+    return false if selected_filters[filter].blank?
+
+    selected_filters[filter].include?(value.to_s)
   end
 
   def status_filters
diff --git a/app/models/case_log.rb b/app/models/case_log.rb
index aa926d5b4..99ea8fdb5 100644
--- a/app/models/case_log.rb
+++ b/app/models/case_log.rb
@@ -35,6 +35,13 @@ class CaseLog < ApplicationRecord
 
   scope :for_organisation, ->(org) { where(owning_organisation: org).or(where(managing_organisation: org)) }
   scope :filter_by_status, ->(status) { where status: status }
+  scope :filter_by_years, lambda { |years|
+    first_year = years.shift
+    query = filter_by_year(first_year)
+    years.each { |year| query = query.or(filter_by_year(year)) }
+    query.all
+  }
+  scope :filter_by_year, ->(year) { where(startdate: Time.utc(year.to_i, 4, 1)...Time.utc(year.to_i + 1, 4, 1)) }
 
   AUTOGENERATED_FIELDS = %w[id status created_at updated_at discarded_at].freeze
   OPTIONAL_FIELDS = %w[postcode_known la_known first_time_property_let_as_social_housing tenant_code propcode].freeze
@@ -51,6 +58,8 @@ class CaseLog < ApplicationRecord
   end
 
   def collection_start_year
+    return unless startdate
+
     window_end_date = Time.zone.local(startdate.year, 4, 1)
     startdate < window_end_date ? startdate.year - 1 : startdate.year
   end
diff --git a/app/views/case_logs/_log_filters.erb b/app/views/case_logs/_log_filters.erb
index 4e5593a62..09432faa7 100644
--- a/app/views/case_logs/_log_filters.erb
+++ b/app/views/case_logs/_log_filters.erb
@@ -2,9 +2,9 @@
     <div class="app-filter" >
         <div class="app-filter__header">
             <h2 class="govuk-heading-m">Filters</h2>
-            <button 
-            class="app-filter__close" 
-            type="button" 
+            <button
+            class="app-filter__close"
+            type="button"
             data-controller="filter"
             data-action="click->filter#toggleFilter">
             Close
@@ -13,17 +13,9 @@
         <div class="app-filter__content">
             <div class="govuk-form-group app-filter__group">
                 <%= form_with url: "/logs", html: { method: :get } do |f| %>
-                    <%= f.govuk_check_boxes_fieldset :status, legend: { text: "Status", size: "s"} do %>
-                        <div class="govuk-checkboxes govuk-checkboxes--small" data-module="govuk-checkboxes">
-                            <% statuses = status_filters %>
-                            <% statuses.map do |key, option| %>
-                                <%= f.govuk_check_box "status", "#{key}",
-                                label: { text: option },
-                                checked: filter_selected?(key),
-                                size: "s" %>
-                            <% end %>
-                        </div>
-                    <% end %>
+                    <% years = {"2021": "2021/22", "2022": "2022/23"} %>
+                    <%= render partial: "filters/checkbox_filter", locals: { f: f, options: years, label: "Collection year", category: "years" } %>
+                    <%= render partial: "filters/checkbox_filter", locals: { f: f, options: status_filters, label: "Status", category: "status" } %>
                     <%= f.govuk_submit "Apply filters", class: "govuk-!-margin-top-4" %>
                 <% end %>
             </div>
diff --git a/app/views/filters/_checkbox_filter.html.erb b/app/views/filters/_checkbox_filter.html.erb
new file mode 100644
index 000000000..c49156be7
--- /dev/null
+++ b/app/views/filters/_checkbox_filter.html.erb
@@ -0,0 +1,10 @@
+<%= f.govuk_check_boxes_fieldset category.to_sym, legend: { text: label, size: "s"} do %>
+    <div class="govuk-checkboxes govuk-checkboxes--small" data-module="govuk-checkboxes">
+        <% options.map do |key, option| %>
+            <%= f.govuk_check_box category, "#{key}",
+            label: { text: option },
+            checked: filter_selected?(category, key),
+            size: "s" %>
+        <% end %>
+    </div>
+<% end %>
diff --git a/spec/helpers/filters_helper_spec.rb b/spec/helpers/filters_helper_spec.rb
index d79f8dfcd..739b2e937 100644
--- a/spec/helpers/filters_helper_spec.rb
+++ b/spec/helpers/filters_helper_spec.rb
@@ -3,9 +3,9 @@ require "rails_helper"
 RSpec.describe FiltersHelper do
   describe "#filter_selected?" do
     context "when no filters are selected" do
-      it "returns true for all filters" do
-        expect(filter_selected?("completed")).to be_truthy
-        expect(filter_selected?("in_progress")).to be_truthy
+      it "returns false for all filters" do
+        expect(filter_selected?("status", "completed")).to be_falsey
+        expect(filter_selected?("status", "in_progress")).to be_falsey
       end
     end
 
@@ -15,11 +15,11 @@ RSpec.describe FiltersHelper do
       end
 
       it "returns false for non selected filters" do
-        expect(filter_selected?("completed")).to be_falsey
+        expect(filter_selected?("status", "completed")).to be_falsey
       end
 
       it "returns true for selected filter" do
-        expect(filter_selected?("in_progress")).to be_truthy
+        expect(filter_selected?("status", "in_progress")).to be_truthy
       end
     end
   end
diff --git a/spec/models/case_log_spec.rb b/spec/models/case_log_spec.rb
index b648f27f2..971b75056 100644
--- a/spec/models/case_log_spec.rb
+++ b/spec/models/case_log_spec.rb
@@ -1825,4 +1825,36 @@ RSpec.describe CaseLog do
       end
     end
   end
+
+  describe "scopes" do
+    before do
+      FactoryBot.create(:case_log, :in_progress, startdate: Time.utc(2021, 5, 3))
+      FactoryBot.create(:case_log, :completed, startdate: Time.utc(2021, 5, 3))
+      FactoryBot.create(:case_log, startdate: Time.utc(2022, 6, 3))
+    end
+
+    context "when filtering by year" do
+      it "allows filtering on a single year" do
+        expect(described_class.filter_by_years(%w[2021]).count).to eq(2)
+      end
+
+      it "allows filtering by multiple years using OR" do
+        expect(described_class.filter_by_years(%w[2021 2022]).count).to eq(3)
+      end
+
+      it "can filter by year(s) AND status" do
+        expect(described_class.filter_by_years(%w[2021 2022]).filter_by_status("completed").count).to eq(1)
+      end
+    end
+
+    context "when filtering on status" do
+      it "allows filtering on a single status" do
+        expect(described_class.filter_by_status(%w[in_progress]).count).to eq(2)
+      end
+
+      it "allows filtering on multiple statuses" do
+        expect(described_class.filter_by_status(%w[in_progress completed]).count).to eq(3)
+      end
+    end
+  end
 end
diff --git a/spec/requests/case_logs_controller_spec.rb b/spec/requests/case_logs_controller_spec.rb
index 88d2e7468..1031960bd 100644
--- a/spec/requests/case_logs_controller_spec.rb
+++ b/spec/requests/case_logs_controller_spec.rb
@@ -174,37 +174,104 @@ RSpec.describe CaseLogsController, type: :request do
         end
 
         context "when filtering" do
-          let!(:in_progress_case_log) do
-            FactoryBot.create(:case_log, :in_progress,
-                              owning_organisation: organisation,
-                              managing_organisation: organisation)
-          end
-          let!(:completed_case_log) do
-            FactoryBot.create(:case_log, :completed,
-                              owning_organisation: organisation,
-                              managing_organisation: organisation)
-          end
+          context "with status filter" do
+            let!(:in_progress_case_log) do
+              FactoryBot.create(:case_log, :in_progress,
+                                owning_organisation: organisation,
+                                managing_organisation: organisation)
+            end
+            let!(:completed_case_log) do
+              FactoryBot.create(:case_log, :completed,
+                                owning_organisation: organisation,
+                                managing_organisation: organisation)
+            end
+
+            it "shows case logs for multiple selected statuses" do
+              get "/logs?status[]=in_progress&status[]=completed", headers: headers, params: {}
+              expect(page).to have_link(in_progress_case_log.id.to_s)
+              expect(page).to have_link(completed_case_log.id.to_s)
+            end
 
-          it "shows case logs for multiple selected statuses" do
-            get "/logs?status[]=in_progress&status[]=completed", headers: headers, params: {}
-            expect(page).to have_link(in_progress_case_log.id.to_s)
-            expect(page).to have_link(completed_case_log.id.to_s)
+            it "shows case logs for one selected status" do
+              get "/logs?status[]=in_progress", headers: headers, params: {}
+              expect(page).to have_link(in_progress_case_log.id.to_s)
+              expect(page).not_to have_link(completed_case_log.id.to_s)
+            end
+
+            it "does not reset the filters" do
+              get "/logs?status[]=in_progress", headers: headers, params: {}
+              expect(page).to have_link(in_progress_case_log.id.to_s)
+              expect(page).not_to have_link(completed_case_log.id.to_s)
+
+              get "/logs", headers: headers, params: {}
+              expect(page).to have_link(in_progress_case_log.id.to_s)
+              expect(page).not_to have_link(completed_case_log.id.to_s)
+            end
           end
 
-          it "shows case logs for one selected status" do
-            get "/logs?status[]=in_progress", headers: headers, params: {}
-            expect(page).to have_link(in_progress_case_log.id.to_s)
-            expect(page).not_to have_link(completed_case_log.id.to_s)
+          context "with year filter" do
+            let!(:case_log_2021) do
+              FactoryBot.create(:case_log, :in_progress,
+                                owning_organisation: organisation,
+                                startdate: Time.zone.local(2022, 3, 1),
+                                managing_organisation: organisation)
+            end
+            let!(:case_log_2022) do
+              FactoryBot.create(:case_log, :completed,
+                                owning_organisation: organisation,
+                                mrcdate: Time.zone.local(2022, 2, 1),
+                                startdate: Time.zone.local(2022, 12, 1),
+                                managing_organisation: organisation)
+            end
+
+            it "shows case logs for multiple selected years" do
+              get "/logs?years[]=2021&years[]=2022", headers: headers, params: {}
+              expect(page).to have_link(case_log_2021.id.to_s)
+              expect(page).to have_link(case_log_2022.id.to_s)
+            end
+
+            it "shows case logs for one selected year" do
+              get "/logs?years[]=2021", headers: headers, params: {}
+              expect(page).to have_link(case_log_2021.id.to_s)
+              expect(page).not_to have_link(case_log_2022.id.to_s)
+            end
           end
 
-          it "does not reset the filters" do
-            get "/logs?status[]=in_progress", headers: headers, params: {}
-            expect(page).to have_link(in_progress_case_log.id.to_s)
-            expect(page).not_to have_link(completed_case_log.id.to_s)
+          context "with year and status filter" do
+            let!(:case_log_2021) do
+              FactoryBot.create(:case_log, :in_progress,
+                                owning_organisation: organisation,
+                                startdate: Time.zone.local(2022, 3, 1),
+                                managing_organisation: organisation)
+            end
+            let!(:case_log_2022) do
+              FactoryBot.create(:case_log, :completed,
+                                owning_organisation: organisation,
+                                mrcdate: Time.zone.local(2022, 2, 1),
+                                startdate: Time.zone.local(2022, 12, 1),
+                                managing_organisation: organisation)
+            end
+            let!(:case_log_2022_in_progress) do
+              FactoryBot.create(:case_log, :in_progress,
+                                owning_organisation: organisation,
+                                mrcdate: Time.zone.local(2022, 2, 1),
+                                startdate: Time.zone.local(2022, 12, 1),
+                                managing_organisation: organisation)
+            end
+
+            it "shows case logs for multiple selected statuses and years" do
+              get "/logs?years[]=2021&years[]=2022&status[]=in_progress&status[]=completed", headers: headers, params: {}
+              expect(page).to have_link(case_log_2021.id.to_s)
+              expect(page).to have_link(case_log_2022.id.to_s)
+              expect(page).to have_link(case_log_2022_in_progress.id.to_s)
+            end
 
-            get "/logs", headers: headers, params: {}
-            expect(page).to have_link(in_progress_case_log.id.to_s)
-            expect(page).not_to have_link(completed_case_log.id.to_s)
+            it "shows case logs for one selected status" do
+              get "/logs?years[]=2022&status[]=in_progress", headers: headers, params: {}
+              expect(page).to have_link(case_log_2022_in_progress.id.to_s)
+              expect(page).not_to have_link(case_log_2021.id.to_s)
+              expect(page).not_to have_link(case_log_2022.id.to_s)
+            end
           end
         end
       end