From 27c9701dccab4fc6c1da55ce11d8e8a18aa1fe0a Mon Sep 17 00:00:00 2001 From: Kat Date: Tue, 1 Aug 2023 14:42:00 +0100 Subject: [PATCH] Create a scope for duplicate groups --- app/helpers/duplicate_logs_helper.rb | 44 ++------------------ app/models/lettings_log.rb | 24 ++++++++++- app/models/organisation.rb | 12 ++++++ app/models/sales_log.rb | 23 +++++++++++ app/models/user.rb | 12 ++++++ spec/factories/sales_log.rb | 1 + spec/helpers/duplicate_logs_helper_spec.rb | 48 ++++++++++++++++++++-- 7 files changed, 120 insertions(+), 44 deletions(-) diff --git a/app/helpers/duplicate_logs_helper.rb b/app/helpers/duplicate_logs_helper.rb index 4af9ddfbd..1e8c987e5 100644 --- a/app/helpers/duplicate_logs_helper.rb +++ b/app/helpers/duplicate_logs_helper.rb @@ -30,54 +30,18 @@ module DuplicateLogsHelper def duplicates_for_user(user) { - lettings: lettings_duplicate_sets_from_collection(LettingsLog.created_by(user), user.organisation), - sales: sales_duplicate_sets_from_collection(SalesLog.created_by(user), user.organisation), + lettings: user.duplicate_lettings_logs_sets, + sales: user.duplicate_sales_logs_sets, } end def duplicates_for_organisation(organisation) { - lettings: lettings_duplicate_sets_from_collection(LettingsLog.filter_by_organisation(organisation), organisation), - sales: sales_duplicate_sets_from_collection(SalesLog.filter_by_organisation(organisation), organisation), + lettings: organisation.duplicate_lettings_logs_sets, + sales: organisation.duplicate_sales_logs_sets, } end - def sales_duplicate_sets_from_collection(logs, organisation) - duplicate_sets = [] - duplicate_ids_seen = Set.new - - logs.visible.each do |log| - 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) - end - - duplicate_sets - end - - def lettings_duplicate_sets_from_collection(logs, organisation) - duplicate_sets = [] - duplicate_ids_seen = Set.new - - logs.visible.each do |log| - next if duplicate_ids_seen.include? log.id - - duplicates = LettingsLog.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 - end - - duplicate_sets - end - def duplicate_sets_count(user, organisation) duplicates = if user.support? duplicates_for_organisation(organisation) diff --git a/app/models/lettings_log.rb b/app/models/lettings_log.rb index 7f210326e..35954953c 100644 --- a/app/models/lettings_log.rb +++ b/app/models/lettings_log.rb @@ -72,7 +72,29 @@ class LettingsLog < 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 } + + scope :duplicates, lambda { |created_by_id = nil| + scope = visible + .group(*DUPLICATE_LOG_ATTRIBUTES) + .having("COUNT(*) > 1") + + if created_by_id + scope = scope.where("EXISTS ( + SELECT 1 + FROM lettings_logs AS inner_logs + WHERE lettings_logs.tenancycode = inner_logs.tenancycode + AND lettings_logs.startdate = inner_logs.startdate + AND lettings_logs.age1_known = inner_logs.age1_known + AND lettings_logs.age1 = inner_logs.age1 + AND lettings_logs.sex1 = inner_logs.sex1 + AND lettings_logs.ecstat1 = inner_logs.ecstat1 + AND lettings_logs.tcharge = inner_logs.tcharge + AND inner_logs.created_by_id = ? + )", created_by_id) + end + + scope + } AUTOGENERATED_FIELDS = %w[id status created_at updated_at discarded_at].freeze OPTIONAL_FIELDS = %w[tenancycode propcode chcharge].freeze diff --git a/app/models/organisation.rb b/app/models/organisation.rb index dac2b0ff5..d776ed09f 100644 --- a/app/models/organisation.rb +++ b/app/models/organisation.rb @@ -132,4 +132,16 @@ class Organisation < ApplicationRecord :active end + + def duplicate_lettings_logs_sets + duplicate_sets = lettings_logs.duplicates.pluck("ARRAY_AGG(id)") + + duplicate_sets.map { |array_str| array_str ? array_str.map(&:to_i) : [] } + end + + def duplicate_sales_logs_sets + duplicate_sets = sales_logs.duplicates.pluck("ARRAY_AGG(id)") + + duplicate_sets.map { |array_str| array_str ? array_str.map(&:to_i) : [] } + end end diff --git a/app/models/sales_log.rb b/app/models/sales_log.rb index 813402fcc..240b38105 100644 --- a/app/models/sales_log.rb +++ b/app/models/sales_log.rb @@ -54,6 +54,29 @@ class SalesLog < Log } scope :after_date, ->(date) { where("saledate >= ?", date) } + scope :duplicates, lambda { |created_by_id = nil| + scope = visible + .group(*DUPLICATE_LOG_ATTRIBUTES) + .having("COUNT(*) > 1") + + if created_by_id + scope = scope.where("EXISTS ( + SELECT 1 + FROM sales_logs AS inner_logs + WHERE sales_logs.purchid = inner_logs.purchid + AND sales_logs.saledate = inner_logs.saledate + AND sales_logs.age1_known = inner_logs.age1_known + AND sales_logs.age1 = inner_logs.age1 + AND sales_logs.sex1 = inner_logs.sex1 + AND sales_logs.ecstat1 = inner_logs.ecstat1 + AND sales_logs.postcode_full = inner_logs.postcode_full + AND inner_logs.created_by_id = ? + )", created_by_id) + end + + scope + } + OPTIONAL_FIELDS = %w[purchid othtype].freeze RETIREMENT_AGES = { "M" => 65, "F" => 60, "X" => 65 }.freeze DUPLICATE_LOG_ATTRIBUTES = %w[owning_organisation_id purchid saledate age1_known age1 sex1 ecstat1 postcode_full].freeze diff --git a/app/models/user.rb b/app/models/user.rb index eccdaec47..0e3ed7578 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -203,6 +203,18 @@ class User < ApplicationRecord super && active? end + def duplicate_lettings_logs_sets + duplicate_sets = lettings_logs.duplicates(id).pluck("ARRAY_AGG(id)") + + duplicate_sets.map { |array_str| array_str ? array_str.map(&:to_i) : [] } + end + + def duplicate_sales_logs_sets + duplicate_sets = sales_logs.duplicates(id).pluck("ARRAY_AGG(id)") + + duplicate_sets.map { |array_str| array_str ? array_str.map(&:to_i) : [] } + end + protected # Checks whether a password is needed or not. For validations only. diff --git a/spec/factories/sales_log.rb b/spec/factories/sales_log.rb index 54687c928..177ed600c 100644 --- a/spec/factories/sales_log.rb +++ b/spec/factories/sales_log.rb @@ -41,6 +41,7 @@ FactoryBot.define do type { 8 } jointpur { 2 } saledate { Time.zone.today } + age1_known { 1 } age1 { 20 } sex1 { "F" } ecstat1 { 1 } diff --git a/spec/helpers/duplicate_logs_helper_spec.rb b/spec/helpers/duplicate_logs_helper_spec.rb index 410ce1541..8cb971832 100644 --- a/spec/helpers/duplicate_logs_helper_spec.rb +++ b/spec/helpers/duplicate_logs_helper_spec.rb @@ -47,6 +47,28 @@ RSpec.describe DuplicateLogsHelper do end end + context "when there is a set of duplicate lettings logs not associated with the user" do + before do + create_list(:lettings_log, 3, :duplicate, tenancycode: "other", owning_organisation: org) + end + + it "returns the ids of the duplicates in a hash under the lettings key" do + expect(result).to be_a Hash + expect(result[:lettings]).to be_empty + end + end + + context "when there is a set of duplicate sales logs not associated with the user" do + before do + create_list(:sales_log, 3, :duplicate, purchid: "other", owning_organisation: org) + end + + it "returns the ids of the duplicates in a hash under the sales key" do + expect(result).to be_a Hash + expect(result[:sales]).to be_empty + end + end + context "when there are multiple sets of duplicates across lettings and sales" do let!(:duplicate_lettings_log) { create(:lettings_log, :duplicate, created_by: user_same_org) } let!(:duplicate_sales_log) { create(:sales_log, :duplicate, created_by: user_same_org) } @@ -65,13 +87,13 @@ RSpec.describe DuplicateLogsHelper do end end - describe "#sales_duplicate_sets_from_collection" do + describe "#duplicates_for_organisation" 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([]) + expect(duplicates_for_organisation(organisation)).to eq({ lettings: [], sales: [] }) end end @@ -80,6 +102,15 @@ RSpec.describe DuplicateLogsHelper do 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) } + let!(:duplicate_lettings_logs) { create_list(:lettings_log, 4, :duplicate, tenancycode: "set 1", owning_organisation: organisation) } + let!(:duplicate_lettings_logs_too) { create_list(:lettings_log, 5, :duplicate, postcode_full: "B1 1BB", owning_organisation: organisation) } + let!(:duplicate_lettings_logs_3) { create_list(:lettings_log, 3, :duplicate, age1: 38, owning_organisation: organisation) } + + before do + create_list(:sales_log, 3, :duplicate, discarded_at: Time.zone.now, status: 4, owning_organisation: organisation) + create_list(:lettings_log, 3, :duplicate, discarded_at: Time.zone.now, status: 4, owning_organisation: organisation) + end + it "returns them all with no repeats" do expected_sales_duplicates_result = [ duplicate_sales_logs.map(&:id), @@ -87,7 +118,18 @@ RSpec.describe DuplicateLogsHelper do duplicate_sales_logs_3.map(&:id), ] - expect(sales_duplicate_sets_from_collection(sales_logs, organisation)).to match_array(expected_sales_duplicates_result) + expected_lettings_duplicates_result = [ + duplicate_lettings_logs.map(&:id), + duplicate_lettings_logs_too.map(&:id), + duplicate_lettings_logs_3.map(&:id), + ] + + expect(duplicates_for_organisation(organisation)[:lettings]).to match_array( + expected_lettings_duplicates_result.map { |nested_array| match_array(nested_array) }, + ) + expect(duplicates_for_organisation(organisation)[:sales]).to match_array( + expected_sales_duplicates_result.map { |nested_array| match_array(nested_array) }, + ) end end end