CLDC-2871 Fix bulk upload scheme lookup bug (#1971)

* feat: wip tests

* feat: update tests and functionality

* refactor: lint

* feat: update tests and don't add errors to scheme/location fields unless determined

* feat: update duplicate log behaviour and tests

* refactor: lint

* feat: use needstype helpers

* feat: use needstype helpers and test tweak

* feat: make tests explicit regarding needstype

* feat: make tests explicit regarding needstype

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

@@ -329,9 +329,26 @@ class BulkUpload::Lettings::Year2023::RowParser
             },
             on: :after_log
 
+  validates :field_15,
+            presence: {
+              if: proc { supported_housing? && log_uses_old_scheme_id? },
+              message: I18n.t("validations.not_answered", question: "management group code"),
+              category: :setup,
+            },
+            on: :after_log
+
   validates :field_16,
             presence: {
-              if: proc { [2, 4, 6, 8, 10, 12].include?(field_5) },
+              if: proc { supported_housing? },
+              message: I18n.t("validations.not_answered", question: "scheme code"),
+              category: :setup,
+            },
+            on: :after_log
+
+  validates :field_17,
+            presence: {
+              if: proc { supported_housing? && log_uses_new_scheme_id? },
+              message: I18n.t("validations.not_answered", question: "location code"),
               category: :setup,
             },
             on: :after_log
@@ -464,9 +481,9 @@ class BulkUpload::Lettings::Year2023::RowParser
       "field_8",   # startdate
       "field_9",   # startdate
       "field_13",  # tenancycode
-      field_4 != 1 ? "field_17" : nil,  # location
-      field_4 != 2 ? "field_23" : nil,  # postcode
-      field_4 != 2 ? "field_24" : nil,  # postcode
+      !general_needs? ? location_field.to_s : nil, # location
+      !supported_housing? ? "field_23" : nil,  # postcode
+      !supported_housing? ? "field_24" : nil,  # postcode
       "field_46",  # age1
       "field_47",  # sex1
       "field_50",  # ecstat1
@@ -560,8 +577,8 @@ private
       "ecstat1",
       "owning_organisation",
       "tcharge",
-      field_4 != 2 ? "postcode_full" : nil,
-      field_4 != 1 ? "location" : nil,
+      !supported_housing? ? "postcode_full" : nil,
+      !general_needs? ? "location" : nil,
       "tenancycode",
       log.chcharge.present? ? "chcharge" : nil,
     ].compact
@@ -732,45 +749,45 @@ private
   def validate_location_related
     return if scheme.blank? || location.blank?
 
-    unless location.scheme == scheme
+    if location.scheme != scheme && location_field.present?
       block_log_creation!
-      errors.add(:field_17, "Scheme code must relate to a location that is owned by owning organisation or managing organisation", category: :setup)
+      errors.add(location_field, "#{scheme_or_management_group.capitalize} code must relate to a #{location_or_scheme} that is owned by owning organisation or managing organisation", category: :setup)
     end
   end
 
   def validate_location_exists
-    if scheme && field_17.present? && location.nil?
-      errors.add(:field_17, "Location could not be found with the provided scheme code", category: :setup)
+    if scheme && location_id.present? && location.nil? && location_field.present?
+      errors.add(location_field, "#{location_or_scheme.capitalize} could not be found with the provided #{scheme_or_management_group} code", category: :setup)
     end
   end
 
   def validate_location_data_given
-    if supported_housing? && field_17.blank?
-      errors.add(:field_17, I18n.t("validations.not_answered", question: "scheme code"), category: "setup")
+    if supported_housing? && location_id.blank? && location_field.present?
+      errors.add(location_field, I18n.t("validations.not_answered", question: "#{location_or_scheme} code"), category: "setup")
     end
   end
 
   def validate_scheme_related
-    return unless field_16.present? && scheme.present?
+    return unless scheme_id.present? && scheme.present?
 
     owned_by_owning_org = owning_organisation && scheme.owning_organisation == owning_organisation
     owned_by_managing_org = managing_organisation && scheme.owning_organisation == managing_organisation
 
-    unless owned_by_owning_org || owned_by_managing_org
+    if !(owned_by_owning_org || owned_by_managing_org) && scheme_field.present?
       block_log_creation!
-      errors.add(:field_16, "This management group code does not belong to your organisation, or any of your stock owners / managing agents", category: :setup)
+      errors.add(scheme_field, "This #{scheme_or_management_group} code does not belong to your organisation, or any of your stock owners / managing agents", category: :setup)
     end
   end
 
   def validate_scheme_exists
-    if field_16.present? && scheme.nil?
-      errors.add(:field_16, "The management group code is not correct", category: :setup)
+    if scheme_id.present? && scheme_field.present? && scheme.nil?
+      errors.add(scheme_field, "The #{scheme_or_management_group} code is not correct", category: :setup)
     end
   end
 
   def validate_scheme_data_given
-    if supported_housing? && field_16.blank?
-      errors.add(:field_16, I18n.t("validations.not_answered", question: "management group code"), category: "setup")
+    if supported_housing? && scheme_field.present? && scheme_id.blank?
+      errors.add(scheme_field, I18n.t("validations.not_answered", question: "#{scheme_or_management_group} code"), category: "setup")
     end
   end
 
@@ -851,16 +868,17 @@ private
       errors.add(:field_8, error_message) # startdate
       errors.add(:field_9, error_message) # startdate
       errors.add(:field_13, error_message) # tenancycode
-      errors.add(:field_17, error_message) if field_4 != 1 # location
-      errors.add(:field_23, error_message) if field_4 != 2 # postcode_full
-      errors.add(:field_24, error_message) if field_4 != 2 # postcode_full
-      errors.add(:field_25, error_message) if field_4 != 2 # la
+      errors.add(location_field, error_message) if !general_needs? && location_field.present? # location
+      errors.add(:field_16, error_message) if !general_needs? && location_field.blank? # add to Scheme field as unclear whether log uses New or Old CORE ids
+      errors.add(:field_23, error_message) unless supported_housing? # postcode_full
+      errors.add(:field_24, error_message) unless supported_housing? # postcode_full
+      errors.add(:field_25, error_message) unless supported_housing? # la
       errors.add(:field_46, error_message) # age1
       errors.add(:field_47, error_message) # sex1
       errors.add(:field_50, error_message) # ecstat1
       errors.add(:field_132, error_message) # tcharge
       errors.add(:field_127, error_message) if log.chcharge.present? # chcharge
-      errors.add(:field_125, error_message) if bulk_upload.needstype != 1 # household_charge
+      errors.add(:field_125, error_message) unless general_needs? # household_charge
     end
   end
 
@@ -876,8 +894,8 @@ private
       owning_organisation_id: [:field_1],
       managing_organisation_id: [:field_2],
       renewal: [:field_6],
-      scheme: %i[field_16],
-      location: %i[field_17],
+      scheme: [scheme_field],
+      location: [location_field],
       created_by: [:field_3],
       needstype: [:field_4],
       rent_type: %i[field_5 field_10 field_11],
@@ -1258,13 +1276,51 @@ private
   end
 
   def scheme
-    @scheme ||= Scheme.find_by_id_on_multiple_fields(field_16)
+    return if field_16.nil?
+
+    @scheme ||= Scheme.find_by_id_on_multiple_fields(scheme_id)
   end
 
   def location
     return if scheme.nil?
 
-    @location ||= scheme.locations.find_by_id_on_multiple_fields(field_17)
+    @location ||= scheme.locations.find_by_id_on_multiple_fields(location_id)
+  end
+
+  def log_uses_new_scheme_id?
+    field_16&.start_with?("S")
+  end
+
+  def log_uses_old_scheme_id?
+    field_16.present? && !field_16.start_with?("S")
+  end
+
+  def scheme_field
+    return :field_16 if log_uses_new_scheme_id?
+    return :field_15 if log_uses_old_scheme_id?
+  end
+
+  def scheme_id
+    return field_16 if log_uses_new_scheme_id?
+    return field_15 if log_uses_old_scheme_id?
+  end
+
+  def location_field
+    return :field_17 if log_uses_new_scheme_id?
+    return :field_16 if log_uses_old_scheme_id?
+  end
+
+  def location_id
+    return field_17 if log_uses_new_scheme_id?
+    return field_16 if log_uses_old_scheme_id?
+  end
+
+  def scheme_or_management_group
+    log_uses_new_scheme_id? ? "scheme" : "management group"
+  end
+
+  def location_or_scheme
+    log_uses_new_scheme_id? ? "location" : "scheme"
   end
 
   def renttype

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

@@ -20,7 +20,7 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do
       field_1: owning_org.old_visible_id,
       field_2: managing_org.old_visible_id,
       field_4: "1",
-      field_5: "2",
+      field_5: "1",
       field_6: "2",
       field_7: now.day.to_s,
       field_8: now.month.to_s,
@@ -327,7 +327,7 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do
                 :field_8, # startdate
                 :field_9, # startdate
                 :field_13, # tenancycode
-                :field_17, # location
+                :field_16, # location
                 :field_46, # age1
                 :field_47, # sex1
                 :field_50, # ecstat1
@@ -342,106 +342,290 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do
             end
           end
 
-          context "when a supported housing log with chcharges already exists in the db" do
-            let(:bulk_upload) { create(:bulk_upload, :lettings, user:, needstype: 2) }
-            let(:attributes) do
-              valid_attributes.merge({ field_16: scheme.old_visible_id,
-                                       field_4: "2",
-                                       field_5: "2",
-                                       field_17: location.old_visible_id,
-                                       field_1: owning_org.old_visible_id,
-                                       field_125: 0,
-                                       field_44: 4,
-                                       field_127: "88" })
-            end
+          context "with old core scheme and location ids" do
+            context "when a supported housing log already exists in the db" do
+              let(:attributes) { { bulk_upload:, field_4: "2", field_16: "123" } }
 
-            before do
-              parser.log.save!
-              parser.instance_variable_set(:@valid, nil)
+              before do
+                parser.log.save!
+                parser.instance_variable_set(:@valid, nil)
+              end
+
+              it "is not a valid row" do
+                expect(parser).not_to be_valid
+              end
+
+              it "adds an error to all the fields used to determine duplicates" do
+                parser.valid?
+
+                error_message = "This is a duplicate log"
+
+                [
+                  :field_1, # owning_organisation
+                  :field_7, # startdate
+                  :field_8, # startdate
+                  :field_9, # startdate
+                  :field_13, # tenancycode
+                  :field_16, # location
+                  :field_46, # age1
+                  :field_47, # sex1
+                  :field_50, # ecstat1
+                  :field_132, # tcharge
+                ].each do |field|
+                  expect(parser.errors[field]).to include(error_message)
+                end
+
+                expect(parser.errors[:field_23]).not_to include(error_message)
+                expect(parser.errors[:field_24]).not_to include(error_message)
+                expect(parser.errors[:field_25]).not_to include(error_message)
+              end
             end
 
-            it "is not a valid row" do
-              expect(parser).not_to be_valid
+            context "when a supported housing log with chcharges already exists in the db" do
+              let(:bulk_upload) { create(:bulk_upload, :lettings, user:, needstype: 2) }
+              let(:attributes) do
+                valid_attributes.merge({ field_15: scheme.old_visible_id,
+                                         field_4: "2",
+                                         field_5: "2",
+                                         field_16: location.old_visible_id,
+                                         field_1: owning_org.old_visible_id,
+                                         field_125: 0,
+                                         field_44: 4,
+                                         field_127: "88" })
+              end
+
+              before do
+                parser.log.save!
+                parser.instance_variable_set(:@valid, nil)
+              end
+
+              it "is not a valid row" do
+                expect(parser).not_to be_valid
+              end
+
+              it "adds an error to all the fields used to determine duplicates" do
+                parser.valid?
+
+                error_message = "This is a duplicate log"
+
+                [
+                  :field_1, # owning_organisation
+                  :field_7, # startdate
+                  :field_8, # startdate
+                  :field_9, # startdate
+                  :field_13, # tenancycode
+                  :field_16, # location
+                  :field_46, # age1
+                  :field_47, # sex1
+                  :field_50, # ecstat1
+                  :field_127, # chcharge
+                  :field_125, # household_charge
+                ].each do |field|
+                  expect(parser.errors[field]).to include(error_message)
+                end
+
+                expect(parser.errors[:field_23]).not_to include(error_message)
+                expect(parser.errors[:field_24]).not_to include(error_message)
+                expect(parser.errors[:field_25]).not_to include(error_message)
+              end
             end
 
-            it "adds an error to all the fields used to determine duplicates" do
-              parser.valid?
+            context "when a supported housing log different chcharges already exists in the db" do
+              let(:bulk_upload) { create(:bulk_upload, :lettings, user:, needstype: 2) }
+              let(:attributes) do
+                valid_attributes.merge({ field_15: scheme.old_visible_id,
+                                         field_4: "2",
+                                         field_5: "2",
+                                         field_16: location.old_visible_id,
+                                         field_1: owning_org.old_visible_id,
+                                         field_125: 0,
+                                         field_44: 4,
+                                         field_127: "88" })
+              end
+              let(:attributes_too) do
+                valid_attributes.merge({ field_15: scheme.old_visible_id,
+                                         field_4: "2",
+                                         field_5: "2",
+                                         field_16: location.old_visible_id,
+                                         field_1: owning_org.old_visible_id,
+                                         field_125: 0,
+                                         field_44: 4,
+                                         field_127: "98" })
+              end
+              let(:parser_too) { described_class.new(attributes_too) }
 
-              error_message = "This is a duplicate log"
+              before do
+                parser.log.save!
+                parser.instance_variable_set(:@valid, nil)
+              end
 
-              [
-                :field_1, # owning_organisation
-                :field_7, # startdate
-                :field_8, # startdate
-                :field_9, # startdate
-                :field_13, # tenancycode
-                :field_17, # location
-                :field_46, # age1
-                :field_47, # sex1
-                :field_50, # ecstat1
-                :field_127, # chcharge
-                :field_125, # household_charge
-              ].each do |field|
-                expect(parser.errors[field]).to include(error_message)
+              it "is a valid row" do
+                expect(parser_too).to be_valid
               end
 
-              expect(parser.errors[:field_23]).not_to include(error_message)
-              expect(parser.errors[:field_24]).not_to include(error_message)
-              expect(parser.errors[:field_25]).not_to include(error_message)
+              it "does not add an error to all the fields used to determine duplicates" do
+                parser_too.valid?
+
+                error_message = "This is a duplicate log"
+
+                [
+                  :field_1, # owning_organisation
+                  :field_7, # startdate
+                  :field_8, # startdate
+                  :field_9, # startdate
+                  :field_13, # tenancycode
+                  :field_16, # location
+                  :field_46, # age1
+                  :field_47, # sex1
+                  :field_50, # ecstat1
+                  :field_132, # tcharge
+                ].each do |field|
+                  expect(parser_too.errors[field]).not_to include(error_message)
+                end
+              end
             end
           end
 
-          context "when a supported housing log different chcharges already exists in the db" do
-            let(:bulk_upload) { create(:bulk_upload, :lettings, user:, needstype: 2) }
-            let(:attributes) do
-              valid_attributes.merge({ field_16: scheme.old_visible_id,
-                                       field_4: "2",
-                                       field_5: "2",
-                                       field_17: location.old_visible_id,
-                                       field_1: owning_org.old_visible_id,
-                                       field_125: 0,
-                                       field_44: 4,
-                                       field_127: "88" })
-            end
-            let(:attributes_too) do
-              valid_attributes.merge({ field_16: scheme.old_visible_id,
-                                       field_4: "2",
-                                       field_5: "2",
-                                       field_17: location.old_visible_id,
-                                       field_1: owning_org.old_visible_id,
-                                       field_125: 0,
-                                       field_44: 4,
-                                       field_127: "98" })
-            end
-            let(:parser_too) { described_class.new(attributes_too) }
+          context "with new core scheme and location ids" do
+            context "when a supported housing log already exists in the db" do
+              let(:attributes) { { bulk_upload:, field_4: "2", field_16: "S123" } }
 
-            before do
-              parser.log.save!
-              parser.instance_variable_set(:@valid, nil)
+              before do
+                parser.log.save!
+                parser.instance_variable_set(:@valid, nil)
+              end
+
+              it "is not a valid row" do
+                expect(parser).not_to be_valid
+              end
+
+              it "adds an error to all the fields used to determine duplicates" do
+                parser.valid?
+
+                error_message = "This is a duplicate log"
+
+                [
+                  :field_1, # owning_organisation
+                  :field_7, # startdate
+                  :field_8, # startdate
+                  :field_9, # startdate
+                  :field_13, # tenancycode
+                  :field_17, # location
+                  :field_46, # age1
+                  :field_47, # sex1
+                  :field_50, # ecstat1
+                  :field_132, # tcharge
+                ].each do |field|
+                  expect(parser.errors[field]).to include(error_message)
+                end
+
+                expect(parser.errors[:field_23]).not_to include(error_message)
+                expect(parser.errors[:field_24]).not_to include(error_message)
+                expect(parser.errors[:field_25]).not_to include(error_message)
+              end
             end
 
-            it "is a valid row" do
-              expect(parser_too).to be_valid
+            context "when a supported housing log with chcharges already exists in the db" do
+              let(:bulk_upload) { create(:bulk_upload, :lettings, user:, needstype: 2) }
+              let(:attributes) do
+                valid_attributes.merge({ field_16: "S#{scheme.id}",
+                                         field_4: "2",
+                                         field_5: "2",
+                                         field_17: location.id,
+                                         field_1: owning_org.old_visible_id,
+                                         field_125: 0,
+                                         field_44: 4,
+                                         field_127: "88" })
+              end
+
+              before do
+                parser.log.save!
+                parser.instance_variable_set(:@valid, nil)
+              end
+
+              it "is not a valid row" do
+                expect(parser).not_to be_valid
+              end
+
+              it "adds an error to all the fields used to determine duplicates" do
+                parser.valid?
+
+                error_message = "This is a duplicate log"
+
+                [
+                  :field_1, # owning_organisation
+                  :field_7, # startdate
+                  :field_8, # startdate
+                  :field_9, # startdate
+                  :field_13, # tenancycode
+                  :field_17, # location
+                  :field_46, # age1
+                  :field_47, # sex1
+                  :field_50, # ecstat1
+                  :field_127, # chcharge
+                  :field_125, # household_charge
+                ].each do |field|
+                  expect(parser.errors[field]).to include(error_message)
+                end
+
+                expect(parser.errors[:field_23]).not_to include(error_message)
+                expect(parser.errors[:field_24]).not_to include(error_message)
+                expect(parser.errors[:field_25]).not_to include(error_message)
+              end
             end
 
-            it "does not add an error to all the fields used to determine duplicates" do
-              parser_too.valid?
+            context "when a supported housing log different chcharges already exists in the db" do
+              let(:bulk_upload) { create(:bulk_upload, :lettings, user:, needstype: 2) }
+              let(:attributes) do
+                valid_attributes.merge({ field_16: "S#{scheme.id}",
+                                         field_4: "2",
+                                         field_5: "2",
+                                         field_17: location.id,
+                                         field_1: owning_org.old_visible_id,
+                                         field_125: 0,
+                                         field_44: 4,
+                                         field_127: "88" })
+              end
+              let(:attributes_too) do
+                valid_attributes.merge({ field_16: "S#{scheme.id}",
+                                         field_4: "2",
+                                         field_5: "2",
+                                         field_17: location.id,
+                                         field_1: owning_org.old_visible_id,
+                                         field_125: 0,
+                                         field_44: 4,
+                                         field_127: "98" })
+              end
+              let(:parser_too) { described_class.new(attributes_too) }
 
-              error_message = "This is a duplicate log"
+              before do
+                parser.log.save!
+                parser.instance_variable_set(:@valid, nil)
+              end
 
-              [
-                :field_1, # owning_organisation
-                :field_7, # startdate
-                :field_8, # startdate
-                :field_9, # startdate
-                :field_13, # tenancycode
-                :field_17, # location
-                :field_46, # age1
-                :field_47, # sex1
-                :field_50, # ecstat1
-                :field_132, # tcharge
-              ].each do |field|
-                expect(parser_too.errors[field]).not_to include(error_message)
+              it "is a valid row" do
+                expect(parser_too).to be_valid
+              end
+
+              it "does not add an error to all the fields used to determine duplicates" do
+                parser_too.valid?
+
+                error_message = "This is a duplicate log"
+
+                [
+                  :field_1, # owning_organisation
+                  :field_7, # startdate
+                  :field_8, # startdate
+                  :field_9, # startdate
+                  :field_13, # tenancycode
+                  :field_17, # location
+                  :field_46, # age1
+                  :field_47, # sex1
+                  :field_50, # ecstat1
+                  :field_132, # tcharge
+                ].each do |field|
+                  expect(parser_too.errors[field]).not_to include(error_message)
+                end
               end
             end
           end
@@ -652,108 +836,206 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do
       end
     end
 
-    describe "#field_16" do
+    describe "#field_15, field_16, field_17" do # scheme and location fields
       context "when nullable not permitted" do
-        let(:attributes) { { bulk_upload:, field_5: "2", field_16: nil } }
+        let(:attributes) { { bulk_upload:, field_4: "2", field_5: "2", field_15: nil, field_16: nil, field_17: nil } }
 
         it "cannot be nulled" do
-          expect(parser.errors[:field_16]).to be_present
+          expect(parser.errors[:field_15]).to be_blank
+          expect(parser.errors[:field_16]).to eq(["You must answer scheme code"])
+          expect(parser.errors[:field_17]).to be_blank
         end
       end
 
       context "when nullable permitted" do
-        let(:attributes) { { bulk_upload:, field_5: "1", field_16: nil } }
+        let(:attributes) { { bulk_upload:, field_4: "1", field_5: "1", field_15: nil, field_16: nil, field_17: nil } }
 
         it "can be nulled" do
           expect(parser.errors[:field_15]).to be_blank
+          expect(parser.errors[:field_16]).to be_blank
+          expect(parser.errors[:field_17]).to be_blank
         end
       end
 
-      context "when matching scheme cannot be found" do
-        let(:attributes) { { bulk_upload:, field_5: "1", field_16: "123" } }
+      context "when using New CORE ids" do
+        let(:scheme) { create(:scheme, :with_old_visible_id, owning_organisation: owning_org) }
+        let(:location) { create(:location, :with_old_visible_id, scheme:) }
+
+        context "when matching scheme cannot be found" do
+          let(:attributes) { { bulk_upload:, field_1: owning_org.old_visible_id, field_4: "2", field_5: "2", field_16: "S123", field_17: location.id } }
 
-        it "returns a setup error" do
-          expect(parser.errors.where(:field_16, category: :setup)).to be_present
+          it "returns a setup error" do
+            expect(parser.errors[:field_15]).to be_blank
+            expect(parser.errors.where(:field_16, category: :setup).map(&:message)).to eq(["The scheme code is not correct"])
+            expect(parser.errors[:field_17]).to be_blank
+          end
         end
-      end
 
-      context "when scheme belongs to someone else" do
-        let(:other_scheme) { create(:scheme, :with_old_visible_id) }
-        let(:attributes) { { bulk_upload:, field_5: "1", field_16: other_scheme.old_visible_id, field_1: owning_org.old_visible_id } }
+        context "when missing location" do
+          let(:attributes) { { bulk_upload:, field_1: owning_org.old_visible_id, field_4: "2", field_5: "2", field_16: "S#{scheme.id}", field_17: nil } }
 
-        it "returns a setup error" do
-          expect(parser.errors.where(:field_16, category: :setup).map(&:message)).to include("This management group code does not belong to your organisation, or any of your stock owners / managing agents")
+          it "returns a setup error" do
+            expect(parser.errors[:field_15]).to be_blank
+            expect(parser.errors[:field_16]).to be_blank
+            expect(parser.errors.where(:field_17, category: :setup).map(&:message)).to eq(["You must answer location code"])
+          end
         end
-      end
 
-      context "when scheme belongs to owning org" do
-        let(:scheme) { create(:scheme, :with_old_visible_id, owning_organisation: owning_org) }
-        let(:attributes) { { bulk_upload:, field_5: "1", field_16: scheme.old_visible_id, field_1: owning_org.old_visible_id } }
+        context "when matching location cannot be found" do
+          let(:attributes) { { bulk_upload:, field_1: owning_org.old_visible_id, field_4: "2", field_5: "2", field_16: "S#{scheme.id}", field_17: "123" } }
 
-        it "does not return an error" do
-          expect(parser.errors[:field_16]).to be_blank
+          it "returns a setup error" do
+            expect(parser.errors[:field_15]).to be_blank
+            expect(parser.errors[:field_16]).to be_blank
+            expect(parser.errors.where(:field_17, category: :setup).map(&:message)).to eq(["Location could not be found with the provided scheme code"])
+          end
         end
-      end
 
-      context "when scheme belongs to managing org" do
-        let(:scheme) { create(:scheme, :with_old_visible_id, owning_organisation: managing_org) }
-        let(:attributes) { { bulk_upload:, field_5: "1", field_16: scheme.old_visible_id, field_2: managing_org.old_visible_id } }
+        context "when matching location exists" do
+          let(:attributes) { { bulk_upload:, field_1: owning_org.old_visible_id, field_4: "2", field_5: "2", field_16: "S#{scheme.id}", field_17: location.id } }
 
-        it "does not return an error" do
-          expect(parser.errors[:field_16]).to be_blank
+          it "does not return an error" do
+            expect(parser.errors[:field_15]).to be_blank
+            expect(parser.errors[:field_16]).to be_blank
+            expect(parser.errors[:field_17]).to be_blank
+          end
         end
-      end
-    end
 
-    describe "#field_17" do
-      context "when location does not exist" do
-        let(:scheme) { create(:scheme, :with_old_visible_id, owning_organisation: owning_org) }
-        let(:attributes) do
-          {
-            bulk_upload:,
-            field_5: "1",
-            field_16: scheme.old_visible_id,
-            field_17: "dontexist",
-            field_1: owning_org.old_visible_id,
-          }
+        context "when location exists but not related" do
+          let(:other_scheme) { create(:scheme, :with_old_visible_id) }
+          let(:other_location) { create(:location, :with_old_visible_id, scheme: other_scheme) }
+          let(:attributes) { { bulk_upload:, field_1: owning_org.old_visible_id, field_4: "2", field_5: "2", field_16: "S#{scheme.id}", field_17: other_location.id } }
+
+          it "returns a setup error" do
+            expect(parser.errors[:field_15]).to be_blank
+            expect(parser.errors[:field_16]).to be_blank
+            expect(parser.errors.where(:field_17, category: :setup).map(&:message)).to eq(["Location could not be found with the provided scheme code"])
+          end
         end
 
-        it "returns a setup error" do
-          expect(parser.errors.where(:field_17, category: :setup)).to be_present
+        context "when scheme belongs to someone else" do
+          let(:other_scheme) { create(:scheme, :with_old_visible_id) }
+          let(:other_location) { create(:location, :with_old_visible_id, scheme: other_scheme) }
+          let(:attributes) { { bulk_upload:, field_4: "2", field_5: "2", field_16: "S#{other_scheme.id}", field_17: other_location.id, field_1: owning_org.old_visible_id } }
+
+          it "returns a setup error" do
+            expect(parser.errors[:field_15]).to be_blank
+            expect(parser.errors.where(:field_16, category: :setup).map(&:message)).to eq(["This scheme code does not belong to your organisation, or any of your stock owners / managing agents"])
+            expect(parser.errors[:field_17]).to be_blank
+          end
+        end
+
+        context "when scheme belongs to owning org" do
+          let(:attributes) { { bulk_upload:, field_4: "2", field_5: "2", field_16: "S#{scheme.id}", field_17: location.id, field_1: owning_org.old_visible_id } }
+
+          it "does not return an error" do
+            expect(parser.errors[:field_15]).to be_blank
+            expect(parser.errors[:field_16]).to be_blank
+            expect(parser.errors[:field_17]).to be_blank
+          end
+        end
+
+        context "when scheme belongs to managing org" do
+          let(:managing_org_scheme) { create(:scheme, :with_old_visible_id, owning_organisation: managing_org) }
+          let(:managing_org_location) { create(:location, :with_old_visible_id, scheme: managing_org_scheme) }
+          let(:attributes) { { bulk_upload:, field_4: "2", field_5: "2", field_16: "S#{managing_org_scheme.id}", field_17: managing_org_location.id, field_2: managing_org.old_visible_id } }
+
+          it "does not return an error" do
+            expect(parser.errors[:field_15]).to be_blank
+            expect(parser.errors[:field_16]).to be_blank
+            expect(parser.errors[:field_17]).to be_blank
+          end
         end
       end
 
-      context "when location exists" do
+      context "when using Old CORE ids" do
         let(:scheme) { create(:scheme, :with_old_visible_id, owning_organisation: owning_org) }
-        let(:attributes) do
-          {
-            bulk_upload:,
-            field_5: "1",
-            field_16: scheme.old_visible_id,
-            field_17: location.old_visible_id,
-            field_1: owning_org.old_visible_id,
-          }
+        let(:location) { create(:location, :with_old_visible_id, scheme:) }
+
+        context "when matching scheme cannot be found" do
+          let(:attributes) { { bulk_upload:, field_1: owning_org.old_visible_id, field_4: "2", field_5: "2", field_15: "123", field_16: location.old_visible_id } }
+
+          it "returns a setup error" do
+            expect(parser.errors.where(:field_15, category: :setup).map(&:message)).to eq(["The management group code is not correct"])
+            expect(parser.errors[:field_16]).to be_blank
+            expect(parser.errors[:field_17]).to be_blank
+          end
         end
 
-        it "does not return an error" do
-          expect(parser.errors[:field_17]).to be_blank
+        context "when missing location" do
+          let(:attributes) { { bulk_upload:, field_1: owning_org.old_visible_id, field_4: "2", field_5: "2", field_15: scheme.old_visible_id, field_16: nil } }
+
+          it "returns a setup error" do
+            expect(parser.errors[:field_15]).to be_blank
+            expect(parser.errors.where(:field_16, category: :setup).map(&:message)).to eq(["You must answer scheme code"])
+            expect(parser.errors[:field_17]).to be_blank
+          end
         end
-      end
 
-      context "when location exists but not related" do
-        let(:location) { create(:scheme, :with_old_visible_id) }
-        let(:attributes) do
-          {
-            bulk_upload:,
-            field_5: "1",
-            field_16: scheme.old_visible_id,
-            field_17: location.old_visible_id,
-            field_1: owning_org.old_visible_id,
-          }
+        context "when matching location cannot be found" do
+          let(:attributes) { { bulk_upload:, field_1: owning_org.old_visible_id, field_4: "2", field_5: "2", field_15: scheme.old_visible_id, field_16: "123" } }
+
+          it "returns a setup error" do
+            expect(parser.errors[:field_15]).to be_blank
+            expect(parser.errors.where(:field_16, category: :setup).map(&:message)).to eq(["Scheme could not be found with the provided management group code"])
+            expect(parser.errors[:field_17]).to be_blank
+          end
         end
 
-        it "returns a setup error" do
-          expect(parser.errors.where(:field_17, category: :setup)).to be_present
+        context "when matching location exists" do
+          let(:attributes) { { bulk_upload:, field_1: owning_org.old_visible_id, field_4: "2", field_5: "2", field_15: scheme.old_visible_id, field_16: location.old_visible_id } }
+
+          it "does not return an error" do
+            expect(parser.errors[:field_15]).to be_blank
+            expect(parser.errors[:field_16]).to be_blank
+            expect(parser.errors[:field_17]).to be_blank
+          end
+        end
+
+        context "when location exists but not related" do
+          let(:other_scheme) { create(:scheme, :with_old_visible_id) }
+          let(:other_location) { create(:location, :with_old_visible_id, scheme: other_scheme) }
+          let(:attributes) { { bulk_upload:, field_1: owning_org.old_visible_id, field_4: "2", field_5: "2", field_15: scheme.old_visible_id, field_16: other_location.old_visible_id } }
+
+          it "returns a setup error" do
+            expect(parser.errors[:field_15]).to be_blank
+            expect(parser.errors.where(:field_16, category: :setup).map(&:message)).to eq(["Scheme could not be found with the provided management group code"])
+            expect(parser.errors[:field_17]).to be_blank
+          end
+        end
+
+        context "when scheme belongs to someone else" do
+          let(:other_scheme) { create(:scheme, :with_old_visible_id) }
+          let(:other_location) { create(:location, :with_old_visible_id, scheme: other_scheme) }
+          let(:attributes) { { bulk_upload:, field_4: "2", field_5: "2", field_15: other_scheme.old_visible_id, field_16: other_location.old_visible_id, field_1: owning_org.old_visible_id } }
+
+          it "returns a setup error" do
+            expect(parser.errors.where(:field_15, category: :setup).map(&:message)).to eq(["This management group code does not belong to your organisation, or any of your stock owners / managing agents"])
+            expect(parser.errors[:field_16]).to be_blank
+            expect(parser.errors[:field_17]).to be_blank
+          end
+        end
+
+        context "when scheme belongs to owning org" do
+          let(:attributes) { { bulk_upload:, field_4: "2", field_5: "2", field_15: scheme.old_visible_id, field_16: location.old_visible_id, field_1: owning_org.old_visible_id } }
+
+          it "does not return an error" do
+            expect(parser.errors[:field_15]).to be_blank
+            expect(parser.errors[:field_16]).to be_blank
+            expect(parser.errors[:field_17]).to be_blank
+          end
+        end
+
+        context "when scheme belongs to managing org" do
+          let(:managing_org_scheme) { create(:scheme, :with_old_visible_id, owning_organisation: managing_org) }
+          let(:managing_org_location) { create(:location, :with_old_visible_id, scheme: managing_org_scheme) }
+          let(:attributes) { { bulk_upload:, field_4: "2", field_5: "2", field_15: managing_org_scheme.old_visible_id, field_16: managing_org_location.old_visible_id, field_2: managing_org.old_visible_id } }
+
+          it "does not return an error" do
+            expect(parser.errors[:field_15]).to be_blank
+            expect(parser.errors[:field_16]).to be_blank
+            expect(parser.errors[:field_17]).to be_blank
+          end
         end
       end
     end
@@ -1410,7 +1692,15 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do
 
     describe "#location" do
       context "when lookup is via new core id" do
-        let(:attributes) { { bulk_upload:, field_16: scheme.old_visible_id, field_17: location.id, field_1: owning_org } }
+        let(:attributes) { { bulk_upload:, field_16: "S#{scheme.id}", field_17: location.id, field_1: owning_org } }
+
+        it "assigns the correct location" do
+          expect(parser.log.location).to eql(location)
+        end
+      end
+
+      context "when lookup is via old core id" do
+        let(:attributes) { { bulk_upload:, field_15: scheme.old_visible_id, field_16: location.old_visible_id, field_1: owning_org } }
 
         it "assigns the correct location" do
           expect(parser.log.location).to eql(location)
@@ -1419,13 +1709,21 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do
     end
 
     describe "#scheme" do
-      context "when lookup is via id prefixed with S" do
+      context "when lookup is via new core id" do
         let(:attributes) { { bulk_upload:, field_16: "S#{scheme.id}", field_1: owning_org } }
 
         it "assigns the correct scheme" do
           expect(parser.log.scheme).to eql(scheme)
         end
       end
+
+      context "when lookup is via old core id" do
+        let(:attributes) { { bulk_upload:, field_15: scheme.old_visible_id, field_16: location.old_visible_id, field_1: owning_org } }
+
+        it "assigns the correct scheme" do
+          expect(parser.log.scheme).to eql(scheme)
+        end
+      end
     end
 
     describe "#owning_organisation" do