Merge branch 'main' into CLDC-2257-refactor-filters-3

app/helpers/check_answers_helper.rb

@@ -46,10 +46,22 @@ private
   end
 
   def get_answer_label(question, lettings_log)
-    question.answer_label(lettings_log, current_user).presence || "<span class=\"app-!-colour-muted\">You didn’t answer this question</span>".html_safe
+    question.answer_label(lettings_log, current_user).presence || unanswered_value(log: lettings_log)
   end
 
   def get_question_label(question)
     [question.question_number_string, question.check_answer_label.to_s.presence || question.header.to_s].compact.join(" - ")
   end
+
+  def unanswered_value(log:)
+    if bulk_uploaded?(log:)
+      "<span class=\"app-!-colour-red\">You still need to answer this question</span>".html_safe
+    else
+      "<span class=\"app-!-colour-muted\">You didn’t answer this question</span>".html_safe
+    end
+  end
+
+  def bulk_uploaded?(log:)
+    log.bulk_upload
+  end
 end

app/services/bulk_upload/lettings/validator.rb

@@ -1,5 +1,3 @@
-require "csv"
-
 class BulkUpload::Lettings::Validator
   include ActiveModel::Validations
 
@@ -16,9 +14,11 @@ class BulkUpload::Lettings::Validator
   end
 
   def call
-    row_parsers.each_with_index do |row_parser, index|
+    row_parsers.each(&:valid?)
-      row_parser.valid?
 
+    validate_duplicate_rows if FeatureToggle.bulk_upload_duplicate_log_check_enabled?
+
+    row_parsers.each_with_index do |row_parser, index|
       row = index + row_offset + 1
 
       row_parser.errors.each do |error|
@@ -69,25 +69,25 @@ class BulkUpload::Lettings::Validator
     errors.count == errors.where(category: "soft_validation").count && errors.count.positive?
   end
 
-  def over_column_error_threshold?
+  def any_logs_already_exist?
-    fields = ("field_1".."field_134").to_a
+    row_parsers.any?(&:log_already_exists?)
-    percentage_threshold = (row_parsers.size * COLUMN_PERCENTAGE_ERROR_THRESHOLD).ceil
+  end
 
-    fields.any? do |field|
+private
-      count = row_parsers.count { |row_parser| row_parser.errors[field].present? }
 
-      next if count < COLUMN_ABSOLUTE_ERROR_THRESHOLD
+  # n^2 algo
+  def validate_duplicate_rows
+    row_parsers.each do |rp|
+      dupe = row_parsers.reject { |r| r.object_id.equal?(rp.object_id) }.any? do |rp_counter|
+        rp.spreadsheet_duplicate_hash == rp_counter.spreadsheet_duplicate_hash
+      end
 
-      count > percentage_threshold
+      if dupe
+        rp.add_duplicate_found_in_spreadsheet_errors
       end
     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

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

@@ -438,6 +438,29 @@ class BulkUpload::Lettings::Year2022::RowParser
       .exists?(duplicate_check_fields.index_with { |field| log.public_send(field) })
   end
 
+  def spreadsheet_duplicate_hash
+    attributes.slice(
+      "field_5",   # location
+      "field_12",  # age1
+      "field_20",  # sex1
+      "field_35",  # ecstat1
+      "field_84",  # tcharge
+      "field_96",  # startdate
+      "field_97",  # startdate
+      "field_98",  # startdate
+      "field_100", # propcode
+      "field_108", # postcode
+      "field_109", # postcode
+      "field_111", # owning org
+    )
+  end
+
+  def add_duplicate_found_in_spreadsheet_errors
+    spreadsheet_duplicate_hash.each_key do |field|
+      errors.add(field, :spreadsheet_dupe, category: :setup)
+    end
+  end
+
 private
 
   def validate_declaration_acceptance

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

@@ -455,6 +455,29 @@ class BulkUpload::Lettings::Year2023::RowParser
       .exists?(duplicate_check_fields.index_with { |field| log.public_send(field) })
   end
 
+  def spreadsheet_duplicate_hash
+    attributes.slice(
+      "field_1",   # owning org
+      "field_7",   # startdate
+      "field_8",   # startdate
+      "field_9",   # startdate
+      "field_14",  # propcode
+      "field_17",  # location
+      "field_23",  # postcode
+      "field_24",  # postcode
+      "field_46",  # age1
+      "field_47",  # sex1
+      "field_50",  # ecstat1
+      "field_132", # tcharge
+    )
+  end
+
+  def add_duplicate_found_in_spreadsheet_errors
+    spreadsheet_duplicate_hash.each_key do |field|
+      errors.add(field, :spreadsheet_dupe, category: :setup)
+    end
+  end
+
 private
 
   def validate_declaration_acceptance

app/services/bulk_upload/processor.rb

@@ -6,6 +6,8 @@ class BulkUpload::Processor
   end
 
   def call
+    destroy_any_existing_errors_from_prior_run
+
     download
 
     return send_failure_mail(errors: validator.errors.full_messages) if validator.invalid?
@@ -46,6 +48,10 @@ class BulkUpload::Processor
 
 private
 
+  def destroy_any_existing_errors_from_prior_run
+    bulk_upload.bulk_upload_errors.destroy_all
+  end
+
   def send_how_to_fix_upload_mail
     BulkUploadMailer
       .send_how_to_fix_upload_mail(bulk_upload:)

app/services/bulk_upload/sales/log_creator.rb

@@ -16,7 +16,6 @@ class BulkUpload::Sales::LogCreator
       row_parser.log.bulk_upload = bulk_upload
       row_parser.log.skip_update_status = true
       row_parser.log.status = "pending"
-
       row_parser.log.status_cache = row_parser.log.calculate_status
 
       begin

app/services/bulk_upload/sales/validator.rb

@@ -13,9 +13,11 @@ class BulkUpload::Sales::Validator
   end
 
   def call
-    row_parsers.each_with_index do |row_parser, index|
+    row_parsers.each(&:valid?)
-      row_parser.valid?
+
+    validate_duplicate_rows if FeatureToggle.bulk_upload_duplicate_log_check_enabled?
 
+    row_parsers.each_with_index do |row_parser, index|
       row = index + row_offset + 1
 
       row_parser.errors.each do |error|
@@ -67,6 +69,19 @@ class BulkUpload::Sales::Validator
 
 private
 
+  # n^2 algo
+  def validate_duplicate_rows
+    row_parsers.each do |rp|
+      dupe = row_parsers.reject { |r| r.object_id.equal?(rp.object_id) }.any? do |rp_counter|
+        rp.spreadsheet_duplicate_hash == rp_counter.spreadsheet_duplicate_hash
+      end
+
+      if dupe
+        rp.add_duplicate_found_in_spreadsheet_errors
+      end
+    end
+  end
+
   def any_logs_invalid?
     row_parsers.any? { |row_parser| row_parser.log.invalid? }
   end

app/services/bulk_upload/sales/year2022/row_parser.rb

@@ -324,6 +324,27 @@ class BulkUpload::Sales::Year2022::RowParser
             if: proc { field_84 == 12 },
             on: :after_log
 
+  validates :field_68,
+            presence: {
+              message: I18n.t("validations.not_answered", question: "What was the full purchase price?"),
+            },
+            if: :shared_ownership?,
+            on: :after_log
+
+  validates :field_77,
+            presence: {
+              message: I18n.t("validations.not_answered", question: "What was the full purchase price?"),
+            },
+            if: :discounted_ownership?,
+            on: :after_log
+
+  validates :field_87,
+            presence: {
+              message: I18n.t("validations.not_answered", question: "What was the full purchase price?"),
+            },
+            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
 
   validates :field_113, presence: { message: I18n.t("validations.not_answered", question: "ownership type"), category: :setup }, on: :after_log
@@ -411,6 +432,27 @@ class BulkUpload::Sales::Year2022::RowParser
     field_1
   end
 
+  def spreadsheet_duplicate_hash
+    attributes.slice(
+      "field_1",   # purcahser code
+      "field_2",   # saledate
+      "field_3",   # saledate
+      "field_4",   # saledate
+      "field_7",   # age1
+      "field_13",  # sex1
+      "field_24",  # ecstat1
+      "field_54",  # postcode
+      "field_55",  # postcode
+      "field_92",  # owning org
+    )
+  end
+
+  def add_duplicate_found_in_spreadsheet_errors
+    spreadsheet_duplicate_hash.each_key do |field|
+      errors.add(field, :spreadsheet_dupe, category: :setup)
+    end
+  end
+
 private
 
   def validate_data_protection_answered
@@ -517,7 +559,6 @@ private
       lanomagr: %i[field_65],
       frombeds: %i[field_66],
       fromprop: %i[field_67],
-      value: %i[field_68 field_77 field_87],
       equity: %i[field_69],
       mortgage: %i[field_70 field_80 field_88],
       extrabor: %i[field_71 field_81 field_89],
@@ -1013,15 +1054,11 @@ private
       next if log.optional_fields.include?(question.id)
       next if question.completed?(log)
 
-      if setup_question?(question)
       fields.each do |field|
         unless errors.any? { |e| fields.include?(e.attribute) }
+          if setup_question?(question)
             errors.add(field, I18n.t("validations.not_answered", question: question.check_answer_label&.downcase), category: :setup)
-          end
-        end
           else
-        fields.each do |field|
-          unless errors.any? { |e| fields.include?(e.attribute) }
             errors.add(field, I18n.t("validations.not_answered", question: question.check_answer_label&.downcase))
           end
         end

app/services/bulk_upload/sales/year2023/row_parser.rb

@@ -533,6 +533,27 @@ class BulkUpload::Sales::Year2023::RowParser
     field_6
   end
 
+  def spreadsheet_duplicate_hash
+    attributes.slice(
+      "field_1",  # owning org
+      "field_3",  # saledate
+      "field_4",  # saledate
+      "field_5",  # saledate
+      "field_6",  # purchaser_code
+      "field_24", # postcode
+      "field_25", # postcode
+      "field_30", # age1
+      "field_31", # sex1
+      "field_35", # ecstat1
+    )
+  end
+
+  def add_duplicate_found_in_spreadsheet_errors
+    spreadsheet_duplicate_hash.each_key do |field|
+      errors.add(field, :spreadsheet_dupe, category: :setup)
+    end
+  end
+
 private
 
   def validate_data_protection_answered

app/views/form/_check_answers_summary_list.html.erb

@@ -2,13 +2,16 @@
   <% questions.each do |question| %>
     <% summary_list.row do |row| %>
       <% row.key { get_question_label(question) } %>
+
       <% row.value do %>
         <%= simple_format(
           get_answer_label(question, @log),
           wrapper_tag: "span",
           class: "govuk-!-margin-right-4",
         ) %>
+
         <% extra_value = question.get_extra_check_answer_value(@log) %>
+
         <% if extra_value && question.answer_label(@log, current_user).present? %>
           <%= simple_format(
             extra_value,
@@ -16,10 +19,12 @@
             class: "govuk-!-font-weight-regular app-!-colour-muted",
           ) %>
         <% end %>
+
         <% question.get_inferred_answers(@log).each do |inferred_answer| %>
           <span class="govuk-!-font-weight-regular app-!-colour-muted"><%= inferred_answer %></span>
         <% end %>
       <% end %>
+
       <% if @log.collection_period_open? %>
         <% row.action(
           text: question.action_text(@log),

config/cloud_foundry/review_manifest.yml

@@ -8,7 +8,7 @@ defaults: &defaults
       instances: 1
       memory: 1G
     - type: worker
-      command: bundle exec sidekiq
+      command: bundle exec sidekiq -t 3
       health-check-type: process
       instances: 1
   health-check-type: http

config/initializers/sidekiq.rb

@@ -32,4 +32,8 @@ Sidekiq.configure_server do |config|
   config.on(:startup) do
     Sidekiq::Cron::Job.load_from_hash YAML.load_file("config/sidekiq_cron_schedule.yml")
   end
+
+  config.on(:shutdown) do
+    Sidekiq::CLI.instance.launcher.quiet
+  end
 end

config/locales/en.yml

@@ -41,10 +41,17 @@ en:
   activemodel:
     errors:
       models:
-        bulk_upload/sales/year2022/row_parser:
+        bulk_upload/row_parser: &bulk_upload__row_parser__base
           inclusion: Enter a valid value for %{question}
+          spreadsheet_dupe: This is a duplicate of a log in your file
+        bulk_upload/lettings/year2022/row_parser:
+          <<: *bulk_upload__row_parser__base
+        bulk_upload/lettings/year2023/row_parser:
+          <<: *bulk_upload__row_parser__base
+        bulk_upload/sales/year2022/row_parser:
+          <<: *bulk_upload__row_parser__base
         bulk_upload/sales/year2023/row_parser:
-          inclusion: Enter a valid value for %{question}
+          <<: *bulk_upload__row_parser__base
         bulk_upload/lettings/validator:
           attributes:
             base:

docs/setup.md

@@ -16,6 +16,8 @@ Dependencies:
 
 We recommend using [RBenv](https://github.com/rbenv/rbenv) to manage Ruby versions.
 
+We recommend using [nvm](https://github.com/nvm-sh/nvm) to manage NodeJS versions.
+
 1. Install PostgreSQL
 
     macOS:
@@ -70,10 +72,12 @@ We recommend using [RBenv](https://github.com/rbenv/rbenv) to manage Ruby versio
 
 5. Install JavaScript dependencies
 
-    macOS:
+   Note that we currently use node v16, which is no longer the latest LTS version so you will need to specify the version number when installing
 
+    macOS (using nvm):
    ```bash
-    brew install node
+   nvm install 16
+   nvm use 16
    brew install yarn
    ```
 
@@ -168,6 +172,8 @@ We recommend using [RBenv](https://github.com/rbenv/rbenv) to manage Ruby versio
     bundle exec rspec
     ```
    
+    Note that these tests assume you have firefox installed.
+
 ## Using Docker
 
 1. Build the image:

manifest.yml

@@ -8,7 +8,7 @@ defaults: &defaults
       instances: 2
       memory: 1G
     - type: worker
-      command: bundle exec sidekiq
+      command: bundle exec sidekiq -t 3
       health-check-type: process
       instances: 2
   health-check-type: http
@@ -31,7 +31,7 @@ applications:
         instances: 4
         memory: 1G
       - type: worker
-        command: bundle exec sidekiq
+        command: bundle exec sidekiq -t 3
         health-check-type: process
         instances: 2
     env:

spec/factories/sales_log.rb

@@ -36,6 +36,7 @@ FactoryBot.define do
       jointpur { 2 }
     end
     trait :completed do
+      purchid { rand(999_999_999).to_s }
       ownershipsch { 2 }
       type { 8 }
       saledate { Time.utc(2023, 2, 2, 10, 36, 49) }

spec/helpers/check_answers_helper_spec.rb

@@ -11,8 +11,6 @@ RSpec.describe CheckAnswersHelper do
       Singleton.__init__(FormHandler)
       example.run
     end
-    Timecop.return
-    Singleton.__init__(FormHandler)
   end
 
   describe "display_answered_questions_summary" do
@@ -37,4 +35,16 @@ RSpec.describe CheckAnswersHelper do
       end
     end
   end
+
+  describe "#get_answer_label" do
+    context "when unanswered and bulk upload" do
+      let(:question) { log.form.questions.sample }
+      let(:bulk_upload) { build(:bulk_upload, :sales) }
+      let(:log) { build(:sales_log, bulk_upload:) }
+
+      it "is red" do
+        expect(get_answer_label(question, log)).to include("red")
+      end
+    end
+  end
 end

spec/models/sales_log_spec.rb

@@ -165,7 +165,7 @@ RSpec.describe SalesLog, type: :model do
 
     it "correctly derives and saves exday, exmonth and exyear" do
       sales_log.update!(exdate: Time.gm(2022, 5, 4), saledate: Time.gm(2022, 7, 4), ownershipsch: 1, type: 18, staircase: 2, resale: 2, proplen: 0)
-      record_from_db = ActiveRecord::Base.connection.execute("select exday, exmonth, exyear from sales_logs where id=#{sales_log.id}").to_a[0]
+      record_from_db = described_class.find(sales_log.id)
       expect(record_from_db["exday"]).to eq(4)
       expect(record_from_db["exmonth"]).to eq(5)
       expect(record_from_db["exyear"]).to eq(2022)
@@ -173,25 +173,25 @@ RSpec.describe SalesLog, type: :model do
 
     it "correctly derives and saves deposit for outright sales when no mortgage is used" do
       sales_log.update!(value: 123_400, deposit: nil, mortgageused: 2, ownershipsch: 3, type: 10, companybuy: 1, jointpur: 1, jointmore: 1)
-      record_from_db = ActiveRecord::Base.connection.execute("select deposit from sales_logs where id=#{sales_log.id}").to_a[0]
+      record_from_db = described_class.find(sales_log.id)
       expect(record_from_db["deposit"]).to eq(123_400)
     end
 
     it "does not derive deposit if the sale isn't outright" do
       sales_log.update!(value: 123_400, deposit: nil, mortgageused: 2, ownershipsch: 2)
-      record_from_db = ActiveRecord::Base.connection.execute("select deposit from sales_logs where id=#{sales_log.id}").to_a[0]
+      record_from_db = described_class.find(sales_log.id)
       expect(record_from_db["deposit"]).to eq(nil)
     end
 
     it "does not derive deposit if the mortgage is used" do
       sales_log.update!(value: 123_400, deposit: nil, mortgageused: 1, ownershipsch: 3, type: 10, companybuy: 1, jointpur: 1, jointmore: 1)
-      record_from_db = ActiveRecord::Base.connection.execute("select deposit from sales_logs where id=#{sales_log.id}").to_a[0]
+      record_from_db = described_class.find(sales_log.id)
       expect(record_from_db["deposit"]).to eq(nil)
     end
 
     it "correctly derives and saves pcode1 and pcode1 and pcode2" do
       sales_log.update!(postcode_full: "W6 0SP")
-      record_from_db = ActiveRecord::Base.connection.execute("select pcode1, pcode2 from sales_logs where id=#{sales_log.id}").to_a[0]
+      record_from_db = described_class.find(sales_log.id)
       expect(record_from_db["pcode1"]).to eq("W6")
       expect(record_from_db["pcode2"]).to eq("0SP")
     end
@@ -200,7 +200,7 @@ RSpec.describe SalesLog, type: :model do
       # to avoid log failing validations when mortgage value is removed:
       new_grant_value = sales_log.grant + sales_log.mortgage
       sales_log.update!(mortgageused: 2, grant: new_grant_value)
-      record_from_db = ActiveRecord::Base.connection.execute("select mortgage from sales_logs where id=#{sales_log.id}").to_a[0]
+      record_from_db = described_class.find(sales_log.id)
       expect(record_from_db["mortgage"]).to eq(0.0)
     end
 
@@ -209,7 +209,7 @@ RSpec.describe SalesLog, type: :model do
       new_grant_value = sales_log.grant + sales_log.mortgage
       sales_log.update!(mortgageused: 2, grant: new_grant_value)
       sales_log.update!(mortgageused: 1)
-      record_from_db = ActiveRecord::Base.connection.execute("select mortgage from sales_logs where id=#{sales_log.id}").to_a[0]
+      record_from_db = described_class.find(sales_log.id)
       expect(record_from_db["mortgage"]).to eq(nil)
     end
 
@@ -276,7 +276,7 @@ RSpec.describe SalesLog, type: :model do
     end
 
     def check_postcode_fields(postcode_field)
-      record_from_db = ActiveRecord::Base.connection.execute("select #{postcode_field} from sales_logs where id=#{address_sales_log.id}").to_a[0]
+      record_from_db = described_class.find(address_sales_log.id)
       expect(address_sales_log[postcode_field]).to eq("M1 1AE")
       expect(record_from_db[postcode_field]).to eq("M1 1AE")
     end
@@ -311,7 +311,7 @@ RSpec.describe SalesLog, type: :model do
     end
 
     it "correctly infers la" do
-      record_from_db = ActiveRecord::Base.connection.execute("select la from sales_logs where id=#{address_sales_log.id}").to_a[0]
+      record_from_db = described_class.find(address_sales_log.id)
       expect(address_sales_log.la).to eq("E08000003")
       expect(record_from_db["la"]).to eq("E08000003")
     end
@@ -340,7 +340,7 @@ RSpec.describe SalesLog, type: :model do
       end
 
       it "correctly sets la as nil" do
-        record_from_db = ActiveRecord::Base.connection.execute("select la from sales_logs where id=#{address_sales_log_22_23.id}").to_a[0]
+        record_from_db = described_class.find(address_sales_log_22_23.id)
         expect(address_sales_log_22_23.la).to eq(nil)
         expect(record_from_db["la"]).to eq(nil)
       end
@@ -370,7 +370,7 @@ RSpec.describe SalesLog, type: :model do
       end
 
       it "correctly infers new la" do
-        record_from_db = ActiveRecord::Base.connection.execute("select la from sales_logs where id=#{address_sales_log_23_24.id}").to_a[0]
+        record_from_db = described_class.find(address_sales_log_23_24.id)
         expect(address_sales_log_23_24.la).to eq("E06000064")
         expect(record_from_db["la"]).to eq("E06000064")
       end
@@ -400,7 +400,7 @@ RSpec.describe SalesLog, type: :model do
     it "correctly resets all fields if property postcode not known" do
       address_sales_log.update!({ pcodenk: 1 })
 
-      record_from_db = ActiveRecord::Base.connection.execute("select la, postcode_full from sales_logs where id=#{address_sales_log.id}").to_a[0]
+      record_from_db = described_class.find(address_sales_log.id)
       expect(record_from_db["postcode_full"]).to eq(nil)
       expect(address_sales_log.la).to eq(nil)
       expect(record_from_db["la"]).to eq(nil)
@@ -410,14 +410,14 @@ RSpec.describe SalesLog, type: :model do
       address_sales_log.update!({ pcodenk: 1 })
       address_sales_log.update!({ la: "E09000033" })
 
-      record_from_db = ActiveRecord::Base.connection.execute("select la, postcode_full from sales_logs where id=#{address_sales_log.id}").to_a[0]
+      record_from_db = described_class.find(address_sales_log.id)
       expect(record_from_db["postcode_full"]).to eq(nil)
       expect(address_sales_log.la).to eq("E09000033")
       expect(record_from_db["la"]).to eq("E09000033")
 
       address_sales_log.update!({ pcodenk: 0, postcode_full: "M1 1AD" })
 
-      record_from_db = ActiveRecord::Base.connection.execute("select la, postcode_full from sales_logs where id=#{address_sales_log.id}").to_a[0]
+      record_from_db = described_class.find(address_sales_log.id)
       expect(record_from_db["postcode_full"]).to eq("M1 1AD")
       expect(address_sales_log.la).to eq("E08000003")
       expect(record_from_db["la"]).to eq("E08000003")
@@ -453,35 +453,35 @@ RSpec.describe SalesLog, type: :model do
     end
 
     it "correctly derives and saves hhmemb" do
-      record_from_db = ActiveRecord::Base.connection.execute("select hhmemb from sales_logs where id=#{sales_log.id}").to_a[0]
+      record_from_db = described_class.find(sales_log.id)
       expect(record_from_db["hhmemb"]).to eq(6)
     end
 
     it "correctly derives and saves hhmemb if it's a joint purchase" do
       sales_log.update!(jointpur: 2, jointmore: 2)
-      record_from_db = ActiveRecord::Base.connection.execute("select hhmemb from sales_logs where id=#{sales_log.id}").to_a[0]
+      record_from_db = described_class.find(sales_log.id)
       expect(record_from_db["hhmemb"]).to eq(5)
     end
 
     it "correctly derives and saves totchild" do
-      record_from_db = ActiveRecord::Base.connection.execute("select totchild from sales_logs where id=#{sales_log.id}").to_a[0]
+      record_from_db = described_class.find(sales_log.id)
       expect(record_from_db["totchild"]).to eq(2)
     end
 
     it "correctly derives and saves totadult" do
-      record_from_db = ActiveRecord::Base.connection.execute("select totadult from sales_logs where id=#{sales_log.id}").to_a[0]
+      record_from_db = described_class.find(sales_log.id)
       expect(record_from_db["totadult"]).to eq(4)
     end
 
     it "correctly derives and saves hhtype" do
-      record_from_db = ActiveRecord::Base.connection.execute("select hhtype from sales_logs where id=#{sales_log.id}").to_a[0]
+      record_from_db = described_class.find(sales_log.id)
       expect(record_from_db["hhtype"]).to eq(9)
     end
   end
 
   context "when saving previous address" do
     def check_previous_postcode_fields(postcode_field)
-      record_from_db = ActiveRecord::Base.connection.execute("select #{postcode_field} from sales_logs where id=#{address_sales_log.id}").to_a[0]
+      record_from_db = described_class.find(address_sales_log.id)
       expect(address_sales_log[postcode_field]).to eq("M1 1AE")
       expect(record_from_db[postcode_field]).to eq("M1 1AE")
     end
@@ -519,7 +519,7 @@ RSpec.describe SalesLog, type: :model do
     end
 
     it "correctly infers prevloc" do
-      record_from_db = ActiveRecord::Base.connection.execute("select prevloc from sales_logs where id=#{address_sales_log.id}").to_a[0]
+      record_from_db = described_class.find(address_sales_log.id)
       expect(address_sales_log.prevloc).to eq("E08000003")
       expect(record_from_db["prevloc"]).to eq("E08000003")
     end
@@ -537,7 +537,7 @@ RSpec.describe SalesLog, type: :model do
     it "correctly resets all fields if previous postcode not known" do
       address_sales_log.update!({ ppcodenk: 1 })
 
-      record_from_db = ActiveRecord::Base.connection.execute("select prevloc, ppostcode_full from sales_logs where id=#{address_sales_log.id}").to_a[0]
+      record_from_db = described_class.find(address_sales_log.id)
       expect(record_from_db["ppostcode_full"]).to eq(nil)
       expect(address_sales_log.prevloc).to eq(nil)
       expect(record_from_db["prevloc"]).to eq(nil)

spec/services/bulk_upload/lettings/validator_spec.rb

@@ -290,6 +290,23 @@ RSpec.describe BulkUpload::Lettings::Validator do
       end
     end
 
+    context "when duplicate rows present" do
+      let(:file) { Tempfile.new }
+      let(:path) { file.path }
+      let(:log) { build(:lettings_log, :completed) }
+
+      before do
+        file.write(BulkUpload::LettingsLogToCsv.new(log:, line_ending: "\r\n").default_2023_field_numbers_row)
+        file.write(BulkUpload::LettingsLogToCsv.new(log:, line_ending: "\r\n").to_2023_csv_row)
+        file.write(BulkUpload::LettingsLogToCsv.new(log:, line_ending: "\r\n").to_2023_csv_row)
+        file.close
+      end
+
+      it "creates errors" do
+        expect { validator.call }.to change(BulkUploadError.where(category: :setup, error: "This is a duplicate of a log in your file"), :count).by(24)
+      end
+    end
+
     context "with unix line endings" do
       let(:fixture_path) { file_fixture("2022_23_lettings_bulk_upload.csv") }
       let(:file) { Tempfile.new }

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

@@ -1760,4 +1760,16 @@ RSpec.describe BulkUpload::Lettings::Year2022::RowParser do
       end
     end
   end
+
+  describe "#spreadsheet_duplicate_hash" do
+    it "returns a hash" do
+      expect(parser.spreadsheet_duplicate_hash).to be_a(Hash)
+    end
+  end
+
+  describe "#add_duplicate_found_in_spreadsheet_errors" do
+    it "adds errors" do
+      expect { parser.add_duplicate_found_in_spreadsheet_errors }.to change(parser.errors, :size)
+    end
+  end
 end

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

@@ -1830,4 +1830,16 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do
       end
     end
   end
+
+  describe "#spreadsheet_duplicate_hash" do
+    it "returns a hash" do
+      expect(parser.spreadsheet_duplicate_hash).to be_a(Hash)
+    end
+  end
+
+  describe "#add_duplicate_found_in_spreadsheet_errors" do
+    it "adds errors" do
+      expect { parser.add_duplicate_found_in_spreadsheet_errors }.to change(parser.errors, :size)
+    end
+  end
 end

spec/services/bulk_upload/processor_spec.rb

@@ -8,6 +8,16 @@ RSpec.describe BulkUpload::Processor do
   let(:owning_org) { create(:organisation, old_visible_id: 123) }
 
   describe "#call" do
+    context "when errors exist from prior job run" do
+      let!(:existing_error) { create(:bulk_upload_error, bulk_upload:) }
+
+      it "destroys existing errors" do
+        processor.call
+
+        expect { existing_error.reload }.to raise_error(ActiveRecord::RecordNotFound)
+      end
+    end
+
     context "when the bulk upload itself is not considered valid" do
       let(:mock_downloader) do
         instance_double(

spec/services/bulk_upload/sales/validator_spec.rb

@@ -125,6 +125,22 @@ RSpec.describe BulkUpload::Sales::Validator do
         expect { validator.call }.to change(BulkUploadError, :count)
       end
     end
+
+    context "when duplicate rows present" do
+      let(:file) { Tempfile.new }
+      let(:path) { file.path }
+      let(:log) { build(:sales_log, :completed) }
+
+      before do
+        file.write(BulkUpload::SalesLogToCsv.new(log:, col_offset: 0).to_2022_csv_row)
+        file.write(BulkUpload::SalesLogToCsv.new(log:, col_offset: 0).to_2022_csv_row)
+        file.close
+      end
+
+      it "creates errors" do
+        expect { validator.call }.to change(BulkUploadError.where(category: :setup, error: "This is a duplicate of a log in your file"), :count).by(20)
+      end
+    end
   end
 
   describe "#create_logs?" do

spec/services/bulk_upload/sales/year2022/row_parser_spec.rb

@@ -419,6 +419,30 @@ RSpec.describe BulkUpload::Sales::Year2022::RowParser do
       end
     end
 
+    context "when type is shared ownership" do
+      let(:attributes) { valid_attributes.merge({ field_113: 1, field_68: nil }) }
+
+      it "has error on correct fields" do
+        expect(parser.errors).to include(:field_68)
+      end
+    end
+
+    context "when type is discounted ownership" do
+      let(:attributes) { valid_attributes.merge({ field_113: 2, field_77: nil }) }
+
+      it "has error on correct fields" do
+        expect(parser.errors).to include(:field_77)
+      end
+    end
+
+    context "when type is outright sale" do
+      let(:attributes) { valid_attributes.merge({ field_113: 3, field_87: nil }) }
+
+      it "has error on correct fields" do
+        expect(parser.errors).to include(:field_87)
+      end
+    end
+
     describe "#field_93" do # username for created_by
       context "when blank" do
         let(:attributes) { { bulk_upload:, field_93: "" } }
@@ -832,4 +856,16 @@ RSpec.describe BulkUpload::Sales::Year2022::RowParser do
       end
     end
   end
+
+  describe "#spreadsheet_duplicate_hash" do
+    it "returns a hash" do
+      expect(parser.spreadsheet_duplicate_hash).to be_a(Hash)
+    end
+  end
+
+  describe "#add_duplicate_found_in_spreadsheet_errors" do
+    it "adds errors" do
+      expect { parser.add_duplicate_found_in_spreadsheet_errors }.to change(parser.errors, :size)
+    end
+  end
 end

spec/services/bulk_upload/sales/year2023/row_parser_spec.rb

@@ -1081,4 +1081,16 @@ RSpec.describe BulkUpload::Sales::Year2023::RowParser do
       end
     end
   end
+
+  describe "#spreadsheet_duplicate_hash" do
+    it "returns a hash" do
+      expect(parser.spreadsheet_duplicate_hash).to be_a(Hash)
+    end
+  end
+
+  describe "#add_duplicate_found_in_spreadsheet_errors" do
+    it "adds errors" do
+      expect { parser.add_duplicate_found_in_spreadsheet_errors }.to change(parser.errors, :size)
+    end
+  end
 end

spec/services/csv/sales_log_csv_service_spec.rb

@@ -3,7 +3,7 @@ require "rails_helper"
 RSpec.describe Csv::SalesLogCsvService do
   let(:form_handler_mock) { instance_double(FormHandler) }
   let(:organisation) { create(:organisation) }
-  let!(:log) { create(:sales_log, :completed, owning_organisation: organisation) }
+  let!(:log) { create(:sales_log, :completed, owning_organisation: organisation, purchid: nil) }
   let(:service) { described_class.new(export_type: "labels") }
   let(:csv) { CSV.parse(service.prepare_csv(SalesLog.all)) }