From ca8b7970127b8db56661d5515290298c200da349 Mon Sep 17 00:00:00 2001
From: Phil Lee <asmega@users.noreply.github.com>
Date: Mon, 13 Feb 2023 15:40:39 +0000
Subject: [PATCH] bulk upload ignores blank rows in csv (#1295)

---
 .../bulk_upload/lettings/log_creator.rb       |  2 ++
 .../bulk_upload/lettings/row_parser.rb        |  6 ++++
 .../bulk_upload/lettings/log_creator_spec.rb  | 17 +++++++++++
 .../bulk_upload/lettings/row_parser_spec.rb   | 30 +++++++++++++++++--
 4 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/app/services/bulk_upload/lettings/log_creator.rb b/app/services/bulk_upload/lettings/log_creator.rb
index 9dd4a2f66..625d53f43 100644
--- a/app/services/bulk_upload/lettings/log_creator.rb
+++ b/app/services/bulk_upload/lettings/log_creator.rb
@@ -10,6 +10,8 @@ class BulkUpload::Lettings::LogCreator
     row_parsers.each do |row_parser|
       row_parser.valid?
 
+      next if row_parser.blank_row?
+
       row_parser.log.blank_invalid_non_setup_fields!
       row_parser.log.bulk_upload = bulk_upload
 
diff --git a/app/services/bulk_upload/lettings/row_parser.rb b/app/services/bulk_upload/lettings/row_parser.rb
index 42e762be1..9d5604587 100644
--- a/app/services/bulk_upload/lettings/row_parser.rb
+++ b/app/services/bulk_upload/lettings/row_parser.rb
@@ -154,6 +154,8 @@ class BulkUpload::Lettings::RowParser
   def valid?
     errors.clear
 
+    return true if blank_row?
+
     super
 
     log.valid?
@@ -166,6 +168,10 @@ class BulkUpload::Lettings::RowParser
     errors.blank?
   end
 
+  def blank_row?
+    attribute_set.to_hash.reject { |k, _| %w[bulk_upload].include?(k) }.values.compact.empty?
+  end
+
   def log
     @log ||= LettingsLog.new(attributes_for_log)
   end
diff --git a/spec/services/bulk_upload/lettings/log_creator_spec.rb b/spec/services/bulk_upload/lettings/log_creator_spec.rb
index fc7d1cdc0..39dfd00fe 100644
--- a/spec/services/bulk_upload/lettings/log_creator_spec.rb
+++ b/spec/services/bulk_upload/lettings/log_creator_spec.rb
@@ -24,6 +24,23 @@ RSpec.describe BulkUpload::Lettings::LogCreator do
       end
     end
 
+    context "when a valid csv with several blank rows" do
+      let(:file) { Tempfile.new }
+      let(:path) { file.path }
+      let(:log) { LettingsLog.new }
+
+      before do
+        file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_csv_row)
+        file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_csv_row)
+        file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_csv_row)
+        file.rewind
+      end
+
+      it "ignores them and does not create the logs" do
+        expect { service.call }.not_to change(LettingsLog, :count)
+      end
+    end
+
     context "when a valid csv with row with one invalid non setup field" do
       let(:file) { Tempfile.new }
       let(:path) { file.path }
diff --git a/spec/services/bulk_upload/lettings/row_parser_spec.rb b/spec/services/bulk_upload/lettings/row_parser_spec.rb
index 29ce6efbb..8f609838c 100644
--- a/spec/services/bulk_upload/lettings/row_parser_spec.rb
+++ b/spec/services/bulk_upload/lettings/row_parser_spec.rb
@@ -31,6 +31,24 @@ RSpec.describe BulkUpload::Lettings::RowParser do
     FormHandler.instance.use_fake_forms!
   end
 
+  describe "#blank_row?" do
+    context "when a new object" do
+      it "returns true" do
+        expect(parser).to be_blank_row
+      end
+    end
+
+    context "when any field is populated" do
+      before do
+        parser.field_1 = "1"
+      end
+
+      it "returns false" do
+        expect(parser).not_to be_blank_row
+      end
+    end
+  end
+
   describe "validations" do
     before do
       stub_request(:get, /api.postcodes.io/)
@@ -40,6 +58,14 @@ RSpec.describe BulkUpload::Lettings::RowParser do
     end
 
     describe "#valid?" do
+      context "when the row is blank" do
+        let(:attributes) { { bulk_upload: } }
+
+        it "returns true" do
+          expect(parser).to be_valid
+        end
+      end
+
       context "when calling the method multiple times" do
         let(:attributes) { { bulk_upload:, field_134: 2 } }
 
@@ -172,7 +198,7 @@ RSpec.describe BulkUpload::Lettings::RowParser do
 
     describe "#field_1" do
       context "when null" do
-        let(:attributes) { { bulk_upload:, field_1: nil } }
+        let(:attributes) { { bulk_upload:, field_1: nil, field_4: "1" } }
 
         it "returns an error" do
           expect(parser.errors[:field_1]).to be_present
@@ -347,7 +373,7 @@ RSpec.describe BulkUpload::Lettings::RowParser do
 
     describe "fields 96, 97, 98 => startdate" do
       context "when any one of these fields is blank" do
-        let(:attributes) { { bulk_upload:, field_96: nil, field_97: nil, field_98: nil } }
+        let(:attributes) { { bulk_upload:, field_1: "1", field_96: nil, field_97: nil, field_98: nil } }
 
         it "returns an error" do
           parser.valid?