Refactor

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

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 }
 

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)

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

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

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