Merge branch 'main' into CLDC-3857-Add-new-questions-to-organisation-setup

Gemfile.lock

@@ -343,7 +343,7 @@ GEM
       activesupport (>= 3.0.0)
     raabro (1.4.0)
     racc (1.8.1)
-    rack (3.1.14)
+    rack (3.1.16)
     rack-attack (6.7.0)
       rack (>= 1.0, < 4)
     rack-mini-profiler (3.3.1)

app/models/form/lettings/questions/created_by_id.rb

@@ -18,17 +18,16 @@ class Form::Lettings::Questions::CreatedById < ::Form::Question
     users = []
     users += if current_user.support?
                [
-                 (
+                 if log.owning_organisation
-                   if log.owning_organisation
+                   log.owning_organisation.absorbing_organisation.present? ? log.owning_organisation&.absorbing_organisation&.users : log.owning_organisation&.users
-                     log.owning_organisation.absorbing_organisation.present? ? log.owning_organisation&.absorbing_organisation&.users&.visible : log.owning_organisation&.users&.visible
+                 end&.visible&.activated,
-                   end),
+                 if log.managing_organisation
-                 (
+                   log.managing_organisation.absorbing_organisation.present? ? log.managing_organisation&.absorbing_organisation&.users : log.managing_organisation.users
-                   if log.managing_organisation
+                 end&.visible&.activated,
-                     log.managing_organisation.absorbing_organisation.present? ? log.managing_organisation&.absorbing_organisation&.users&.visible : log.managing_organisation.users&.visible
-                   end),
                ].flatten
              else
-               current_user.organisation.users.visible
+               # ensure data coordinators can't assign a log to an inactive user
+               current_user.organisation.users.visible.activated
              end.uniq.compact
 
     users.each_with_object(ANSWER_OPTS.dup) do |user, hsh|

app/models/form/sales/questions/created_by_id.rb

@@ -19,13 +19,13 @@ class Form::Sales::Questions::CreatedById < ::Form::Question
     users = []
     users += if current_user.support?
                [
-                 (
+                 if log.managing_organisation
-                   if log.managing_organisation
+                   log.managing_organisation.absorbing_organisation.present? ? log.managing_organisation&.absorbing_organisation&.users : log.managing_organisation.users
-                     log.managing_organisation.absorbing_organisation.present? ? log.managing_organisation&.absorbing_organisation&.users&.visible : log.managing_organisation.users.visible
+                 end&.visible&.activated,
-                   end),
                ].flatten
              else
-               log.managing_organisation.users.visible
+               # ensure data coordinators can't assign a log to an inactive user
+               log.managing_organisation.users.visible.activated
              end.uniq.compact
     users.each_with_object(ANSWER_OPTS.dup) do |user, hsh|
       hsh[user.id] = present_user(user)

app/models/form/sales/questions/managing_organisation.rb

@@ -72,5 +72,5 @@ private
     true
   end
 
-  QUESTION_NUMBER_FROM_YEAR = { 2024 => 2, 2025 => 3 }.freeze
+  QUESTION_NUMBER_FROM_YEAR = { 2024 => 2, 2025 => 2 }.freeze
 end

app/models/form/sales/questions/owning_organisation_id.rb

@@ -93,5 +93,5 @@ private
     "#{name} (inactive as of #{merge_date.to_fs(:govuk_date)})"
   end
 
-  QUESTION_NUMBER_FROM_YEAR = { 2023 => nil, 2024 => 1, 2025 => 2 }.freeze
+  QUESTION_NUMBER_FROM_YEAR = { 2023 => nil, 2024 => 1, 2025 => 1 }.freeze
 end

app/models/form/sales/questions/sale_date.rb

@@ -6,5 +6,5 @@ class Form::Sales::Questions::SaleDate < ::Form::Question
     @question_number = QUESTION_NUMBER_FROM_YEAR[form.start_date.year] || QUESTION_NUMBER_FROM_YEAR[QUESTION_NUMBER_FROM_YEAR.keys.max]
   end
 
-  QUESTION_NUMBER_FROM_YEAR = { 2023 => 1, 2024 => 3, 2025 => 1 }.freeze
+  QUESTION_NUMBER_FROM_YEAR = { 2023 => 1, 2024 => 3, 2025 => 3 }.freeze
 end

app/models/form/sales/subsections/setup.rb

@@ -7,9 +7,9 @@ class Form::Sales::Subsections::Setup < ::Form::Subsection
 
   def pages
     @pages ||= [
-      Form::Sales::Pages::SaleDate.new(nil, nil, self),
       Form::Sales::Pages::OwningOrganisation.new(nil, nil, self),
       Form::Sales::Pages::ManagingOrganisation.new(nil, nil, self),
+      Form::Sales::Pages::SaleDate.new(nil, nil, self),
       Form::Sales::Pages::CreatedBy.new(nil, nil, self),
       Form::Sales::Pages::PurchaserCode.new(nil, nil, self),
       Form::Sales::Pages::OwnershipScheme.new(nil, nil, self),

app/models/user.rb

@@ -83,6 +83,8 @@ class User < ApplicationRecord
   }
   scope :not_signed_in, -> { where(last_sign_in_at: nil, active: true) }
   scope :deactivated, -> { where(active: false) }
+  scope :activated, -> { where(active: true) }
+  # in some cases we only count the user as active if they completed the onboarding flow and signed in, rather than just being added
   scope :active_status, -> { where(active: true).where.not(last_sign_in_at: nil) }
   scope :visible, lambda { |user = nil|
     if user && !user.support?
@@ -269,7 +271,14 @@ class User < ApplicationRecord
   end
 
   def valid_for_authentication?
-    super && active?
+    super && account_is_active?
+  end
+
+  def account_is_active?
+    unless active?
+      throw(:warden, message: :inactive_account)
+    end
+    true
   end
 
   def editable_duplicate_lettings_logs_sets

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

@@ -945,9 +945,10 @@ private
       errors.add(field, I18n.t("#{ERROR_BASE_KEY}.charges.missing_charges", sentence_fragment: charge))
     end
 
-    other_charge_fields.each do |field, _charge|
+    # if there were any errors, add an error to all the remaining present charges to make this entire section invalid
-      blank_charge_fields.each do |_blank_field, blank_charge|
+    if blank_charge_fields.any?
-        errors.add(field, I18n.t("#{ERROR_BASE_KEY}.charges.missing_charges", sentence_fragment: blank_charge))
+      other_charge_fields.each_key do |field|
+        errors.add(field, I18n.t("#{ERROR_BASE_KEY}.charges.related_to_missing_charge"))
       end
     end
   end
@@ -1069,7 +1070,7 @@ private
       leftreg: %i[field_76],
       reservist: %i[field_77],
       preg_occ: %i[field_78],
-      housingneeds: %i[field_78],
+      housingneeds: %i[field_79 field_80 field_81 field_82 field_83 field_84],
 
       illness: %i[field_85],
 

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

@@ -944,9 +944,10 @@ private
       errors.add(field, I18n.t("#{ERROR_BASE_KEY}.charges.missing_charges", sentence_fragment: charge))
     end
 
-    other_charge_fields.each do |field, _charge|
+    # if there were any errors, add an error to all the remaining present charges to make this entire section invalid
-      blank_charge_fields.each do |_blank_field, blank_charge|
+    if blank_charge_fields.any?
-        errors.add(field, I18n.t("#{ERROR_BASE_KEY}.charges.missing_charges", sentence_fragment: blank_charge))
+      other_charge_fields.each_key do |field|
+        errors.add(field, I18n.t("#{ERROR_BASE_KEY}.charges.related_to_missing_charge"))
       end
     end
   end

app/services/csv/sales_log_csv_service.rb

@@ -259,6 +259,7 @@ module Csv
       ordered_questions = FormHandler.instance.ordered_questions_for_year(@year, "sales")
       ordered_questions.reject! { |q| q.id.match?(/((?<!la)_known)|(_check)|(_asked)|nationality_all_group|nationality_all_buyer2_group/) }
       ordered_questions.reject! { |q| q.id.match?(/organisation_id|created_by|assigned_to|soctenant|has_management_fee|management_fee|grant|discount|has_mscharge|mscharge|extrabor/) } if @year >= 2025
+      order_saledate_question_before_owning_organisation_question(ordered_questions)
       attributes = insert_checkbox_options(ordered_questions)
       final_attributes = insert_derived_and_related_attributes(non_question_fields + attributes)
       order_address_fields_for_support(final_attributes)
@@ -321,6 +322,21 @@ module Csv
       end
     end
 
+    # as part of CLDC-3719 it was decided to move the saledate question to be first in the form
+    # this caused issues reported in CLDC-4025 where the user only enter saledates for their active organisation
+    # we decided to move the organisation question back to being first
+    # however, we did not want to reorder the CSV export as this would disrupt existing users' data pipelines
+    # so, this function reorders questions back when exporting CSVs
+    # next year, we can remove this function as we will be reordering the csv fields anyway
+    def order_saledate_question_before_owning_organisation_question(ordered_questions)
+      saledate_question_index = ordered_questions.find_index { |q| q.id == "saledate" }
+      owning_organisation_index = ordered_questions.find_index { |q| q.id == "owning_organisation_id" }
+      if saledate_question_index && owning_organisation_index
+        saledate_question = ordered_questions.delete_at(saledate_question_index)
+        ordered_questions.insert(owning_organisation_index, saledate_question)
+      end
+    end
+
     def non_question_fields
       case @year
       when 2022

app/services/merge/merge_organisations_service.rb

@@ -26,7 +26,7 @@ class Merge::MergeOrganisationsService
       log_success_message
     rescue ActiveRecord::RecordInvalid => e
       Rails.logger.error("Organisation merge failed with: #{e.message}")
-      raise ActiveRecord::Rollback
+      raise
     end
   end
 
@@ -102,7 +102,11 @@ private
         location_to_set = scheme_to_set.locations.find_by(name: lettings_log.location&.name, postcode: lettings_log.location&.postcode)
 
         lettings_log.scheme = scheme_to_set if scheme_to_set.present?
-        lettings_log.location = location_to_set if location_to_set.present?
+        # in some cases the lettings_log location is nil even if scheme is present (they're two different questions).
+        # in certain cases the location_to_set query can find a location in the scheme with nil values for name and postcode,
+        # so we can end up setting the location to non nil.
+        # hence the extra check here
+        lettings_log.location = location_to_set if location_to_set.present? && lettings_log.location.present?
       end
       lettings_log.owning_organisation = @absorbing_organisation
       lettings_log.managing_organisation = @absorbing_organisation if lettings_log.managing_organisation == merging_organisation

config/locales/devise.en.yml

@@ -16,6 +16,7 @@ en:
       timeout: "Your session expired. Sign in again to continue."
       unauthenticated: "You need to sign in or sign up before continuing."
       unconfirmed: "You must confirm your email address before continuing."
+      inactive_account: "Your account has been deactivated. Ask a data coordinator at your organisation to reactivate your account. Contact the helpdesk if you don't know who your data coordinator is."
     mailer:
       confirmation_instructions:
         subject: "Confirmation instructions."

config/locales/validations/lettings/2024/bulk_upload.en.yml

@@ -58,6 +58,7 @@ en:
             invalid: "Select a valid nationality."
           charges:
             missing_charges: "Please enter the %{sentence_fragment}. If there is no %{sentence_fragment}, please enter '0'."
+            related_to_missing_charge: "You must enter all these fields: basic rent, service charge, personal service charge and support charge. Please enter ‘0' for any types of charges that you don’t issue."
           reasonpref:
             conflict:
               dont_know: "You cannot select 'Don't know' if any of the other reasonable preference reasons are also selected."

config/locales/validations/lettings/2025/bulk_upload.en.yml

@@ -58,6 +58,7 @@ en:
             invalid: "Select a valid nationality."
           charges:
             missing_charges: "Please enter the %{sentence_fragment}. If there is no %{sentence_fragment}, please enter '0'."
+            related_to_missing_charge: "You must enter all these fields: basic rent, service charge, personal service charge and support charge. Please enter ‘0' for any types of charges that you don’t issue."
           reasonpref:
             conflict:
               dont_know: "You cannot select 'Don't know' if any of the other reasonable preference reasons are also selected."

docs/adr/index.md

@@ -1,6 +1,6 @@
 ---
 has_children: true
-nav_order: 12
+nav_order: 13
 ---
 
 # Architecture decisions

docs/documentation_website.md

@@ -1,5 +1,5 @@
 ---
-nav_order: 13
+nav_order: 14
 ---
 
 # This documentation website

docs/exports.md

@@ -141,6 +141,10 @@ Full exports can only be run via a **rake task**.
 
 <!-- Update this section when sales exports are added, as they will affect rake tasks -->
 
-If the collection size is very large, full exports may fail due to memory issues. In such cases, it is better to batch exports into chunks of ~60,000 records and run several partial exports over multiple days. The `values_updated_at` field can help with this.
+If the collection size is very large, full exports may fail due to memory issues. In such cases:
+
+- Delete the incomplete export files from the S3 bucket.
+- It is better to batch exports into chunks of ~60,000 records and run several partial exports over multiple days. The `values_updated_at` field can help with this.
+- Rerun the task with more memory allocated, see 'Running Rake Tasks'.
 
 The simplest approach is to mark a batch of logs for export each day and allow scheduled morning exports to handle them.

docs/rake.md

@@ -0,0 +1,74 @@
+---
+nav_order: 11
+---
+
+# Running Rake Tasks
+
+On CORE, we sometimes need to run a Rake task manually on one of our deployed environments.
+
+## Rake Tasks
+
+Rake tasks are defined in the `lib/tasks` directory of a Rails application.
+
+## Running Rake Tasks locally
+
+This can be done from the command line:
+
+```bash
+bundle exec rake <task>
+```
+
+## Running Rake Tasks on CORE infrastructure
+
+### Get access to an AWS CLI
+
+TODO docs on this
+
+### Set up environment variables
+
+Set the env as appropriate:
+
+```bash
+export env=prod
+```
+
+Other options are `staging` or `review-XXXX`, where `XXXX` is the review app number.
+
+Set up the Rake Task as appropriate:
+
+```bash
+export rake_task=<task>
+```
+
+Where `<task>` is the name of the Rake task you want to run, local equivalent would be `bundle exec rake <task>`.
+
+Set up the CPU and memory requirements for the task:
+
+```bash
+export cpu_value=1024
+export memory_value=2048
+```
+
+See [the AWS documentation](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task_definition_parameters.html#task_size) for valid CPU and memory pairs.
+
+Set the other environment variables:
+
+```bash
+export cluster=core-$env-app
+export service=core-$env-app
+export ad_hoc_task_definition=core-$env-ad-hoc
+export network=$(aws ecs describe-services --cluster $cluster --services $service --query services[0].networkConfiguration)
+export overrides="{ \"containerOverrides\" : [{ \"name\" : \"app\", \"command\" : [\"bundle\", \"exec\", \"rake\", \"$rake_task\"], \"memory\" : $memory_value, \"cpu\" : $cpu_value }] }"
+```
+
+### Start the Rake Task
+
+```bash
+aws ecs run-task --cluster $cluster --task-definition $ad_hoc_task_definition --network-configuration "$network" --overrides "$overrides" --launch-type FARGATE --query tasks[0].taskArn
+```
+
+The task ARN will be printed to the console.
+
+### View the Task progress
+
+This can be viewed in the AWS console by navigating to the ECS cluster specified in the environment variables, listing all tasks in the cluster, and selecting the task with the ARN printed in the previous step.

spec/models/form/lettings/questions/created_by_id_spec.rb

@@ -59,6 +59,17 @@ RSpec.describe Form::Lettings::Questions::CreatedById, type: :model do
           expect(question.displayed_answer_options(lettings_log, support_user)).to eq(expected_option_for_users(managing_org_user.organisation.users.visible + owning_org_user.organisation.users.visible))
         end
       end
+
+      context "when organisation has inactive users" do
+        before do
+          create(:user, name: "Inactive user", active: false, organisation: owning_org_user.organisation)
+          create(:user, name: "Inactive managing user", active: false, organisation: managing_org_user.organisation)
+        end
+
+        it "does not display inactive users" do
+          expect(question.displayed_answer_options(lettings_log, support_user)).to eq(expected_option_for_users(managing_org_user.organisation.users.visible.activated + owning_org_user.organisation.users.visible.activated))
+        end
+      end
     end
   end
 
@@ -86,6 +97,16 @@ RSpec.describe Form::Lettings::Questions::CreatedById, type: :model do
           expect(question.displayed_answer_options(lettings_log, data_coordinator)).to eq(expected_option_for_users(data_coordinator.organisation.users.visible))
         end
       end
+
+      context "when organisation has inactive users" do
+        before do
+          create(:user, name: "Inactive user", active: false, organisation: data_coordinator.organisation)
+        end
+
+        it "does not display inactive users" do
+          expect(question.displayed_answer_options(lettings_log, data_coordinator)).to eq(expected_option_for_users(data_coordinator.organisation.users.visible.activated))
+        end
+      end
     end
   end
 end

spec/models/form/sales/questions/created_by_id_spec.rb

@@ -55,6 +55,16 @@ RSpec.describe Form::Sales::Questions::CreatedById, type: :model do
           expect(question.displayed_answer_options(sales_log, support_user)).to eq(expected_option_for_users(owning_org_user.organisation.users.visible))
         end
       end
+
+      context "when organisation has inactive users" do
+        before do
+          create(:user, name: "Inactive user", active: false, organisation: owning_org_user.organisation)
+        end
+
+        it "does not display inactive users" do
+          expect(question.displayed_answer_options(sales_log, support_user)).to eq(expected_option_for_users(owning_org_user.organisation.users.visible.activated))
+        end
+      end
     end
   end
 
@@ -86,6 +96,16 @@ RSpec.describe Form::Sales::Questions::CreatedById, type: :model do
           expect(question.displayed_answer_options(sales_log, data_coordinator)).to eq(expected_option_for_users(owning_org_user.organisation.users.visible))
         end
       end
+
+      context "when organisation has inactive users" do
+        before do
+          create(:user, name: "Inactive user", active: false, organisation: data_coordinator.organisation)
+        end
+
+        it "does not display deleted users" do
+          expect(question.displayed_answer_options(sales_log, data_coordinator)).to eq(expected_option_for_users(owning_org_user.organisation.users.visible.activated))
+        end
+      end
     end
   end
 end

spec/models/form/sales/subsections/setup_spec.rb

@@ -29,9 +29,9 @@ RSpec.describe Form::Sales::Subsections::Setup, type: :model do
     it "has correct pages" do
       expect(setup.pages.map(&:id)).to eq(
         %w[
-          completion_date
           owning_organisation
           managing_organisation
+          completion_date
           assigned_to
           purchaser_code
           ownership_scheme
@@ -56,9 +56,9 @@ RSpec.describe Form::Sales::Subsections::Setup, type: :model do
     it "has correct pages" do
       expect(setup.pages.map(&:id)).to eq(
         %w[
-          completion_date
           owning_organisation
           managing_organisation
+          completion_date
           assigned_to
           purchaser_code
           ownership_scheme
@@ -87,9 +87,9 @@ RSpec.describe Form::Sales::Subsections::Setup, type: :model do
     it "has correct pages" do
       expect(setup.pages.map(&:id)).to eq(
         %w[
-          completion_date
           owning_organisation
           managing_organisation
+          completion_date
           assigned_to
           purchaser_code
           ownership_scheme

spec/requests/form_controller_spec.rb

@@ -182,7 +182,7 @@ RSpec.describe FormController, type: :request do
 
       it "correctly sets owning organisation" do
         post "/sales-logs/#{sales_log.id}/owning-organisation", params: params
-        expect(response).to redirect_to("/sales-logs/#{sales_log.id}/assigned-to")
+        expect(response).to redirect_to("/sales-logs/#{sales_log.id}/completion-date")
         follow_redirect!
         sales_log.reload
         expect(sales_log.owning_organisation).to eq(managing_organisation)
@@ -209,7 +209,7 @@ RSpec.describe FormController, type: :request do
 
       it "does not reset assigned to" do
         post "/sales-logs/#{sales_log.id}/owning-organisation", params: params
-        expect(response).to redirect_to("/sales-logs/#{sales_log.id}/assigned-to")
+        expect(response).to redirect_to("/sales-logs/#{sales_log.id}/completion-date")
         follow_redirect!
         sales_log.reload
         expect(sales_log.assigned_to).to eq(assigned_to)
@@ -238,7 +238,7 @@ RSpec.describe FormController, type: :request do
 
       it "does not reset assigned to" do
         post "/sales-logs/#{sales_log.id}/owning-organisation", params: params
-        expect(response).to redirect_to("/sales-logs/#{sales_log.id}/assigned-to")
+        expect(response).to redirect_to("/sales-logs/#{sales_log.id}/completion-date")
         follow_redirect!
         sales_log.reload
         expect(sales_log.assigned_to).to eq(assigned_to)

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

@@ -2766,7 +2766,7 @@ RSpec.describe BulkUpload::Lettings::Year2024::RowParser do
             expect(parser.errors[:field_125]).to eql([I18n.t("validations.lettings.2024.bulk_upload.charges.missing_charges", sentence_fragment: "basic rent")])
             expect(parser.errors[:field_126]).to eql([I18n.t("validations.lettings.2024.bulk_upload.charges.missing_charges", sentence_fragment: "service charge")])
             expect(parser.errors[:field_127]).to eql([I18n.t("validations.lettings.2024.bulk_upload.charges.missing_charges", sentence_fragment: "personal service charge")])
-            expect(parser.errors[:field_128]).to eql([I18n.t("validations.lettings.2024.bulk_upload.charges.missing_charges", sentence_fragment: "basic rent"), I18n.t("validations.lettings.2024.bulk_upload.charges.missing_charges", sentence_fragment: "service charge"), I18n.t("validations.lettings.2024.bulk_upload.charges.missing_charges", sentence_fragment: "personal service charge")])
+            expect(parser.errors[:field_128]).to eql([I18n.t("validations.lettings.2024.bulk_upload.charges.related_to_missing_charge")])
           end
         end
       end
@@ -2787,9 +2787,9 @@ RSpec.describe BulkUpload::Lettings::Year2024::RowParser do
 
           it "adds an error to all charges" do
             parser.valid?
-            expect(parser.errors[:field_125]).to eql([I18n.t("validations.lettings.2024.bulk_upload.charges.missing_charges", sentence_fragment: "support charge")])
+            expect(parser.errors[:field_125]).to eql([I18n.t("validations.lettings.2024.bulk_upload.charges.related_to_missing_charge")])
-            expect(parser.errors[:field_126]).to eql([I18n.t("validations.lettings.2024.bulk_upload.charges.missing_charges", sentence_fragment: "support charge")])
+            expect(parser.errors[:field_126]).to eql([I18n.t("validations.lettings.2024.bulk_upload.charges.related_to_missing_charge")])
-            expect(parser.errors[:field_127]).to eql([I18n.t("validations.lettings.2024.bulk_upload.charges.missing_charges", sentence_fragment: "support charge")])
+            expect(parser.errors[:field_127]).to eql([I18n.t("validations.lettings.2024.bulk_upload.charges.related_to_missing_charge")])
             expect(parser.errors[:field_128]).to eql([I18n.t("validations.lettings.2024.bulk_upload.charges.missing_charges", sentence_fragment: "support charge")])
           end
         end

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

@@ -2496,7 +2496,7 @@ RSpec.describe BulkUpload::Lettings::Year2025::RowParser do
           expect(parser.errors[:field_124]).to eql([I18n.t("validations.lettings.2025.bulk_upload.charges.missing_charges", sentence_fragment: "basic rent")])
           expect(parser.errors[:field_125]).to eql([I18n.t("validations.lettings.2025.bulk_upload.charges.missing_charges", sentence_fragment: "service charge")])
           expect(parser.errors[:field_126]).to eql([I18n.t("validations.lettings.2025.bulk_upload.charges.missing_charges", sentence_fragment: "personal service charge")])
-          expect(parser.errors[:field_127]).to eql([I18n.t("validations.lettings.2025.bulk_upload.charges.missing_charges", sentence_fragment: "basic rent"), I18n.t("validations.lettings.2025.bulk_upload.charges.missing_charges", sentence_fragment: "service charge"), I18n.t("validations.lettings.2025.bulk_upload.charges.missing_charges", sentence_fragment: "personal service charge")])
+          expect(parser.errors[:field_127]).to eql([I18n.t("validations.lettings.2025.bulk_upload.charges.related_to_missing_charge")])
         end
       end
 
@@ -2515,9 +2515,9 @@ RSpec.describe BulkUpload::Lettings::Year2025::RowParser do
 
         it "adds an error to all charges" do
           parser.valid?
-          expect(parser.errors[:field_124]).to eql([I18n.t("validations.lettings.2025.bulk_upload.charges.missing_charges", sentence_fragment: "support charge")])
+          expect(parser.errors[:field_124]).to eql([I18n.t("validations.lettings.2025.bulk_upload.charges.related_to_missing_charge")])
-          expect(parser.errors[:field_125]).to eql([I18n.t("validations.lettings.2025.bulk_upload.charges.missing_charges", sentence_fragment: "support charge")])
+          expect(parser.errors[:field_125]).to eql([I18n.t("validations.lettings.2025.bulk_upload.charges.related_to_missing_charge")])
-          expect(parser.errors[:field_126]).to eql([I18n.t("validations.lettings.2025.bulk_upload.charges.missing_charges", sentence_fragment: "support charge")])
+          expect(parser.errors[:field_126]).to eql([I18n.t("validations.lettings.2025.bulk_upload.charges.related_to_missing_charge")])
           expect(parser.errors[:field_127]).to eql([I18n.t("validations.lettings.2025.bulk_upload.charges.missing_charges", sentence_fragment: "support charge")])
         end
       end

spec/services/merge/merge_organisations_service_spec.rb

@@ -59,7 +59,7 @@ RSpec.describe Merge::MergeOrganisationsService do
         allow(Organisation).to receive(:find).with(absorbing_organisation.id).and_return(absorbing_organisation)
         allow(absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid)
         expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid")
-        merge_organisations_service.call
+        expect { merge_organisations_service.call }.to raise_error(ActiveRecord::RecordInvalid)
 
         absorbing_organisation.reload
         merging_organisation.reload
@@ -103,7 +103,7 @@ RSpec.describe Merge::MergeOrganisationsService do
           allow(Organisation).to receive(:find).with(absorbing_organisation.id).and_return(absorbing_organisation)
           allow(absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid)
           expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid")
-          merge_organisations_service.call
+          expect { merge_organisations_service.call }.to raise_error(ActiveRecord::RecordInvalid)
 
           absorbing_organisation.reload
           merging_organisation.reload
@@ -140,7 +140,7 @@ RSpec.describe Merge::MergeOrganisationsService do
           allow(Organisation).to receive(:find).with(absorbing_organisation.id).and_return(absorbing_organisation)
           allow(absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid)
           expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid")
-          merge_organisations_service.call
+          expect { merge_organisations_service.call }.to raise_error(ActiveRecord::RecordInvalid)
 
           absorbing_organisation.reload
           expect(absorbing_organisation.child_organisations.count).to eq(3)
@@ -557,7 +557,7 @@ RSpec.describe Merge::MergeOrganisationsService do
             allow(Organisation).to receive(:find).with(absorbing_organisation.id).and_return(absorbing_organisation)
             allow(absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid)
             expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid")
-            merge_organisations_service.call
+            expect { merge_organisations_service.call }.to raise_error(ActiveRecord::RecordInvalid)
 
             absorbing_organisation.reload
             merging_organisation.reload
@@ -590,7 +590,7 @@ RSpec.describe Merge::MergeOrganisationsService do
           allow(Organisation).to receive(:find).with(absorbing_organisation.id).and_return(absorbing_organisation)
           allow(absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid)
           expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid")
-          merge_organisations_service.call
+          expect { merge_organisations_service.call }.to raise_error(ActiveRecord::RecordInvalid)
 
           absorbing_organisation.reload
           expect(absorbing_organisation.sales_logs.count).to eq(0)
@@ -652,7 +652,7 @@ RSpec.describe Merge::MergeOrganisationsService do
             allow(Organisation).to receive(:find).with(absorbing_organisation.id).and_return(absorbing_organisation)
             allow(absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid)
             expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid")
-            merge_organisations_service.call
+            expect { merge_organisations_service.call }.to raise_error(ActiveRecord::RecordInvalid)
 
             absorbing_organisation.reload
             expect(absorbing_organisation.sales_logs.count).to eq(0)
@@ -688,7 +688,7 @@ RSpec.describe Merge::MergeOrganisationsService do
             allow(Organisation).to receive(:find).with(absorbing_organisation.id).and_return(absorbing_organisation)
             allow(absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid)
             expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid")
-            merge_organisations_service.call
+            expect { merge_organisations_service.call }.to raise_error(ActiveRecord::RecordInvalid)
 
             absorbing_organisation.reload
             expect(absorbing_organisation.lettings_logs.count).to eq(0)
@@ -707,6 +707,27 @@ RSpec.describe Merge::MergeOrganisationsService do
             expect(owned_lettings_log.managing_organisation).to eq(absorbing_organisation)
           end
 
+          it "does not change the lettings log location" do
+            scheme = create(:scheme, owning_organisation: merging_organisation)
+            create(:location, scheme:, name: nil, postcode: nil)
+            # necessary to have a couple valid locations else the scheme will be invalid
+            create(:location, scheme:)
+            create(:location, scheme:)
+            incomplete_lettings_log = build(:lettings_log, scheme:, owning_organisation: merging_organisation, startdate: Time.zone.today)
+            incomplete_lettings_log.save!(validate: false)
+
+            # if the location is overwritten with the nil one above, it will fail validation
+            # since a rollback will occur incomplete_lettings_log will not change so there's nothing to verify later
+            # so instead we verify that no rollback occurs
+            expect(Rails.logger).not_to receive(:error)
+            merge_organisations_service.call
+
+            incomplete_lettings_log.reload
+
+            # also ensure it wasn't overwritten with a valid location
+            expect(incomplete_lettings_log.location).to be_nil
+          end
+
           context "with merge date in closed collection year" do
             subject(:merge_organisations_service) { described_class.new(absorbing_organisation_id: absorbing_organisation.id, merging_organisation_ids:, merge_date: Time.zone.local(2021, 3, 3)) }
 
@@ -831,7 +852,7 @@ RSpec.describe Merge::MergeOrganisationsService do
             allow(Organisation).to receive(:find).with(absorbing_organisation.id).and_return(absorbing_organisation)
             allow(absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid)
             expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid")
-            merge_organisations_service.call
+            expect { merge_organisations_service.call }.to raise_error(ActiveRecord::RecordInvalid)
 
             absorbing_organisation.reload
             merging_organisation.reload
@@ -950,7 +971,7 @@ RSpec.describe Merge::MergeOrganisationsService do
         allow(Organisation).to receive(:find).with(absorbing_organisation.id).and_return(absorbing_organisation)
         allow(absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid)
         expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid")
-        merge_organisations_service.call
+        expect { merge_organisations_service.call }.to raise_error(ActiveRecord::RecordInvalid)
 
         absorbing_organisation.reload
         merging_organisation.reload
@@ -1075,7 +1096,7 @@ RSpec.describe Merge::MergeOrganisationsService do
           allow(Organisation).to receive(:find).with(absorbing_organisation.id).and_return(absorbing_organisation)
           allow(absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid)
           expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid")
-          merge_organisations_service.call
+          expect { merge_organisations_service.call }.to raise_error(ActiveRecord::RecordInvalid)
 
           absorbing_organisation.reload
           merging_organisation.reload
@@ -1145,7 +1166,7 @@ RSpec.describe Merge::MergeOrganisationsService do
         allow(Organisation).to receive(:find).with(new_absorbing_organisation.id).and_return(new_absorbing_organisation)
         allow(new_absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid)
         expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid")
-        merge_organisations_service.call
+        expect { merge_organisations_service.call }.to raise_error(ActiveRecord::RecordInvalid)
 
         new_absorbing_organisation.reload
         merging_organisation.reload
@@ -1189,7 +1210,7 @@ RSpec.describe Merge::MergeOrganisationsService do
           allow(Organisation).to receive(:find).with(new_absorbing_organisation.id).and_return(new_absorbing_organisation)
           allow(new_absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid)
           expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid")
-          merge_organisations_service.call
+          expect { merge_organisations_service.call }.to raise_error(ActiveRecord::RecordInvalid)
 
           new_absorbing_organisation.reload
           merging_organisation.reload
@@ -1226,7 +1247,7 @@ RSpec.describe Merge::MergeOrganisationsService do
           allow(Organisation).to receive(:find).with(new_absorbing_organisation.id).and_return(new_absorbing_organisation)
           allow(new_absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid)
           expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid")
-          merge_organisations_service.call
+          expect { merge_organisations_service.call }.to raise_error(ActiveRecord::RecordInvalid)
 
           new_absorbing_organisation.reload
           expect(new_absorbing_organisation.child_organisations.count).to eq(3)
@@ -1331,7 +1352,7 @@ RSpec.describe Merge::MergeOrganisationsService do
           allow(Organisation).to receive(:find).with(new_absorbing_organisation.id).and_return(new_absorbing_organisation)
           allow(new_absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid)
           expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid")
-          merge_organisations_service.call
+          expect { merge_organisations_service.call }.to raise_error(ActiveRecord::RecordInvalid)
 
           new_absorbing_organisation.reload
           merging_organisation.reload
@@ -1363,7 +1384,7 @@ RSpec.describe Merge::MergeOrganisationsService do
           allow(Organisation).to receive(:find).with(new_absorbing_organisation.id).and_return(new_absorbing_organisation)
           allow(new_absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid)
           expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid")
-          merge_organisations_service.call
+          expect { merge_organisations_service.call }.to raise_error(ActiveRecord::RecordInvalid)
 
           new_absorbing_organisation.reload
           expect(new_absorbing_organisation.sales_logs.count).to eq(0)
@@ -1402,7 +1423,7 @@ RSpec.describe Merge::MergeOrganisationsService do
             allow(Organisation).to receive(:find).with(new_absorbing_organisation.id).and_return(new_absorbing_organisation)
             allow(new_absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid)
             expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid")
-            merge_organisations_service.call
+            expect { merge_organisations_service.call }.to raise_error(ActiveRecord::RecordInvalid)
 
             new_absorbing_organisation.reload
             expect(new_absorbing_organisation.sales_logs.count).to eq(0)
@@ -1438,7 +1459,7 @@ RSpec.describe Merge::MergeOrganisationsService do
             allow(Organisation).to receive(:find).with(new_absorbing_organisation.id).and_return(new_absorbing_organisation)
             allow(new_absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid)
             expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid")
-            merge_organisations_service.call
+            expect { merge_organisations_service.call }.to raise_error(ActiveRecord::RecordInvalid)
 
             new_absorbing_organisation.reload
             expect(new_absorbing_organisation.lettings_logs.count).to eq(0)
@@ -1544,7 +1565,7 @@ RSpec.describe Merge::MergeOrganisationsService do
             allow(Organisation).to receive(:find).with(new_absorbing_organisation.id).and_return(new_absorbing_organisation)
             allow(new_absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid)
             expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid")
-            merge_organisations_service.call
+            expect { merge_organisations_service.call }.to raise_error(ActiveRecord::RecordInvalid)
 
             new_absorbing_organisation.reload
             merging_organisation.reload
@@ -1634,7 +1655,7 @@ RSpec.describe Merge::MergeOrganisationsService do
         allow(Organisation).to receive(:find).with(new_absorbing_organisation.id).and_return(new_absorbing_organisation)
         allow(new_absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid)
         expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid")
-        merge_organisations_service.call
+        expect { merge_organisations_service.call }.to raise_error(ActiveRecord::RecordInvalid)
 
         new_absorbing_organisation.reload
         merging_organisation.reload
@@ -1676,7 +1697,7 @@ RSpec.describe Merge::MergeOrganisationsService do
           allow(Organisation).to receive(:find).with(new_absorbing_organisation.id).and_return(new_absorbing_organisation)
           allow(new_absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid)
           expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid")
-          merge_organisations_service.call
+          expect { merge_organisations_service.call }.to raise_error(ActiveRecord::RecordInvalid)
 
           new_absorbing_organisation.reload
           merging_organisation.reload