From 903cf817494aecc77a1653d3a53c4e05e3ad5807 Mon Sep 17 00:00:00 2001
From: Phil Lee <asmega@users.noreply.github.com>
Date: Tue, 23 May 2023 09:50:18 +0100
Subject: [PATCH] CLDC-2305 Bulk upload setup fields validations (#1633)

# Context

- https://digital.dclg.gov.uk/jira/browse/CLDC-2305
- This is rework to add missing validations for sales setup fields for bulk upload

# Changes

- There is code that clears log fields if they are not a valid option. in order to generate errors for these we check certain fields for content prior to `log.valid?` being called. Unfortunately there does not appear to be an easy way to access to valid options therefore these values have been hard coded
- Privacy notice must be accepted other considered a setup error
- The `type` question can actually be one of three questions with the same identifier. these are excluded as part of `validate_valid_radio_option` and instead validated with a different mechanism. The rationale being that `log.valid?` will clear data before we get there
- Remove tests associated to upload threshold which are no longer required
---
 app/models/bulk_upload_error.rb               |   1 +
 .../bulk_upload/sales/year2022/row_parser.rb  |  84 ++++++++++-
 .../bulk_upload/sales/year2023/row_parser.rb  |  75 +++++++++-
 .../show.html.erb                             |   2 +-
 .../summary.html.erb                          |   2 +-
 .../bulk_upload_sales_results/show.html.erb   |   2 +-
 .../summary.html.erb                          |   2 +-
 config/locales/en.yml                         |   4 +
 .../bulk_upload/sales/validator_spec.rb       |  46 ------
 .../sales/year2022/row_parser_spec.rb         | 137 +++++++++++++++---
 .../sales/year2023/row_parser_spec.rb         | 116 ++++++++++++++-
 .../show.html.erb_spec.rb                     |  35 ++++-
 .../summary.html.erb_spec.rb                  |  35 ++++-
 .../show.html.erb_spec.rb                     |  39 +++++
 .../summary.html.erb_spec.rb                  |  39 +++++
 15 files changed, 512 insertions(+), 107 deletions(-)
 create mode 100644 spec/views/bulk_upload_sales_results/show.html.erb_spec.rb
 create mode 100644 spec/views/bulk_upload_sales_results/summary.html.erb_spec.rb

diff --git a/app/models/bulk_upload_error.rb b/app/models/bulk_upload_error.rb
index 0c298ddf8..b9ad3ddde 100644
--- a/app/models/bulk_upload_error.rb
+++ b/app/models/bulk_upload_error.rb
@@ -1,6 +1,7 @@
 class BulkUploadError < ApplicationRecord
   belongs_to :bulk_upload
 
+  scope :order_by_row, -> { order(row: :asc) }
   scope :order_by_cell, -> { order(Arel.sql("LPAD(cell, 10, '0')")) }
   scope :order_by_col, -> { order(Arel.sql("LPAD(col, 10, '0')")) }
 end
diff --git a/app/services/bulk_upload/sales/year2022/row_parser.rb b/app/services/bulk_upload/sales/year2022/row_parser.rb
index f73fbcfa7..7eafd7e06 100644
--- a/app/services/bulk_upload/sales/year2022/row_parser.rb
+++ b/app/services/bulk_upload/sales/year2022/row_parser.rb
@@ -265,14 +265,74 @@ class BulkUpload::Sales::Year2022::RowParser
   validates :field_4, presence: { message: I18n.t("validations.not_answered", question: "sale completion date (year)"), category: :setup }, on: :after_log
   validates :field_4, format: { with: /\A\d{2}\z/, message: I18n.t("validations.setup.saledate.year_not_two_digits") }, on: :after_log
 
+  validates :field_57,
+            inclusion: {
+              in: [2, 16, 18, 24, 28, 30, 31],
+              category: :setup,
+              question: QUESTIONS[:field_57].downcase,
+            },
+            if: proc { field_57.present? && shared_ownership? },
+            on: :before_log
+
+  validates :field_57,
+            presence: {
+              message: I18n.t("validations.not_answered", question: "shared ownership type"),
+              category: :setup,
+            },
+            if: :shared_ownership?,
+            on: :after_log
+
+  validates :field_76,
+            inclusion: {
+              in: [8, 14, 27, 9, 29, 21, 22],
+              category: :setup,
+              question: QUESTIONS[:field_76].downcase,
+            },
+            if: proc { field_76.present? && discounted_ownership? },
+            on: :before_log
+
+  validates :field_76,
+            presence: {
+              message: I18n.t("validations.not_answered", question: "discounted ownership type"),
+              category: :setup,
+            },
+            if: :discounted_ownership?,
+            on: :after_log
+
+  validates :field_84,
+            inclusion: {
+              in: [10, 12],
+              category: :setup,
+              question: QUESTIONS[:field_84].downcase,
+            },
+            if: proc { field_84.present? && outright_sale? },
+            on: :before_log
+
+  validates :field_84,
+            presence: {
+              message: I18n.t("validations.not_answered", question: "type of outright sale"),
+              category: :setup,
+            },
+            if: :outright_sale?,
+            on: :after_log
+
+  validates :field_85,
+            presence: {
+              message: I18n.t("validations.not_answered", question: "type of outright sale"),
+              category: :setup,
+            },
+            if: proc { field_84 == 12 },
+            on: :after_log
+
+  validates :field_109, presence: { message: I18n.t("validations.not_answered", question: "more than 2 buyers"), category: :setup }, if: :joint_purchase?, on: :after_log
+
   validates :field_113, presence: { message: I18n.t("validations.not_answered", question: "ownership type"), category: :setup }, on: :after_log
-  validates :field_57, presence: { message: I18n.t("validations.not_answered", question: "shared ownership type"), category: :setup }, if: :shared_ownership?, on: :after_log
-  validates :field_76, presence: { message: I18n.t("validations.not_answered", question: "shared ownership type"), category: :setup }, if: :discounted_ownership?, on: :after_log
-  validates :field_84, presence: { message: I18n.t("validations.not_answered", question: "shared ownership type"), category: :setup }, if: :outright_sale?, on: :after_log
+
+  validates :field_114, presence: { message: I18n.t("validations.not_answered", question: "company buyer"), category: :setup }, if: :outright_sale?, on: :after_log
+
   validates :field_115, presence: { message: I18n.t("validations.not_answered", question: "will the buyers live in the property"), category: :setup }, if: :outright_sale?, on: :after_log
+
   validates :field_116, presence: { message: I18n.t("validations.not_answered", question: "joint purchase"), category: :setup }, if: :joint_purchase_asked?, on: :after_log
-  validates :field_114, presence: { message: I18n.t("validations.not_answered", question: "company buyer"), category: :setup }, if: :outright_sale?, on: :after_log
-  validates :field_109, presence: { message: I18n.t("validations.not_answered", question: "more than 2 buyers"), category: :setup }, if: :joint_purchase?, on: :after_log
 
   validate :validate_buyer1_economic_status, on: :before_log
   validate :validate_nulls, on: :after_log
@@ -288,6 +348,8 @@ class BulkUpload::Sales::Year2022::RowParser
   validate :validate_incomplete_soft_validations, on: :after_log
   validate :validate_if_log_already_exists, on: :after_log, if: -> { FeatureToggle.bulk_upload_duplicate_log_check_enabled? }
 
+  validate :validate_data_protection_answered, on: :after_log
+
   def self.question_for_field(field)
     QUESTIONS[field]
   end
@@ -351,6 +413,12 @@ class BulkUpload::Sales::Year2022::RowParser
 
 private
 
+  def validate_data_protection_answered
+    unless field_112 == 1
+      errors.add(:field_112, I18n.t("validations.not_answered", question: QUESTIONS[:field_112].downcase), category: :setup)
+    end
+  end
+
   def buyer_not_interviewed?
     field_6 == 1
   end
@@ -957,6 +1025,8 @@ private
 
   def validate_valid_radio_option
     log.attributes.each do |question_id, _v|
+      next if question_id == "type"
+
       question = log.form.get_question(question_id, log)
 
       next unless question&.type == "radio"
@@ -966,9 +1036,9 @@ private
 
       if setup_question?(question)
         fields.each do |field|
-          if errors[field].blank?
+          if errors[field].none?
             block_log_creation!
-            errors.add(field, I18n.t("validations.invalid_option", question: QUESTIONS[field]), category: :setup)
+            errors.add(field, I18n.t("validations.invalid_option", question: QUESTIONS[field].downcase), category: :setup)
           end
         end
       else
diff --git a/app/services/bulk_upload/sales/year2023/row_parser.rb b/app/services/bulk_upload/sales/year2023/row_parser.rb
index 1e1bfbc0c..f2c3e6d8b 100644
--- a/app/services/bulk_upload/sales/year2023/row_parser.rb
+++ b/app/services/bulk_upload/sales/year2023/row_parser.rb
@@ -340,6 +340,15 @@ class BulkUpload::Sales::Year2023::RowParser
             },
             on: :after_log
 
+  validates :field_8,
+            inclusion: {
+              in: [2, 30, 18, 16, 24, 28, 31, 32],
+              if: proc { field_8.present? },
+              category: :setup,
+              question: QUESTIONS[:field_8].downcase,
+            },
+            on: :before_log
+
   validates :field_8,
             presence: {
               message: I18n.t("validations.not_answered", question: "shared ownership type"),
@@ -348,30 +357,57 @@ class BulkUpload::Sales::Year2023::RowParser
             },
             on: :after_log
 
+  validates :field_9,
+            inclusion: {
+              in: [8, 14, 27, 9, 29, 21, 22],
+              if: proc { field_9.present? },
+              category: :setup,
+              question: QUESTIONS[:field_9].downcase,
+            },
+            on: :before_log
+
   validates :field_9,
             presence: {
-              message: I18n.t("validations.not_answered", question: "shared ownership type"),
+              message: I18n.t("validations.not_answered", question: "discounted ownership type"),
               category: :setup,
               if: :discounted_ownership?,
             },
             on: :after_log
 
+  validates :field_10,
+            inclusion: {
+              in: [10, 12],
+              if: proc { field_10.present? },
+              category: :setup,
+              question: QUESTIONS[:field_10].downcase,
+            },
+            on: :before_log
+
   validates :field_10,
             presence: {
-              message: I18n.t("validations.not_answered", question: "shared ownership type"),
+              message: I18n.t("validations.not_answered", question: "type of ouright sale"),
               category: :setup,
               if: :outright_sale?,
             },
             on: :after_log
 
-  validates :field_13,
+  validates :field_11,
             presence: {
-              message: I18n.t("validations.not_answered", question: "will the buyers live in the property"),
+              message: I18n.t("validations.not_answered", question: "type of outright sale"),
               category: :setup,
-              if: :outright_sale?,
+              if: proc { field_10 == 12 },
             },
             on: :after_log
 
+  validates :field_12,
+            inclusion: {
+              in: [1, 2],
+              if: proc { outright_sale? && field_12.present? },
+              category: :setup,
+              question: QUESTIONS[:field_12].downcase,
+            },
+            on: :before_log
+
   validates :field_12,
             presence: {
               message: I18n.t("validations.not_answered", question: "company buyer"),
@@ -380,6 +416,23 @@ class BulkUpload::Sales::Year2023::RowParser
             },
             on: :after_log
 
+  validates :field_13,
+            inclusion: {
+              in: [1, 2],
+              if: proc { outright_sale? && field_13.present? },
+              category: :setup,
+              question: QUESTIONS[:field_13].downcase,
+            },
+            on: :before_log
+
+  validates :field_13,
+            presence: {
+              message: I18n.t("validations.not_answered", question: "will the buyers live in the property"),
+              category: :setup,
+              if: :outright_sale?,
+            },
+            on: :after_log
+
   validates :field_14,
             presence: {
               message: I18n.t("validations.not_answered", question: "joint purchase"),
@@ -413,6 +466,8 @@ class BulkUpload::Sales::Year2023::RowParser
   validate :validate_address_fields, on: :after_log
   validate :validate_if_log_already_exists, on: :after_log, if: -> { FeatureToggle.bulk_upload_duplicate_log_check_enabled? }
 
+  validate :validate_data_protection_answered, on: :after_log
+
   def self.question_for_field(field)
     QUESTIONS[field]
   end
@@ -480,6 +535,12 @@ class BulkUpload::Sales::Year2023::RowParser
 
 private
 
+  def validate_data_protection_answered
+    unless field_29 == 1
+      errors.add(:field_29, I18n.t("validations.not_answered", question: QUESTIONS[:field_29].downcase), category: :setup)
+    end
+  end
+
   def prevtenbuy2
     case field_72
     when "R"
@@ -1156,6 +1217,8 @@ private
     log.attributes.each do |question_id, _v|
       question = log.form.get_question(question_id, log)
 
+      next if question_id == "type"
+
       next unless question&.type == "radio"
       next if log[question_id].blank? || question.answer_options.key?(log[question_id].to_s) || !question.page.routed_to?(log, nil)
 
@@ -1163,7 +1226,7 @@ private
 
       if setup_question?(question)
         fields.each do |field|
-          if errors[field].blank?
+          if errors[field].none?
             block_log_creation!
             errors.add(field, I18n.t("validations.invalid_option", question: QUESTIONS[field]), category: :setup)
           end
diff --git a/app/views/bulk_upload_lettings_results/show.html.erb b/app/views/bulk_upload_lettings_results/show.html.erb
index dea114e63..63c7d3241 100644
--- a/app/views/bulk_upload_lettings_results/show.html.erb
+++ b/app/views/bulk_upload_lettings_results/show.html.erb
@@ -13,7 +13,7 @@
 
 <div class="govuk-grid-row">
   <div class="govuk-grid-column-full">
-    <% @bulk_upload.bulk_upload_errors.order_by_cell.group_by(&:row).each do |_row, errors_for_row| %>
+    <% @bulk_upload.bulk_upload_errors.order_by_row.order_by_cell.group_by(&:row).each do |_row, errors_for_row| %>
       <%= render BulkUploadErrorRowComponent.new(bulk_upload_errors: errors_for_row) %>
     <% end %>
   </div>
diff --git a/app/views/bulk_upload_lettings_results/summary.html.erb b/app/views/bulk_upload_lettings_results/summary.html.erb
index 51aa652f8..6f70f9800 100644
--- a/app/views/bulk_upload_lettings_results/summary.html.erb
+++ b/app/views/bulk_upload_lettings_results/summary.html.erb
@@ -20,7 +20,7 @@
     <% end %>
 
     <% c.with_tab(label: "Full error report") do %>
-      <% @bulk_upload.bulk_upload_errors.order_by_cell.group_by(&:row).each do |_row, errors_for_row| %>
+      <% @bulk_upload.bulk_upload_errors.order_by_row.order_by_cell.group_by(&:row).each do |_row, errors_for_row| %>
         <%= render BulkUploadErrorRowComponent.new(bulk_upload_errors: errors_for_row) %>
       <% end %>
     <% end %>
diff --git a/app/views/bulk_upload_sales_results/show.html.erb b/app/views/bulk_upload_sales_results/show.html.erb
index 161edb013..24f84ebf6 100644
--- a/app/views/bulk_upload_sales_results/show.html.erb
+++ b/app/views/bulk_upload_sales_results/show.html.erb
@@ -13,7 +13,7 @@
 
 <div class="govuk-grid-row">
   <div class="govuk-grid-column-full">
-    <% @bulk_upload.bulk_upload_errors.order_by_cell.group_by(&:row).each do |_row, errors_for_row| %>
+    <% @bulk_upload.bulk_upload_errors.order_by_row.order_by_cell.group_by(&:row).each do |_row, errors_for_row| %>
       <%= render BulkUploadErrorRowComponent.new(bulk_upload_errors: errors_for_row) %>
     <% end %>
   </div>
diff --git a/app/views/bulk_upload_sales_results/summary.html.erb b/app/views/bulk_upload_sales_results/summary.html.erb
index 2d02848c0..fc872cb31 100644
--- a/app/views/bulk_upload_sales_results/summary.html.erb
+++ b/app/views/bulk_upload_sales_results/summary.html.erb
@@ -20,7 +20,7 @@
     <% end %>
 
     <% c.with_tab(label: "Full error report") do %>
-      <% @bulk_upload.bulk_upload_errors.order_by_cell.group_by(&:row).each do |_row, errors_for_row| %>
+      <% @bulk_upload.bulk_upload_errors.order_by_row.order_by_cell.group_by(&:row).each do |_row, errors_for_row| %>
         <%= render BulkUploadErrorRowComponent.new(bulk_upload_errors: errors_for_row) %>
       <% end %>
     <% end %>
diff --git a/config/locales/en.yml b/config/locales/en.yml
index e57e01da4..64bc5687f 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -41,6 +41,10 @@ en:
   activemodel:
     errors:
       models:
+        bulk_upload/sales/year2022/row_parser:
+          inclusion: Enter a valid value for %{question}
+        bulk_upload/sales/year2023/row_parser:
+          inclusion: Enter a valid value for %{question}
         bulk_upload/lettings/validator:
           attributes:
             base:
diff --git a/spec/services/bulk_upload/sales/validator_spec.rb b/spec/services/bulk_upload/sales/validator_spec.rb
index a496ad2a3..1df7d067f 100644
--- a/spec/services/bulk_upload/sales/validator_spec.rb
+++ b/spec/services/bulk_upload/sales/validator_spec.rb
@@ -173,51 +173,5 @@ RSpec.describe BulkUpload::Sales::Validator do
         expect(validator).not_to be_create_logs
       end
     end
-
-    context "when a column has error rate below absolute threshold" do
-      context "when a column is over 60% error threshold" do
-        let(:log_1) { build(:sales_log, :completed, created_by: user) }
-        let(:log_2) { build(:sales_log, :in_progress, created_by: user, saledate: Time.zone.local(2022, 5, 1)) }
-        let(:log_3) { build(:sales_log, :in_progress, created_by: user, saledate: Time.zone.local(2022, 5, 1)) }
-        let(:log_4) { build(:sales_log, :in_progress, created_by: user, saledate: Time.zone.local(2022, 5, 1)) }
-        let(:log_5) { build(:sales_log, :in_progress, created_by: user, saledate: Time.zone.local(2022, 5, 1)) }
-
-        before do
-          file.write(BulkUpload::SalesLogToCsv.new(log: log_1, line_ending: "\r\n", col_offset: 0).to_2022_csv_row)
-          file.write(BulkUpload::SalesLogToCsv.new(log: log_2, line_ending: "\r\n", col_offset: 0).to_2022_csv_row)
-          file.write(BulkUpload::SalesLogToCsv.new(log: log_3, line_ending: "\r\n", col_offset: 0).to_2022_csv_row)
-          file.write(BulkUpload::SalesLogToCsv.new(log: log_4, line_ending: "\r\n", col_offset: 0).to_2022_csv_row)
-          file.write(BulkUpload::SalesLogToCsv.new(log: log_5, line_ending: "\r\n", col_offset: 0).to_2022_csv_row)
-          file.close
-        end
-
-        it "returns true" do
-          validator.call
-          expect(validator).to be_create_logs
-        end
-      end
-
-      context "when a column is under 60% error threshold" do
-        let(:log_1) { build(:sales_log, :completed, created_by: user) }
-        let(:log_2) { build(:sales_log, :completed, created_by: user) }
-        let(:log_3) { build(:sales_log, :in_progress, created_by: user, saledate: Time.zone.local(2022, 5, 1)) }
-        let(:log_4) { build(:sales_log, :in_progress, created_by: user, saledate: Time.zone.local(2022, 5, 1)) }
-        let(:log_5) { build(:sales_log, :in_progress, created_by: user, saledate: Time.zone.local(2022, 5, 1)) }
-
-        before do
-          file.write(BulkUpload::SalesLogToCsv.new(log: log_1, line_ending: "\r\n", col_offset: 0).to_2022_csv_row)
-          file.write(BulkUpload::SalesLogToCsv.new(log: log_2, line_ending: "\r\n", col_offset: 0).to_2022_csv_row)
-          file.write(BulkUpload::SalesLogToCsv.new(log: log_3, line_ending: "\r\n", col_offset: 0).to_2022_csv_row)
-          file.write(BulkUpload::SalesLogToCsv.new(log: log_4, line_ending: "\r\n", col_offset: 0).to_2022_csv_row)
-          file.write(BulkUpload::SalesLogToCsv.new(log: log_5, line_ending: "\r\n", col_offset: 0).to_2022_csv_row)
-          file.close
-        end
-
-        it "returns true" do
-          validator.call
-          expect(validator).to be_create_logs
-        end
-      end
-    end
   end
 end
diff --git a/spec/services/bulk_upload/sales/year2022/row_parser_spec.rb b/spec/services/bulk_upload/sales/year2022/row_parser_spec.rb
index 5758a3701..b3c008682 100644
--- a/spec/services/bulk_upload/sales/year2022/row_parser_spec.rb
+++ b/spec/services/bulk_upload/sales/year2022/row_parser_spec.rb
@@ -198,7 +198,7 @@ RSpec.describe BulkUpload::Sales::Year2022::RowParser do
       it "has errors on correct setup fields" do
         errors = parser.errors.select { |e| e.options[:category] == :setup }.map(&:attribute)
 
-        expect(errors).to eql(%i[field_2 field_3 field_4 field_113 field_92])
+        expect(errors).to eql(%i[field_2 field_3 field_4 field_113 field_92 field_112])
       end
     end
 
@@ -212,9 +212,9 @@ RSpec.describe BulkUpload::Sales::Year2022::RowParser do
       end
 
       it "has errors on correct setup fields" do
-        errors = parser.errors.select { |e| e.options[:category] == :setup }.map(&:attribute)
+        errors = parser.errors.select { |e| e.options[:category] == :setup }.map(&:attribute).sort
 
-        expect(errors).to eql(%i[field_2 field_3 field_4 field_57 field_116 field_92])
+        expect(errors).to eql(%i[field_112 field_116 field_2 field_3 field_4 field_57 field_92])
       end
     end
 
@@ -232,7 +232,7 @@ RSpec.describe BulkUpload::Sales::Year2022::RowParser do
       it "has errors on correct setup fields" do
         errors = parser.errors.select { |e| e.options[:category] == :setup }.map(&:attribute)
 
-        expect(errors).to eql(%i[field_2 field_3 field_4 field_109 field_92])
+        expect(errors).to eql(%i[field_2 field_3 field_4 field_109 field_92 field_112])
       end
     end
 
@@ -246,9 +246,9 @@ RSpec.describe BulkUpload::Sales::Year2022::RowParser do
       end
 
       it "has errors on correct setup fields" do
-        errors = parser.errors.select { |e| e.options[:category] == :setup }.map(&:attribute)
+        errors = parser.errors.select { |e| e.options[:category] == :setup }.map(&:attribute).sort
 
-        expect(errors).to eql(%i[field_2 field_3 field_4 field_76 field_116 field_92])
+        expect(errors).to eql(%i[field_112 field_116 field_2 field_3 field_4 field_76 field_92])
       end
     end
 
@@ -266,7 +266,7 @@ RSpec.describe BulkUpload::Sales::Year2022::RowParser do
       it "has errors on correct setup fields" do
         errors = parser.errors.select { |e| e.options[:category] == :setup }.map(&:attribute)
 
-        expect(errors).to eql(%i[field_2 field_3 field_4 field_109 field_92])
+        expect(errors).to eql(%i[field_2 field_3 field_4 field_109 field_92 field_112])
       end
     end
 
@@ -280,9 +280,9 @@ RSpec.describe BulkUpload::Sales::Year2022::RowParser do
       end
 
       it "has errors on correct setup fields" do
-        errors = parser.errors.select { |e| e.options[:category] == :setup }.map(&:attribute)
+        errors = parser.errors.select { |e| e.options[:category] == :setup }.map(&:attribute).sort
 
-        expect(errors).to eql(%i[field_2 field_3 field_4 field_84 field_115 field_114 field_92])
+        expect(errors).to eql(%i[field_112 field_114 field_115 field_2 field_3 field_4 field_84 field_92])
       end
     end
 
@@ -291,16 +291,57 @@ RSpec.describe BulkUpload::Sales::Year2022::RowParser do
         {
           bulk_upload:,
           field_1: "test id",
-          field_113: "3",
           field_84: "12",
+          field_85: "other sale type",
+          field_113: "3",
           field_114: "2",
         }
       end
 
       it "has errors on correct setup fields" do
-        errors = parser.errors.select { |e| e.options[:category] == :setup }.map(&:attribute)
+        errors = parser.errors.select { |e| e.options[:category] == :setup }.map(&:attribute).sort
 
-        expect(errors).to eql(%i[field_2 field_3 field_4 field_115 field_116 field_92])
+        expect(errors).to eql(%i[field_2 field_3 field_4 field_115 field_116 field_92 field_112].sort)
+      end
+    end
+
+    describe "#field_57" do # type of shared ownership scheme
+      context "when an invalid option" do
+        let(:attributes) { setup_section_params.merge(field_57: "100", field_113: "1") }
+
+        it "returns setup error" do
+          expect(parser.errors.where(:field_57, category: :setup).map(&:message)).to include("Enter a valid value for what is the type of shared ownership sale?")
+        end
+      end
+    end
+
+    describe "#field_76" do # type of discounted ownership scheme
+      context "when an invalid option" do
+        let(:attributes) { setup_section_params.merge({ field_76: "100", field_113: "2" }) }
+
+        it "returns setup error" do
+          expect(parser.errors.where(:field_76, category: :setup).map(&:message)).to eql(["Enter a valid value for what is the type of discounted ownership sale?"])
+        end
+      end
+    end
+
+    describe "#field_84" do # type of outright sale
+      context "when an invalid option" do
+        let(:attributes) { setup_section_params.merge({ field_84: "100", field_113: "3" }) }
+
+        it "returns setup error" do
+          expect(parser.errors.where(:field_84, category: :setup).map(&:message)).to eql(["Enter a valid value for what is the type of outright sale?"])
+        end
+      end
+    end
+
+    describe "#field_85" do # type of other outright sale
+      context "when cant be blank" do
+        let(:attributes) { setup_section_params.merge({ field_85: nil, field_84: "12" }) }
+
+        it "returns setup error" do
+          expect(parser.errors.where(:field_85, category: :setup).map(&:message)).to eql(["You must answer type of outright sale"])
+        end
       end
     end
 
@@ -451,6 +492,66 @@ RSpec.describe BulkUpload::Sales::Year2022::RowParser do
       end
     end
 
+    describe "#field_109" do # more that 2 joint purchasers?
+      context "when an invalid option" do
+        let(:attributes) { setup_section_params.merge({ field_109: "100", field_116: "1" }) }
+
+        it "returns setup error" do
+          expect(parser.errors.where(:field_109, category: :setup).map(&:message)).to include("Enter a valid value for are there more than two joint purchasers of this property?")
+        end
+      end
+    end
+
+    describe "#field_112" do # data protection
+      context "when not accepted" do
+        let(:attributes) { setup_section_params.merge(field_112: nil) }
+
+        it "returns setup error" do
+          expect(parser.errors.where(:field_112, category: :setup).map(&:message)).to eql(["You must answer data protection question"])
+        end
+      end
+    end
+
+    describe "#field_113" do # purchase made thru ownership scheme?
+      context "when an invalid option" do
+        let(:attributes) { setup_section_params.merge({ field_113: "100" }) }
+
+        it "returns setup error" do
+          expect(parser.errors.where(:field_113, category: :setup).map(&:message)).to include("Enter a valid value for was this purchase made through an ownership scheme?")
+        end
+      end
+    end
+
+    describe "#field_114" do # is buyer a company?
+      context "when an invalid option" do
+        let(:attributes) { setup_section_params.merge({ field_114: "100", field_113: "3" }) }
+
+        it "returns setup error" do
+          expect(parser.errors.where(:field_114, category: :setup).map(&:message)).to include("Enter a valid value for is the buyer a company?")
+        end
+      end
+    end
+
+    describe "#field_115" do # will buyers live in property?
+      context "when an invalid option" do
+        let(:attributes) { setup_section_params.merge({ field_115: "100", field_113: "3", field_114: "2" }) }
+
+        it "returns setup error" do
+          expect(parser.errors.where(:field_115, category: :setup).map(&:message)).to include("Enter a valid value for will the buyers live in the property?")
+        end
+      end
+    end
+
+    describe "#field_116" do # joint purchase?
+      context "when an invalid option" do
+        let(:attributes) { setup_section_params.merge({ field_116: "100" }) }
+
+        it "returns setup error" do
+          expect(parser.errors.where(:field_116, category: :setup).map(&:message)).to include("Enter a valid value for is this a joint purchase?")
+        end
+      end
+    end
+
     describe "#field_117" do
       context "when not a possible value" do
         let(:attributes) { valid_attributes.merge({ field_117: "3" }) }
@@ -477,11 +578,9 @@ RSpec.describe BulkUpload::Sales::Year2022::RowParser do
         let(:attributes) { setup_section_params.merge({ field_2: nil, field_3: nil, field_4: nil }) }
 
         it "returns them as setup errors" do
-          setup_errors = parser.errors.select { |e| e.options[:category] == :setup }
-
-          expect(setup_errors.find { |e| e.attribute == :field_2 }).to be_present
-          expect(setup_errors.find { |e| e.attribute == :field_3 }).to be_present
-          expect(setup_errors.find { |e| e.attribute == :field_4 }).to be_present
+          expect(parser.errors.where(:field_2, category: :setup)).to be_present
+          expect(parser.errors.where(:field_3, category: :setup)).to be_present
+          expect(parser.errors.where(:field_4, category: :setup)).to be_present
         end
       end
 
@@ -630,10 +729,6 @@ RSpec.describe BulkUpload::Sales::Year2022::RowParser do
         it "is not permitted as a setup error" do
           expect(parser.errors.where(:field_57, category: :setup)).to be_present
         end
-
-        it "blocks log creation" do
-          expect(parser).to be_block_log_creation
-        end
       end
     end
   end
diff --git a/spec/services/bulk_upload/sales/year2023/row_parser_spec.rb b/spec/services/bulk_upload/sales/year2023/row_parser_spec.rb
index 4f1e87b0f..c63e83a12 100644
--- a/spec/services/bulk_upload/sales/year2023/row_parser_spec.rb
+++ b/spec/services/bulk_upload/sales/year2023/row_parser_spec.rb
@@ -238,7 +238,7 @@ RSpec.describe BulkUpload::Sales::Year2023::RowParser do
       it "has errors on correct setup fields" do
         errors = parser.errors.select { |e| e.options[:category] == :setup }.map(&:attribute).sort
 
-        expect(errors).to eql(%i[field_1 field_3 field_4 field_5 field_7])
+        expect(errors).to eql(%i[field_1 field_29 field_3 field_4 field_5 field_7])
       end
     end
 
@@ -254,7 +254,7 @@ RSpec.describe BulkUpload::Sales::Year2023::RowParser do
       it "has errors on correct setup fields" do
         errors = parser.errors.select { |e| e.options[:category] == :setup }.map(&:attribute).sort
 
-        expect(errors).to eql(%i[field_1 field_14 field_3 field_4 field_5 field_8])
+        expect(errors).to eql(%i[field_1 field_14 field_29 field_3 field_4 field_5 field_8])
       end
     end
 
@@ -272,7 +272,7 @@ RSpec.describe BulkUpload::Sales::Year2023::RowParser do
       it "has errors on correct setup fields" do
         errors = parser.errors.select { |e| e.options[:category] == :setup }.map(&:attribute).sort
 
-        expect(errors).to eql(%i[field_1 field_15 field_3 field_4 field_5])
+        expect(errors).to eql(%i[field_1 field_15 field_29 field_3 field_4 field_5])
       end
     end
 
@@ -288,7 +288,7 @@ RSpec.describe BulkUpload::Sales::Year2023::RowParser do
       it "has errors on correct setup fields" do
         errors = parser.errors.select { |e| e.options[:category] == :setup }.map(&:attribute).sort
 
-        expect(errors).to eql(%i[field_1 field_14 field_3 field_4 field_5 field_9])
+        expect(errors).to eql(%i[field_1 field_14 field_29 field_3 field_4 field_5 field_9])
       end
     end
 
@@ -306,7 +306,7 @@ RSpec.describe BulkUpload::Sales::Year2023::RowParser do
       it "has errors on correct setup fields" do
         errors = parser.errors.select { |e| e.options[:category] == :setup }.map(&:attribute).sort
 
-        expect(errors).to eql(%i[field_1 field_3 field_4 field_5 field_7])
+        expect(errors).to eql(%i[field_1 field_29 field_3 field_4 field_5 field_7])
       end
     end
 
@@ -322,7 +322,7 @@ RSpec.describe BulkUpload::Sales::Year2023::RowParser do
       it "has errors on correct setup fields" do
         errors = parser.errors.select { |e| e.options[:category] == :setup }.map(&:attribute).sort
 
-        expect(errors).to eql(%i[field_1 field_10 field_12 field_13 field_3 field_4 field_5])
+        expect(errors).to eql(%i[field_1 field_10 field_12 field_13 field_29 field_3 field_4 field_5])
       end
     end
 
@@ -340,7 +340,7 @@ RSpec.describe BulkUpload::Sales::Year2023::RowParser do
       it "has errors on correct setup fields" do
         errors = parser.errors.select { |e| e.options[:category] == :setup }.map(&:attribute).sort
 
-        expect(errors).to eql(%i[field_1 field_13 field_14 field_3 field_4 field_5])
+        expect(errors).to eql(%i[field_1 field_11 field_13 field_14 field_29 field_3 field_4 field_5])
       end
     end
 
@@ -565,6 +565,98 @@ RSpec.describe BulkUpload::Sales::Year2023::RowParser do
       end
     end
 
+    describe "#field_7" do # ownership scheme
+      context "when an invalid option" do
+        let(:attributes) { setup_section_params.merge({ field_7: "100" }) }
+
+        it "returns setup error" do
+          expect(parser.errors.where(:field_7, category: :setup)).to be_present
+        end
+      end
+    end
+
+    describe "#field_8" do # type for shared ownership sale
+      context "when an invalid option" do
+        let(:attributes) { setup_section_params.merge({ field_8: "100" }) }
+
+        it "returns setup error" do
+          expect(parser.errors.where(:field_8, category: :setup)).to be_present
+        end
+      end
+    end
+
+    describe "#field_9" do # type for discounted sale
+      context "when an invalid option" do
+        let(:attributes) { setup_section_params.merge({ field_9: "100" }) }
+
+        it "returns setup error" do
+          expect(parser.errors.where(:field_9, category: :setup)).to be_present
+        end
+      end
+    end
+
+    describe "#field_10" do # type for outright sale
+      context "when an invalid option" do
+        let(:attributes) { setup_section_params.merge({ field_10: "100" }) }
+
+        it "returns setup error" do
+          expect(parser.errors.where(:field_10, category: :setup)).to be_present
+        end
+      end
+    end
+
+    describe "#field_11" do # type of other outright sale
+      context "when an invalid option" do
+        let(:attributes) { setup_section_params.merge({ field_10: "12", field_11: nil }) }
+
+        it "returns setup error" do
+          expect(parser.errors.where(:field_11, category: :setup)).to be_present
+        end
+      end
+    end
+
+    describe "#field_12" do # buyer a company?
+      context "when an invalid option" do
+        let(:attributes) { setup_section_params.merge({ field_7: "3", field_12: "100" }) }
+
+        it "returns setup error" do
+          expect(parser.errors.where(:field_12).map(&:message)).to include("Enter a valid value for is the buyer a company?")
+          expect(parser.errors.where(:field_12, category: :setup)).to be_present
+        end
+      end
+    end
+
+    describe "#field_13" do # will buyers live in the property?
+      context "when an invalid option" do
+        let(:attributes) { setup_section_params.merge({ field_7: "3", field_13: "100" }) }
+
+        it "returns setup error" do
+          expect(parser.errors.where(:field_13).map(&:message)).to eql(["Enter a valid value for will the buyers live in the property?"])
+          expect(parser.errors.where(:field_13, category: :setup)).to be_present
+        end
+      end
+    end
+
+    describe "#field_14" do # joint purchase
+      context "when an invalid option" do
+        let(:attributes) { setup_section_params.merge({ field_14: "100" }) }
+
+        it "returns a setup error" do
+          expect(parser.errors.where(:field_14, category: :setup)).to be_present
+        end
+      end
+    end
+
+    describe "#field_15" do # more than 2 joint buyers?
+      context "when invalid option and must be answered" do
+        let(:attributes) { setup_section_params.merge({ field_14: "1", field_15: "100" }) }
+
+        it "returns a setup error" do
+          expect(parser.errors.where(:field_15, category: :setup)).to be_present
+        end
+      end
+    end
+
     describe "#field_19" do # UPRN
       context "when UPRN known and lookup found" do
         let(:attributes) { setup_section_params.merge({ field_19: "100023336956" }) }
@@ -638,6 +730,16 @@ RSpec.describe BulkUpload::Sales::Year2023::RowParser do
       end
     end
 
+    describe "#field_29" do # data protection
+      let(:attributes) { setup_section_params.merge({ field_29: nil }) }
+
+      context "when not answered" do
+        it "returns a setup error" do
+          expect(parser.errors.where(:field_29, category: :setup)).to be_present
+        end
+      end
+    end
+
     [
       %w[age1_known age1 field_30],
       %w[age2_known age2 field_38],
diff --git a/spec/views/bulk_upload_lettings_results/show.html.erb_spec.rb b/spec/views/bulk_upload_lettings_results/show.html.erb_spec.rb
index 286e1e279..62da07d01 100644
--- a/spec/views/bulk_upload_lettings_results/show.html.erb_spec.rb
+++ b/spec/views/bulk_upload_lettings_results/show.html.erb_spec.rb
@@ -3,18 +3,37 @@ require "rails_helper"
 RSpec.describe "bulk_upload_lettings_results/show.html.erb" do
   let(:bulk_upload) { create(:bulk_upload, :lettings) }
 
-  before do
-    create(:bulk_upload_error, bulk_upload:, cell: "AA100", row: "100", col: "AA")
-    create(:bulk_upload_error, bulk_upload:, cell: "Z100", row: "100", col: "Z")
+  context "when mutiple rows in wrong order" do
+    before do
+      create(:bulk_upload_error, bulk_upload:, cell: "C14", row: "14", col: "C")
+      create(:bulk_upload_error, bulk_upload:, cell: "D10", row: "10", col: "D")
+    end
+
+    it "renders errors order by row" do
+      assign(:bulk_upload, bulk_upload)
+
+      render
+
+      fragment = Capybara::Node::Simple.new(rendered)
+
+      expect(fragment.find_css(".x-govuk-summary-card__title strong").map(&:inner_text)).to eql(["Row 10", "Row 14"])
+    end
   end
 
-  it "renders errors ordered by cell" do
-    assign(:bulk_upload, bulk_upload)
+  context "when 1 row with 2 errors" do
+    before do
+      create(:bulk_upload_error, bulk_upload:, cell: "AA100", row: "100", col: "AA")
+      create(:bulk_upload_error, bulk_upload:, cell: "Z100", row: "100", col: "Z")
+    end
+
+    it "renders errors ordered by cell" do
+      assign(:bulk_upload, bulk_upload)
 
-    render
+      render
 
-    fragment = Capybara::Node::Simple.new(rendered)
+      fragment = Capybara::Node::Simple.new(rendered)
 
-    expect(fragment.find_css("table tbody th").map(&:inner_text)).to eql(%w[Z100 AA100])
+      expect(fragment.find_css("table tbody th").map(&:inner_text)).to eql(%w[Z100 AA100])
+    end
   end
 end
diff --git a/spec/views/bulk_upload_lettings_results/summary.html.erb_spec.rb b/spec/views/bulk_upload_lettings_results/summary.html.erb_spec.rb
index 25499133d..242e58d4e 100644
--- a/spec/views/bulk_upload_lettings_results/summary.html.erb_spec.rb
+++ b/spec/views/bulk_upload_lettings_results/summary.html.erb_spec.rb
@@ -3,18 +3,37 @@ require "rails_helper"
 RSpec.describe "bulk_upload_lettings_results/summary.html.erb" do
   let(:bulk_upload) { create(:bulk_upload, :lettings) }
 
-  before do
-    create(:bulk_upload_error, bulk_upload:, cell: "AA100", row: "100", col: "AA")
-    create(:bulk_upload_error, bulk_upload:, cell: "Z100", row: "100", col: "Z")
+  context "when mutiple rows in wrong order" do
+    before do
+      create(:bulk_upload_error, bulk_upload:, cell: "C14", row: "14", col: "C")
+      create(:bulk_upload_error, bulk_upload:, cell: "D10", row: "10", col: "D")
+    end
+
+    it "renders errors order by row" do
+      assign(:bulk_upload, bulk_upload)
+
+      render
+
+      fragment = Capybara::Node::Simple.new(rendered)
+
+      expect(fragment.find_css(".x-govuk-summary-card__title strong").map(&:inner_text)).to eql(["Row 10", "Row 14"])
+    end
   end
 
-  it "renders errors ordered by cell" do
-    assign(:bulk_upload, bulk_upload)
+  context "when 1 row with 2 errors" do
+    before do
+      create(:bulk_upload_error, bulk_upload:, cell: "AA100", row: "100", col: "AA")
+      create(:bulk_upload_error, bulk_upload:, cell: "Z100", row: "100", col: "Z")
+    end
+
+    it "renders errors ordered by cell" do
+      assign(:bulk_upload, bulk_upload)
 
-    render
+      render
 
-    fragment = Capybara::Node::Simple.new(rendered)
+      fragment = Capybara::Node::Simple.new(rendered)
 
-    expect(fragment.find_css("table tbody th").map(&:inner_text)).to eql(%w[Z100 AA100])
+      expect(fragment.find_css("table tbody th").map(&:inner_text)).to eql(%w[Z100 AA100])
+    end
   end
 end
diff --git a/spec/views/bulk_upload_sales_results/show.html.erb_spec.rb b/spec/views/bulk_upload_sales_results/show.html.erb_spec.rb
new file mode 100644
index 000000000..b553d27ce
--- /dev/null
+++ b/spec/views/bulk_upload_sales_results/show.html.erb_spec.rb
@@ -0,0 +1,39 @@
+require "rails_helper"
+
+RSpec.describe "bulk_upload_sales_results/show.html.erb" do
+  let(:bulk_upload) { create(:bulk_upload, :sales) }
+
+  context "when mutiple rows in wrong order" do
+    before do
+      create(:bulk_upload_error, bulk_upload:, cell: "C14", row: "14", col: "C")
+      create(:bulk_upload_error, bulk_upload:, cell: "D10", row: "10", col: "D")
+    end
+
+    it "renders errors order by row" do
+      assign(:bulk_upload, bulk_upload)
+
+      render
+
+      fragment = Capybara::Node::Simple.new(rendered)
+
+      expect(fragment.find_css(".x-govuk-summary-card__title strong").map(&:inner_text)).to eql(["Row 10", "Row 14"])
+    end
+  end
+
+  context "when 1 row with 2 errors" do
+    before do
+      create(:bulk_upload_error, bulk_upload:, cell: "AA100", row: "100", col: "AA")
+      create(:bulk_upload_error, bulk_upload:, cell: "Z100", row: "100", col: "Z")
+    end
+
+    it "renders errors ordered by cell" do
+      assign(:bulk_upload, bulk_upload)
+
+      render
+
+      fragment = Capybara::Node::Simple.new(rendered)
+
+      expect(fragment.find_css("table tbody th").map(&:inner_text)).to eql(%w[Z100 AA100])
+    end
+  end
+end
diff --git a/spec/views/bulk_upload_sales_results/summary.html.erb_spec.rb b/spec/views/bulk_upload_sales_results/summary.html.erb_spec.rb
new file mode 100644
index 000000000..4ac2e3807
--- /dev/null
+++ b/spec/views/bulk_upload_sales_results/summary.html.erb_spec.rb
@@ -0,0 +1,39 @@
+require "rails_helper"
+
+RSpec.describe "bulk_upload_sales_results/summary.html.erb" do
+  let(:bulk_upload) { create(:bulk_upload, :sales) }
+
+  context "when mutiple rows in wrong order" do
+    before do
+      create(:bulk_upload_error, bulk_upload:, cell: "C14", row: "14", col: "C")
+      create(:bulk_upload_error, bulk_upload:, cell: "D10", row: "10", col: "D")
+    end
+
+    it "renders errors order by row" do
+      assign(:bulk_upload, bulk_upload)
+
+      render
+
+      fragment = Capybara::Node::Simple.new(rendered)
+
+      expect(fragment.find_css(".x-govuk-summary-card__title strong").map(&:inner_text)).to eql(["Row 10", "Row 14"])
+    end
+  end
+
+  context "when 1 row with 2 errors" do
+    before do
+      create(:bulk_upload_error, bulk_upload:, cell: "AA100", row: "100", col: "AA")
+      create(:bulk_upload_error, bulk_upload:, cell: "Z100", row: "100", col: "Z")
+    end
+
+    it "renders errors ordered by cell" do
+      assign(:bulk_upload, bulk_upload)
+
+      render
+
+      fragment = Capybara::Node::Simple.new(rendered)
+
+      expect(fragment.find_css("table tbody th").map(&:inner_text)).to eql(%w[Z100 AA100])
+    end
+  end
+end