CLDC-4297: Align new service charges with bu spec (#3240)

* CLDC-4270: Add an error for fields with the wrong type for 2026 lettings

* CLDC-4270: Add tests

* CLDC-4270: Port changes to other years' parsers

sales 2025 2026
lettings 2026

* fixup! CLDC-4270: Add an error for fields with the wrong type for 2026 lettings

* fixup! CLDC-4270: Port changes to other years' parsers

* CLDC-4270: Fix tests post merge

* fixup! CLDC-4270: Add tests

* CLDC-4270: Add ? to boolean method

* CLDC-4263: align new service charge questions with bu spec

* CLDC-4263: fix "no" mapping and allow decimal mscharge

* CLDC-4263: update tests

* CLDC-4297: clarify test naming

* CLDC-4297: add tests for inferences and confirm R inference

* CLDC-4297: add test override

* CLDC-4297: remove R tests

* CLDC-4297: add R behaviour back in

* CLDC-4297: updated error handling

* CLDC-4297: add warning comment on block_log_creation!

* CLDC-4297: test updates

* CLDC-4297: comment update

* CLDC-4297: allow 0 for newservicecharges

* CLDC-4297: test improvements

* CLDC-4297: test negatives and unify order

* CLDC-4297: misc cleanup

* CLDC-4297: specify same value errors

* CLDC-4297: don't allow 0 for mscharge and update tests

* CLDC-4297: update case insensitive field tests

* CLDC-4297: don't allow 0.0

* CLDC-4297: test 0.0 and r explicitly

* CLDC-4297: refactor and simplify

* CLDC-4297: final validation test tweaks

---------

Co-authored-by: samyou-softwire <samuel.young@softwire.com>

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

@@ -525,6 +525,9 @@ class BulkUpload::Lettings::Year2025::RowParser
     @log ||= LettingsLog.new(attributes_for_log)
   end
 
+  # Will send a "Bulk upload failed" email rather than an "Errors in bulk upload" email.
+  # The body of the "Bulk upload failed" email says there are errors in the setup section,
+  # so only use this method for setup section errors.
   def block_log_creation!
     self.block_log_creation = true
   end

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

@@ -560,6 +560,9 @@ class BulkUpload::Lettings::Year2026::RowParser
     @log ||= LettingsLog.new(attributes_for_log)
   end
 
+  # Will send a "Bulk upload failed" email rather than an "Errors in bulk upload" email.
+  # The body of the "Bulk upload failed" email says there are errors in the setup section,
+  # so only use this method for setup section errors.
   def block_log_creation!
     self.block_log_creation = true
   end

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

@@ -1290,6 +1290,9 @@ private
     end
   end
 
+  # Will send a "Bulk upload failed" email rather than an "Errors in bulk upload" email.
+  # The body of the "Bulk upload failed" email says there are errors in the setup section,
+  # so only use this method for setup section errors.
   def block_log_creation!
     self.block_log_creation = true
   end

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

@@ -152,6 +152,7 @@ class BulkUpload::Sales::Year2026::RowParser
   }.freeze
 
   ERROR_BASE_KEY = "validations.sales.2026.bulk_upload".freeze
+  NUMBER_OR_R_FORMAT = /\A(\d+(\.\d+)?|R)\z/i
 
   CASE_INSENSITIVE_FIELDS = [
     :field_29, # Age of buyer 1
@@ -176,6 +177,11 @@ class BulkUpload::Sales::Year2026::RowParser
 
     :field_103, # What is the length of the mortgage in years? - Shared ownership
     :field_133, # What is the length of the mortgage in years? - Discounted ownership
+
+    :field_107, # What are the total monthly service charges for the property?
+    :field_125, # What are the monthly service charges for the property?
+    :field_126, # New monthly service charge amount
+    :field_136, # What are the total monthly leasehold charges for the property?
   ].freeze
 
   attribute :bulk_upload
@@ -296,7 +302,7 @@ class BulkUpload::Sales::Year2026::RowParser
   attribute :field_104, :decimal
   attribute :field_105, :decimal
   attribute :field_106, :decimal
-  attribute :field_107, :decimal
+  attribute :field_107, :string
   attribute :field_108, :decimal
 
   attribute :field_109, :decimal
@@ -315,8 +321,8 @@ class BulkUpload::Sales::Year2026::RowParser
   attribute :field_122, :integer
   attribute :field_123, :decimal
   attribute :field_124, :decimal
-  attribute :field_125, :integer
+  attribute :field_125, :string
-  attribute :field_126, :decimal
+  attribute :field_126, :string
 
   attribute :field_127, :integer
   attribute :field_128, :decimal
@@ -327,7 +333,7 @@ class BulkUpload::Sales::Year2026::RowParser
   attribute :field_133, :string
   attribute :field_134, :integer
   attribute :field_135, :decimal
-  attribute :field_136, :decimal
+  attribute :field_136, :string
 
   validates :field_1,
             presence: {
@@ -496,6 +502,8 @@ class BulkUpload::Sales::Year2026::RowParser
   validate :validate_buyer_2_nationality, on: :after_log
   validate :validate_mortlen_field_if_buyer_interviewed, on: :after_log
 
+  validate :validate_service_charge_fields, on: :after_log
+
   validate :validate_nulls, on: :after_log
 
   def self.question_for_field(field)
@@ -899,7 +907,7 @@ private
       gender_same_as_sex6: %i[field_68],
       gender_description6: %i[field_69],
 
-      hasservicechargeschanged: %i[field_125],
+      hasservicechargeschanged: %i[field_126],
       newservicecharges: %i[field_126],
     }
   end
@@ -950,9 +958,6 @@ private
     attributes["gender_same_as_sex6"] = field_68
     attributes["gender_description6"] = field_69
 
-    attributes["hasservicechargeschanged"] = field_125
-    attributes["newservicecharges"] = field_126
-
     attributes["relat2"] = relationship_from_is_partner(field_37)
     attributes["relat3"] = relationship_from_is_partner(field_47)
     attributes["relat4"] = relationship_from_is_partner(field_53)
@@ -1026,8 +1031,6 @@ private
 
     attributes["cashdis"] = field_105
     attributes["mrent"] = mrent
-    attributes["mscharge"] = mscharge if mscharge&.positive?
-    attributes["has_mscharge"] = attributes["mscharge"].present? ? 1 : 0
     attributes["grant"] = field_129
     attributes["discount"] = field_130
 
@@ -1097,6 +1100,11 @@ private
     attributes["management_fee"] = field_108
     attributes["has_management_fee"] = field_108.present? && field_108.positive? ? 1 : 0
 
+    attributes["has_mscharge"] = has_mscharge_value
+    attributes["mscharge"] = mscharge_value
+    attributes["hasservicechargeschanged"] = hasservicechargeschanged_value
+    attributes["newservicecharges"] = newservicecharges_value
+
     attributes
   end
 
@@ -1256,11 +1264,36 @@ private
   end
 
   def mscharge
-    return field_107 if shared_ownership?
+    return field_107 if shared_ownership_initial_purchase?
+    return field_125 if staircasing?
 
     field_136 if discounted_ownership?
   end
 
+  def has_mscharge_value
+    return unless mscharge.present? && mscharge.match?(NUMBER_OR_R_FORMAT)
+
+    mscharge.casecmp?("R") ? 0 : 1
+  end
+
+  def mscharge_value
+    return unless mscharge.present? && mscharge.match?(NUMBER_OR_R_FORMAT) && !mscharge.casecmp?("R")
+
+    mscharge.to_d
+  end
+
+  def hasservicechargeschanged_value
+    return unless field_126.present? && field_126.match?(NUMBER_OR_R_FORMAT)
+
+    field_126.casecmp?("R") ? 2 : 1
+  end
+
+  def newservicecharges_value
+    return unless field_126.present? && field_126.match?(NUMBER_OR_R_FORMAT) && !field_126.casecmp?("R")
+
+    field_126.to_d
+  end
+
   def mortlen
     return field_103 if shared_ownership?
 
@@ -1333,10 +1366,11 @@ private
   end
 
   def mscharge_fields
-    return [:field_107] if shared_ownership?
+    return [:field_107] if shared_ownership_initial_purchase?
+    return [:field_125] if staircasing?
     return [:field_136] if discounted_ownership?
 
-    %i[field_107 field_136]
+    %i[field_107 field_125 field_136]
   end
 
   def mortlen_fields
@@ -1389,6 +1423,29 @@ private
     end
   end
 
+  def validate_service_charge_fields
+    message = I18n.t("#{ERROR_BASE_KEY}.mscharge.invalid")
+
+    if shared_ownership_initial_purchase? && field_107.present? && !field_107.match?(NUMBER_OR_R_FORMAT)
+      errors.add(:field_107, message)
+    end
+
+    if staircasing?
+      if field_125.present? && !field_125.match?(NUMBER_OR_R_FORMAT)
+        errors.add(:field_125, message)
+      end
+
+      if field_126.present? && !field_126.match?(NUMBER_OR_R_FORMAT)
+        errors.add(:field_126, I18n.t("#{ERROR_BASE_KEY}.newservicecharges.invalid"))
+      end
+    end
+
+    if discounted_ownership? && field_136.present? && !field_136.match?(NUMBER_OR_R_FORMAT)
+      errors.add(:field_136, message)
+    end
+  end
+
+  # Will send a "Bulk upload failed" email rather than an "Errors in bulk upload" email
   def block_log_creation!
     self.block_log_creation = true
   end

config/locales/validations/sales/2026/bulk_upload.en.yml

@@ -44,6 +44,10 @@ en:
             not_answered: "Enter either the UPRN or the full address."
           nationality:
             invalid: "Select a valid nationality."
+          mscharge:
+            invalid: "Service charge must be a positive number or the letter R."
+          newservicecharges:
+            invalid: "New service charge must be a number or the letter R."
           mortlen:
             invalid: "Mortgage length must be a number or the letter R"
             invalid_for_interviewed: "You indicated that you interviewed the buyer(s), but selected “Don’t know” for mortgage length. Please provide the mortgage length or update your response."

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

@@ -1334,7 +1334,7 @@ RSpec.describe BulkUpload::Sales::Year2023::RowParser do
     context "when mscharge is given, but is set to 0 for shared ownership" do
       let(:attributes) { valid_attributes.merge(field_114: "0") }
 
-      it "does not override variables correctly" do
+      it "does not override variables" do
         log = parser.log
         expect(log["has_mscharge"]).to eq(0) # no
         expect(log["mscharge"]).to be_nil
@@ -1344,7 +1344,7 @@ RSpec.describe BulkUpload::Sales::Year2023::RowParser do
     context "when mscharge is given, but is set to 0 for discounted ownership" do
       let(:attributes) { valid_attributes.merge(field_7: "2", field_126: "0") }
 
-      it "does not override variables correctly" do
+      it "does not override variables" do
         log = parser.log
         expect(log["has_mscharge"]).to eq(0) # no
         expect(log["mscharge"]).to be_nil
@@ -1354,7 +1354,7 @@ RSpec.describe BulkUpload::Sales::Year2023::RowParser do
     context "when mscharge is given, but is set to 0 for outright sale" do
       let(:attributes) { valid_attributes.merge(field_7: "3", field_135: "0") }
 
-      it "does not override variables correctly" do
+      it "does not override variables" do
         log = parser.log
         expect(log["has_mscharge"]).to eq(0) # no
         expect(log["mscharge"]).to be_nil

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

@@ -1973,7 +1973,7 @@ RSpec.describe BulkUpload::Sales::Year2024::RowParser do
     context "when mscharge is given, but is set to 0 for shared ownership" do
       let(:attributes) { valid_attributes.merge(field_112: "0") }
 
-      it "does not override variables correctly" do
+      it "does not override variables" do
         log = parser.log
         expect(log["has_mscharge"]).to eq(0) # no
         expect(log["mscharge"]).to be_nil
@@ -1983,7 +1983,7 @@ RSpec.describe BulkUpload::Sales::Year2024::RowParser do
     context "when mscharge is given, but is set to 0 for discounted ownership" do
       let(:attributes) { valid_attributes.merge(field_8: "2", field_124: "0") }
 
-      it "does not override variables correctly" do
+      it "does not override variables" do
         log = parser.log
         expect(log["has_mscharge"]).to eq(0) # no
         expect(log["mscharge"]).to be_nil
@@ -1993,7 +1993,7 @@ RSpec.describe BulkUpload::Sales::Year2024::RowParser do
     context "when mscharge is given, but is set to 0 for outright sale" do
       let(:attributes) { valid_attributes.merge(field_8: "3", field_131: "0") }
 
-      it "does not override variables correctly" do
+      it "does not override variables" do
         log = parser.log
         expect(log["has_mscharge"]).to eq(0) # no
         expect(log["mscharge"]).to be_nil

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

@@ -1889,7 +1889,7 @@ RSpec.describe BulkUpload::Sales::Year2025::RowParser do
     context "when mscharge is given, but is set to 0 for shared ownership" do
       let(:attributes) { valid_attributes.merge(field_94: "0") }
 
-      it "does not override variables correctly" do
+      it "does not override variables" do
         log = parser.log
         expect(log["has_mscharge"]).to eq(0) # no
         expect(log["mscharge"]).to be_nil
@@ -1899,7 +1899,7 @@ RSpec.describe BulkUpload::Sales::Year2025::RowParser do
     context "when mscharge is given, but is set to 0 for discounted ownership" do
       let(:attributes) { valid_attributes.merge(field_8: "2", field_121: "0") }
 
-      it "does not override variables correctly" do
+      it "does not override variables" do
         log = parser.log
         expect(log["has_mscharge"]).to eq(0) # no
         expect(log["mscharge"]).to be_nil

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

@@ -116,7 +116,7 @@ RSpec.describe BulkUpload::Sales::Year2026::RowParser do
       field_31: "1",
       field_40: "2",
       field_41: "Non-binary",
-      field_125: "1",
+      field_125: "200",
       field_126: "150",
     }
   end
@@ -301,7 +301,7 @@ RSpec.describe BulkUpload::Sales::Year2026::RowParser do
 
         context "and case insensitive fields are set to lowercase" do
           let(:case_insensitive_fields) { %w[field_30 field_39 field_49 field_55 field_61 field_67] }
-          let(:case_insensitive_integer_fields_with_r_option) { %w[field_29 field_38 field_48 field_54 field_60 field_66 field_77 field_88 field_83 field_85 field_103 field_133] }
+          let(:case_insensitive_integer_fields_with_r_option) { %w[field_29 field_38 field_48 field_54 field_60 field_66 field_77 field_88 field_83 field_85 field_103 field_107 field_125 field_126 field_133 field_136] }
           let(:attributes) do
             valid_attributes
               .merge(case_insensitive_fields.each_with_object({}) { |field, h| h[field.to_sym] = valid_attributes[field.to_sym]&.downcase })
@@ -1948,25 +1948,557 @@ RSpec.describe BulkUpload::Sales::Year2026::RowParser do
       end
     end
 
-    context "when mscharge is given, but is set to 0 for shared ownership" do
+    context "with service charges fields" do
-      let(:attributes) { valid_attributes.merge(field_107: "0") }
+      context "with mscharge for shared ownership initial purchase (field_107)" do
+        context "when positive" do
+          let(:attributes) { valid_attributes.merge(field_10: "2", field_107: "100") }
 
-      it "does not override variables correctly" do
+          it "does not add a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_107]).to be_blank
+          end
+
+          it "sets has_mscharge to yes and mscharge to the value" do
+            log = parser.log
+            expect(log["has_mscharge"]).to eq(1)
+            expect(log["mscharge"]).to eq(100)
+          end
+        end
+
+        context "when set to 1" do
+          let(:attributes) { valid_attributes.merge(field_10: "2", field_107: "1") }
+
+          it "does not add a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_107]).to be_blank
+          end
+
+          it "sets has_mscharge to yes and mscharge to the value" do
+            log = parser.log
+            expect(log["has_mscharge"]).to eq(1)
+            expect(log["mscharge"]).to eq(1)
+          end
+        end
+
+        context "when set to 0" do
+          let(:attributes) { valid_attributes.merge(field_10: "2", field_107: "0") }
+
+          it "does not add a bulk upload format validation error but adds a site validation error" do
+            parser.valid?
+            expect(parser.errors[:field_107]).not_to include(I18n.t("validations.sales.2026.bulk_upload.mscharge.invalid"))
+            expect(parser.errors[:field_107]).to include(I18n.t("validations.sales.financial.mscharge.monthly_leasehold_charges.not_zero"))
+          end
+
+          it "sets has_mscharge to yes and mscharge to the value" do
+            log = parser.log
+            expect(log["has_mscharge"]).to eq(1)
+            expect(log["mscharge"]).to eq(0)
+          end
+        end
+
+        context "when set to 0.0" do
+          let(:attributes) { valid_attributes.merge(field_10: "2", field_107: "0.0") }
+
+          it "does not add a bulk upload format validation error but adds a site validation error" do
+            parser.valid?
+            expect(parser.errors[:field_107]).not_to include(I18n.t("validations.sales.2026.bulk_upload.mscharge.invalid"))
+            expect(parser.errors[:field_107]).to include(I18n.t("validations.sales.financial.mscharge.monthly_leasehold_charges.not_zero"))
+          end
+
+          it "sets has_mscharge to yes and mscharge to the value" do
+            log = parser.log
+            expect(log["has_mscharge"]).to eq(1)
+            expect(log["mscharge"]).to eq(0)
+          end
+        end
+
+        context "when set to R" do
+          let(:attributes) { valid_attributes.merge(field_10: "2", field_107: "R") }
+
+          it "does not add a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_107]).to be_blank
+          end
+
+          it "sets has_mscharge to no and does not set mscharge" do
+            log = parser.log
+            expect(log["has_mscharge"]).to eq(0)
+            expect(log["mscharge"]).to be_nil
+          end
+        end
+
+        context "when set to lowercase r" do
+          let(:attributes) { valid_attributes.merge(field_10: "2", field_107: "r") }
+
+          it "does not add a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_107]).to be_blank
+          end
+
+          it "sets has_mscharge to no and does not set mscharge" do
+            log = parser.log
+            expect(log["has_mscharge"]).to eq(0)
+            expect(log["mscharge"]).to be_nil
+          end
+        end
+
+        context "when an invalid string" do
+          let(:attributes) { valid_attributes.merge(field_10: "2", field_107: "X") }
+
+          it "adds a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_107]).to include(I18n.t("validations.sales.2026.bulk_upload.mscharge.invalid"))
+          end
+
+          it "does not set has_mscharge or mscharge" do
+            log = parser.log
+            expect(log["has_mscharge"]).to be_nil
+            expect(log["mscharge"]).to be_nil
+          end
+        end
+
+        context "when blank" do
+          let(:attributes) { valid_attributes.merge(field_10: "2", field_107: nil) }
+
+          it "does not add a bulk upload format validation error but adds a site validation error" do
+            parser.valid?
+            expect(parser.errors[:field_107]).not_to include(I18n.t("validations.sales.2026.bulk_upload.mscharge.invalid"))
+            expect(parser.errors[:field_107]).to include("You must answer property service charges.")
+          end
+
+          it "does not set has_mscharge or mscharge" do
             log = parser.log
-        expect(log["has_mscharge"]).to eq(0) # no
+            expect(log["has_mscharge"]).to be_nil
             expect(log["mscharge"]).to be_nil
           end
         end
 
-    context "when mscharge is given, but is set to 0 for discounted ownership" do
+        context "when negative" do
+          let(:attributes) { valid_attributes.merge(field_10: "2", field_107: "-100") }
+
+          it "adds a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_107]).to include(I18n.t("validations.sales.2026.bulk_upload.mscharge.invalid"))
+          end
+
+          it "does not set has_mscharge or mscharge" do
+            log = parser.log
+            expect(log["has_mscharge"]).to be_nil
+            expect(log["mscharge"]).to be_nil
+          end
+        end
+      end
+
+      context "with mscharge for staircasing (field_125)" do
+        context "when positive" do
+          let(:attributes) { valid_attributes.merge(field_125: "100") }
+
+          it "does not add a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_125]).to be_blank
+          end
+
+          it "sets has_mscharge to yes and mscharge to the value" do
+            log = parser.log
+            expect(log["has_mscharge"]).to eq(1)
+            expect(log["mscharge"]).to eq(100)
+          end
+        end
+
+        context "when set to 1" do
+          let(:attributes) { valid_attributes.merge(field_125: "1") }
+
+          it "does not add a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_125]).to be_blank
+          end
+
+          it "sets has_mscharge to yes and mscharge to the value" do
+            log = parser.log
+            expect(log["has_mscharge"]).to eq(1)
+            expect(log["mscharge"]).to eq(1)
+          end
+        end
+
+        context "when set to 0" do
+          let(:attributes) { valid_attributes.merge(field_125: "0") }
+
+          it "does not add a bulk upload format validation error but adds a site validation error" do
+            parser.valid?
+            expect(parser.errors[:field_125]).not_to include(I18n.t("validations.sales.2026.bulk_upload.mscharge.invalid"))
+            expect(parser.errors[:field_125]).to include(I18n.t("validations.sales.financial.mscharge.monthly_leasehold_charges.not_zero"))
+          end
+
+          it "sets has_mscharge to yes and mscharge to the value" do
+            log = parser.log
+            expect(log["has_mscharge"]).to eq(1)
+            expect(log["mscharge"]).to eq(0)
+          end
+        end
+
+        context "when set to 0.0" do
+          let(:attributes) { valid_attributes.merge(field_125: "0.0") }
+
+          it "does not add a bulk upload format validation error but adds a site validation error" do
+            parser.valid?
+            expect(parser.errors[:field_125]).not_to include(I18n.t("validations.sales.2026.bulk_upload.mscharge.invalid"))
+            expect(parser.errors[:field_125]).to include(I18n.t("validations.sales.financial.mscharge.monthly_leasehold_charges.not_zero"))
+          end
+
+          it "sets has_mscharge to yes and mscharge to the value" do
+            log = parser.log
+            expect(log["has_mscharge"]).to eq(1)
+            expect(log["mscharge"]).to eq(0)
+          end
+        end
+
+        context "when set to R" do
+          let(:attributes) { valid_attributes.merge(field_125: "R") }
+
+          it "does not add a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_125]).to be_blank
+          end
+
+          it "sets has_mscharge to no and does not set mscharge" do
+            log = parser.log
+            expect(log["has_mscharge"]).to eq(0)
+            expect(log["mscharge"]).to be_nil
+          end
+        end
+
+        context "when set to lowercase r" do
+          let(:attributes) { valid_attributes.merge(field_125: "r") }
+
+          it "does not add a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_125]).to be_blank
+          end
+
+          it "sets has_mscharge to no and does not set mscharge" do
+            log = parser.log
+            expect(log["has_mscharge"]).to eq(0)
+            expect(log["mscharge"]).to be_nil
+          end
+        end
+
+        context "when an invalid string" do
+          let(:attributes) { valid_attributes.merge(field_125: "X") }
+
+          it "adds a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_125]).to include(I18n.t("validations.sales.2026.bulk_upload.mscharge.invalid"))
+          end
+
+          it "does not set has_mscharge or mscharge" do
+            log = parser.log
+            expect(log["has_mscharge"]).to be_nil
+            expect(log["mscharge"]).to be_nil
+          end
+        end
+
+        context "when blank" do
+          let(:attributes) { valid_attributes.merge(field_125: nil) }
+
+          it "does not add a bulk upload format validation error but adds a site validation error" do
+            parser.valid?
+            expect(parser.errors[:field_125]).not_to include(I18n.t("validations.sales.2026.bulk_upload.mscharge.invalid"))
+            expect(parser.errors[:field_125]).to include("You must answer property service charges.")
+          end
+
+          it "does not set has_mscharge or mscharge" do
+            log = parser.log
+            expect(log["has_mscharge"]).to be_nil
+            expect(log["mscharge"]).to be_nil
+          end
+        end
+
+        context "when negative" do
+          let(:attributes) { valid_attributes.merge(field_125: "-100") }
+
+          it "adds a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_125]).to include(I18n.t("validations.sales.2026.bulk_upload.mscharge.invalid"))
+          end
+
+          it "does not set has_mscharge or mscharge" do
+            log = parser.log
+            expect(log["has_mscharge"]).to be_nil
+            expect(log["mscharge"]).to be_nil
+          end
+        end
+      end
+
+      context "with mscharge for discounted ownership (field_136)" do
+        context "when positive" do
+          let(:attributes) { valid_attributes.merge(field_8: "2", field_136: "100") }
+
+          it "does not add a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_136]).to be_blank
+          end
+
+          it "sets has_mscharge to yes and mscharge to the value" do
+            log = parser.log
+            expect(log["has_mscharge"]).to eq(1)
+            expect(log["mscharge"]).to eq(100)
+          end
+        end
+
+        context "when set to 1" do
+          let(:attributes) { valid_attributes.merge(field_8: "2", field_136: "1") }
+
+          it "does not add a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_136]).to be_blank
+          end
+
+          it "sets has_mscharge to yes and mscharge to the value" do
+            log = parser.log
+            expect(log["has_mscharge"]).to eq(1)
+            expect(log["mscharge"]).to eq(1)
+          end
+        end
+
+        context "when set to 0" do
           let(:attributes) { valid_attributes.merge(field_8: "2", field_136: "0") }
 
-      it "does not override variables correctly" do
+          it "does not add a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_136]).to be_blank
+          end
+
+          it "sets has_mscharge to yes and mscharge to the value" do
+            log = parser.log
+            expect(log["has_mscharge"]).to eq(1)
+            expect(log["mscharge"]).to eq(0)
+          end
+        end
+
+        context "when set to 0.0" do
+          let(:attributes) { valid_attributes.merge(field_8: "2", field_136: "0.0") }
+
+          it "does not add a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_136]).to be_blank
+          end
+
+          it "sets has_mscharge to yes and mscharge to the value" do
+            log = parser.log
+            expect(log["has_mscharge"]).to eq(1)
+            expect(log["mscharge"]).to eq(0)
+          end
+        end
+
+        context "when set to R" do
+          let(:attributes) { valid_attributes.merge(field_8: "2", field_136: "R") }
+
+          it "does not add a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_136]).to be_blank
+          end
+
+          it "sets has_mscharge to no and does not set mscharge" do
+            log = parser.log
+            expect(log["has_mscharge"]).to eq(0)
+            expect(log["mscharge"]).to be_nil
+          end
+        end
+
+        context "when set to lowercase r" do
+          let(:attributes) { valid_attributes.merge(field_8: "2", field_136: "r") }
+
+          it "does not add a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_136]).to be_blank
+          end
+
+          it "sets has_mscharge to no and does not set mscharge" do
+            log = parser.log
+            expect(log["has_mscharge"]).to eq(0)
+            expect(log["mscharge"]).to be_nil
+          end
+        end
+
+        context "when an invalid string" do
+          let(:attributes) { valid_attributes.merge(field_8: "2", field_136: "X") }
+
+          it "adds a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_136]).to include(I18n.t("validations.sales.2026.bulk_upload.mscharge.invalid"))
+          end
+
+          it "does not set has_mscharge or mscharge" do
+            log = parser.log
+            expect(log["has_mscharge"]).to be_nil
+            expect(log["mscharge"]).to be_nil
+          end
+        end
+
+        context "when blank" do
+          let(:attributes) { valid_attributes.merge(field_8: "2", field_136: nil) }
+
+          it "does not add a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_136]).to be_blank
+          end
+
+          it "does not set has_mscharge or mscharge" do
+            log = parser.log
+            expect(log["has_mscharge"]).to be_nil
+            expect(log["mscharge"]).to be_nil
+          end
+        end
+
+        context "when negative" do
+          let(:attributes) { valid_attributes.merge(field_8: "2", field_136: "-100") }
+
+          it "adds a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_136]).to include(I18n.t("validations.sales.2026.bulk_upload.mscharge.invalid"))
+          end
+
+          it "does not set has_mscharge or mscharge" do
             log = parser.log
-        expect(log["has_mscharge"]).to eq(0) # no
+            expect(log["has_mscharge"]).to be_nil
             expect(log["mscharge"]).to be_nil
           end
         end
+      end
+
+      context "with newservicecharges (field_126)" do
+        context "when positive" do
+          let(:attributes) { valid_attributes.merge(field_126: "150") }
+
+          it "does not add a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_126]).to be_blank
+          end
+
+          it "sets hasservicechargeschanged to yes and newservicecharges to the value" do
+            log = parser.log
+            expect(log["hasservicechargeschanged"]).to eq(1)
+            expect(log["newservicecharges"]).to eq(150)
+          end
+        end
+
+        context "when set to 0" do
+          let(:attributes) { valid_attributes.merge(field_126: "0") }
+
+          it "does not add a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_126]).to be_blank
+          end
+
+          it "sets hasservicechargeschanged to yes and newservicecharges to 0" do
+            log = parser.log
+            expect(log["hasservicechargeschanged"]).to eq(1)
+            expect(log["newservicecharges"]).to eq(0)
+          end
+        end
+
+        context "when set to 0.0" do
+          let(:attributes) { valid_attributes.merge(field_126: "0.0") }
+
+          it "does not add a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_126]).to be_blank
+          end
+
+          it "sets hasservicechargeschanged to yes and newservicecharges to 0" do
+            log = parser.log
+            expect(log["hasservicechargeschanged"]).to eq(1)
+            expect(log["newservicecharges"]).to eq(0)
+          end
+        end
+
+        context "when set to R" do
+          let(:attributes) { valid_attributes.merge(field_126: "R") }
+
+          it "does not add a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_126]).to be_blank
+          end
+
+          it "sets hasservicechargeschanged to no and does not set newservicecharges" do
+            log = parser.log
+            expect(log["hasservicechargeschanged"]).to eq(2)
+            expect(log["newservicecharges"]).to be_nil
+          end
+        end
+
+        context "when set to lowercase r" do
+          let(:attributes) { valid_attributes.merge(field_126: "r") }
+
+          it "does not add a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_126]).to be_blank
+          end
+
+          it "sets hasservicechargeschanged to no and does not set newservicecharges" do
+            log = parser.log
+            expect(log["hasservicechargeschanged"]).to eq(2)
+            expect(log["newservicecharges"]).to be_nil
+          end
+        end
+
+        context "when an invalid string" do
+          let(:attributes) { valid_attributes.merge(field_126: "X") }
+
+          it "adds a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_126]).to include(I18n.t("validations.sales.2026.bulk_upload.newservicecharges.invalid"))
+          end
+
+          it "does not set hasservicechargeschanged or newservicecharges" do
+            log = parser.log
+            expect(log["hasservicechargeschanged"]).to be_nil
+            expect(log["newservicecharges"]).to be_nil
+          end
+        end
+
+        context "when blank" do
+          let(:attributes) { valid_attributes.merge(field_126: nil) }
+
+          it "does not add a bulk upload format validation error but adds a site validation error" do
+            parser.valid?
+            expect(parser.errors[:field_126]).not_to include(I18n.t("validations.sales.2026.bulk_upload.newservicecharges.invalid"))
+            expect(parser.errors[:field_126]).to include("You must answer service charge will change.")
+          end
+
+          it "does not set hasservicechargeschanged or newservicecharges" do
+            log = parser.log
+            expect(log["hasservicechargeschanged"]).to be_nil
+            expect(log["newservicecharges"]).to be_nil
+          end
+        end
+
+        context "when negative" do
+          let(:attributes) { valid_attributes.merge(field_126: "-150") }
+
+          it "adds a validation error" do
+            parser.valid?
+            expect(parser.errors[:field_126]).to include(I18n.t("validations.sales.2026.bulk_upload.newservicecharges.invalid"))
+          end
+
+          it "does not set hasservicechargeschanged or newservicecharges" do
+            log = parser.log
+            expect(log["hasservicechargeschanged"]).to be_nil
+            expect(log["newservicecharges"]).to be_nil
+          end
+        end
+      end
+
+      context "when newservicecharges equals mscharge (field_125 == field_126)" do
+        let(:attributes) { valid_attributes.merge(field_125: "200", field_126: "200") }
+
+        it "adds validation errors to both fields" do
+          parser.valid?
+          expect(parser.errors[:field_125]).to include(I18n.t("validations.sales.financial.mscharge.same_as_new"))
+          expect(parser.errors[:field_126]).to include(I18n.t("validations.sales.financial.newservicecharges.same_as_previous"))
+        end
+      end
+    end
 
     describe "shared ownership sale type" do
       context "when 32 is selected for shared ownership type" do