diff --git a/Gemfile.lock b/Gemfile.lock index ebcdee307..f95ed70bc 100644 --- a/Gemfile.lock +++ b/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) diff --git a/app/models/form/lettings/questions/created_by_id.rb b/app/models/form/lettings/questions/created_by_id.rb index ea6375152..e4d6dfbbd 100644 --- a/app/models/form/lettings/questions/created_by_id.rb +++ b/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 - log.owning_organisation.absorbing_organisation.present? ? log.owning_organisation&.absorbing_organisation&.users&.visible : log.owning_organisation&.users&.visible - end), - ( - if log.managing_organisation - log.managing_organisation.absorbing_organisation.present? ? log.managing_organisation&.absorbing_organisation&.users&.visible : log.managing_organisation.users&.visible - end), + if log.owning_organisation + log.owning_organisation.absorbing_organisation.present? ? log.owning_organisation&.absorbing_organisation&.users : log.owning_organisation&.users + end&.visible&.activated, + if log.managing_organisation + log.managing_organisation.absorbing_organisation.present? ? log.managing_organisation&.absorbing_organisation&.users : log.managing_organisation.users + end&.visible&.activated, ].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| diff --git a/app/models/form/sales/questions/created_by_id.rb b/app/models/form/sales/questions/created_by_id.rb index 516afd2bc..500873138 100644 --- a/app/models/form/sales/questions/created_by_id.rb +++ b/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 - log.managing_organisation.absorbing_organisation.present? ? log.managing_organisation&.absorbing_organisation&.users&.visible : log.managing_organisation.users.visible - end), + if log.managing_organisation + log.managing_organisation.absorbing_organisation.present? ? log.managing_organisation&.absorbing_organisation&.users : log.managing_organisation.users + end&.visible&.activated, ].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) diff --git a/app/models/form/sales/questions/managing_organisation.rb b/app/models/form/sales/questions/managing_organisation.rb index ffe143ab5..2079f37f5 100644 --- a/app/models/form/sales/questions/managing_organisation.rb +++ b/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 diff --git a/app/models/form/sales/questions/owning_organisation_id.rb b/app/models/form/sales/questions/owning_organisation_id.rb index d4ece8554..54afdb123 100644 --- a/app/models/form/sales/questions/owning_organisation_id.rb +++ b/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 diff --git a/app/models/form/sales/questions/sale_date.rb b/app/models/form/sales/questions/sale_date.rb index 8adefd87c..a60a5eddf 100644 --- a/app/models/form/sales/questions/sale_date.rb +++ b/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 diff --git a/app/models/form/sales/subsections/setup.rb b/app/models/form/sales/subsections/setup.rb index be6c20400..42eea0026 100644 --- a/app/models/form/sales/subsections/setup.rb +++ b/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), diff --git a/app/models/user.rb b/app/models/user.rb index 1733aeb7d..b504db9bc 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/services/bulk_upload/lettings/year2024/row_parser.rb b/app/services/bulk_upload/lettings/year2024/row_parser.rb index 822550887..66d46ead5 100644 --- a/app/services/bulk_upload/lettings/year2024/row_parser.rb +++ b/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| - blank_charge_fields.each do |_blank_field, blank_charge| - errors.add(field, I18n.t("#{ERROR_BASE_KEY}.charges.missing_charges", sentence_fragment: blank_charge)) + # if there were any errors, add an error to all the remaining present charges to make this entire section invalid + if blank_charge_fields.any? + 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], diff --git a/app/services/bulk_upload/lettings/year2025/row_parser.rb b/app/services/bulk_upload/lettings/year2025/row_parser.rb index 4521fb369..0745056d2 100644 --- a/app/services/bulk_upload/lettings/year2025/row_parser.rb +++ b/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| - blank_charge_fields.each do |_blank_field, blank_charge| - errors.add(field, I18n.t("#{ERROR_BASE_KEY}.charges.missing_charges", sentence_fragment: blank_charge)) + # if there were any errors, add an error to all the remaining present charges to make this entire section invalid + if blank_charge_fields.any? + other_charge_fields.each_key do |field| + errors.add(field, I18n.t("#{ERROR_BASE_KEY}.charges.related_to_missing_charge")) end end end diff --git a/app/services/csv/sales_log_csv_service.rb b/app/services/csv/sales_log_csv_service.rb index 9f16a3f8b..0b6612a19 100644 --- a/app/services/csv/sales_log_csv_service.rb +++ b/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?(/((?= 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 diff --git a/app/services/merge/merge_organisations_service.rb b/app/services/merge/merge_organisations_service.rb index 40749b54c..f87d811a4 100644 --- a/app/services/merge/merge_organisations_service.rb +++ b/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 diff --git a/config/locales/devise.en.yml b/config/locales/devise.en.yml index 58d68497c..d5d50b5f9 100644 --- a/config/locales/devise.en.yml +++ b/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." diff --git a/config/locales/validations/lettings/2024/bulk_upload.en.yml b/config/locales/validations/lettings/2024/bulk_upload.en.yml index d0c6269e2..857ce7f57 100644 --- a/config/locales/validations/lettings/2024/bulk_upload.en.yml +++ b/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." diff --git a/config/locales/validations/lettings/2025/bulk_upload.en.yml b/config/locales/validations/lettings/2025/bulk_upload.en.yml index 0d38dc840..5612750f7 100644 --- a/config/locales/validations/lettings/2025/bulk_upload.en.yml +++ b/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." diff --git a/docs/adr/index.md b/docs/adr/index.md index 8d97c823d..2f26ec915 100644 --- a/docs/adr/index.md +++ b/docs/adr/index.md @@ -1,6 +1,6 @@ --- has_children: true -nav_order: 12 +nav_order: 13 --- # Architecture decisions diff --git a/docs/documentation_website.md b/docs/documentation_website.md index 9f29505ba..728038fc5 100644 --- a/docs/documentation_website.md +++ b/docs/documentation_website.md @@ -1,5 +1,5 @@ --- -nav_order: 13 +nav_order: 14 --- # This documentation website diff --git a/docs/exports.md b/docs/exports.md index 1588defc2..a35d7632e 100644 --- a/docs/exports.md +++ b/docs/exports.md @@ -141,6 +141,10 @@ Full exports can only be run via a **rake task**. -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. diff --git a/docs/rake.md b/docs/rake.md new file mode 100644 index 000000000..0fc28eb5c --- /dev/null +++ b/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 +``` + +## 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= +``` + +Where `` is the name of the Rake task you want to run, local equivalent would be `bundle exec rake `. + +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. diff --git a/spec/models/form/lettings/questions/created_by_id_spec.rb b/spec/models/form/lettings/questions/created_by_id_spec.rb index 1cae12396..75736ab44 100644 --- a/spec/models/form/lettings/questions/created_by_id_spec.rb +++ b/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 diff --git a/spec/models/form/sales/questions/created_by_id_spec.rb b/spec/models/form/sales/questions/created_by_id_spec.rb index 0c21a21c8..0ae984da4 100644 --- a/spec/models/form/sales/questions/created_by_id_spec.rb +++ b/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 diff --git a/spec/models/form/sales/subsections/setup_spec.rb b/spec/models/form/sales/subsections/setup_spec.rb index f93e7ef5e..87f848c91 100644 --- a/spec/models/form/sales/subsections/setup_spec.rb +++ b/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 diff --git a/spec/requests/form_controller_spec.rb b/spec/requests/form_controller_spec.rb index a6c80abc6..c5c158a92 100644 --- a/spec/requests/form_controller_spec.rb +++ b/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) diff --git a/spec/services/bulk_upload/lettings/year2024/row_parser_spec.rb b/spec/services/bulk_upload/lettings/year2024/row_parser_spec.rb index 1e3657dbe..091ac68d9 100644 --- a/spec/services/bulk_upload/lettings/year2024/row_parser_spec.rb +++ b/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_126]).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.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.related_to_missing_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 diff --git a/spec/services/bulk_upload/lettings/year2025/row_parser_spec.rb b/spec/services/bulk_upload/lettings/year2025/row_parser_spec.rb index 902985943..84f6bb2a7 100644 --- a/spec/services/bulk_upload/lettings/year2025/row_parser_spec.rb +++ b/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_125]).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.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.related_to_missing_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 diff --git a/spec/services/merge/merge_organisations_service_spec.rb b/spec/services/merge/merge_organisations_service_spec.rb index 9355a757a..2be375c17 100644 --- a/spec/services/merge/merge_organisations_service_spec.rb +++ b/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