feat: refactor to avoid redundant re-validations and test

app/models/lettings_log.rb

@@ -531,14 +531,11 @@ class LettingsLog < Log
   def blank_compound_invalid_non_setup_fields!
     super
 
-    self.postcode_known = nil if errors.of_kind?(:postcode_full, :wrong_format)
+    self.postcode_known = nil if errors.attribute_names.include? :postcode_full
 
     if errors.of_kind?(:earnings, :under_hard_min)
-      self.earnings = nil
       self.incfreq = nil
     end
-
-    valid?
   end
 
 private

app/models/log.rb

@@ -98,15 +98,13 @@ class Log < ApplicationRecord
   def blank_invalid_non_setup_fields!
     setup_ids = form.setup_sections.flat_map(&:subsections).flat_map(&:questions).map(&:id)
 
-    blank_compound_invalid_non_setup_fields!
-
     errors.each do |error|
       next if setup_ids.include?(error.attribute.to_s)
 
       public_send("#{error.attribute}=", nil)
     end
 
-    valid?
+    blank_compound_invalid_non_setup_fields!
   end
 
   (1..8).each do |person_num|
@@ -141,7 +139,7 @@ class Log < ApplicationRecord
   end
 
   def blank_compound_invalid_non_setup_fields!
-    self.ppcodenk = nil if errors.of_kind?(:ppostcode_full, :wrong_format)
+    self.ppcodenk = nil if errors.attribute_names.include? :ppostcode_full
 
     if errors.of_kind?(:uprn, :uprn_error)
       self.uprn_known = nil
@@ -152,8 +150,6 @@ class Log < ApplicationRecord
       self.postcode_full = nil
       self.county = nil
     end
-
-    valid?
   end
 
 private

app/models/sales_log.rb

@@ -363,12 +363,6 @@ class SalesLog < Log
   def blank_compound_invalid_non_setup_fields!
     super
 
-    self.pcodenk = nil if errors.of_kind?(:postcode_full, :wrong_format)
-
-    if errors.of_kind?(:postcode_full, :postcodes_not_matching)
-      self.ppcodenk = nil
-    end
-
-    valid?
+    self.pcodenk = nil if errors.attribute_names.include? :postcode_full
   end
 end

app/services/bulk_upload/sales/validator.rb

@@ -37,8 +37,15 @@ class BulkUpload::Sales::Validator
   def create_logs?
     return false if any_setup_errors?
     return false if row_parsers.any?(&:block_log_creation?)
+    return false if any_logs_already_exist? && FeatureToggle.bulk_upload_duplicate_log_check_enabled?
 
-    row_parsers.all? { |row_parser| row_parser.log.valid? }
+    row_parsers.each do |row_parser|
+      row_parser.log.blank_invalid_non_setup_fields!
+    end
+
+    return false if any_logs_invalid?
+
+    true
   end
 
   def any_setup_errors?
@@ -49,8 +56,16 @@ class BulkUpload::Sales::Validator
       .positive?
   end
 
+  def any_logs_already_exist?
+    row_parsers.any?(&:log_already_exists?)
+  end
+
 private
 
+  def any_logs_invalid?
+    row_parsers.any? { |row_parser| row_parser.log.invalid? }
+  end
+
   def csv_parser
     @csv_parser ||= case bulk_upload.year
                     when 2022

spec/services/bulk_upload/lettings/log_creator_spec.rb

@@ -82,6 +82,36 @@ RSpec.describe BulkUpload::Lettings::LogCreator do
       end
     end
 
+    context "when a valid csv with row with compound errors on non setup field" do
+      let(:file) { Tempfile.new }
+      let(:path) { file.path }
+      let(:log) do
+        build(
+          :lettings_log,
+          :completed,
+          earnings: 0,
+          incfreq: 1,
+        )
+      end
+
+      before do
+        file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_2023_csv_row)
+        file.rewind
+      end
+
+      it "creates the log" do
+        expect { service.call }.to change(LettingsLog, :count).by(1)
+      end
+
+      it "blanks invalid fields" do
+        service.call
+
+        record = LettingsLog.last
+        expect(record.earnings).to be_blank
+        expect(record.incfreq).to be_blank
+      end
+    end
+
     context "when pre-creating logs" do
       subject(:service) { described_class.new(bulk_upload:, path:) }
 

spec/services/bulk_upload/sales/log_creator_spec.rb

@@ -75,6 +75,42 @@ RSpec.describe BulkUpload::Sales::LogCreator do
       end
     end
 
+    context "when a valid csv with row with compound errors on non setup field" do
+      let(:file) { Tempfile.new }
+      let(:path) { file.path }
+      let(:log) do
+        build(
+          :sales_log,
+          :completed,
+          ownershipsch: 2,
+          pcodenk: 0,
+          ppcodenk: 0,
+          postcode_full: "AA11AA",
+          ppostcode_full: "BB22BB",
+        )
+      end
+
+      before do
+        file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_2022_sales_csv_row)
+        file.rewind
+      end
+
+      it "creates the log" do
+        expect { service.call }.to change(SalesLog, :count).by(1)
+      end
+
+      it "blanks invalid field" do
+        service.call
+
+        record = SalesLog.last
+        expect(record.pcodenk).to be_blank
+        expect(record.postcode_full).to be_blank
+        expect(record.ppcodenk).to be_blank
+        expect(record.ppostcode_full).to be_blank
+      end
+    end
+
+
     context "when pre-creating logs" do
       subject(:service) { described_class.new(bulk_upload:, path:) }