From 59472545c0636c248a3c9bcd8a86f14cbf2256c0 Mon Sep 17 00:00:00 2001 From: Kat Date: Tue, 1 Aug 2023 09:58:02 +0100 Subject: [PATCH] Refactor --- app/helpers/duplicate_logs_helper.rb | 4 +- app/models/lettings_log.rb | 2 +- app/models/sales_log.rb | 1 + spec/helpers/duplicate_logs_helper_spec.rb | 51 ++++++++++++++----- .../duplicate_logs_controller_spec.rb | 8 +-- .../requests/lettings_logs_controller_spec.rb | 25 +++++---- 6 files changed, 60 insertions(+), 31 deletions(-) diff --git a/app/helpers/duplicate_logs_helper.rb b/app/helpers/duplicate_logs_helper.rb index 81c699b0d..4af9ddfbd 100644 --- a/app/helpers/duplicate_logs_helper.rb +++ b/app/helpers/duplicate_logs_helper.rb @@ -47,14 +47,14 @@ module DuplicateLogsHelper duplicate_ids_seen = Set.new logs.visible.each do |log| - next if duplicate_ids_seen.include? log.id + next if duplicate_ids_seen.include?(log.id) duplicates = SalesLog.filter_by_organisation(organisation).duplicate_logs(log) next if duplicates.none? duplicate_ids = [log.id, *duplicates.map(&:id)] duplicate_sets << duplicate_ids - duplicate_ids_seen.merge duplicate_ids + duplicate_ids_seen.merge(duplicate_ids) end duplicate_sets diff --git a/app/models/lettings_log.rb b/app/models/lettings_log.rb index 0ba989472..7f210326e 100644 --- a/app/models/lettings_log.rb +++ b/app/models/lettings_log.rb @@ -60,7 +60,6 @@ class LettingsLog < Log scope :postcode_answered, ->(log) { where(postcode_full: log.postcode_full).or(where(needstype: 2)) } scope :duplicate_logs, lambda { |log| visible - .where(log.slice(*DUPLICATE_LOG_ATTRIBUTES)) .where.not(id: log.id) .where.not(startdate: nil) .where.not(sex1: nil) @@ -71,6 +70,7 @@ class LettingsLog < Log .chcharge_answered .location_answered(log) .postcode_answered(log) + .where(log.slice(*DUPLICATE_LOG_ATTRIBUTES)) } scope :all_duplicates, -> { select(:id, :propcode, :tenancycode).group(:tenancycode, :propcode, :age1).having("count(*) > 1").size } diff --git a/app/models/sales_log.rb b/app/models/sales_log.rb index 99f5f3b46..813402fcc 100644 --- a/app/models/sales_log.rb +++ b/app/models/sales_log.rb @@ -42,6 +42,7 @@ class SalesLog < Log } scope :filter_by_organisation, ->(owning_organisation, _user = nil) { where(owning_organisation:) } scope :filter_by_owning_organisation, ->(owning_organisation, _user = nil) { where(owning_organisation:) } + scope :age1_answered, -> { where.not(age1: nil).or(where(age1_known: [1, 2])) } scope :duplicate_logs, lambda { |log| visible.where(log.slice(*DUPLICATE_LOG_ATTRIBUTES)) .where.not(id: log.id) diff --git a/spec/helpers/duplicate_logs_helper_spec.rb b/spec/helpers/duplicate_logs_helper_spec.rb index 0eeca38d8..410ce1541 100644 --- a/spec/helpers/duplicate_logs_helper_spec.rb +++ b/spec/helpers/duplicate_logs_helper_spec.rb @@ -1,22 +1,20 @@ require "rails_helper" RSpec.describe DuplicateLogsHelper do - let(:org) { create(:organisation) } - let(:other_org) { create(:organisation) } - let(:current_user) { create(:user, organisation: org) } - let(:user_same_org) { create(:user, organisation: org) } - let(:user_other_org) { create(:user, organisation: other_org) } - let(:empty_duplicates) { { lettings: [], sales: [] } } - - let!(:lettings_log) { create(:lettings_log, :duplicate, created_by: current_user) } - let!(:sales_log) { create(:sales_log, :duplicate, created_by: current_user) } - describe "#duplicates_for_user" do + let(:org) { create(:organisation) } + let(:other_org) { create(:organisation) } + let(:current_user) { create(:user, organisation: org) } + let(:user_same_org) { create(:user, organisation: org) } + let(:user_other_org) { create(:user, organisation: other_org) } + + let!(:lettings_log) { create(:lettings_log, :duplicate, created_by: current_user) } + let!(:sales_log) { create(:sales_log, :duplicate, created_by: current_user) } let(:result) { duplicates_for_user(current_user) } context "when there are no duplicates" do - it "returns nil" do - expect(result).to eq empty_duplicates + it "returns empty duplicates" do + expect(result).to eq({ lettings: [], sales: [] }) end end @@ -27,7 +25,7 @@ RSpec.describe DuplicateLogsHelper do end it "does not locate duplicates" do - expect(result).to eq empty_duplicates + expect(result).to eq({ lettings: [], sales: [] }) end end @@ -66,4 +64,31 @@ RSpec.describe DuplicateLogsHelper do end end end + + describe "#sales_duplicate_sets_from_collection" do + let(:organisation) { create(:organisation) } + let(:sales_logs) { SalesLog.filter_by_organisation(organisation) } + + context "when there are no duplicates" do + it "returns empty duplicates" do + expect(sales_duplicate_sets_from_collection(sales_logs, organisation)).to eq([]) + end + end + + context "when there are multiple sets of sales duplicates" do + let!(:duplicate_sales_logs) { create_list(:sales_log, 4, :duplicate, purchid: "set 1", owning_organisation: organisation) } + let!(:duplicate_sales_logs_too) { create_list(:sales_log, 5, :duplicate, postcode_full: "B1 1BB", owning_organisation: organisation) } + let!(:duplicate_sales_logs_3) { create_list(:sales_log, 3, :duplicate, age1: 38, owning_organisation: organisation) } + + it "returns them all with no repeats" do + expected_sales_duplicates_result = [ + duplicate_sales_logs.map(&:id), + duplicate_sales_logs_too.map(&:id), + duplicate_sales_logs_3.map(&:id), + ] + + expect(sales_duplicate_sets_from_collection(sales_logs, organisation)).to match_array(expected_sales_duplicates_result) + end + end + end end diff --git a/spec/requests/duplicate_logs_controller_spec.rb b/spec/requests/duplicate_logs_controller_spec.rb index 123a7117b..0bcfcfed1 100644 --- a/spec/requests/duplicate_logs_controller_spec.rb +++ b/spec/requests/duplicate_logs_controller_spec.rb @@ -324,7 +324,7 @@ RSpec.describe DuplicateLogsController, type: :request do let(:user) { create(:user, :support) } it "renders not found" do - get duplicate_logs_path + get duplicate_logs_path(organisation_id: user.organisation.id) expect(response).to have_http_status(:not_found) end end @@ -333,7 +333,7 @@ RSpec.describe DuplicateLogsController, type: :request do let(:user) { create(:user, :data_coordinator) } it "renders not found" do - get duplicate_logs_path + get duplicate_logs_path(organisation_id: user.organisation.id) expect(response).to have_http_status(:not_found) end end @@ -386,8 +386,8 @@ RSpec.describe DuplicateLogsController, type: :request do end it "has the correct headers" do - headers = page.find_css("th").map(&:text) - expect(headers).to include("Type of logs", "Log IDs") + expect(page).to have_content("Type of logs") + expect(page).to have_content("Log IDs") end it "has the correct number of rows for each log type" do diff --git a/spec/requests/lettings_logs_controller_spec.rb b/spec/requests/lettings_logs_controller_spec.rb index c00f49805..b4701517f 100644 --- a/spec/requests/lettings_logs_controller_spec.rb +++ b/spec/requests/lettings_logs_controller_spec.rb @@ -306,11 +306,16 @@ RSpec.describe LettingsLogsController, type: :request do expect(page).to have_link("Download (CSV, codes only)", href: "/lettings-logs/csv-download?codes_only=true") end - it "does not show a notification banner even if there are duplicate logs for this user" do - allow_any_instance_of(DuplicateLogsHelper).to receive(:duplicates_for_user).and_return({ lettings: [[1, 2]], sales: [] }) # rubocop:disable RSpec/AnyInstance - get lettings_logs_path - expect(page).not_to have_content "duplicate logs" - expect(page).not_to have_link "Review logs" + context "when there are duplicate logs for this user" do + before do + FactoryBot.create_list(:lettings_log, 2, :duplicate, owning_organisation: user.organisation, created_by: user) + end + + it "does not show a notification banner even if there are duplicate logs for this user" do + get lettings_logs_path + expect(page).not_to have_content "duplicate logs" + expect(page).not_to have_link "Review logs" + end end context "when there are no logs in the database" do @@ -884,12 +889,8 @@ RSpec.describe LettingsLogsController, type: :request do end context "and there are duplicate logs for this user" do - let(:duplicates) { { lettings:, sales: } } - let(:lettings) { [[1, 2]] } - let(:sales) { [] } - before do - allow_any_instance_of(DuplicateLogsHelper).to receive(:duplicates_for_user).and_return duplicates # rubocop:disable RSpec/AnyInstance + FactoryBot.create_list(:lettings_log, 2, :duplicate, owning_organisation: user.organisation, created_by: user) end it "displays a notification banner with a link to review logs" do @@ -906,7 +907,9 @@ RSpec.describe LettingsLogsController, type: :request do end context "when there are multiple sets of duplicates" do - let(:sales) { [[3, 4]] } + before do + FactoryBot.create_list(:sales_log, 2, :duplicate, owning_organisation: user.organisation, created_by: user) + end it "displays the correct copy in the banner" do get lettings_logs_path