From 40b751b9fcc3a51e3cb57a783917ad2c51502cfd Mon Sep 17 00:00:00 2001 From: J G <7750475+moarpheus@users.noreply.github.com> Date: Mon, 30 May 2022 09:10:55 +0100 Subject: [PATCH] Cldc 1278 log search follow up (#623) * test to download only search results * testing I can download only logs matching both search and filter * rubocop * matching style for title * ILIKE for search * rubocop * using ILIKE * upated rack due to vulnerability * refactoring * refactoring * refactored test * refactored matching search results on multiple page tests for title * no pagination title * no pagination title for when no logs * fixed all title related tests for filter and search * rubocop * removing extra space in the view - unneccesary * better test when searching for postcode * failing lint? * refactored condition * refactored content to links * refactored content to links in case logs --- Gemfile.lock | 2 +- app/controllers/case_logs_controller.rb | 11 ++--- app/controllers/organisations_controller.rb | 9 +--- app/models/case_log.rb | 6 +-- app/views/case_logs/index.html.erb | 4 +- app/views/organisations/logs.html.erb | 5 +- spec/requests/case_logs_controller_spec.rb | 48 ++++++++++++++----- .../requests/organisations_controller_spec.rb | 41 ++++++++-------- 8 files changed, 71 insertions(+), 55 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index ea322cb65..71c6bd92f 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -270,7 +270,7 @@ GEM puma (5.6.4) nio4r (~> 2.0) racc (1.6.0) - rack (2.2.3) + rack (2.2.3.1) rack-attack (6.6.1) rack (>= 1.0, < 3) rack-mini-profiler (2.3.4) diff --git a/app/controllers/case_logs_controller.rb b/app/controllers/case_logs_controller.rb index bdf9e9e9b..22e6cb599 100644 --- a/app/controllers/case_logs_controller.rb +++ b/app/controllers/case_logs_controller.rb @@ -12,21 +12,16 @@ class CaseLogsController < ApplicationController set_session_filters all_logs = current_user.case_logs + unpaginated_filtered_logs = filtered_case_logs(filtered_collection(all_logs, search_term)) - @pagy, @case_logs = pagy( - filtered_case_logs( - filtered_collection( - all_logs, search_term - ), - ), - ) + @pagy, @case_logs = pagy(unpaginated_filtered_logs) @searched = search_term.presence @total_count = all_logs.size respond_to do |format| format.html format.csv do - send_data filtered_case_logs(current_user.case_logs).to_csv, filename: "logs-#{Time.zone.now}.csv" + send_data unpaginated_filtered_logs.to_csv, filename: "logs-#{Time.zone.now}.csv" end end end diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb index ad195b624..c1b37aab4 100644 --- a/app/controllers/organisations_controller.rb +++ b/app/controllers/organisations_controller.rb @@ -55,14 +55,9 @@ class OrganisationsController < ApplicationController set_session_filters(specific_org: true) organisation_logs = CaseLog.all.where(owning_organisation_id: @organisation.id) + unpaginated_filtered_logs = filtered_case_logs(filtered_collection(organisation_logs, search_term)) - @pagy, @case_logs = pagy( - filtered_case_logs( - filtered_collection( - organisation_logs, search_term - ), - ), - ) + @pagy, @case_logs = pagy(unpaginated_filtered_logs) @searched = search_term.presence @total_count = organisation_logs.size diff --git a/app/models/case_log.rb b/app/models/case_log.rb index 5533b99be..3823cfdeb 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -52,9 +52,9 @@ class CaseLog < ApplicationRecord } scope :filter_by_id, ->(id) { where(id:) } - scope :filter_by_tenant_code, ->(code) { where("lower(tenant_code) = ?", code.downcase) } - scope :filter_by_propcode, ->(code) { where("lower(propcode) = ?", code.downcase) } - scope :filter_by_postcode, ->(code) { where(postcode_full: code.upcase.gsub(/\s+/, "")) } + scope :filter_by_tenant_code, ->(tenant_code) { where("tenant_code ILIKE ?", "%#{tenant_code}%") } + scope :filter_by_propcode, ->(propcode) { where("propcode ILIKE ?", "%#{propcode}%") } + scope :filter_by_postcode, ->(postcode_full) { where("postcode_full ILIKE ?", "%#{postcode_full.gsub(/\s+/, '')}%") } scope :search_by, lambda { |param| filter_by_id(param) .or(filter_by_tenant_code(param)) diff --git a/app/views/case_logs/index.html.erb b/app/views/case_logs/index.html.erb index 8bd085acc..b18e22861 100644 --- a/app/views/case_logs/index.html.erb +++ b/app/views/case_logs/index.html.erb @@ -1,9 +1,9 @@ <% item_label = @pagy.count > 1 ? "logs" : "log" %> <% if @searched.present? %> - <% title = "Logs (search results for ‘#{@searched}’#{@pagy.last > 1 ? ", page #{@pagy.page} of #{@pagy.last}" : ''}) - Submit social housing and sales data (CORE) - GOV.UK" %> + <% title = "Your organisation (#{@pagy.count} #{item_label} matching ‘#{@searched}’ of #{@total_count} total logs)" %> <% else %> - <% title = "Logs #{@pagy.last > 1 ? "(page #{@pagy.page} of #{@pagy.last}) " : ''}- Submit social housing and sales data (CORE) - GOV.UK" %> + <% title = "Your organisation (Logs)" %> <% end %> <% content_for :title, title %> diff --git a/app/views/organisations/logs.html.erb b/app/views/organisations/logs.html.erb index be6a3d5d3..a5e62dbf8 100644 --- a/app/views/organisations/logs.html.erb +++ b/app/views/organisations/logs.html.erb @@ -1,9 +1,8 @@ <% item_label = @pagy.count > 1 ? "logs" : "log" %> - <% if @searched.present? %> - <% title = "Logs (search results for ‘#{@searched}’#{@pagy.last > 1 ? ", page #{@pagy.page} of #{@pagy.last}" : ''}) - Submit social housing and sales data (CORE) - GOV.UK" %> + <% title = "Your organisation (#{@pagy.count} #{item_label} matching ‘#{@searched}’ of #{@total_count} total logs)" %> <% else %> - <% title = "Logs #{@pagy.last > 1 ? "(page #{@pagy.page} of #{@pagy.last}) " : ''}- Submit social housing and sales data (CORE) - GOV.UK" %> + <% title = "Your organisation (Logs)" %> <% end %> <% content_for :title, title %> diff --git a/spec/requests/case_logs_controller_spec.rb b/spec/requests/case_logs_controller_spec.rb index 4e59378df..ef78e0b5f 100644 --- a/spec/requests/case_logs_controller_spec.rb +++ b/spec/requests/case_logs_controller_spec.rb @@ -191,7 +191,7 @@ RSpec.describe CaseLogsController, type: :request do it "page has correct title" do get "/logs", headers: headers, params: {} - expect(page).to have_title("Logs - Submit social housing and sales data (CORE) - GOV.UK") + expect(page).to have_title("Your organisation (Logs) - Submit social housing lettings and sales data (CORE) - GOV.UK") end end @@ -322,17 +322,18 @@ RSpec.describe CaseLogsController, type: :request do context "when using a search query" do let(:logs) { FactoryBot.create_list(:case_log, 3, :completed, owning_organisation: user.organisation) } let(:log_to_search) { FactoryBot.create(:case_log, :completed, owning_organisation: user.organisation) } + let(:log_total_count) { CaseLog.where(owning_organisation: user.organisation).count } it "has search results in the title" do get "/logs?search=#{log_to_search.id}", headers: headers, params: {} - expect(page).to have_content("Logs (search results for ‘#{log_to_search.id}’) - Submit social housing and sales data (CORE) - GOV.UK") + expect(page).to have_title("Your organisation (1 log matching ‘#{log_to_search.id}’ of #{log_total_count} total logs) - Submit social housing lettings and sales data (CORE) - GOV.UK") end it "shows case logs matching the id" do get "/logs?search=#{log_to_search.id}", headers: headers, params: {} - expect(page).to have_content(log_to_search.id.to_s) + expect(page).to have_link(log_to_search.id.to_s) logs.each do |log| - expect(page).not_to have_content(log.id.to_s) + expect(page).not_to have_link(log.id.to_s) end end @@ -346,7 +347,7 @@ RSpec.describe CaseLogsController, type: :request do it "shows case logs matching the property reference" do get "/logs?search=#{log_to_search.propcode}", headers: headers, params: {} - expect(page).to have_content(log_to_search.id.to_s) + expect(page).to have_link(log_to_search.id.to_s) logs.each do |log| expect(page).not_to have_link(log.id.to_s) end @@ -366,7 +367,7 @@ RSpec.describe CaseLogsController, type: :request do it "displays all matching logs" do get "/logs?search=#{log_to_search.postcode_full}", headers: headers, params: {} expect(page).to have_link(log_to_search.id.to_s) - expect(page).to have_content(matching_postcode_log.id) + expect(page).to have_link(matching_postcode_log.id.to_s) logs.each do |log| expect(page).not_to have_link(log.id.to_s) end @@ -374,16 +375,18 @@ RSpec.describe CaseLogsController, type: :request do end context "when there are more than 1 page of search results" do - let(:logs) { FactoryBot.create_list(:case_log, 30, :completed, owning_organisation: user.organisation, postcode_full: "XX1 1YY") } + let(:postcode) { "XX11YY" } + let(:logs) { FactoryBot.create_list(:case_log, 30, :completed, owning_organisation: user.organisation, postcode_full: postcode) } + let(:log_total_count) { CaseLog.where(owning_organisation: user.organisation).count } it "has title with pagination details for page 1" do get "/logs?search=#{logs[0].postcode_full}", headers: headers, params: {} - expect(page).to have_content("Logs (search results for ‘#{logs[0].postcode_full}’, page 1 of 2) - Submit social housing and sales data (CORE) - GOV.UK") + expect(page).to have_title("Your organisation (#{logs.count} logs matching ‘#{postcode}’ of #{log_total_count} total logs) (page 1 of 2) - Submit social housing lettings and sales data (CORE) - GOV.UK") end it "has title with pagination details for page 2" do get "/logs?search=#{logs[0].postcode_full}&page=2", headers: headers, params: {} - expect(page).to have_content("Logs (search results for ‘#{logs[0].postcode_full}’, page 2 of 2) - Submit social housing and sales data (CORE) - GOV.UK") + expect(page).to have_title("Your organisation (#{logs.count} logs matching ‘#{postcode}’ of #{log_total_count} total logs) (page 2 of 2) - Submit social housing lettings and sales data (CORE) - GOV.UK") end end @@ -463,7 +466,7 @@ RSpec.describe CaseLogsController, type: :request do end it "does not have pagination in the title" do - expect(page).to have_title("Logs - Submit social housing and sales data (CORE) - GOV.UK") + expect(page).to have_title("Your organisation (Logs) - Submit social housing lettings and sales data (CORE) - GOV.UK") end it "shows the download csv link" do @@ -539,7 +542,7 @@ RSpec.describe CaseLogsController, type: :request do end it "has pagination in the title" do - expect(page).to have_title("Logs (page 1 of 2) - Submit social housing and sales data (CORE) - GOV.UK") + expect(page).to have_title("Your organisation (Logs) (page 1 of 2) - Submit social housing lettings and sales data (CORE) - GOV.UK") end end @@ -564,7 +567,7 @@ RSpec.describe CaseLogsController, type: :request do end it "has pagination in the title" do - expect(page).to have_title("Logs (page 2 of 2) - Submit social housing and sales data (CORE) - GOV.UK") + expect(page).to have_title("Your organisation (Logs) (page 2 of 2) - Submit social housing lettings and sales data (CORE) - GOV.UK") end end end @@ -753,6 +756,27 @@ RSpec.describe CaseLogsController, type: :request do csv = CSV.parse(response.body) expect(csv.count).to eq(2) end + + it "dowloads searched logs" do + get "/logs?search=#{case_log.id}", headers:, params: {} + csv = CSV.parse(response.body) + expect(csv.count).to eq(2) + end + + context "when both filter and search applied" do + let(:postcode) { "XX1 1TG" } + + before do + FactoryBot.create(:case_log, :in_progress, postcode_full: postcode, owning_organisation: organisation) + FactoryBot.create(:case_log, :completed, postcode_full: postcode, owning_organisation: organisation) + end + + it "dowloads logs matching both csv and filter logs" do + get "/logs?status[]=completed&search=#{postcode}", headers:, params: {} + csv = CSV.parse(response.body) + expect(csv.count).to eq(2) + end + end end context "when there are more than 20 logs" do diff --git a/spec/requests/organisations_controller_spec.rb b/spec/requests/organisations_controller_spec.rb index d965071fe..2b01d62e2 100644 --- a/spec/requests/organisations_controller_spec.rb +++ b/spec/requests/organisations_controller_spec.rb @@ -407,41 +407,42 @@ RSpec.describe OrganisationsController, type: :request do context "when using a search query" do let(:logs) { FactoryBot.create_list(:case_log, 3, :completed, owning_organisation: user.organisation) } let(:log_to_search) { FactoryBot.create(:case_log, :completed, owning_organisation: user.organisation) } + let(:log_total_count) { CaseLog.where(owning_organisation: user.organisation).count } it "has search results in the title" do get "/organisations/#{organisation.id}/logs?search=#{log_to_search.id}", headers: headers, params: {} - expect(page).to have_content("Logs (search results for ‘#{log_to_search.id}’) - Submit social housing and sales data (CORE) - GOV.UK") + expect(page).to have_title("Your organisation (1 log matching ‘#{log_to_search.id}’ of #{log_total_count} total logs) - Submit social housing lettings and sales data (CORE) - GOV.UK") end it "shows case logs matching the id" do get "/organisations/#{organisation.id}/logs?search=#{log_to_search.id}", headers: headers, params: {} - expect(page).to have_content(log_to_search.id) + expect(page).to have_link(log_to_search.id.to_s) logs.each do |log| - expect(page).not_to have_content(log.id) + expect(page).not_to have_link(log.id.to_s) end end it "shows case logs matching the tenant code" do get "/organisations/#{organisation.id}/logs?search=#{log_to_search.tenant_code}", headers: headers, params: {} - expect(page).to have_content(log_to_search.id) + expect(page).to have_link(log_to_search.id.to_s) logs.each do |log| - expect(page).not_to have_content(log.id) + expect(page).not_to have_link(log.id.to_s) end end it "shows case logs matching the property reference" do get "/organisations/#{organisation.id}/logs?search=#{log_to_search.propcode}", headers: headers, params: {} - expect(page).to have_content(log_to_search.id) + expect(page).to have_link(log_to_search.id.to_s) logs.each do |log| - expect(page).not_to have_content(log.id) + expect(page).not_to have_link(log.id.to_s) end end it "shows case logs matching the property postcode" do get "/organisations/#{organisation.id}/logs?search=#{log_to_search.postcode_full}", headers: headers, params: {} - expect(page).to have_content(log_to_search.id) + expect(page).to have_link(log_to_search.id.to_s) logs.each do |log| - expect(page).not_to have_content(log.id) + expect(page).not_to have_link(log.id.to_s) end end @@ -450,25 +451,27 @@ RSpec.describe OrganisationsController, type: :request do it "displays all matching logs" do get "/organisations/#{organisation.id}/logs?search=#{log_to_search.postcode_full}", headers: headers, params: {} - expect(page).to have_content(log_to_search.id) - expect(page).to have_content(matching_postcode_log.id) + expect(page).to have_link(log_to_search.id.to_s) + expect(page).to have_link(matching_postcode_log.id.to_s) logs.each do |log| - expect(page).not_to have_content(log.id) + expect(page).not_to have_link(log.id.to_s) end end end context "when there are more than 1 page of search results" do - let(:logs) { FactoryBot.create_list(:case_log, 30, :completed, owning_organisation: user.organisation, postcode_full: "XX1 1YY") } + let(:postcode) { "XX11YY" } + let(:logs) { FactoryBot.create_list(:case_log, 30, :completed, owning_organisation: user.organisation, postcode_full: postcode) } + let(:log_total_count) { CaseLog.where(owning_organisation: user.organisation).count } it "has title with pagination details for page 1" do get "/organisations/#{organisation.id}/logs?search=#{logs[0].postcode_full}", headers: headers, params: {} - expect(page).to have_content("Logs (search results for ‘#{logs[0].postcode_full}’, page 1 of 2) - Submit social housing and sales data (CORE) - GOV.UK") + expect(page).to have_title("Your organisation (#{logs.count} logs matching ‘#{postcode}’ of #{log_total_count} total logs) (page 1 of 2) - Submit social housing lettings and sales data (CORE) - GOV.UK") end it "has title with pagination details for page 2" do get "/organisations/#{organisation.id}/logs?search=#{logs[0].postcode_full}&page=2", headers: headers, params: {} - expect(page).to have_content("Logs (search results for ‘#{logs[0].postcode_full}’, page 2 of 2) - Submit social housing and sales data (CORE) - GOV.UK") + expect(page).to have_title("Your organisation (#{logs.count} logs matching ‘#{postcode}’ of #{log_total_count} total logs) (page 2 of 2) - Submit social housing lettings and sales data (CORE) - GOV.UK") end end @@ -476,9 +479,9 @@ RSpec.describe OrganisationsController, type: :request do it "doesn't display any logs" do get "/organisations/#{organisation.id}/logs?search=foobar", headers:, params: {} logs.each do |log| - expect(page).not_to have_content(log.id) + expect(page).not_to have_link(log.id.to_s) end - expect(page).not_to have_content(log_to_search.id) + expect(page).not_to have_link(log_to_search.id.to_s) end end @@ -486,9 +489,9 @@ RSpec.describe OrganisationsController, type: :request do it "doesn't display any logs" do get "/organisations/#{organisation.id}/logs?search=", headers:, params: {} logs.each do |log| - expect(page).not_to have_content(log.id) + expect(page).not_to have_link(log.id.to_s) end - expect(page).not_to have_content(log_to_search.id) + expect(page).not_to have_link(log_to_search.id.to_s) end end