Merge branch 'main' into CLDC-2588-Update-soft-validation-errors-in-bulk-upload

Gemfile.lock

@@ -379,7 +379,7 @@ GEM
     responders (3.1.1)
       actionpack (>= 5.2)
       railties (>= 5.2)
-    rexml (3.3.2)
+    rexml (3.3.3)
       strscan
     roo (2.10.1)
       nokogiri (~> 1)

app/controllers/form_controller.rb

@@ -26,6 +26,8 @@ class FormController < ApplicationController
           flash[:notice] = "You have successfully updated #{updated_question_string}"
         end
 
+        update_duplication_tracking
+
         pages_requiring_update = pages_requiring_update(shown_page_ids_with_unanswered_questions_before_update)
         redirect_to(successful_redirect_path(pages_requiring_update))
       else
@@ -192,24 +194,43 @@ private
     params[@log.model_name.param_key]["interruption_page_referrer_type"].presence
   end
 
-  def successful_redirect_path(pages_to_check)
+  def update_duplication_tracking
-    if FeatureToggle.deduplication_flow_enabled?
+    class_name = @log.class.name.underscore
-      if is_referrer_type?("duplicate_logs") || is_referrer_type?("duplicate_logs_banner")
+    dynamic_duplicates = current_user.send(class_name.pluralize).duplicate_logs(@log)
-        return correcting_duplicate_logs_redirect_path
+
+    if dynamic_duplicates.any?
+      saved_duplicates = @log.duplicates
+      if saved_duplicates.none? || duplicates_changed?(dynamic_duplicates, saved_duplicates)
+        duplicate_set_id = dynamic_duplicates.first.duplicate_set_id || new_duplicate_set_id(@log)
+        update_logs_with_duplicate_set_id(@log, dynamic_duplicates, duplicate_set_id)
+        saved_duplicates.first.update!(duplicate_set_id: nil) if saved_duplicates.count == 1
       end
+    else
+      remove_fixed_duplicate_set_ids(@log)
+    end
+  end
+
+  def successful_redirect_path(pages_to_check)
+    class_name = @log.class.name.underscore
+
+    if is_referrer_type?("duplicate_logs") || is_referrer_type?("duplicate_logs_banner")
+      original_log = current_user.send(class_name.pluralize).find_by(id: from_referrer_query("original_log_id"))
 
-      dynamic_duplicates = @log.lettings? ? current_user.lettings_logs.duplicate_logs(@log) : current_user.sales_logs.duplicate_logs(@log)
+      if original_log.present? && current_user.send(class_name.pluralize).duplicate_logs(original_log).any?
-      if dynamic_duplicates.any?
+        if @log.duplicates.none?
-        saved_duplicates = @log.duplicates
+          flash[:notice] = deduplication_success_banner
-        if saved_duplicates.none? || duplicates_changed?(dynamic_duplicates, saved_duplicates)
-          duplicate_set_id = dynamic_duplicates.first.duplicate_set_id || new_duplicate_set_id(@log)
-          update_logs_with_duplicate_set_id(@log, dynamic_duplicates, duplicate_set_id)
-          saved_duplicates.first.update!(duplicate_set_id: nil) if saved_duplicates.count == 1
         end
-        return send("#{@log.class.name.underscore}_duplicate_logs_path", @log, original_log_id: @log.id)
+        return send("#{class_name}_duplicate_logs_path", original_log, original_log_id: original_log.id, referrer: params[:referrer], organisation_id: params[:organisation_id])
+      else
+        flash[:notice] = deduplication_success_banner
+        return send("#{class_name}_duplicate_logs_path", "#{class_name}_id".to_sym => from_referrer_query("first_remaining_duplicate_id"), original_log_id: from_referrer_query("original_log_id"), referrer: params[:referrer], organisation_id: params[:organisation_id])
       end
     end
 
+    if @log.duplicates.any?
+      return send("#{@log.class.name.underscore}_duplicate_logs_path", @log, original_log_id: @log.id)
+    end
+
     if is_referrer_type?("check_answers")
       next_page_id = form.next_page_id(@page, @log, current_user)
       next_page = form.get_page(next_page_id)
@@ -315,35 +336,6 @@ private
 
   CONFIRMATION_PAGE_IDS = %w[uprn_confirmation uprn_selection].freeze
 
-  def correcting_duplicate_logs_redirect_path
-    class_name = @log.class.name.underscore
-
-    original_log = current_user.send(class_name.pluralize).find_by(id: from_referrer_query("original_log_id"))
-    dynamic_duplicates = current_user.send(class_name.pluralize).duplicate_logs(@log)
-
-    if dynamic_duplicates.any?
-      saved_duplicates = @log.duplicates
-      if duplicates_changed?(dynamic_duplicates, saved_duplicates)
-        duplicate_set_id = dynamic_duplicates.first.duplicate_set_id || new_duplicate_set_id(@log)
-        update_logs_with_duplicate_set_id(@log, dynamic_duplicates, duplicate_set_id)
-        saved_duplicates.first.update!(duplicate_set_id: nil) if saved_duplicates.count == 1
-      end
-    else
-      remove_fixed_duplicate_set_ids(@log)
-    end
-
-    if original_log.present? && current_user.send(class_name.pluralize).duplicate_logs(original_log).any?
-      if dynamic_duplicates.none?
-        flash[:notice] = deduplication_success_banner
-      end
-      send("#{class_name}_duplicate_logs_path", original_log, original_log_id: original_log.id, referrer: params[:referrer], organisation_id: params[:organisation_id])
-    else
-      remove_fixed_duplicate_set_ids(original_log)
-      flash[:notice] = deduplication_success_banner
-      send("#{class_name}_duplicate_logs_path", "#{class_name}_id".to_sym => from_referrer_query("first_remaining_duplicate_id"), original_log_id: from_referrer_query("original_log_id"), referrer: params[:referrer], organisation_id: params[:organisation_id])
-    end
-  end
-
   def deduplication_success_banner
     deduplicated_log_link = "<a class=\"govuk-notification-banner__link govuk-!-font-weight-bold\" href=\"#{send("#{@log.class.name.underscore}_path", @log)}\">Log #{@log.id}</a>"
     changed_labels = {

app/models/validations/financial_validations.rb

@@ -149,7 +149,12 @@ module Validations::FinancialValidations
 
     unless record.managing_organisation.rent_periods.include? record.period
       record.errors.add :period, :wrong_rent_period, message: I18n.t(
-        "validations.financial.rent_period.invalid_for_org",
+        "validations.financial.rent_period.invalid_for_org.period",
+        org_name: record.managing_organisation.name,
+        rent_period: record.form.get_question("period", record).label_from_value(record.period).downcase,
+      )
+      record.errors.add :managing_organisation_id, :skip_bu_error, message: I18n.t(
+        "validations.financial.rent_period.invalid_for_org.managing_org",
         org_name: record.managing_organisation.name,
         rent_period: record.form.get_question("period", record).label_from_value(record.period).downcase,
       )

app/services/bulk_upload/lettings/year2024/row_parser.rb

@@ -467,6 +467,7 @@ class BulkUpload::Lettings::Year2024::RowParser
 
       fields.each do |field|
         next if errors.include?(field)
+        next if error.type == :skip_bu_error
 
         question = log.form.get_question(error.attribute, log)
 

app/services/feature_toggle.rb

@@ -11,10 +11,6 @@ class FeatureToggle
     !Rails.env.development?
   end
 
-  def self.deduplication_flow_enabled?
-    true
-  end
-
   def self.duplicate_summary_enabled?
     true
   end

config/locales/en.yml

@@ -436,7 +436,9 @@ en:
         under_10: "Enter a total charge that is at least £10.00 per week"
         less_than_shortfall: "The total charge must be more than the outstanding amount"
       rent_period:
-        invalid_for_org: "%{org_name} does not use %{rent_period} as a rent period. Choose another rent period, or a data coordinator can add rent periods to your organisation"
+        invalid_for_org:
+          period: "%{org_name} does not use %{rent_period} as a rent period. Choose another rent period, or a data coordinator can add rent periods to your organisation"
+          managing_org: "%{org_name} does not use %{rent_period} as a rent period. Set another rent period on this log, or a data coordinator can add rent periods to this organisation"
       carehome:
         out_of_range: "Household rent and other charges must be between %{min_chcharge} and %{max_chcharge} if paying %{period}"
         not_provided: "Enter how much rent and other charges the household pays %{period}"

docs/Gemfile.lock

@@ -226,7 +226,7 @@ GEM
     rb-fsevent (0.11.2)
     rb-inotify (0.10.1)
       ffi (~> 1.0)
-    rexml (3.3.2)
+    rexml (3.3.3)
       strscan
     rouge (3.26.0)
     ruby2_keywords (0.0.5)

spec/models/validations/financial_validations_spec.rb

@@ -165,7 +165,13 @@ RSpec.describe Validations::FinancialValidations do
         financial_validator.validate_rent_period(record)
         expect(record.errors["period"])
           .to include(match I18n.t(
-            "validations.financial.rent_period.invalid_for_org",
+            "validations.financial.rent_period.invalid_for_org.period",
+            org_name: user.organisation.name,
+            rent_period: "every 4 weeks",
+          ))
+        expect(record.errors["managing_organisation_id"])
+          .to include(match I18n.t(
+            "validations.financial.rent_period.invalid_for_org.managing_org",
             org_name: user.organisation.name,
             rent_period: "every 4 weeks",
           ))

spec/requests/form_controller_spec.rb

@@ -763,39 +763,162 @@ RSpec.describe FormController, type: :request do
             }
           end
 
-          before do
+          context "when the log will not be a duplicate" do
-            post "/lettings-logs/#{lettings_log.id}/#{page_id.dasherize}", params:
+            before do
-          end
+              post "/lettings-logs/#{lettings_log.id}/#{page_id.dasherize}", params:
+            end
 
-          it "re-renders the same page with errors if validation fails" do
+            it "re-renders the same page with errors if validation fails" do
-            expect(response).to have_http_status(:redirect)
+              expect(response).to have_http_status(:redirect)
-          end
+            end
 
-          it "only updates answers that apply to the page being submitted" do
+            it "only updates answers that apply to the page being submitted" do
-            lettings_log.reload
+              lettings_log.reload
-            expect(lettings_log.age1).to eq(answer)
+              expect(lettings_log.age1).to eq(answer)
-            expect(lettings_log.age2).to be nil
+              expect(lettings_log.age2).to be nil
+            end
+
+            it "tracks who updated the record" do
+              lettings_log.reload
+              whodunnit_actor = lettings_log.versions.last.actor
+              expect(whodunnit_actor).to be_a(User)
+              expect(whodunnit_actor.id).to eq(user.id)
+            end
           end
 
-          it "tracks who updated the record" do
+          context "when the answer makes the log a duplicate" do
-            lettings_log.reload
+            context "with one other log" do
-            whodunnit_actor = lettings_log.versions.last.actor
+              let(:new_duplicate) { create(:lettings_log) }
-            expect(whodunnit_actor).to be_a(User)
+
-            expect(whodunnit_actor.id).to eq(user.id)
+              before do
+                allow(LettingsLog).to receive(:duplicate_logs).and_return(LettingsLog.where(id: new_duplicate.id))
+                post "/lettings-logs/#{lettings_log.id}/#{page_id.dasherize}", params:
+              end
+
+              it "sets a new duplicate set id on both logs" do
+                lettings_log.reload
+                new_duplicate.reload
+                expect(lettings_log.duplicate_set_id).not_to be_nil
+                expect(lettings_log.duplicate_set_id).to eql(new_duplicate.duplicate_set_id)
+              end
+
+              it "redirects to the duplicate logs page" do
+                expect(response).to redirect_to("/lettings-logs/#{lettings_log.id}/duplicate-logs?original_log_id=#{lettings_log.id}")
+                follow_redirect!
+                expect(page).to have_content("These logs are duplicates")
+              end
+            end
+
+            context "with a set of other logs" do
+              let(:duplicate_set_id) { 100 }
+              let(:new_duplicates) { create_list(:lettings_log, 2, duplicate_set_id:) }
+
+              before do
+                allow(LettingsLog).to receive(:duplicate_logs).and_return(LettingsLog.where(id: new_duplicates.pluck(:id)))
+                post "/lettings-logs/#{lettings_log.id}/#{page_id.dasherize}", params:
+              end
+
+              it "sets the logs duplicate set id to that of the set it is now part of" do
+                lettings_log.reload
+                expect(lettings_log.duplicate_set_id).to eql(duplicate_set_id)
+                new_duplicates.each do |log|
+                  log.reload
+                  expect(log.duplicate_set_id).to eql(duplicate_set_id)
+                end
+              end
+
+              it "redirects to the duplicate logs page" do
+                expect(response).to redirect_to("/lettings-logs/#{lettings_log.id}/duplicate-logs?original_log_id=#{lettings_log.id}")
+                follow_redirect!
+                expect(page).to have_content("These logs are duplicates")
+              end
+            end
+
+            context "when the log was previously in a different duplicate set" do
+              context "with a single other log" do
+                let(:old_duplicate_set_id) { 110 }
+                let!(:old_duplicate) { create(:lettings_log, duplicate_set_id: old_duplicate_set_id) }
+                let(:lettings_log) { create(:lettings_log, assigned_to: user, duplicate_set_id: old_duplicate_set_id) }
+                let(:new_duplicate) { create(:lettings_log) }
+
+                before do
+                  allow(LettingsLog).to receive(:duplicate_logs).and_return(LettingsLog.where(id: new_duplicate.id))
+                  post "/lettings-logs/#{lettings_log.id}/#{page_id.dasherize}", params:
+                end
+
+                it "updates the relevant duplicate set ids" do
+                  lettings_log.reload
+                  old_duplicate.reload
+                  new_duplicate.reload
+                  expect(old_duplicate.duplicate_set_id).to be_nil
+                  expect(lettings_log.duplicate_set_id).not_to be_nil
+                  expect(lettings_log.duplicate_set_id).to eql(new_duplicate.duplicate_set_id)
+                end
+              end
+
+              context "with multiple other logs" do
+                let(:old_duplicate_set_id) { 120 }
+                let!(:old_duplicates) { create_list(:lettings_log, 2, duplicate_set_id: old_duplicate_set_id) }
+                let(:lettings_log) { create(:lettings_log, assigned_to: user, duplicate_set_id: old_duplicate_set_id) }
+                let(:new_duplicate) { create(:lettings_log) }
+
+                before do
+                  allow(LettingsLog).to receive(:duplicate_logs).and_return(LettingsLog.where(id: new_duplicate.id))
+                  post "/lettings-logs/#{lettings_log.id}/#{page_id.dasherize}", params:
+                end
+
+                it "updates the relevant duplicate set ids" do
+                  lettings_log.reload
+                  new_duplicate.reload
+                  old_duplicates.each do |log|
+                    log.reload
+                    expect(log.duplicate_set_id).to eql(old_duplicate_set_id)
+                  end
+                  expect(lettings_log.duplicate_set_id).not_to be_nil
+                  expect(lettings_log.duplicate_set_id).not_to eql(old_duplicate_set_id)
+                  expect(lettings_log.duplicate_set_id).to eql(new_duplicate.duplicate_set_id)
+                end
+              end
+            end
           end
 
-          context "and duplicate logs" do
+          context "when the answer makes the log stop being a duplicate" do
-            let(:duplicate_logs) { create_list(:lettings_log, 2) }
+            context "when the log had one duplicate" do
+              let(:old_duplicate_set_id) { 130 }
+              let!(:old_duplicate) { create(:lettings_log, duplicate_set_id: old_duplicate_set_id) }
+              let(:lettings_log) { create(:lettings_log, assigned_to: user, duplicate_set_id: old_duplicate_set_id) }
 
-            before do
+              before do
-              allow(LettingsLog).to receive(:duplicate_logs).and_return(duplicate_logs)
+                allow(LettingsLog).to receive(:duplicate_logs).and_return(LettingsLog.none)
-              post "/lettings-logs/#{lettings_log.id}/#{page_id.dasherize}", params:
+                post "/lettings-logs/#{lettings_log.id}/#{page_id.dasherize}", params:
+              end
+
+              it "updates the relevant duplicate set ids" do
+                lettings_log.reload
+                old_duplicate.reload
+                expect(old_duplicate.duplicate_set_id).to be_nil
+                expect(lettings_log.duplicate_set_id).to be_nil
+              end
             end
 
-            it "redirects to the duplicate logs page" do
+            context "when the log had multiple duplicates" do
-              expect(response).to redirect_to("/lettings-logs/#{lettings_log.id}/duplicate-logs?original_log_id=#{lettings_log.id}")
+              let(:old_duplicate_set_id) { 140 }
-              follow_redirect!
+              let!(:old_duplicates) { create_list(:lettings_log, 2, duplicate_set_id: old_duplicate_set_id) }
-              expect(page).to have_content("These logs are duplicates")
+              let(:lettings_log) { create(:lettings_log, assigned_to: user, duplicate_set_id: old_duplicate_set_id) }
+
+              before do
+                allow(LettingsLog).to receive(:duplicate_logs).and_return(LettingsLog.none)
+                post "/lettings-logs/#{lettings_log.id}/#{page_id.dasherize}", params:
+              end
+
+              it "updates the relevant duplicate set ids" do
+                lettings_log.reload
+                old_duplicates.each do |log|
+                  log.reload
+                  expect(log.duplicate_set_id).to eql(old_duplicate_set_id)
+                end
+                expect(lettings_log.duplicate_set_id).to be_nil
+              end
             end
           end
         end
@@ -816,19 +939,141 @@ RSpec.describe FormController, type: :request do
               },
             }
           end
+          let(:page_id) { "buyer_1_age" }
 
-          context "and duplicate logs" do
+          context "when the answer makes the log a duplicate" do
-            let!(:duplicate_logs) { create_list(:sales_log, 2) }
+            context "with one other log" do
+              let(:new_duplicate) { create(:sales_log) }
 
-            before do
+              before do
-              allow(SalesLog).to receive(:duplicate_logs).and_return(duplicate_logs)
+                allow(SalesLog).to receive(:duplicate_logs).and_return(SalesLog.where(id: new_duplicate.id))
-              post "/sales-logs/#{sales_log.id}/buyer-1-age", params:
+                post "/sales-logs/#{sales_log.id}/#{page_id.dasherize}", params:
+              end
+
+              it "sets a new duplicate set id on both logs" do
+                sales_log.reload
+                new_duplicate.reload
+                expect(sales_log.duplicate_set_id).not_to be_nil
+                expect(sales_log.duplicate_set_id).to eql(new_duplicate.duplicate_set_id)
+              end
+
+              it "redirects to the duplicate logs page" do
+                expect(response).to redirect_to("/sales-logs/#{sales_log.id}/duplicate-logs?original_log_id=#{sales_log.id}")
+                follow_redirect!
+                expect(page).to have_content("These logs are duplicates")
+              end
             end
 
-            it "redirects to the duplicate logs page" do
+            context "with a set of other logs" do
-              expect(response).to redirect_to("/sales-logs/#{sales_log.id}/duplicate-logs?original_log_id=#{sales_log.id}")
+              let(:duplicate_set_id) { 100 }
-              follow_redirect!
+              let(:new_duplicates) { create_list(:sales_log, 2, duplicate_set_id:) }
-              expect(page).to have_content("These logs are duplicates")
+
+              before do
+                allow(SalesLog).to receive(:duplicate_logs).and_return(SalesLog.where(id: new_duplicates.pluck(:id)))
+                post "/sales-logs/#{sales_log.id}/#{page_id.dasherize}", params:
+              end
+
+              it "sets the logs duplicate set id to that of the set it is now part of" do
+                sales_log.reload
+                expect(sales_log.duplicate_set_id).to eql(duplicate_set_id)
+                new_duplicates.each do |log|
+                  log.reload
+                  expect(log.duplicate_set_id).to eql(duplicate_set_id)
+                end
+              end
+
+              it "redirects to the duplicate logs page" do
+                expect(response).to redirect_to("/sales-logs/#{sales_log.id}/duplicate-logs?original_log_id=#{sales_log.id}")
+                follow_redirect!
+                expect(page).to have_content("These logs are duplicates")
+              end
+            end
+
+            context "when the log was previously in a different duplicate set" do
+              context "with a single other log" do
+                let(:old_duplicate_set_id) { 110 }
+                let!(:old_duplicate) { create(:sales_log, duplicate_set_id: old_duplicate_set_id) }
+                let(:sales_log) { create(:sales_log, assigned_to: user, duplicate_set_id: old_duplicate_set_id) }
+                let(:new_duplicate) { create(:sales_log) }
+
+                before do
+                  allow(SalesLog).to receive(:duplicate_logs).and_return(SalesLog.where(id: new_duplicate.id))
+                  post "/sales-logs/#{sales_log.id}/#{page_id.dasherize}", params:
+                end
+
+                it "updates the relevant duplicate set ids" do
+                  sales_log.reload
+                  old_duplicate.reload
+                  new_duplicate.reload
+                  expect(old_duplicate.duplicate_set_id).to be_nil
+                  expect(sales_log.duplicate_set_id).not_to be_nil
+                  expect(sales_log.duplicate_set_id).to eql(new_duplicate.duplicate_set_id)
+                end
+              end
+
+              context "with multiple other logs" do
+                let(:old_duplicate_set_id) { 120 }
+                let!(:old_duplicates) { create_list(:sales_log, 2, duplicate_set_id: old_duplicate_set_id) }
+                let(:sales_log) { create(:sales_log, assigned_to: user, duplicate_set_id: old_duplicate_set_id) }
+                let(:new_duplicate) { create(:sales_log) }
+
+                before do
+                  allow(SalesLog).to receive(:duplicate_logs).and_return(SalesLog.where(id: new_duplicate.id))
+                  post "/sales-logs/#{sales_log.id}/#{page_id.dasherize}", params:
+                end
+
+                it "updates the relevant duplicate set ids" do
+                  sales_log.reload
+                  new_duplicate.reload
+                  old_duplicates.each do |log|
+                    log.reload
+                    expect(log.duplicate_set_id).to eql(old_duplicate_set_id)
+                  end
+                  expect(sales_log.duplicate_set_id).not_to be_nil
+                  expect(sales_log.duplicate_set_id).not_to eql(old_duplicate_set_id)
+                  expect(sales_log.duplicate_set_id).to eql(new_duplicate.duplicate_set_id)
+                end
+              end
+            end
+          end
+
+          context "when the answer makes the log stop being a duplicate" do
+            context "when the log had one duplicate" do
+              let(:old_duplicate_set_id) { 130 }
+              let!(:old_duplicate) { create(:sales_log, duplicate_set_id: old_duplicate_set_id) }
+              let(:sales_log) { create(:sales_log, assigned_to: user, duplicate_set_id: old_duplicate_set_id) }
+
+              before do
+                allow(SalesLog).to receive(:duplicate_logs).and_return(SalesLog.none)
+                post "/sales-logs/#{sales_log.id}/#{page_id.dasherize}", params:
+              end
+
+              it "updates the relevant duplicate set ids" do
+                sales_log.reload
+                old_duplicate.reload
+                expect(old_duplicate.duplicate_set_id).to be_nil
+                expect(sales_log.duplicate_set_id).to be_nil
+              end
+            end
+
+            context "when the log had multiple duplicates" do
+              let(:old_duplicate_set_id) { 140 }
+              let!(:old_duplicates) { create_list(:sales_log, 2, duplicate_set_id: old_duplicate_set_id) }
+              let(:sales_log) { create(:sales_log, assigned_to: user, duplicate_set_id: old_duplicate_set_id) }
+
+              before do
+                allow(SalesLog).to receive(:duplicate_logs).and_return(SalesLog.none)
+                post "/sales-logs/#{sales_log.id}/#{page_id.dasherize}", params:
+              end
+
+              it "updates the relevant duplicate set ids" do
+                sales_log.reload
+                old_duplicates.each do |log|
+                  log.reload
+                  expect(log.duplicate_set_id).to eql(old_duplicate_set_id)
+                end
+                expect(sales_log.duplicate_set_id).to be_nil
+              end
             end
           end
         end

spec/services/bulk_upload/lettings/year2024/row_parser_spec.rb

@@ -750,6 +750,18 @@ RSpec.describe BulkUpload::Lettings::Year2024::RowParser do
             expect(parser.errors[:field_15]).to eq(["You must answer tenant has seen the privacy notice"])
           end
         end
+
+        context "when there is a :skip_bu_error error" do
+          let(:managing_org) { create(:organisation, :with_old_visible_id, rent_periods: [4, 1]) }
+          let(:attributes) { valid_attributes.merge({ field_123: 3, field_128: 80 }) }
+
+          it "does not add that error" do
+            parser.valid?
+
+            expect(parser.log.errors.map(&:attribute).sort).to eql(%i[managing_organisation_id period])
+            expect(parser.errors.map(&:attribute)).to eql(%i[field_123])
+          end
+        end
       end
 
       describe "#validate_nulls" do