From fbb67f5fabac25241e5a76517e127427fbac0474 Mon Sep 17 00:00:00 2001 From: Arthur Campbell <51094020+arfacamble@users.noreply.github.com> Date: Thu, 2 Feb 2023 17:05:42 +0000 Subject: [PATCH 1/9] CLDC-1923 amend validation message for single ended ranges (#1253) * replace range validation with multiple to account for single-ended ranges * add test for question with min but no max * add min and max to household count question and remove bespoke validation. Remove code allowing max and no min given this situation does not seem to exist int eh form * amend minor typo, add back a soft validation that had mysteriously gone missing --- .../shared_ownership_deposit_value_check.rb | 2 +- .../questions/number_of_others_in_property.rb | 2 + .../sales/household_validations.rb | 8 ---- app/models/validations/shared_validations.rb | 20 ++++++--- config/locales/en.yml | 6 ++- ...ared_ownership_deposit_value_check_spec.rb | 2 +- .../validations/household_validations_spec.rb | 4 +- .../sales/household_validations_spec.rb | 42 ------------------- .../validations/shared_validations_spec.rb | 24 +++++++---- .../requests/lettings_logs_controller_spec.rb | 2 +- 10 files changed, 40 insertions(+), 72 deletions(-) diff --git a/app/models/form/sales/pages/shared_ownership_deposit_value_check.rb b/app/models/form/sales/pages/shared_ownership_deposit_value_check.rb index 907968028..be2126482 100644 --- a/app/models/form/sales/pages/shared_ownership_deposit_value_check.rb +++ b/app/models/form/sales/pages/shared_ownership_deposit_value_check.rb @@ -8,7 +8,7 @@ class Form::Sales::Pages::SharedOwnershipDepositValueCheck < ::Form::Page ] @informative_text = {} @title_text = { - "translation" => "soft_validations.shared_owhership_deposit.title_text", + "translation" => "soft_validations.shared_ownership_deposit.title_text", "arguments" => [ { "key" => "expected_shared_ownership_deposit_value", diff --git a/app/models/form/sales/questions/number_of_others_in_property.rb b/app/models/form/sales/questions/number_of_others_in_property.rb index 3eb1f34e6..cf590291b 100644 --- a/app/models/form/sales/questions/number_of_others_in_property.rb +++ b/app/models/form/sales/questions/number_of_others_in_property.rb @@ -7,5 +7,7 @@ class Form::Sales::Questions::NumberOfOthersInProperty < ::Form::Question @type = "numeric" @hint_text = "You can provide details for a maximum of 4 other people." @width = 2 + @min = 0 + @max = 4 end end diff --git a/app/models/validations/sales/household_validations.rb b/app/models/validations/sales/household_validations.rb index 4f2290268..f9318ab0f 100644 --- a/app/models/validations/sales/household_validations.rb +++ b/app/models/validations/sales/household_validations.rb @@ -1,14 +1,6 @@ module Validations::Sales::HouseholdValidations include Validations::SharedValidations - def validate_number_of_other_people_living_in_the_property(record) - return if record.hholdcount.blank? - - unless record.hholdcount >= 0 && record.hholdcount <= 4 - record.errors.add :hholdcount, I18n.t("validations.numeric.valid", field: "Number of other people living in the property", min: 0, max: 4) - end - end - def validate_household_number_of_other_members(record) (2..6).each do |n| validate_person_age_matches_relationship(record, n) diff --git a/app/models/validations/shared_validations.rb b/app/models/validations/shared_validations.rb index 2d7ac0c9e..8452b9a52 100644 --- a/app/models/validations/shared_validations.rb +++ b/app/models/validations/shared_validations.rb @@ -20,20 +20,16 @@ module Validations::SharedValidations next unless question.min || question.max next unless record[question.id] - field = question.check_answer_label || question.id - min = [question.prefix, number_with_delimiter(question.min, delimiter: ","), question.suffix].join("") - max = [question.prefix, number_with_delimiter(question.max, delimiter: ","), question.suffix].join("") - begin answer = Float(record.public_send("#{question.id}_before_type_cast")) rescue ArgumentError - record.errors.add question.id.to_sym, I18n.t("validations.numeric.valid", field:, min:, max:) + add_range_error(record, question) end next unless answer if (question.min && question.min > answer) || (question.max && question.max < answer) - record.errors.add question.id.to_sym, I18n.t("validations.numeric.valid", field:, min:, max:) + add_range_error(record, question) end end end @@ -108,4 +104,16 @@ private def person_is_partner?(relationship) relationship == "P" end + + def add_range_error(record, question) + field = question.check_answer_label || question.id + min = [question.prefix, number_with_delimiter(question.min, delimiter: ","), question.suffix].join("") if question.min + max = [question.prefix, number_with_delimiter(question.max, delimiter: ","), question.suffix].join("") if question.max + + if min && max + record.errors.add question.id.to_sym, I18n.t("validations.numeric.within_range", field:, min:, max:) + elsif min + record.errors.add question.id.to_sym, I18n.t("validations.numeric.above_min", field:, min:) + end + end end diff --git a/config/locales/en.yml b/config/locales/en.yml index a469aa737..22aa05214 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -131,7 +131,8 @@ en: other_field_missing: "If %{main_field_label} is other then %{other_field_label} must be provided" other_field_not_required: "%{other_field_label} must not be provided if %{main_field_label} was not other" numeric: - valid: "%{field} must be between %{min} and %{max}" + within_range: "%{field} must be between %{min} and %{max}" + above_min: "%{field} must be at least %{min}" date: invalid_date: "Enter a date in the correct format, for example 31 1 2022" outside_collection_window: "Enter a date within the current collection windows" @@ -470,9 +471,10 @@ en: title_text: "You told us the time between the start of the tenancy and the major repairs completion date is more than 2 years" void_date: title_text: "You told us the time between the start of the tenancy and the void date is more than 2 years" - shared_owhership_deposit: + shared_ownership_deposit: title_text: "Mortgage, deposit and cash discount total should equal £%{expected_shared_ownership_deposit_value}" old_persons_shared_ownership: "At least one buyer should be aged over 64 for Older persons’ shared ownership scheme" + staircase_bought_seems_high: "You said %{percentage}% was bought in this staircasing transaction, which seems high. Are you sure?" monthly_charges_over_soft_max: title_text: "The amount of monthly charges is high for this type of property and sale type" diff --git a/spec/models/form/sales/pages/shared_ownership_deposit_value_check_spec.rb b/spec/models/form/sales/pages/shared_ownership_deposit_value_check_spec.rb index afe25f4d3..893a5cb5b 100644 --- a/spec/models/form/sales/pages/shared_ownership_deposit_value_check_spec.rb +++ b/spec/models/form/sales/pages/shared_ownership_deposit_value_check_spec.rb @@ -33,7 +33,7 @@ RSpec.describe Form::Sales::Pages::SharedOwnershipDepositValueCheck, type: :mode it "has the correct title_text" do expect(page.title_text).to eq({ - "translation" => "soft_validations.shared_owhership_deposit.title_text", + "translation" => "soft_validations.shared_ownership_deposit.title_text", "arguments" => [ { "key" => "expected_shared_ownership_deposit_value", diff --git a/spec/models/validations/household_validations_spec.rb b/spec/models/validations/household_validations_spec.rb index 0d1be05ae..daa3feef1 100644 --- a/spec/models/validations/household_validations_spec.rb +++ b/spec/models/validations/household_validations_spec.rb @@ -463,14 +463,14 @@ RSpec.describe Validations::HouseholdValidations do record.hhmemb = -1 household_validator.validate_numeric_min_max(record) expect(record.errors["hhmemb"]) - .to include(match I18n.t("validations.numeric.valid", field: "Number of Household Members", min: 0, max: 8)) + .to include(match I18n.t("validations.numeric.within_range", field: "Number of Household Members", min: 0, max: 8)) end it "validates that the number of household members cannot be more than 8" do record.hhmemb = 9 household_validator.validate_numeric_min_max(record) expect(record.errors["hhmemb"]) - .to include(match I18n.t("validations.numeric.valid", field: "Number of Household Members", min: 0, max: 8)) + .to include(match I18n.t("validations.numeric.within_range", field: "Number of Household Members", min: 0, max: 8)) end it "expects that the number of other household members is between the min and max" do diff --git a/spec/models/validations/sales/household_validations_spec.rb b/spec/models/validations/sales/household_validations_spec.rb index 26d369a35..10367390e 100644 --- a/spec/models/validations/sales/household_validations_spec.rb +++ b/spec/models/validations/sales/household_validations_spec.rb @@ -5,48 +5,6 @@ RSpec.describe Validations::Sales::HouseholdValidations do let(:validator_class) { Class.new { include Validations::Sales::HouseholdValidations } } - describe "#validate_number_of_other_people_living_in_the_property" do - context "when within permitted bounds" do - let(:record) { build(:sales_log, hholdcount: 2) } - - it "does not add an error" do - household_validator.validate_number_of_other_people_living_in_the_property(record) - - expect(record.errors[:hholdcount]).not_to be_present - end - end - - context "when blank" do - let(:record) { build(:sales_log, hholdcount: nil) } - - it "does not add an error" do - household_validator.validate_number_of_other_people_living_in_the_property(record) - - expect(record.errors[:hholdcount]).not_to be_present - end - end - - context "when below lower bound" do - let(:record) { build(:sales_log, hholdcount: -1) } - - it "adds an error" do - household_validator.validate_number_of_other_people_living_in_the_property(record) - - expect(record.errors[:hholdcount]).to be_present - end - end - - context "when higher than upper bound" do - let(:record) { build(:sales_log, hholdcount: 5) } - - it "adds an error" do - household_validator.validate_number_of_other_people_living_in_the_property(record) - - expect(record.errors[:hholdcount]).to be_present - end - end - end - describe "household member validations" do let(:record) { build(:sales_log) } diff --git a/spec/models/validations/shared_validations_spec.rb b/spec/models/validations/shared_validations_spec.rb index c1d8cb3af..151050d8b 100644 --- a/spec/models/validations/shared_validations_spec.rb +++ b/spec/models/validations/shared_validations_spec.rb @@ -18,42 +18,42 @@ RSpec.describe Validations::SharedValidations do record.age1 = "random" shared_validator.validate_numeric_min_max(record) expect(record.errors["age1"]) - .to include(match I18n.t("validations.numeric.valid", field: "Lead tenant’s age", min: 16, max: 120)) + .to include(match I18n.t("validations.numeric.within_range", field: "Lead tenant’s age", min: 16, max: 120)) end it "validates that other household member ages are a number" do record.age2 = "random" shared_validator.validate_numeric_min_max(record) expect(record.errors["age2"]) - .to include(match I18n.t("validations.numeric.valid", field: "Person 2’s age", min: 1, max: 120)) + .to include(match I18n.t("validations.numeric.within_range", field: "Person 2’s age", min: 1, max: 120)) end it "validates that person 1's age is greater than 16" do record.age1 = 15 shared_validator.validate_numeric_min_max(record) expect(record.errors["age1"]) - .to include(match I18n.t("validations.numeric.valid", field: "Lead tenant’s age", min: 16, max: 120)) + .to include(match I18n.t("validations.numeric.within_range", field: "Lead tenant’s age", min: 16, max: 120)) end it "validates that other household member ages are greater than 1" do record.age2 = 0 shared_validator.validate_numeric_min_max(record) expect(record.errors["age2"]) - .to include(match I18n.t("validations.numeric.valid", field: "Person 2’s age", min: 1, max: 120)) + .to include(match I18n.t("validations.numeric.within_range", field: "Person 2’s age", min: 1, max: 120)) end it "validates that person 1's age is less than 121" do record.age1 = 121 shared_validator.validate_numeric_min_max(record) expect(record.errors["age1"]) - .to include(match I18n.t("validations.numeric.valid", field: "Lead tenant’s age", min: 16, max: 120)) + .to include(match I18n.t("validations.numeric.within_range", field: "Lead tenant’s age", min: 16, max: 120)) end it "validates that other household member ages are greater than 121" do record.age2 = 123 shared_validator.validate_numeric_min_max(record) expect(record.errors["age2"]) - .to include(match I18n.t("validations.numeric.valid", field: "Person 2’s age", min: 1, max: 120)) + .to include(match I18n.t("validations.numeric.within_range", field: "Person 2’s age", min: 1, max: 120)) end it "validates that person 1's age is between 16 and 120" do @@ -69,21 +69,27 @@ RSpec.describe Validations::SharedValidations do end end + it "adds the correct validation text when a question has a min but not a max" do + sales_record.savings = -10 + shared_validator.validate_numeric_min_max(sales_record) + expect(sales_record.errors["savings"]).to include(match I18n.t("validations.numeric.above_min", field: "Buyer’s total savings (to nearest £10) before any deposit paid", min: "£0")) + end + context "when validating percent" do it "validates that suffixes are added in the error message" do sales_record.stairbought = 150 shared_validator.validate_numeric_min_max(sales_record) expect(sales_record.errors["stairbought"]) - .to include(match I18n.t("validations.numeric.valid", field: "Percentage bought in this staircasing transaction", min: "0%", max: "100%")) + .to include(match I18n.t("validations.numeric.within_range", field: "Percentage bought in this staircasing transaction", min: "0%", max: "100%")) end end context "when validating price" do it "validates that £ prefix and , is added in the error message" do - sales_record.income1 = "random" + sales_record.income1 = -5 shared_validator.validate_numeric_min_max(sales_record) expect(sales_record.errors["income1"]) - .to include(match I18n.t("validations.numeric.valid", field: "Buyer 1’s gross annual income", min: "£0", max: "£999,999")) + .to include(match I18n.t("validations.numeric.within_range", field: "Buyer 1’s gross annual income", min: "£0", max: "£999,999")) end end end diff --git a/spec/requests/lettings_logs_controller_spec.rb b/spec/requests/lettings_logs_controller_spec.rb index 1bb0afbf9..57311b70d 100644 --- a/spec/requests/lettings_logs_controller_spec.rb +++ b/spec/requests/lettings_logs_controller_spec.rb @@ -82,7 +82,7 @@ RSpec.describe LettingsLogsController, type: :request do it "validates lettings log parameters" do json_response = JSON.parse(response.body) expect(response).to have_http_status(:unprocessable_entity) - expect(json_response["errors"]).to match_array([["offered", [I18n.t("validations.property.offered.relet_number")]], ["age1", [I18n.t("validations.numeric.valid", field: "Lead tenant’s age", min: 16, max: 120)]]]) + expect(json_response["errors"]).to match_array([["offered", [I18n.t("validations.property.offered.relet_number")]], ["age1", [I18n.t("validations.numeric.within_range", field: "Lead tenant’s age", min: 16, max: 120)]]]) end end From 4597d5b50ac66508ec63a35e3eecc7bee93aae9c Mon Sep 17 00:00:00 2001 From: Arthur Campbell <51094020+arfacamble@users.noreply.github.com> Date: Thu, 2 Feb 2023 17:05:51 +0000 Subject: [PATCH 2/9] CLDC-1828 amend copy on soft validation screen for rent value (#1251) * amend copy on soft validation screen for rent value * amend copy after PO review --- config/forms/2021_2022.json | 8 ++++---- config/forms/2022_2023.json | 32 ++++++++------------------------ config/locales/en.yml | 9 +++------ 3 files changed, 15 insertions(+), 34 deletions(-) diff --git a/config/forms/2021_2022.json b/config/forms/2021_2022.json index e47191f6e..49931ee05 100644 --- a/config/forms/2021_2022.json +++ b/config/forms/2021_2022.json @@ -8411,7 +8411,7 @@ } ], "title_text": { - "translation": "soft_validations.rent.min.title_text", + "translation": "soft_validations.rent.outside_range_title", "arguments": [ { "key": "brent", @@ -8421,7 +8421,7 @@ ] }, "informative_text": { - "translation": "soft_validations.rent.min.hint_text", + "translation": "soft_validations.rent.min_hint_text", "arguments": [ { "key": "soft_min_for_period", @@ -8463,7 +8463,7 @@ } ], "title_text": { - "translation": "soft_validations.rent.max.title_text", + "translation": "soft_validations.rent.outside_range_title", "arguments": [ { "key": "brent", @@ -8473,7 +8473,7 @@ ] }, "informative_text": { - "translation": "soft_validations.rent.max.hint_text", + "translation": "soft_validations.rent.max_hint_text", "arguments": [ { "key": "soft_max_for_period", diff --git a/config/forms/2022_2023.json b/config/forms/2022_2023.json index 7c54b67d4..ebd1bd594 100644 --- a/config/forms/2022_2023.json +++ b/config/forms/2022_2023.json @@ -8376,7 +8376,7 @@ } ], "title_text": { - "translation": "soft_validations.rent.min.title_text", + "translation": "soft_validations.rent.outside_range_title", "arguments": [ { "key": "brent", @@ -8385,16 +8385,7 @@ } ] }, - "informative_text": { - "translation": "soft_validations.rent.min.hint_text", - "arguments": [ - { - "key": "soft_min_for_period", - "label": false, - "i18n_template": "soft_min_for_period" - } - ] - }, + "informative_text": {}, "questions": { "rent_value_check": { "check_answer_label": "Total rent confirmation", @@ -8408,7 +8399,8 @@ } ] }, - "header": "Are you sure this is correct?", + "header": "This rent is lower than expected for this property type, in this area. Check:", + "hint_text": "

Are you sure this is correct?

", "type": "interruption_screen", "answer_options": { "0": { @@ -8428,7 +8420,7 @@ } ], "title_text": { - "translation": "soft_validations.rent.max.title_text", + "translation": "soft_validations.rent.outside_range_title", "arguments": [ { "key": "brent", @@ -8437,16 +8429,7 @@ } ] }, - "informative_text": { - "translation": "soft_validations.rent.max.hint_text", - "arguments": [ - { - "key": "soft_max_for_period", - "label": false, - "i18n_template": "soft_max_for_period" - } - ] - }, + "informative_text": {}, "questions": { "rent_value_check": { "check_answer_label": "Total rent confirmation", @@ -8460,7 +8443,8 @@ } ] }, - "header": "Are you sure this is correct?", + "header": "This rent is higher than expected for this property type, in this area. Check:", + "hint_text": "

Are you sure this is correct?

", "type": "interruption_screen", "answer_options": { "0": { diff --git a/config/locales/en.yml b/config/locales/en.yml index 22aa05214..63b71301c 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -448,12 +448,9 @@ en: in_soft_max_range: message: "Net income is higher than expected based on the lead tenant’s working situation. Are you sure this is correct?" rent: - min: - title_text: "You told us the rent is %{brent}" - hint_text: "The minimum rent expected for this type of property in this local authority is £%{soft_min_for_period}." - max: - title_text: "You told us the rent is %{brent}" - hint_text: "The maximum rent expected for this type of property in this local authority is £%{soft_max_for_period}." + outside_range_title: "You told us the rent is %{brent}" + min_hint_text: "The minimum rent expected for this type of property in this local authority is £%{soft_min_for_period}." + max_hint_text: "The maximum rent expected for this type of property in this local authority is £%{soft_max_for_period}." retirement: min: title: "You told us this person is under %{age} and retired" From 07997ac0eb968cc2085176a5de731432d47aa575 Mon Sep 17 00:00:00 2001 From: Jack <113976590+bibblobcode@users.noreply.github.com> Date: Fri, 3 Feb 2023 09:05:56 +0000 Subject: [PATCH 3/9] CLDC-1931 Update financial year validation message (#1266) --- config/locales/en.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/config/locales/en.yml b/config/locales/en.yml index 63b71301c..2321446bd 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -149,7 +149,7 @@ en: intermediate_rent_product_name: blank: "Enter name of other intermediate rent product" saledate: - financial_year: "Date must be from 22/23 financial year" + financial_year: "Date must be from 22/23 financial year, which is between 1st April 2022 and 31st March 2023" startdate: later_than_14_days_after: "The tenancy start date must not be later than 14 days from today’s date" before_scheme_end_date: "The tenancy start date must be before the end date for this supported housing scheme" @@ -377,7 +377,7 @@ en: no_choices: "You cannot answer this question as you told us nobody in the household has a physical or mental health condition (or other illness) expected to last 12 months or more" postcode: discounted_ownership: "Last settled accommodation and discounted ownership property postcodes must match" - + tenancy: length: fixed_term_not_required: "You must only answer the length of the tenancy if it's fixed-term" @@ -390,7 +390,7 @@ en: declaration: missing: "You must show the DLUHC privacy notice to the tenant before you can submit this log." - + privacynotice: missing: "You must show the DLUHC privacy notice to the buyer before you can submit this log." @@ -472,7 +472,7 @@ en: title_text: "Mortgage, deposit and cash discount total should equal £%{expected_shared_ownership_deposit_value}" old_persons_shared_ownership: "At least one buyer should be aged over 64 for Older persons’ shared ownership scheme" staircase_bought_seems_high: "You said %{percentage}% was bought in this staircasing transaction, which seems high. Are you sure?" - monthly_charges_over_soft_max: + monthly_charges_over_soft_max: title_text: "The amount of monthly charges is high for this type of property and sale type" devise: From d6a5f6718e0396cc22f941eb5350973a4fff5433 Mon Sep 17 00:00:00 2001 From: Jack <113976590+bibblobcode@users.noreply.github.com> Date: Fri, 3 Feb 2023 09:08:19 +0000 Subject: [PATCH 4/9] CLDC 1865 Refactor questions to avoid race conditions (#1255) * Extract time helper methods * Add missing specs for BulkUploadController#start * refactor ManagingOrganisation question * refactor StockOwner question --- app/helpers/collection_time_helper.rb | 16 +++++++ .../questions/managing_organisation.rb | 44 ++++++++----------- .../form/lettings/questions/stock_owner.rb | 42 +++++++----------- app/models/form_handler.rb | 16 +------ spec/helpers/collection_time_helper_spec.rb | 34 ++++++++++++++ .../questions/managing_organisation_spec.rb | 32 ++++++++++++++ spec/models/form_handler_spec.rb | 12 ----- spec/requests/bulk_upload_controller_spec.rb | 42 ++++++++++++++++++ 8 files changed, 158 insertions(+), 80 deletions(-) create mode 100644 app/helpers/collection_time_helper.rb create mode 100644 spec/helpers/collection_time_helper_spec.rb diff --git a/app/helpers/collection_time_helper.rb b/app/helpers/collection_time_helper.rb new file mode 100644 index 000000000..8478d06e7 --- /dev/null +++ b/app/helpers/collection_time_helper.rb @@ -0,0 +1,16 @@ +module CollectionTimeHelper + def current_collection_start_year + today = Time.zone.now + window_end_date = Time.zone.local(today.year, 4, 1) + today < window_end_date ? today.year - 1 : today.year + end + + def collection_start_date(date) + window_end_date = Time.zone.local(date.year, 4, 1) + date < window_end_date ? Time.zone.local(date.year - 1, 4, 1) : Time.zone.local(date.year, 4, 1) + end + + def current_collection_start_date + Time.zone.local(current_collection_start_year, 4, 1) + end +end diff --git a/app/models/form/lettings/questions/managing_organisation.rb b/app/models/form/lettings/questions/managing_organisation.rb index e2e3b3e3d..bd9cbb8b9 100644 --- a/app/models/form/lettings/questions/managing_organisation.rb +++ b/app/models/form/lettings/questions/managing_organisation.rb @@ -1,48 +1,45 @@ class Form::Lettings::Questions::ManagingOrganisation < ::Form::Question - attr_accessor :current_user, :log - def initialize(id, hsh, page) super @id = "managing_organisation_id" @check_answer_label = "Managing agent" @header = "Which organisation manages this letting?" @type = "select" - @answer_options = answer_options end - def answer_options + def answer_options(log = nil, user = nil) opts = { "" => "Select an option" } return opts unless ActiveRecord::Base.connected? - return opts unless current_user + return opts unless user return opts unless log if log.managing_organisation.present? opts = opts.merge({ log.managing_organisation.id => log.managing_organisation.name }) end - if current_user.support? + if user.support? if log.owning_organisation.holds_own_stock? opts[log.owning_organisation.id] = "#{log.owning_organisation.name} (Owning organisation)" end else - opts[current_user.organisation.id] = "#{current_user.organisation.name} (Your organisation)" + opts[user.organisation.id] = "#{user.organisation.name} (Your organisation)" end - opts.merge(managing_organisations_answer_options) + orgs = if user.support? + log.owning_organisation + else + user.organisation + end.managing_agents.pluck(:id, :name).to_h + + opts.merge(orgs) end def displayed_answer_options(log, user) - @current_user = user - @log = log - - answer_options + answer_options(log, user) end - def label_from_value(value, log = nil, user = nil) - @log = log - @current_user = user - + def label_from_value(value, _log = nil, _user = nil) return unless value answer_options[value] @@ -53,25 +50,20 @@ class Form::Lettings::Questions::ManagingOrganisation < ::Form::Question end def hidden_in_check_answers?(log, user = nil) - @current_user = user - @current_user.nil? || !@page.routed_to?(log, user) + user.nil? || !@page.routed_to?(log, user) end def enabled true end + def answer_label(log, _current_user = nil) + Organisation.find_by(id: log.managing_organisation_id)&.name + end + private def selected_answer_option_is_derived?(_log) true end - - def managing_organisations_answer_options - if current_user.support? - log.owning_organisation - else - current_user.organisation - end.managing_agents.pluck(:id, :name).to_h - end end diff --git a/app/models/form/lettings/questions/stock_owner.rb b/app/models/form/lettings/questions/stock_owner.rb index e2d2a633d..531341739 100644 --- a/app/models/form/lettings/questions/stock_owner.rb +++ b/app/models/form/lettings/questions/stock_owner.rb @@ -1,6 +1,4 @@ class Form::Lettings::Questions::StockOwner < ::Form::Question - attr_accessor :current_user, :log - def initialize(id, hsh, page) super @id = "owning_organisation_id" @@ -9,38 +7,38 @@ class Form::Lettings::Questions::StockOwner < ::Form::Question @type = "select" end - def answer_options + def answer_options(log = nil, user = nil) answer_opts = { "" => "Select an option" } return answer_opts unless ActiveRecord::Base.connected? - return answer_opts unless current_user + return answer_opts unless user return answer_opts unless log if log.owning_organisation_id.present? answer_opts = answer_opts.merge({ log.owning_organisation.id => log.owning_organisation.name }) end - if !current_user.support? && current_user.organisation.holds_own_stock? - answer_opts[current_user.organisation.id] = "#{current_user.organisation.name} (Your organisation)" + if !user.support? && user.organisation.holds_own_stock? + answer_opts[user.organisation.id] = "#{user.organisation.name} (Your organisation)" end + stock_owners_answer_options = if user.support? + Organisation + else + user.organisation.stock_owners + end.pluck(:id, :name).to_h + answer_opts.merge(stock_owners_answer_options) end def displayed_answer_options(log, user = nil) - @current_user = user - @log = log - - answer_options + answer_options(log, user) end def label_from_value(value, log = nil, user = nil) - @log = log - @current_user = user - return unless value - answer_options[value] + answer_options(log, user)[value] end def derived? @@ -48,13 +46,11 @@ class Form::Lettings::Questions::StockOwner < ::Form::Question end def hidden_in_check_answers?(_log, user = nil) - @current_user = user + return false if user.support? - return false if current_user.support? + stock_owners = user.organisation.stock_owners - stock_owners = current_user.organisation.stock_owners - - if current_user.organisation.holds_own_stock? + if user.organisation.holds_own_stock? stock_owners.count.zero? else stock_owners.count <= 1 @@ -70,12 +66,4 @@ private def selected_answer_option_is_derived?(_log) true end - - def stock_owners_answer_options - if current_user.support? - Organisation - else - current_user.organisation.stock_owners - end.pluck(:id, :name).to_h - end end diff --git a/app/models/form_handler.rb b/app/models/form_handler.rb index 51a0b6177..098a7b429 100644 --- a/app/models/form_handler.rb +++ b/app/models/form_handler.rb @@ -1,5 +1,6 @@ class FormHandler include Singleton + include CollectionTimeHelper attr_reader :forms def initialize @@ -44,21 +45,6 @@ class FormHandler forms end - def current_collection_start_year - today = Time.zone.now - window_end_date = Time.zone.local(today.year, 4, 1) - today < window_end_date ? today.year - 1 : today.year - end - - def collection_start_date(date) - window_end_date = Time.zone.local(date.year, 4, 1) - date < window_end_date ? Time.zone.local(date.year - 1, 4, 1) : Time.zone.local(date.year, 4, 1) - end - - def current_collection_start_date - Time.zone.local(current_collection_start_year, 4, 1) - end - def form_name_from_start_year(year, type) form_mappings = { 0 => "current_#{type}", 1 => "previous_#{type}", -1 => "next_#{type}" } form_mappings[current_collection_start_year - year] diff --git a/spec/helpers/collection_time_helper_spec.rb b/spec/helpers/collection_time_helper_spec.rb new file mode 100644 index 000000000..3b02802f2 --- /dev/null +++ b/spec/helpers/collection_time_helper_spec.rb @@ -0,0 +1,34 @@ +require "rails_helper" + +RSpec.describe CollectionTimeHelper do + let(:current_user) { create(:user, :data_coordinator) } + let(:user) { create(:user, :data_coordinator) } + + around do |example| + Timecop.freeze(now) do + example.run + end + end + + describe "Current collection start year" do + context "when the date is after 1st of April" do + let(:now) { Time.utc(2022, 8, 3) } + + it "returns the same year as the current start year" do + expect(current_collection_start_year).to eq(2022) + end + + it "returns the correct current start date" do + expect(current_collection_start_date).to eq(Time.zone.local(2022, 4, 1)) + end + end + + context "with the date before 1st of April" do + let(:now) { Time.utc(2022, 2, 3) } + + it "returns the previous year as the current start year" do + expect(current_collection_start_year).to eq(2021) + end + end + end +end diff --git a/spec/models/form/lettings/questions/managing_organisation_spec.rb b/spec/models/form/lettings/questions/managing_organisation_spec.rb index 47cd30458..1a64ee973 100644 --- a/spec/models/form/lettings/questions/managing_organisation_spec.rb +++ b/spec/models/form/lettings/questions/managing_organisation_spec.rb @@ -199,4 +199,36 @@ RSpec.describe Form::Lettings::Questions::ManagingOrganisation, type: :model do end end end + + describe "#answer_label" do + context "when answered" do + let(:managing_organisation) { create(:organisation) } + let(:log) { create(:lettings_log, managing_organisation:) } + + it "returns org name" do + expect(question.answer_label(log)).to eq(managing_organisation.name) + end + end + + context "when unanswered" do + let(:log) { create(:lettings_log, managing_organisation: nil) } + + it "returns nil" do + expect(question.answer_label(log)).to be_nil + end + end + + context "when org does not exist" do + let(:managing_organisation) { create(:organisation) } + let(:log) { create(:lettings_log, managing_organisation:) } + + before do + allow(Organisation).to receive(:find_by).and_return(nil) + end + + it "returns nil" do + expect(question.answer_label(log)).to be_nil + end + end + end end diff --git a/spec/models/form_handler_spec.rb b/spec/models/form_handler_spec.rb index c09dbb998..ce9c66e85 100644 --- a/spec/models/form_handler_spec.rb +++ b/spec/models/form_handler_spec.rb @@ -76,10 +76,6 @@ RSpec.describe FormHandler do context "when the date is after 1st of April" do let(:now) { Time.utc(2022, 8, 3) } - it "returns the same year as the current start year" do - expect(form_handler.current_collection_start_year).to eq(2022) - end - it "returns the correct current lettings form name" do expect(form_handler.form_name_from_start_year(2022, "lettings")).to eq("current_lettings") end @@ -103,19 +99,11 @@ RSpec.describe FormHandler do it "returns the correct next sales form name" do expect(form_handler.form_name_from_start_year(2023, "sales")).to eq("next_sales") end - - it "returns the correct current start date" do - expect(form_handler.current_collection_start_date).to eq(Time.zone.local(2022, 4, 1)) - end end context "with the date before 1st of April" do let(:now) { Time.utc(2022, 2, 3) } - it "returns the previous year as the current start year" do - expect(form_handler.current_collection_start_year).to eq(2021) - end - it "returns the correct current lettings form name" do expect(form_handler.form_name_from_start_year(2021, "lettings")).to eq("current_lettings") end diff --git a/spec/requests/bulk_upload_controller_spec.rb b/spec/requests/bulk_upload_controller_spec.rb index fc3afd21c..7aa0d16e0 100644 --- a/spec/requests/bulk_upload_controller_spec.rb +++ b/spec/requests/bulk_upload_controller_spec.rb @@ -13,6 +13,14 @@ RSpec.describe BulkUploadController, type: :request do end context "when a user is not signed in" do + describe "GET #start" do + before { get start_bulk_upload_lettings_logs_path, headers:, params: {} } + + it "does not let you see the bulk upload page" do + expect(response).to redirect_to("/account/sign-in") + end + end + describe "GET #show" do before { get url, headers:, params: {} } @@ -50,6 +58,40 @@ RSpec.describe BulkUploadController, type: :request do end end + describe "GET #start" do + before do + Timecop.freeze(time) + get start_bulk_upload_lettings_logs_path + end + + after do + Timecop.unfreeze + end + + context "when not crossover period" do + let(:time) { Time.utc(2022, 2, 8) } + + it "redirects to bulk upload path" do + expect(request).to redirect_to( + bulk_upload_lettings_log_path( + id: "prepare-your-file", + form: { year: 2021 }, + ), + ) + end + end + + context "when crossover period" do + let(:time) { Time.utc(2022, 6, 8) } + + it "redirects to bulk upload path" do + expect(request).to redirect_to( + bulk_upload_lettings_log_path(id: "year"), + ) + end + end + end + describe "POST #bulk upload" do context "with a valid file based on the upload template" do let(:request) { post url, params: { bulk_upload: { lettings_log_bulk_upload: valid_file } } } From 45ed2e6ea6c613b94ec1c1d89dbd79a9d7bce986 Mon Sep 17 00:00:00 2001 From: Jack <113976590+bibblobcode@users.noreply.github.com> Date: Fri, 3 Feb 2023 09:09:46 +0000 Subject: [PATCH 5/9] CLDC-1849 Add hint to discounted ownership purchase price question (#1250) * CLDC-1849 Add hint to discounted ownership purchase price * Use PurchasePriceDiscountedOwnership in rtb page * show question with updated hint to certain types of answers * rubocop * Only display shared ownership hint in some occations --- .../form/sales/pages/about_price_not_rtb.rb | 2 +- .../form/sales/pages/about_price_rtb.rb | 2 +- app/models/form/sales/pages/purchase_price.rb | 3 +- .../purchase_price_outright_ownership.rb | 14 ++++++++ .../form/sales/questions/purchase_price.rb | 1 + .../purchase_price_outright_ownership.rb | 12 +++++++ .../discounted_ownership_scheme.rb | 1 + .../form/sales/subsections/outright_sale.rb | 2 +- app/models/sales_log.rb | 4 +++ app/views/form/page.html.erb | 21 +++++++++-- .../purchase_price_outright_ownership_spec.rb | 35 +++++++++++++++++++ .../form/sales/pages/purchase_price_spec.rb | 3 +- .../sales/questions/purchase_price_spec.rb | 4 ++- .../discounted_ownership_scheme_spec.rb | 2 ++ spec/models/form_handler_spec.rb | 4 +-- 15 files changed, 98 insertions(+), 12 deletions(-) create mode 100644 app/models/form/sales/pages/purchase_price_outright_ownership.rb create mode 100644 app/models/form/sales/questions/purchase_price_outright_ownership.rb create mode 100644 spec/models/form/sales/pages/purchase_price_outright_ownership_spec.rb diff --git a/app/models/form/sales/pages/about_price_not_rtb.rb b/app/models/form/sales/pages/about_price_not_rtb.rb index ce55d6cd8..c69b9a890 100644 --- a/app/models/form/sales/pages/about_price_not_rtb.rb +++ b/app/models/form/sales/pages/about_price_not_rtb.rb @@ -11,7 +11,7 @@ class Form::Sales::Pages::AboutPriceNotRtb < ::Form::Page def questions @questions ||= [ - Form::Sales::Questions::Value.new(nil, nil, self), + Form::Sales::Questions::PurchasePrice.new(nil, nil, self), Form::Sales::Questions::Grant.new(nil, nil, self), ] end diff --git a/app/models/form/sales/pages/about_price_rtb.rb b/app/models/form/sales/pages/about_price_rtb.rb index ff338f6f6..35696e966 100644 --- a/app/models/form/sales/pages/about_price_rtb.rb +++ b/app/models/form/sales/pages/about_price_rtb.rb @@ -10,7 +10,7 @@ class Form::Sales::Pages::AboutPriceRtb < ::Form::Page def questions @questions ||= [ - Form::Sales::Questions::Value.new(nil, nil, self), + Form::Sales::Questions::PurchasePrice.new(nil, nil, self), Form::Sales::Questions::Discount.new(nil, nil, self), ] end diff --git a/app/models/form/sales/pages/purchase_price.rb b/app/models/form/sales/pages/purchase_price.rb index dc9f7576e..af13fc682 100644 --- a/app/models/form/sales/pages/purchase_price.rb +++ b/app/models/form/sales/pages/purchase_price.rb @@ -2,8 +2,7 @@ class Form::Sales::Pages::PurchasePrice < ::Form::Page def initialize(id, hsh, subsection) super @depends_on = [ - { "ownershipsch" => 3 }, - { "rent_to_buy_full_ownership?" => true }, + { "ownershipsch" => 2, "rent_to_buy_full_ownership?" => false }, ] end diff --git a/app/models/form/sales/pages/purchase_price_outright_ownership.rb b/app/models/form/sales/pages/purchase_price_outright_ownership.rb new file mode 100644 index 000000000..6bf044d01 --- /dev/null +++ b/app/models/form/sales/pages/purchase_price_outright_ownership.rb @@ -0,0 +1,14 @@ +class Form::Sales::Pages::PurchasePriceOutrightOwnership < ::Form::Page + def initialize(id, hsh, subsection) + super + @depends_on = [ + { "outright_sale_or_discounted_with_full_ownership?" => true }, + ] + end + + def questions + @questions ||= [ + Form::Sales::Questions::PurchasePriceOutrightOwnership.new(nil, nil, self), + ] + end +end diff --git a/app/models/form/sales/questions/purchase_price.rb b/app/models/form/sales/questions/purchase_price.rb index af7e8afb9..6242e45e8 100644 --- a/app/models/form/sales/questions/purchase_price.rb +++ b/app/models/form/sales/questions/purchase_price.rb @@ -8,5 +8,6 @@ class Form::Sales::Questions::PurchasePrice < ::Form::Question @min = 0 @width = 5 @prefix = "£" + @hint_text = "For all schemes, including Right to Acquire (RTA), Right to Buy (RTB), Voluntary Right to Buy (VRTB) or Preserved Right to Buy (PRTB) sales, enter the full price of the property without any discount" end end diff --git a/app/models/form/sales/questions/purchase_price_outright_ownership.rb b/app/models/form/sales/questions/purchase_price_outright_ownership.rb new file mode 100644 index 000000000..9824a4629 --- /dev/null +++ b/app/models/form/sales/questions/purchase_price_outright_ownership.rb @@ -0,0 +1,12 @@ +class Form::Sales::Questions::PurchasePriceOutrightOwnership < ::Form::Question + def initialize(id, hsh, page) + super + @id = "value" + @check_answer_label = "Purchase price" + @header = "What is the full purchase price?" + @type = "numeric" + @min = 0 + @width = 5 + @prefix = "£" + end +end diff --git a/app/models/form/sales/subsections/discounted_ownership_scheme.rb b/app/models/form/sales/subsections/discounted_ownership_scheme.rb index 6070afda6..499411ffc 100644 --- a/app/models/form/sales/subsections/discounted_ownership_scheme.rb +++ b/app/models/form/sales/subsections/discounted_ownership_scheme.rb @@ -14,6 +14,7 @@ class Form::Sales::Subsections::DiscountedOwnershipScheme < ::Form::Subsection Form::Sales::Pages::AboutPriceNotRtb.new(nil, nil, self), Form::Sales::Pages::GrantValueCheck.new(nil, nil, self), Form::Sales::Pages::PurchasePrice.new("purchase_price_discounted_ownership", nil, self), + Form::Sales::Pages::PurchasePriceOutrightOwnership.new("purchase_price_outright_ownership", nil, self), Form::Sales::Pages::DepositAndMortgageValueCheck.new("discounted_ownership_deposit_and_mortgage_value_check_after_value_and_discount", nil, self), Form::Sales::Pages::Mortgageused.new("mortgage_used_discounted_ownership", nil, self), Form::Sales::Pages::MortgageValueCheck.new("discounted_ownership_mortgage_used_mortgage_value_check", nil, self), diff --git a/app/models/form/sales/subsections/outright_sale.rb b/app/models/form/sales/subsections/outright_sale.rb index 5d7d05d10..69f77c4b2 100644 --- a/app/models/form/sales/subsections/outright_sale.rb +++ b/app/models/form/sales/subsections/outright_sale.rb @@ -8,7 +8,7 @@ class Form::Sales::Subsections::OutrightSale < ::Form::Subsection def pages @pages ||= [ - Form::Sales::Pages::PurchasePrice.new("purchase_price_outright_sale", nil, self), + Form::Sales::Pages::PurchasePriceOutrightOwnership.new("purchase_price_outright_sale", nil, self), Form::Sales::Pages::Mortgageused.new("mortgage_used_outright_sale", nil, self), Form::Sales::Pages::MortgageValueCheck.new("outright_sale_mortgage_used_mortgage_value_check", nil, self), Form::Sales::Pages::MortgageAmount.new("mortgage_amount_outright_sale", nil, self), diff --git a/app/models/sales_log.rb b/app/models/sales_log.rb index 1ca6ee8a1..b217ff69c 100644 --- a/app/models/sales_log.rb +++ b/app/models/sales_log.rb @@ -132,6 +132,10 @@ class SalesLog < Log type == 29 end + def outright_sale_or_discounted_with_full_ownership? + ownershipsch == 3 || (ownershipsch == 2 && rent_to_buy_full_ownership?) + end + def is_type_discount? type == 18 end diff --git a/app/views/form/page.html.erb b/app/views/form/page.html.erb index c7b18cdda..6d9d44baa 100644 --- a/app/views/form/page.html.erb +++ b/app/views/form/page.html.erb @@ -34,9 +34,26 @@ <%= govuk_section_break(visible: true, size: "m") %> <% end %> <% if question.type == "interruption_screen" %> - <%= render partial: "form/#{question.type}_question", locals: { question:, caption_text: @subsection.label, page_header: @page.header, lettings_log: @log, title_text: @page.title_text, informative_text: @page.informative_text, form: @form, f:, conditional: false } %> + <%= render partial: "form/#{question.type}_question", locals: { + question:, + caption_text: @subsection.label, + page_header: @page.header, + lettings_log: @log, + title_text: @page.title_text, + informative_text: @page.informative_text, + form: @form, + f:, + conditional: false, + } %> <% else %> - <%= render partial: "form/#{question.type}_question", locals: { question:, caption_text: @page.header_partial.present? ? nil : @subsection.label, page_header: @page.header, lettings_log: @log, f:, conditional: false } %> + <%= render partial: "form/#{question.type}_question", locals: { + question:, + caption_text: @page.header_partial.present? ? nil : @subsection.label, + page_header: @page.header, + lettings_log: @log, + f:, + conditional: false, + } %> <% end %> <% end %> diff --git a/spec/models/form/sales/pages/purchase_price_outright_ownership_spec.rb b/spec/models/form/sales/pages/purchase_price_outright_ownership_spec.rb new file mode 100644 index 000000000..88c357034 --- /dev/null +++ b/spec/models/form/sales/pages/purchase_price_outright_ownership_spec.rb @@ -0,0 +1,35 @@ +require "rails_helper" + +RSpec.describe Form::Sales::Pages::PurchasePriceOutrightOwnership, type: :model do + subject(:page) { described_class.new(page_id, page_definition, subsection) } + + let(:page_id) { "purchase_price" } + let(:page_definition) { nil } + let(:subsection) { instance_double(Form::Subsection) } + + it "has correct subsection" do + expect(page.subsection).to eq(subsection) + end + + it "has correct questions" do + expect(page.questions.map(&:id)).to eq(%w[value]) + end + + it "has the correct id" do + expect(page.id).to eq("purchase_price") + end + + it "has the correct header" do + expect(page.header).to be_nil + end + + it "has the correct description" do + expect(page.description).to be_nil + end + + it "has correct depends_on" do + expect(page.depends_on).to eq([ + { "outright_sale_or_discounted_with_full_ownership?" => true }, + ]) + end +end diff --git a/spec/models/form/sales/pages/purchase_price_spec.rb b/spec/models/form/sales/pages/purchase_price_spec.rb index 4d0be701f..6ec2b7ac0 100644 --- a/spec/models/form/sales/pages/purchase_price_spec.rb +++ b/spec/models/form/sales/pages/purchase_price_spec.rb @@ -29,8 +29,7 @@ RSpec.describe Form::Sales::Pages::PurchasePrice, type: :model do it "has correct depends_on" do expect(page.depends_on).to eq([ - { "ownershipsch" => 3 }, - { "rent_to_buy_full_ownership?" => true }, + { "ownershipsch" => 2, "rent_to_buy_full_ownership?" => false }, ]) end end diff --git a/spec/models/form/sales/questions/purchase_price_spec.rb b/spec/models/form/sales/questions/purchase_price_spec.rb index 8894a592f..ce0aaae9c 100644 --- a/spec/models/form/sales/questions/purchase_price_spec.rb +++ b/spec/models/form/sales/questions/purchase_price_spec.rb @@ -32,7 +32,9 @@ RSpec.describe Form::Sales::Questions::PurchasePrice, type: :model do end it "has the correct hint" do - expect(question.hint_text).to be_nil + expect(question.hint_text).to eq( + "For all schemes, including Right to Acquire (RTA), Right to Buy (RTB), Voluntary Right to Buy (VRTB) or Preserved Right to Buy (PRTB) sales, enter the full price of the property without any discount", + ) end it "has correct width" do diff --git a/spec/models/form/sales/subsections/discounted_ownership_scheme_spec.rb b/spec/models/form/sales/subsections/discounted_ownership_scheme_spec.rb index 5702f7783..525ad06b4 100644 --- a/spec/models/form/sales/subsections/discounted_ownership_scheme_spec.rb +++ b/spec/models/form/sales/subsections/discounted_ownership_scheme_spec.rb @@ -12,6 +12,7 @@ RSpec.describe Form::Sales::Subsections::DiscountedOwnershipScheme, type: :model end it "has correct pages" do + puts discounted_ownership_scheme.pages.map(&:id) expect(discounted_ownership_scheme.pages.map(&:id)).to eq( %w[ living_before_purchase_discounted_ownership @@ -20,6 +21,7 @@ RSpec.describe Form::Sales::Subsections::DiscountedOwnershipScheme, type: :model about_price_not_rtb grant_value_check purchase_price_discounted_ownership + purchase_price_outright_ownership discounted_ownership_deposit_and_mortgage_value_check_after_value_and_discount mortgage_used_discounted_ownership discounted_ownership_mortgage_used_mortgage_value_check diff --git a/spec/models/form_handler_spec.rb b/spec/models/form_handler_spec.rb index ce9c66e85..d332d2b21 100644 --- a/spec/models/form_handler_spec.rb +++ b/spec/models/form_handler_spec.rb @@ -52,14 +52,14 @@ RSpec.describe FormHandler do it "is able to load a current sales form" do form = form_handler.get_form("current_sales") expect(form).to be_a(Form) - expect(form.pages.count).to eq(209) + expect(form.pages.count).to eq(210) expect(form.name).to eq("2022_2023_sales") end it "is able to load a previous sales form" do form = form_handler.get_form("previous_sales") expect(form).to be_a(Form) - expect(form.pages.count).to eq(209) + expect(form.pages.count).to eq(210) expect(form.name).to eq("2021_2022_sales") end end From 5077dc035bfd723d9875c99279ca84f58245ce7d Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Fri, 3 Feb 2023 09:58:32 +0000 Subject: [PATCH 6/9] CLDC-1767 Bulk upload fail email (#1264) * bulk upload file validations send failure email * handle errors during bulk upload parsing --- app/mailers/bulk_upload_mailer.rb | 20 +++-- app/services/bulk_upload/processor.rb | 10 +++ spec/mailers/bulk_upload_mailer_spec.rb | 20 ++++- spec/services/bulk_upload/processor_spec.rb | 86 +++++++++++++++++++++ 4 files changed, 128 insertions(+), 8 deletions(-) diff --git a/app/mailers/bulk_upload_mailer.rb b/app/mailers/bulk_upload_mailer.rb index b406a5c4b..ac8f626f3 100644 --- a/app/mailers/bulk_upload_mailer.rb +++ b/app/mailers/bulk_upload_mailer.rb @@ -63,16 +63,22 @@ class BulkUploadMailer < NotifyMailer ) end - def send_bulk_upload_failed_service_error_mail(user, bulk_upload) + def send_bulk_upload_failed_service_error_mail(bulk_upload:) + bulk_upload_link = if bulk_upload.lettings? + start_bulk_upload_lettings_logs_url + else + start_bulk_upload_sales_logs_url + end + send_email( - user.email, + bulk_upload.user.email, BULK_UPLOAD_FAILED_SERVICE_ERROR_TEMPLATE_ID, { - filename: "[#{bulk_upload} filename]", - upload_timestamp: "[#{bulk_upload} upload_timestamp]", - lettings_or_sales: "[#{bulk_upload} lettings_or_sales]", - year_combo: "[#{bulk_upload} year_combo]", - bulk_upload_link: "[#{bulk_upload} bulk_upload_link]", + filename: bulk_upload.filename, + upload_timestamp: bulk_upload.created_at, + lettings_or_sales: bulk_upload.log_type, + year_combo: bulk_upload.year_combo, + bulk_upload_link:, }, ) end diff --git a/app/services/bulk_upload/processor.rb b/app/services/bulk_upload/processor.rb index 6b444a205..0cf059da2 100644 --- a/app/services/bulk_upload/processor.rb +++ b/app/services/bulk_upload/processor.rb @@ -7,9 +7,15 @@ class BulkUpload::Processor def call download + + return send_failure_mail if validator.invalid? + validator.call create_logs if validator.create_logs? send_success_mail + rescue StandardError => e + Sentry.capture_exception(e) + send_failure_mail ensure downloader.delete_local_file! end @@ -22,6 +28,10 @@ private end end + def send_failure_mail + BulkUploadMailer.send_bulk_upload_failed_service_error_mail(bulk_upload:).deliver_later + end + def user bulk_upload.user end diff --git a/spec/mailers/bulk_upload_mailer_spec.rb b/spec/mailers/bulk_upload_mailer_spec.rb index 526f3584a..3d7c0454f 100644 --- a/spec/mailers/bulk_upload_mailer_spec.rb +++ b/spec/mailers/bulk_upload_mailer_spec.rb @@ -5,7 +5,7 @@ RSpec.describe BulkUploadMailer do let(:notify_client) { instance_double(Notifications::Client) } let(:user) { create(:user, email: "user@example.com") } - let(:bulk_upload) { build(:bulk_upload, :lettings) } + let(:bulk_upload) { build(:bulk_upload, :lettings, user:) } before do allow(Notifications::Client).to receive(:new).and_return(notify_client) @@ -29,4 +29,22 @@ RSpec.describe BulkUploadMailer do mailer.send_bulk_upload_complete_mail(user:, bulk_upload:) end end + + describe "#send_bulk_upload_failed_service_error_mail" do + it "sends correctly formed email" do + expect(notify_client).to receive(:send_email).with( + email_address: user.email, + template_id: described_class::BULK_UPLOAD_FAILED_SERVICE_ERROR_TEMPLATE_ID, + personalisation: { + filename: bulk_upload.filename, + upload_timestamp: bulk_upload.created_at, + lettings_or_sales: bulk_upload.log_type, + year_combo: bulk_upload.year_combo, + bulk_upload_link: start_bulk_upload_lettings_logs_url, + }, + ) + + mailer.send_bulk_upload_failed_service_error_mail(bulk_upload:) + end + end end diff --git a/spec/services/bulk_upload/processor_spec.rb b/spec/services/bulk_upload/processor_spec.rb index 27fd9edd3..7c2ebaf06 100644 --- a/spec/services/bulk_upload/processor_spec.rb +++ b/spec/services/bulk_upload/processor_spec.rb @@ -6,6 +6,91 @@ RSpec.describe BulkUpload::Processor do let(:bulk_upload) { create(:bulk_upload, :lettings) } describe "#call" do + context "when the bulk upload itself is not considered valid" do + let(:mock_downloader) do + instance_double( + BulkUpload::Downloader, + call: nil, + path: file_fixture("2022_23_lettings_bulk_upload.csv"), + delete_local_file!: nil, + ) + end + + let(:mock_validator) do + instance_double( + BulkUpload::Lettings::Validator, + invalid?: true, + call: nil, + ) + end + + before do + allow(BulkUpload::Downloader).to receive(:new).with(bulk_upload:).and_return(mock_downloader) + allow(BulkUpload::Lettings::Validator).to receive(:new).and_return(mock_validator) + end + + it "sends failure email" do + mail_double = instance_double("ActionMailer::MessageDelivery", deliver_later: nil) + + allow(BulkUploadMailer).to receive(:send_bulk_upload_failed_service_error_mail).and_return(mail_double) + + processor.call + + expect(BulkUploadMailer).to have_received(:send_bulk_upload_failed_service_error_mail) + expect(mail_double).to have_received(:deliver_later) + end + + it "does not attempt to validate the contents of the file" do + processor.call + + expect(mock_validator).not_to have_received(:call) + end + end + + context "when the bulk upload processing throws an error" do + let(:mock_downloader) do + instance_double( + BulkUpload::Downloader, + call: nil, + path: file_fixture("2022_23_lettings_bulk_upload.csv"), + delete_local_file!: nil, + ) + end + + let(:mock_validator) do + instance_double( + BulkUpload::Lettings::Validator, + invalid?: false, + ) + end + + before do + allow(BulkUpload::Downloader).to receive(:new).with(bulk_upload:).and_return(mock_downloader) + allow(BulkUpload::Lettings::Validator).to receive(:new).and_return(mock_validator) + + allow(mock_validator).to receive(:call).and_raise(StandardError) + end + + it "sends failure email" do + mail_double = instance_double("ActionMailer::MessageDelivery", deliver_later: nil) + + allow(BulkUploadMailer).to receive(:send_bulk_upload_failed_service_error_mail).and_return(mail_double) + + processor.call + + expect(BulkUploadMailer).to have_received(:send_bulk_upload_failed_service_error_mail) + expect(mail_double).to have_received(:deliver_later) + end + + it "we log the failure" do + allow(Sentry).to receive(:capture_exception) + + processor.call + + expect(Sentry).to have_received(:capture_exception) + end + end + context "when processing a bulk upload with errors" do let(:mock_downloader) do instance_double( @@ -56,6 +141,7 @@ RSpec.describe BulkUpload::Processor do BulkUpload::Lettings::Validator, call: nil, create_logs?: true, + invalid?: false, ) end From a87dc142714f509d33278778ab65cfb1cb334e97 Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Fri, 3 Feb 2023 16:02:20 +0000 Subject: [PATCH 7/9] CLDC-1762 Bulk upload fix errors email (#1267) * refactor bulk upload with extract method * add bulk upload mailer test * bulk upload mails users to fix logs on website * bulk uplad email change resume link * refactor local variable name --- app/mailers/bulk_upload_mailer.rb | 22 ++++-- app/services/bulk_upload/processor.rb | 21 ++++-- spec/mailers/bulk_upload_mailer_spec.rb | 28 ++++++++ spec/services/bulk_upload/processor_spec.rb | 78 ++++++++++++++++++++- 4 files changed, 135 insertions(+), 14 deletions(-) diff --git a/app/mailers/bulk_upload_mailer.rb b/app/mailers/bulk_upload_mailer.rb index ac8f626f3..59d03ce9b 100644 --- a/app/mailers/bulk_upload_mailer.rb +++ b/app/mailers/bulk_upload_mailer.rb @@ -83,16 +83,24 @@ class BulkUploadMailer < NotifyMailer ) end - def send_bulk_upload_with_errors_mail(user, bulk_upload) + def send_bulk_upload_with_errors_mail(bulk_upload:) + count = bulk_upload.logs.where.not(status: %w[completed]).count + + n_logs = pluralize(count, "log") + + title = "We found #{n_logs} with errors" + + error_description = "We created logs from your #{bulk_upload.year_combo} #{bulk_upload.log_type} data. There was a problem with #{count} of the logs. Click the below link to fix these logs." + send_email( - user.email, + bulk_upload.user.email, BULK_UPLOAD_WITH_ERRORS_TEMPLATE_ID, { - title: "[#{bulk_upload} title]", - filename: "[#{bulk_upload} filename]", - upload_timestamp: "[#{bulk_upload} upload_timestamp]", - error_description: "[#{bulk_upload} error_description]", - summary_report_link: "[#{bulk_upload} summary_report_link]", + title:, + filename: bulk_upload.filename, + upload_timestamp: bulk_upload.created_at.to_fs(:govuk_date_and_time), + error_description:, + summary_report_link: resume_bulk_upload_lettings_result_url(bulk_upload), }, ) end diff --git a/app/services/bulk_upload/processor.rb b/app/services/bulk_upload/processor.rb index 0cf059da2..d69f76243 100644 --- a/app/services/bulk_upload/processor.rb +++ b/app/services/bulk_upload/processor.rb @@ -11,8 +11,11 @@ class BulkUpload::Processor return send_failure_mail if validator.invalid? validator.call + create_logs if validator.create_logs? - send_success_mail + + send_fix_errors_mail if created_logs_but_incompleted? + send_success_mail if created_logs_and_all_completed? rescue StandardError => e Sentry.capture_exception(e) send_failure_mail @@ -22,10 +25,20 @@ class BulkUpload::Processor private + def send_fix_errors_mail + BulkUploadMailer.send_bulk_upload_with_errors_mail(bulk_upload:).deliver_later + end + def send_success_mail - if validator.create_logs? && bulk_upload.logs.group(:status).count.keys == %w[completed] - BulkUploadMailer.send_bulk_upload_complete_mail(user:, bulk_upload:).deliver_later - end + BulkUploadMailer.send_bulk_upload_complete_mail(user:, bulk_upload:).deliver_later + end + + def created_logs_but_incompleted? + validator.create_logs? && bulk_upload.logs.where.not(status: %w[completed]).count.positive? + end + + def created_logs_and_all_completed? + validator.create_logs? && bulk_upload.logs.group(:status).count.keys == %w[completed] end def send_failure_mail diff --git a/spec/mailers/bulk_upload_mailer_spec.rb b/spec/mailers/bulk_upload_mailer_spec.rb index 3d7c0454f..a3706beb3 100644 --- a/spec/mailers/bulk_upload_mailer_spec.rb +++ b/spec/mailers/bulk_upload_mailer_spec.rb @@ -47,4 +47,32 @@ RSpec.describe BulkUploadMailer do mailer.send_bulk_upload_failed_service_error_mail(bulk_upload:) end end + + context "when bulk upload has log which is not completed" do + before do + create(:lettings_log, :in_progress, bulk_upload:) + end + + describe "#send_bulk_upload_with_errors_mail" do + let(:error_description) do + "We created logs from your 2022/23 lettings data. There was a problem with 1 of the logs. Click the below link to fix these logs." + end + + it "sends correctly formed email" do + expect(notify_client).to receive(:send_email).with( + email_address: bulk_upload.user.email, + template_id: described_class::BULK_UPLOAD_WITH_ERRORS_TEMPLATE_ID, + personalisation: { + title: "We found 1 log with errors", + filename: bulk_upload.filename, + upload_timestamp: bulk_upload.created_at.to_fs(:govuk_date_and_time), + error_description:, + summary_report_link: "http://localhost:3000/lettings-logs/bulk-upload-results/#{bulk_upload.id}/resume", + }, + ) + + mailer.send_bulk_upload_with_errors_mail(bulk_upload:) + end + end + end end diff --git a/spec/services/bulk_upload/processor_spec.rb b/spec/services/bulk_upload/processor_spec.rb index 7c2ebaf06..b751e7dd7 100644 --- a/spec/services/bulk_upload/processor_spec.rb +++ b/spec/services/bulk_upload/processor_spec.rb @@ -91,7 +91,7 @@ RSpec.describe BulkUpload::Processor do end end - context "when processing a bulk upload with errors" do + context "when processing a bulk upload with errors but below threshold (therefore creates logs)" do let(:mock_downloader) do instance_double( BulkUpload::Downloader, @@ -101,12 +101,68 @@ RSpec.describe BulkUpload::Processor do ) end + let(:mock_validator) do + instance_double( + BulkUpload::Lettings::Validator, + invalid?: false, + call: nil, + create_logs?: true, + ) + end + before do allow(BulkUpload::Downloader).to receive(:new).with(bulk_upload:).and_return(mock_downloader) + allow(BulkUpload::Lettings::Validator).to receive(:new).and_return(mock_validator) + end + + it "deletes the local file afterwards" do + processor.call + + expect(mock_downloader).to have_received(:delete_local_file!) end - it "persist the validation errors" do - expect { processor.call }.to change(BulkUploadError, :count) + it "sends fix errors email" do + mail_double = instance_double("ActionMailer::MessageDelivery", deliver_later: nil) + + allow(BulkUploadMailer).to receive(:send_bulk_upload_with_errors_mail).and_return(mail_double) + + processor.call + + expect(BulkUploadMailer).to have_received(:send_bulk_upload_with_errors_mail) + expect(mail_double).to have_received(:deliver_later) + end + + it "does not send success email" do + allow(BulkUploadMailer).to receive(:send_bulk_upload_complete_mail).and_call_original + + processor.call + + expect(BulkUploadMailer).not_to have_received(:send_bulk_upload_complete_mail) + end + end + + context "when processing a bulk upload with errors but above threshold (therefore does not create logs)" do + let(:mock_downloader) do + instance_double( + BulkUpload::Downloader, + call: nil, + path: file_fixture("2022_23_lettings_bulk_upload.csv"), + delete_local_file!: nil, + ) + end + + let(:mock_validator) do + instance_double( + BulkUpload::Lettings::Validator, + invalid?: false, + call: nil, + create_logs?: false, + ) + end + + before do + allow(BulkUpload::Downloader).to receive(:new).with(bulk_upload:).and_return(mock_downloader) + allow(BulkUpload::Lettings::Validator).to receive(:new).and_return(mock_validator) end it "deletes the local file afterwards" do @@ -115,6 +171,14 @@ RSpec.describe BulkUpload::Processor do expect(mock_downloader).to have_received(:delete_local_file!) end + it "does not send fix errors email" do + allow(BulkUploadMailer).to receive(:send_bulk_upload_with_errors_mail).and_call_original + + processor.call + + expect(BulkUploadMailer).not_to have_received(:send_bulk_upload_with_errors_mail) + end + it "does not send success email" do allow(BulkUploadMailer).to receive(:send_bulk_upload_complete_mail).and_call_original @@ -165,6 +229,14 @@ RSpec.describe BulkUpload::Processor do expect(mock_creator).to have_received(:call) end + it "does not send fix errors email" do + allow(BulkUploadMailer).to receive(:send_bulk_upload_with_errors_mail).and_call_original + + processor.call + + expect(BulkUploadMailer).not_to have_received(:send_bulk_upload_with_errors_mail) + end + it "sends success email" do mail_double = instance_double("ActionMailer::MessageDelivery", deliver_later: nil) From b5407c8f1f3dd797c381020d3b8955497ef726cc Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Mon, 6 Feb 2023 11:13:01 +0000 Subject: [PATCH 8/9] Bulk upload validation tweaks (#1270) * remove duplicate bulk validations - validation was being performed both at CSV level and log level causing a duplicate validation to appear * bulk upload valiation errors now store the message - previouly this was storing just the error type which we do not have a mechanism to pipe these back to user readable error messages --- app/services/bulk_upload/lettings/row_parser.rb | 9 +++------ app/services/bulk_upload/lettings/validator.rb | 2 +- config/locales/en.yml | 1 + spec/services/bulk_upload/lettings/validator_spec.rb | 10 +++++----- 4 files changed, 10 insertions(+), 12 deletions(-) diff --git a/app/services/bulk_upload/lettings/row_parser.rb b/app/services/bulk_upload/lettings/row_parser.rb index d9a4a3e95..1182f4b85 100644 --- a/app/services/bulk_upload/lettings/row_parser.rb +++ b/app/services/bulk_upload/lettings/row_parser.rb @@ -139,13 +139,10 @@ class BulkUpload::Lettings::RowParser attribute :field_133, :integer attribute :field_134, :integer - validates :field_1, presence: true, inclusion: { in: (1..12).to_a } + validates :field_1, presence: { message: I18n.t("validations.not_answered", question: "letting type") }, + inclusion: { in: (1..12).to_a, message: I18n.t("validations.invalid_option", question: "letting type") } validates :field_4, presence: { if: proc { [2, 4, 6, 8, 10, 12].include?(field_1) } } - validates :field_96, presence: true - validates :field_97, presence: true - validates :field_98, presence: true - def valid? errors.clear @@ -176,7 +173,7 @@ private def validate_data_types unless attribute_set["field_1"].value_before_type_cast&.match?(/\A\d+\z/) - errors.add(:field_1, :invalid) + errors.add(:field_1, I18n.t("validations.invalid_number", question: "letting type")) end end diff --git a/app/services/bulk_upload/lettings/validator.rb b/app/services/bulk_upload/lettings/validator.rb index 90da6efb3..6f37c0f3a 100644 --- a/app/services/bulk_upload/lettings/validator.rb +++ b/app/services/bulk_upload/lettings/validator.rb @@ -162,7 +162,7 @@ class BulkUpload::Lettings::Validator row_parser.errors.each do |error| bulk_upload.bulk_upload_errors.create!( field: error.attribute, - error: error.type, + error: error.message, tenant_code: row_parser.field_7, property_ref: row_parser.field_100, row:, diff --git a/config/locales/en.yml b/config/locales/en.yml index 2321446bd..0f3bcc923 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -127,6 +127,7 @@ en: already_added: "You have already added this managing agent" not_answered: "You must answer %{question}" invalid_option: "Enter a valid value for %{question}" + invalid_number: "Enter a number for %{question}" other_field_missing: "If %{main_field_label} is other then %{other_field_label} must be provided" other_field_not_required: "%{other_field_label} must not be provided if %{main_field_label} was not other" diff --git a/spec/services/bulk_upload/lettings/validator_spec.rb b/spec/services/bulk_upload/lettings/validator_spec.rb index ec9b3b7f7..263c83163 100644 --- a/spec/services/bulk_upload/lettings/validator_spec.rb +++ b/spec/services/bulk_upload/lettings/validator_spec.rb @@ -42,15 +42,15 @@ RSpec.describe BulkUpload::Lettings::Validator do it "create validation error with correct values" do validator.call - error = BulkUploadError.first + error = BulkUploadError.order(:row, :field).first - expect(error.field).to eql("field_96") - expect(error.error).to eql("blank") + expect(error.field).to eql("field_11") + expect(error.error).to eql("You must only answer the length of the tenancy if it's fixed-term") expect(error.tenant_code).to eql("123") expect(error.property_ref).to be_nil expect(error.row).to eql("7") - expect(error.cell).to eql("CS7") - expect(error.col).to eql("CS") + expect(error.cell).to eql("L7") + expect(error.col).to eql("L") end end From f356ef38ab93a98eb60f69db7b23198ab92d5ae0 Mon Sep 17 00:00:00 2001 From: natdeanlewissoftwire <94526761+natdeanlewissoftwire@users.noreply.github.com> Date: Mon, 6 Feb 2023 11:19:33 +0000 Subject: [PATCH 9/9] Cldc 1464 completion date validations PO review (#1259) * feat: add 22/23 year validation * feat: wip commit * feat: update i18n * feat: add one year gap exchange/completion validations * tests: add tests for new validations * test: add setup validations tests * test: update factory to pass saledate validation * feat: update seeds * feat: update tests to reflect sales logs shouldn't exist in new service before 2022 * feat: merge with main, improve date behaviour * refactor: cleanup * feat: update tests * feat: enforce saledate not after 22/23 collection year end * feat: check date is valid * feat: add hodate/saledate hard validation to saledate as well * feat: add HandoverDate check to saledate question * test: update * db: update * test: update * feat: wip po updates * feat: add soft_validation to setup * feat: fix bug by making sale date soft validation optional * feat: add tests * test: update * refactor: linting --- .../form/sales/pages/handover_date_check.rb | 8 +++- .../form/sales/pages/sale_date_check.rb | 19 ++++++++ .../sales/questions/handover_date_check.rb | 8 +++- .../form/sales/questions/sale_date_check.rb | 29 +++++++++++ app/models/form/sales/subsections/setup.rb | 1 + .../subsections/shared_ownership_scheme.rb | 2 +- app/models/sales_log.rb | 2 +- app/models/validations/date_validations.rb | 9 ---- .../sales/sale_information_validations.rb | 5 +- .../validations/sales/setup_validations.rb | 4 +- .../validations/sales/soft_validations.rb | 2 +- app/models/validations/shared_validations.rb | 9 ++++ app/services/filter_service.rb | 2 +- config/locales/en.yml | 9 ++-- ...3104238_add_saledate_check_to_sales_log.rb | 5 ++ db/schema.rb | 7 +-- .../sales/pages/handover_date_check_spec.rb | 18 +++++-- .../form/sales/pages/sale_date_check_spec.rb | 47 ++++++++++++++++++ .../questions/handover_date_check_spec.rb | 4 +- .../sales/questions/sale_date_check_spec.rb | 48 +++++++++++++++++++ .../form/sales/subsections/setup_spec.rb | 1 + spec/models/form_handler_spec.rb | 16 ++++++- spec/models/form_spec.rb | 4 +- spec/models/sales_log_spec.rb | 2 +- .../sale_information_validations_spec.rb | 6 +-- 25 files changed, 229 insertions(+), 38 deletions(-) create mode 100644 app/models/form/sales/pages/sale_date_check.rb create mode 100644 app/models/form/sales/questions/sale_date_check.rb create mode 100644 db/migrate/20230203104238_add_saledate_check_to_sales_log.rb create mode 100644 spec/models/form/sales/pages/sale_date_check_spec.rb create mode 100644 spec/models/form/sales/questions/sale_date_check_spec.rb diff --git a/app/models/form/sales/pages/handover_date_check.rb b/app/models/form/sales/pages/handover_date_check.rb index cc0ce9a9b..5d656200b 100644 --- a/app/models/form/sales/pages/handover_date_check.rb +++ b/app/models/form/sales/pages/handover_date_check.rb @@ -1,8 +1,14 @@ class Form::Sales::Pages::HandoverDateCheck < ::Form::Page def initialize(id, hsh, subsection) super - @depends_on = [{ "hodate_3_years_or_more_saledate?" => true }] + @id = "handover_date_check" + @depends_on = [{ "saledate_check" => nil, "hodate_3_years_or_more_saledate?" => true }, + { "saledate_check" => 1, "hodate_3_years_or_more_saledate?" => true }] @informative_text = {} + @title_text = { + "translation" => "validations.sale_information.hodate.must_be_less_than_3_years_from_saledate", + "arguments" => [], + } end def questions diff --git a/app/models/form/sales/pages/sale_date_check.rb b/app/models/form/sales/pages/sale_date_check.rb new file mode 100644 index 000000000..acb34589c --- /dev/null +++ b/app/models/form/sales/pages/sale_date_check.rb @@ -0,0 +1,19 @@ +class Form::Sales::Pages::SaleDateCheck < ::Form::Page + def initialize(id, hsh, subsection) + super + @id = "completion_date_check" + @depends_on = [{ "hodate_check" => nil, "hodate_3_years_or_more_saledate?" => true }, + { "hodate_check" => 1, "hodate_3_years_or_more_saledate?" => true }] + @informative_text = {} + @title_text = { + "translation" => "validations.sale_information.saledate.must_be_less_than_3_years_from_hodate", + "arguments" => [], + } + end + + def questions + @questions ||= [ + Form::Sales::Questions::SaleDateCheck.new(nil, nil, self), + ] + end +end diff --git a/app/models/form/sales/questions/handover_date_check.rb b/app/models/form/sales/questions/handover_date_check.rb index 03a516e55..f19ee0196 100644 --- a/app/models/form/sales/questions/handover_date_check.rb +++ b/app/models/form/sales/questions/handover_date_check.rb @@ -3,7 +3,7 @@ class Form::Sales::Questions::HandoverDateCheck < ::Form::Question super @id = "hodate_check" @check_answer_label = "Practical completion or handover date check" - @header = "Are you sure practical completion or handover date is more than 3 years before exchange date?" + @header = "Are you sure?" @type = "interruption_screen" @answer_options = { "0" => { "value" => "Yes" }, @@ -17,6 +17,12 @@ class Form::Sales::Questions::HandoverDateCheck < ::Form::Question { "hodate_check" => 1, }, + { + "saledate_check" => 0, + }, + { + "saledate_check" => 1, + }, ], } end diff --git a/app/models/form/sales/questions/sale_date_check.rb b/app/models/form/sales/questions/sale_date_check.rb new file mode 100644 index 000000000..b2947434b --- /dev/null +++ b/app/models/form/sales/questions/sale_date_check.rb @@ -0,0 +1,29 @@ +class Form::Sales::Questions::SaleDateCheck < ::Form::Question + def initialize(id, hsh, page) + super + @id = "saledate_check" + @check_answer_label = "Sale completion date check" + @header = "Are you sure?" + @type = "interruption_screen" + @answer_options = { + "0" => { "value" => "Yes" }, + "1" => { "value" => "No" }, + } + @hidden_in_check_answers = { + "depends_on" => [ + { + "hodate_check" => 0, + }, + { + "hodate_check" => 1, + }, + { + "saledate_check" => 0, + }, + { + "saledate_check" => 1, + }, + ], + } + end +end diff --git a/app/models/form/sales/subsections/setup.rb b/app/models/form/sales/subsections/setup.rb index 22dcda8b9..aa3c498fc 100644 --- a/app/models/form/sales/subsections/setup.rb +++ b/app/models/form/sales/subsections/setup.rb @@ -10,6 +10,7 @@ class Form::Sales::Subsections::Setup < ::Form::Subsection Form::Common::Pages::Organisation.new(nil, nil, self), Form::Common::Pages::CreatedBy.new(nil, nil, self), Form::Sales::Pages::SaleDate.new(nil, nil, self), + Form::Sales::Pages::SaleDateCheck.new(nil, nil, self), Form::Sales::Pages::PurchaserCode.new(nil, nil, self), Form::Sales::Pages::OwnershipScheme.new(nil, nil, self), Form::Sales::Pages::SharedOwnershipType.new(nil, nil, self), diff --git a/app/models/form/sales/subsections/shared_ownership_scheme.rb b/app/models/form/sales/subsections/shared_ownership_scheme.rb index d011d5429..04ff9b56e 100644 --- a/app/models/form/sales/subsections/shared_ownership_scheme.rb +++ b/app/models/form/sales/subsections/shared_ownership_scheme.rb @@ -15,7 +15,7 @@ class Form::Sales::Subsections::SharedOwnershipScheme < ::Form::Subsection Form::Sales::Pages::Resale.new(nil, nil, self), Form::Sales::Pages::ExchangeDate.new(nil, nil, self), Form::Sales::Pages::HandoverDate.new(nil, nil, self), - Form::Sales::Pages::HandoverDateCheck.new("handover_date_check", nil, self), + Form::Sales::Pages::HandoverDateCheck.new(nil, nil, self), Form::Sales::Pages::LaNominations.new(nil, nil, self), Form::Sales::Pages::BuyerPrevious.new(nil, nil, self), Form::Sales::Pages::PreviousBedrooms.new(nil, nil, self), diff --git a/app/models/sales_log.rb b/app/models/sales_log.rb index b217ff69c..8a99932bd 100644 --- a/app/models/sales_log.rb +++ b/app/models/sales_log.rb @@ -35,7 +35,7 @@ class SalesLog < Log scope :search_by, ->(param) { filter_by_id(param) } scope :filter_by_organisation, ->(org, _user = nil) { where(owning_organisation: org) } - OPTIONAL_FIELDS = %w[purchid monthly_charges_value_check old_persons_shared_ownership_value_check].freeze + OPTIONAL_FIELDS = %w[saledate_check purchid monthly_charges_value_check old_persons_shared_ownership_value_check].freeze RETIREMENT_AGES = { "M" => 65, "F" => 60, "X" => 65 }.freeze def startdate diff --git a/app/models/validations/date_validations.rb b/app/models/validations/date_validations.rb index ceec8ed9a..3823b3dd2 100644 --- a/app/models/validations/date_validations.rb +++ b/app/models/validations/date_validations.rb @@ -84,15 +84,6 @@ private @second_collection_end_date ||= FormHandler.instance.forms.map { |_name, form| form.end_date }.compact.max end - def date_valid?(question, record) - if record[question].is_a?(ActiveSupport::TimeWithZone) && record[question].year.zero? - record.errors.add question, I18n.t("validations.date.invalid_date") - false - else - true - end - end - def is_rsnvac_first_let?(record) [15, 16, 17].include?(record["rsnvac"]) end diff --git a/app/models/validations/sales/sale_information_validations.rb b/app/models/validations/sales/sale_information_validations.rb index 24fdff3ea..e448b6aca 100644 --- a/app/models/validations/sales/sale_information_validations.rb +++ b/app/models/validations/sales/sale_information_validations.rb @@ -2,8 +2,9 @@ module Validations::Sales::SaleInformationValidations def validate_practical_completion_date_before_saledate(record) return if record.saledate.blank? || record.hodate.blank? - unless record.saledate > record.hodate - record.errors.add :hodate, I18n.t("validations.sale_information.hodate.must_be_before_exdate") + if record.hodate > record.saledate + record.errors.add :hodate, I18n.t("validations.sale_information.hodate.must_be_before_saledate") + record.errors.add :saledate, I18n.t("validations.sale_information.saledate.must_be_after_hodate") end end diff --git a/app/models/validations/sales/setup_validations.rb b/app/models/validations/sales/setup_validations.rb index 0e7a759ee..0ae1f43ea 100644 --- a/app/models/validations/sales/setup_validations.rb +++ b/app/models/validations/sales/setup_validations.rb @@ -1,6 +1,8 @@ module Validations::Sales::SetupValidations + include Validations::SharedValidations + def validate_saledate(record) - return unless record.saledate + return unless record.saledate && date_valid?("saledate", record) unless Time.zone.local(2022, 4, 1) <= record.saledate && record.saledate < Time.zone.local(2023, 4, 1) record.errors.add :saledate, I18n.t("validations.setup.saledate.financial_year") diff --git a/app/models/validations/sales/soft_validations.rb b/app/models/validations/sales/soft_validations.rb index e7b170e89..cba756ea3 100644 --- a/app/models/validations/sales/soft_validations.rb +++ b/app/models/validations/sales/soft_validations.rb @@ -67,7 +67,7 @@ module Validations::Sales::SoftValidations def hodate_3_years_or_more_saledate? return unless hodate && saledate - ((saledate.to_date - hodate.to_date).to_i / 365) >= 3 + saledate - hodate >= 3.years end def grant_outside_common_range? diff --git a/app/models/validations/shared_validations.rb b/app/models/validations/shared_validations.rb index 8452b9a52..3df46bc8b 100644 --- a/app/models/validations/shared_validations.rb +++ b/app/models/validations/shared_validations.rb @@ -99,6 +99,15 @@ module Validations::SharedValidations end end + def date_valid?(question, record) + if record[question].is_a?(ActiveSupport::TimeWithZone) && record[question].year.zero? + record.errors.add question, I18n.t("validations.date.invalid_date") + false + else + true + end + end + private def person_is_partner?(relationship) diff --git a/app/services/filter_service.rb b/app/services/filter_service.rb index affe31f02..71fc19a46 100644 --- a/app/services/filter_service.rb +++ b/app/services/filter_service.rb @@ -36,7 +36,7 @@ class FilterService end def bulk_upload - id = ((logs_filters["bulk_upload_id"] || []).reject(&:blank?))[0] + id = (logs_filters["bulk_upload_id"] || []).reject(&:blank?)[0] @bulk_upload ||= current_user.bulk_uploads.find_by(id:) end diff --git a/config/locales/en.yml b/config/locales/en.yml index 0f3bcc923..5da5974a4 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -427,13 +427,16 @@ en: social_homebuy: "Social HomeBuy buyers should not have lived here before" rent_to_buy: "Rent to Buy buyers should not have lived here before" hodate: - must_be_before_exdate: "Practical completion or handover date must be before exchange date" + must_be_before_saledate: "Practical completion or handover date must be before exchange date" + must_be_less_than_3_years_from_saledate: "You told us practical completion or handover date is more than 3 years before completion date" exdate: - must_be_before_saledate: "Contract exchange date must be less than 1 year before completion date" + must_be_before_saledate: "Contract exchange date must be before completion date" must_be_less_than_1_year_from_saledate: "Contract exchange date must be less than 1 year before completion date" saledate: - must_be_after_exdate: "Completion date must be less than 1 year after contract exchange date" + must_be_after_exdate: "Completion date must be after contract exchange date" must_be_less_than_1_year_from_exdate: "Completion date must be less than 1 year after contract exchange date" + must_be_less_than_3_years_from_hodate: "You told us completion date is more than 3 years after practical completion or handover date" + must_be_after_hodate: "Completion date must be after practical completion or handover date" previous_property_beds: property_type_bedsit: "Bedsit bedroom maximum 1" discounted_ownership_value: "Mortgage, deposit, and grant total must equal £%{value_with_discount}" diff --git a/db/migrate/20230203104238_add_saledate_check_to_sales_log.rb b/db/migrate/20230203104238_add_saledate_check_to_sales_log.rb new file mode 100644 index 000000000..8ee7fdc65 --- /dev/null +++ b/db/migrate/20230203104238_add_saledate_check_to_sales_log.rb @@ -0,0 +1,5 @@ +class AddSaledateCheckToSalesLog < ActiveRecord::Migration[7.0] + def change + add_column :sales_logs, :saledate_check, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index 95e7799a1..99aef7031 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2023_01_27_102334) do +ActiveRecord::Schema[7.0].define(version: 2023_02_03_104238) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -504,12 +504,13 @@ ActiveRecord::Schema[7.0].define(version: 2023_01_27_102334) do t.integer "retirement_value_check" t.integer "hodate_check" t.integer "extrabor_value_check" - t.integer "grant_value_check" - t.integer "staircase_bought_value_check" t.integer "deposit_and_mortgage_value_check" t.integer "shared_ownership_deposit_value_check" + t.integer "grant_value_check" t.integer "old_persons_shared_ownership_value_check" + t.integer "staircase_bought_value_check" t.integer "monthly_charges_value_check" + t.integer "saledate_check" t.index ["bulk_upload_id"], name: "index_sales_logs_on_bulk_upload_id" t.index ["created_by_id"], name: "index_sales_logs_on_created_by_id" t.index ["owning_organisation_id"], name: "index_sales_logs_on_owning_organisation_id" diff --git a/spec/models/form/sales/pages/handover_date_check_spec.rb b/spec/models/form/sales/pages/handover_date_check_spec.rb index 86c74fda6..6bd3f3874 100644 --- a/spec/models/form/sales/pages/handover_date_check_spec.rb +++ b/spec/models/form/sales/pages/handover_date_check_spec.rb @@ -16,18 +16,28 @@ RSpec.describe Form::Sales::Pages::HandoverDateCheck, type: :model do end it "has the correct id" do - expect(page.id).to eq("") + expect(page.id).to eq("handover_date_check") end it "has the correct header" do expect(page.header).to be_nil end + it "has the correct title_text" do + expect(page.title_text).to eq({ + "translation" => "validations.sale_information.hodate.must_be_less_than_3_years_from_saledate", + "arguments" => [], + }) + end + + it "has the correct informative_text" do + expect(page.informative_text).to eq({}) + end + it "has correct depends_on" do expect(page.depends_on).to eq([ - { - "hodate_3_years_or_more_saledate?" => true, - }, + { "hodate_3_years_or_more_saledate?" => true, "saledate_check" => nil }, + { "hodate_3_years_or_more_saledate?" => true, "saledate_check" => 1 }, ]) end diff --git a/spec/models/form/sales/pages/sale_date_check_spec.rb b/spec/models/form/sales/pages/sale_date_check_spec.rb new file mode 100644 index 000000000..f1e0b3e2b --- /dev/null +++ b/spec/models/form/sales/pages/sale_date_check_spec.rb @@ -0,0 +1,47 @@ +require "rails_helper" + +RSpec.describe Form::Sales::Pages::SaleDateCheck, type: :model do + subject(:page) { described_class.new(page_id, page_definition, subsection) } + + let(:page_id) { "" } + let(:page_definition) { nil } + let(:subsection) { instance_double(Form::Subsection) } + + it "has correct subsection" do + expect(page.subsection).to eq(subsection) + end + + it "has correct questions" do + expect(page.questions.map(&:id)).to eq(%w[saledate_check]) + end + + it "has the correct id" do + expect(page.id).to eq("completion_date_check") + end + + it "has the correct header" do + expect(page.header).to be_nil + end + + it "has the correct title_text" do + expect(page.title_text).to eq({ + "translation" => "validations.sale_information.saledate.must_be_less_than_3_years_from_hodate", + "arguments" => [], + }) + end + + it "has the correct informative_text" do + expect(page.informative_text).to eq({}) + end + + it "has correct depends_on" do + expect(page.depends_on).to eq([ + { "hodate_3_years_or_more_saledate?" => true, "hodate_check" => nil }, + { "hodate_3_years_or_more_saledate?" => true, "hodate_check" => 1 }, + ]) + end + + it "is interruption screen page" do + expect(page.interruption_screen?).to eq(true) + end +end diff --git a/spec/models/form/sales/questions/handover_date_check_spec.rb b/spec/models/form/sales/questions/handover_date_check_spec.rb index 0e57abf35..86a46436b 100644 --- a/spec/models/form/sales/questions/handover_date_check_spec.rb +++ b/spec/models/form/sales/questions/handover_date_check_spec.rb @@ -16,7 +16,7 @@ RSpec.describe Form::Sales::Questions::HandoverDateCheck, type: :model do end it "has the correct header" do - expect(question.header).to eq("Are you sure practical completion or handover date is more than 3 years before exchange date?") + expect(question.header).to eq("Are you sure?") end it "has the correct check_answer_label" do @@ -43,6 +43,6 @@ RSpec.describe Form::Sales::Questions::HandoverDateCheck, type: :model do end it "has the correct hidden_in_check_answers" do - expect(question.hidden_in_check_answers).to eq({ "depends_on" => [{ "hodate_check" => 0 }, { "hodate_check" => 1 }] }) + expect(question.hidden_in_check_answers).to eq({ "depends_on" => [{ "hodate_check" => 0 }, { "hodate_check" => 1 }, { "saledate_check" => 0 }, { "saledate_check" => 1 }] }) end end diff --git a/spec/models/form/sales/questions/sale_date_check_spec.rb b/spec/models/form/sales/questions/sale_date_check_spec.rb new file mode 100644 index 000000000..c37b2e870 --- /dev/null +++ b/spec/models/form/sales/questions/sale_date_check_spec.rb @@ -0,0 +1,48 @@ +require "rails_helper" + +RSpec.describe Form::Sales::Questions::SaleDateCheck, type: :model do + subject(:question) { described_class.new(question_id, question_definition, page) } + + let(:question_id) { nil } + let(:question_definition) { nil } + let(:page) { instance_double(Form::Page) } + + it "has correct page" do + expect(question.page).to eq(page) + end + + it "has the correct id" do + expect(question.id).to eq("saledate_check") + end + + it "has the correct header" do + expect(question.header).to eq("Are you sure?") + end + + it "has the correct check_answer_label" do + expect(question.check_answer_label).to eq("Sale completion date check") + end + + it "has the correct type" do + expect(question.type).to eq("interruption_screen") + end + + it "is not marked as derived" do + expect(question.derived?).to be false + end + + it "has the correct hint" do + expect(question.hint_text).to be_nil + end + + it "has the correct answer_options" do + expect(question.answer_options).to eq({ + "0" => { "value" => "Yes" }, + "1" => { "value" => "No" }, + }) + end + + it "has the correct hidden_in_check_answers" do + expect(question.hidden_in_check_answers).to eq({ "depends_on" => [{ "hodate_check" => 0 }, { "hodate_check" => 1 }, { "saledate_check" => 0 }, { "saledate_check" => 1 }] }) + end +end diff --git a/spec/models/form/sales/subsections/setup_spec.rb b/spec/models/form/sales/subsections/setup_spec.rb index ac4f0246c..779b9537e 100644 --- a/spec/models/form/sales/subsections/setup_spec.rb +++ b/spec/models/form/sales/subsections/setup_spec.rb @@ -17,6 +17,7 @@ RSpec.describe Form::Sales::Subsections::Setup, type: :model do organisation created_by completion_date + completion_date_check purchaser_code ownership_scheme shared_ownership_type diff --git a/spec/models/form_handler_spec.rb b/spec/models/form_handler_spec.rb index d332d2b21..9c02a5318 100644 --- a/spec/models/form_handler_spec.rb +++ b/spec/models/form_handler_spec.rb @@ -52,14 +52,14 @@ RSpec.describe FormHandler do it "is able to load a current sales form" do form = form_handler.get_form("current_sales") expect(form).to be_a(Form) - expect(form.pages.count).to eq(210) + expect(form.pages.count).to eq(211) expect(form.name).to eq("2022_2023_sales") end it "is able to load a previous sales form" do form = form_handler.get_form("previous_sales") expect(form).to be_a(Form) - expect(form.pages.count).to eq(210) + expect(form.pages.count).to eq(211) expect(form.name).to eq("2021_2022_sales") end end @@ -76,6 +76,10 @@ RSpec.describe FormHandler do context "when the date is after 1st of April" do let(:now) { Time.utc(2022, 8, 3) } + it "returns the same year as the current start year" do + expect(form_handler.current_collection_start_year).to eq(2022) + end + it "returns the correct current lettings form name" do expect(form_handler.form_name_from_start_year(2022, "lettings")).to eq("current_lettings") end @@ -99,11 +103,19 @@ RSpec.describe FormHandler do it "returns the correct next sales form name" do expect(form_handler.form_name_from_start_year(2023, "sales")).to eq("next_sales") end + + it "returns the correct current start date" do + expect(form_handler.current_collection_start_date).to eq(Time.zone.local(2022, 4, 1)) + end end context "with the date before 1st of April" do let(:now) { Time.utc(2022, 2, 3) } + it "returns the previous year as the current start year" do + expect(form_handler.current_collection_start_year).to eq(2021) + end + it "returns the correct current lettings form name" do expect(form_handler.form_name_from_start_year(2021, "lettings")).to eq("current_lettings") end diff --git a/spec/models/form_spec.rb b/spec/models/form_spec.rb index 71dfd4b60..72223c1b5 100644 --- a/spec/models/form_spec.rb +++ b/spec/models/form_spec.rb @@ -218,9 +218,9 @@ RSpec.describe Form, type: :model do expect(form.sections[0].class).to eq(Form::Sales::Sections::Setup) expect(form.subsections.count).to eq(1) expect(form.subsections.first.id).to eq("setup") - expect(form.pages.count).to eq(14) + expect(form.pages.count).to eq(15) expect(form.pages.first.id).to eq("organisation") - expect(form.questions.count).to eq(15) + expect(form.questions.count).to eq(16) expect(form.questions.first.id).to eq("owning_organisation_id") expect(form.start_date).to eq(Time.zone.parse("2022-04-01")) expect(form.end_date).to eq(Time.zone.parse("2023-07-01")) diff --git a/spec/models/sales_log_spec.rb b/spec/models/sales_log_spec.rb index 0b4fa4459..a5a57f36e 100644 --- a/spec/models/sales_log_spec.rb +++ b/spec/models/sales_log_spec.rb @@ -47,7 +47,7 @@ RSpec.describe SalesLog, type: :model do let(:sales_log) { build(:sales_log) } it "returns optional fields" do - expect(sales_log.optional_fields).to eq(%w[purchid monthly_charges_value_check old_persons_shared_ownership_value_check]) + expect(sales_log.optional_fields).to eq(%w[saledate_check purchid monthly_charges_value_check old_persons_shared_ownership_value_check]) end end diff --git a/spec/models/validations/sales/sale_information_validations_spec.rb b/spec/models/validations/sales/sale_information_validations_spec.rb index 7ca507c1d..055d749ae 100644 --- a/spec/models/validations/sales/sale_information_validations_spec.rb +++ b/spec/models/validations/sales/sale_information_validations_spec.rb @@ -62,7 +62,7 @@ RSpec.describe Validations::Sales::SaleInformationValidations do it "does not add an error" do sale_information_validator.validate_practical_completion_date_before_saledate(record) - expect(record.errors[:hodate]).to be_present + expect(record.errors[:hodate]).not_to be_present end end end @@ -130,10 +130,10 @@ RSpec.describe Validations::Sales::SaleInformationValidations do sale_information_validator.validate_exchange_date(record) expect(record.errors[:exdate]).to eq( - ["Contract exchange date must be less than 1 year before completion date"], + ["Contract exchange date must be before completion date"], ) expect(record.errors[:saledate]).to eq( - ["Completion date must be less than 1 year after contract exchange date"], + ["Completion date must be after contract exchange date"], ) end end