From a65ee5676f5188a9cbde7d2ad86907f8a190b504 Mon Sep 17 00:00:00 2001 From: Manny Dinssa <44172848+Dinssa@users.noreply.github.com> Date: Tue, 22 Oct 2024 14:14:53 +0100 Subject: [PATCH 1/6] CLDC-2399: Bulk upload table styling (#2714) --- .../bulk_upload_error_row_component.html.erb | 30 +++++++++---------- .../bulk_upload_error_row_component.rb | 13 ++++++++ app/frontend/styles/_bulk-uploads.scss | 5 ++++ 3 files changed, 33 insertions(+), 15 deletions(-) diff --git a/app/components/bulk_upload_error_row_component.html.erb b/app/components/bulk_upload_error_row_component.html.erb index 65e38613f..4bd303957 100644 --- a/app/components/bulk_upload_error_row_component.html.erb +++ b/app/components/bulk_upload_error_row_component.html.erb @@ -13,7 +13,7 @@ <% if critical_errors.any? %>

Critical errors

These errors must be fixed to complete your logs.

- <%= govuk_table do |table| %> + <%= govuk_table(html_attributes: { class: potential_errors.any? ? "" : "no-bottom-border" }) do |table| %> <%= table.with_head do |head| %> <% head.with_row do |row| %> <% row.with_cell(header: true, text: "Cell") %> @@ -39,7 +39,7 @@ <% if potential_errors.any? %>

Potential errors

The following groups of cells might have conflicting data. Check the answers and fix any incorrect data.

If the answers are correct, fix the critical errors and reupload the file. You'll need to confirm that the following data is correct when the file only contains potential errors.

- <%= govuk_table do |table| %> + <%= govuk_table(html_attributes: { class: "no-bottom-border" }) do |table| %> <%= table.with_head do |head| %> <% head.with_row do |row| %> <% row.with_cell(header: true, text: "Cell") %> @@ -49,24 +49,24 @@ <% end %> <% end %> - <%= table.with_body do |body| %> - <% potential_errors.group_by(&:error).each do |error_message, errors| %> - <% errors.each_with_index do |error, index| %> - <% row_class = "grouped-rows" %> - <% row_class += " first-row" if index.zero? %> - <% row_class += " last-row" if index == errors.size - 1 %> - <% body.with_row(html_attributes: { class: row_class }) do |row| %> - <% row.with_cell(text: error.cell) %> - <% row.with_cell(text: question_for_field(error.field), html_attributes: { class: "govuk-!-width-one-half" }) %> - <% if index == 0 %> - <% row.with_cell(text: error_message.html_safe, rowspan: errors.size, html_attributes: { class: "govuk-!-font-weight-bold govuk-!-width-one-half grouped-multirow-cell" }) %> - <% end %> - <% row.with_cell(text: error.field.humanize) %> + <%= table.with_body do |body| %> + <% potential_errors.group_by(&:error).each_with_index do |(error_message, errors), group_index| %> + <% total_groups = potential_errors.group_by(&:error).size %> + <% errors.each_with_index do |error, index| %> + <% row_class = row_classes(index, errors.size) %> + <% body.with_row(html_attributes: { class: row_class }) do |row| %> + <% row.with_cell(text: error.cell) %> + <% row.with_cell(text: question_for_field(error.field), html_attributes: { class: "govuk-!-width-one-half" }) %> + <% if index == 0 %> + <% cell_class = cell_classes(group_index, total_groups) %> + <% row.with_cell(text: error_message.html_safe, rowspan: errors.size, html_attributes: { class: cell_class }) %> <% end %> + <% row.with_cell(text: error.field.humanize) %> <% end %> <% end %> <% end %> <% end %> + <% end %> <% end %> diff --git a/app/components/bulk_upload_error_row_component.rb b/app/components/bulk_upload_error_row_component.rb index 887eef10c..1cb4de9d8 100644 --- a/app/components/bulk_upload_error_row_component.rb +++ b/app/components/bulk_upload_error_row_component.rb @@ -62,4 +62,17 @@ class BulkUploadErrorRowComponent < ViewComponent::Base def sales? bulk_upload.log_type == "sales" end + + def row_classes(index, errors_size) + row_class = "grouped-rows" + row_class += " first-row" if index.zero? + row_class += " last-row" if index == errors_size - 1 + row_class + end + + def cell_classes(group_index, total_groups) + cell_class = "govuk-!-font-weight-bold govuk-!-width-one-half" + cell_class += " grouped-multirow-cell" unless group_index == total_groups - 1 + cell_class + end end diff --git a/app/frontend/styles/_bulk-uploads.scss b/app/frontend/styles/_bulk-uploads.scss index eceae6565..65213ee82 100644 --- a/app/frontend/styles/_bulk-uploads.scss +++ b/app/frontend/styles/_bulk-uploads.scss @@ -12,6 +12,11 @@ border-bottom: 1px solid #b1b4b6; } +.no-bottom-border, +.no-bottom-border > tbody > tr:last-of-type td { + border-bottom: none; +} + .text-normal-break { white-space: normal; word-break: break-all; From 443a19f42d5fe5243982e9c385b153e9a326df42 Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Tue, 22 Oct 2024 16:08:44 +0100 Subject: [PATCH 2/6] Update household validations (#2708) --- .../sales/household_validations.rb | 48 +++++++++--------- app/models/validations/shared_validations.rb | 6 ++- .../bulk_upload/sales/year2024/row_parser.rb | 12 ++--- config/locales/en.yml | 11 ---- .../validations/sales/household.en.yml | 47 +++++++++++++++++ .../sales/household_validations_spec.rb | 50 +++++++++---------- 6 files changed, 107 insertions(+), 67 deletions(-) create mode 100644 config/locales/validations/sales/household.en.yml diff --git a/app/models/validations/sales/household_validations.rb b/app/models/validations/sales/household_validations.rb index e006374aa..fb1ea65c2 100644 --- a/app/models/validations/sales/household_validations.rb +++ b/app/models/validations/sales/household_validations.rb @@ -9,9 +9,9 @@ module Validations::Sales::HouseholdValidations return unless record.form.start_date.year >= 2023 if record.buyers_will_live_in? && record.buyer_one_will_not_live_in_property? && record.buyer_two_will_not_live_in_property? - record.errors.add :buylivein, I18n.t("validations.household.buylivein.buyers_will_live_in_property_values_inconsistent_setup") - record.errors.add :buy1livein, I18n.t("validations.household.buylivein.buyers_will_live_in_property_values_inconsistent") - record.errors.add :buy2livein, I18n.t("validations.household.buylivein.buyers_will_live_in_property_values_inconsistent") + record.errors.add :buylivein, I18n.t("validations.sales.household.buylivein.buyers_will_live_in_property_values_inconsistent") + record.errors.add :buy1livein, I18n.t("validations.sales.household.buy1livein.buyers_will_live_in_property_values_inconsistent") + record.errors.add :buy2livein, I18n.t("validations.sales.household.buy2livein.buyers_will_live_in_property_values_inconsistent") end end @@ -20,8 +20,8 @@ module Validations::Sales::HouseholdValidations return unless record.discounted_ownership_sale? && record.prevten if [3, 4, 5, 6, 7, 9, 0].include?(record.prevten) - record.errors.add :prevten, I18n.t("validations.household.prevten.invalid_for_discounted_sale") - record.errors.add :ownershipsch, I18n.t("validations.household.prevten.invalid_for_discounted_sale") + record.errors.add :prevten, I18n.t("validations.sales.household.prevten.prevten_invalid_for_discounted_sale") + record.errors.add :ownershipsch, I18n.t("validations.sales.household.ownershipsch.prevten_invalid_for_discounted_sale") end end @@ -34,11 +34,11 @@ module Validations::Sales::HouseholdValidations next unless age && relationship if age < 16 && !relationship_is_child_other_or_refused?(relationship) - record.errors.add "age#{person_num}", I18n.t("validations.household.age.child_under_16_relat_sales", person_num:) - record.errors.add "relat#{person_num}", I18n.t("validations.household.relat.child_under_16_sales", person_num:) + record.errors.add "age#{person_num}", I18n.t("validations.sales.household.age.child_under_16", person_num:) + record.errors.add "relat#{person_num}", I18n.t("validations.sales.household.relat.child_under_16", person_num:) elsif age >= 20 && person_is_child?(relationship) - record.errors.add "age#{person_num}", I18n.t("validations.household.age.child_over_20") - record.errors.add "relat#{person_num}", I18n.t("validations.household.relat.child_over_20") + record.errors.add "age#{person_num}", I18n.t("validations.sales.household.age.child_over_20") + record.errors.add "relat#{person_num}", I18n.t("validations.sales.household.relat.child_over_20") end end end @@ -58,16 +58,16 @@ module Validations::Sales::HouseholdValidations child = person_is_child?(relationship) if age_between_16_19 && !(student || economic_status_refused) && child - record.errors.add "ecstat#{person_num}", I18n.t("validations.household.ecstat.student_16_19.must_be_student") - record.errors.add "age#{person_num}", I18n.t("validations.household.age.student_16_19.cannot_be_16_19.child_not_student") - record.errors.add "relat#{person_num}", I18n.t("validations.household.relat.student_16_19.cannot_be_child.16_19_not_student") + record.errors.add "ecstat#{person_num}", I18n.t("validations.sales.household.ecstat.student_16_19.must_be_student") + record.errors.add "age#{person_num}", I18n.t("validations.sales.household.age.student_16_19.cannot_be_16_19.child_not_student") + record.errors.add "relat#{person_num}", I18n.t("validations.sales.household.relat.student_16_19.cannot_be_child.16_19_not_student") end next unless !age_between_16_19 && student && child - record.errors.add "age#{person_num}", I18n.t("validations.household.age.student_16_19.must_be_16_19") - record.errors.add "ecstat#{person_num}", I18n.t("validations.household.ecstat.student_16_19.cannot_be_student.child_not_16_19") - record.errors.add "relat#{person_num}", I18n.t("validations.household.relat.student_16_19.cannot_be_child.student_not_16_19") + record.errors.add "age#{person_num}", I18n.t("validations.sales.household.age.student_16_19.must_be_16_19") + record.errors.add "ecstat#{person_num}", I18n.t("validations.sales.household.ecstat.student_16_19.cannot_be_student.child_not_16_19") + record.errors.add "relat#{person_num}", I18n.t("validations.sales.household.relat.student_16_19.cannot_be_child.student_not_16_19") end end @@ -78,12 +78,12 @@ module Validations::Sales::HouseholdValidations next unless age && economic_status if age < 16 && !economic_status_is_child_other_or_refused?(economic_status) && !record.form.start_year_after_2024? - record.errors.add "ecstat#{person_num}", I18n.t("validations.household.ecstat.child_under_16", person_num:) - record.errors.add "age#{person_num}", I18n.t("validations.household.age.child_under_16_ecstat", person_num:) + record.errors.add "ecstat#{person_num}", I18n.t("validations.sales.household.ecstat.child_under_16", person_num:) + record.errors.add "age#{person_num}", I18n.t("validations.sales.household.age.child_under_16_ecstat", person_num:) end if person_is_economic_child?(economic_status) && age > 16 - record.errors.add "ecstat#{person_num}", I18n.t("validations.household.ecstat.child_over_16", person_num:) - record.errors.add "age#{person_num}", I18n.t("validations.household.age.child_over_16", person_num:) + record.errors.add "ecstat#{person_num}", I18n.t("validations.sales.household.ecstat.child_over_16", person_num:) + record.errors.add "age#{person_num}", I18n.t("validations.sales.household.age.child_over_16", person_num:) end end end @@ -99,17 +99,17 @@ module Validations::Sales::HouseholdValidations next unless person_age > buyer_1_age - 12 && person_is_child?(relationship) - record.errors.add "age1", I18n.t("validations.household.age.child_12_years_younger") - record.errors.add "age#{person_num}", I18n.t("validations.household.age.child_12_years_younger") - record.errors.add "relat#{person_num}", I18n.t("validations.household.age.child_12_years_younger") + record.errors.add "age1", I18n.t("validations.sales.household.age1.child_12_years_younger") + record.errors.add "age#{person_num}", I18n.t("validations.sales.household.age.child_12_years_younger") + record.errors.add "relat#{person_num}", I18n.t("validations.sales.household.relat.child_12_years_younger") end end def validate_buyer_not_child(record) return unless record.saledate && record.form.start_year_after_2024? - record.errors.add "ecstat1", I18n.t("validations.household.ecstat.buyer_cannot_be_child", buyer_index: "1") if person_is_economic_child?(record.ecstat1) - record.errors.add "ecstat2", I18n.t("validations.household.ecstat.buyer_cannot_be_child", buyer_index: "2") if person_is_economic_child?(record.ecstat2) && record.joint_purchase? + record.errors.add "ecstat1", I18n.t("validations.sales.household.ecstat1.buyer_cannot_be_child") if person_is_economic_child?(record.ecstat1) + record.errors.add "ecstat2", I18n.t("validations.sales.household.ecstat2.buyer_cannot_be_child") if person_is_economic_child?(record.ecstat2) && record.joint_purchase? end private diff --git a/app/models/validations/shared_validations.rb b/app/models/validations/shared_validations.rb index 217b2c170..a2ae916c1 100644 --- a/app/models/validations/shared_validations.rb +++ b/app/models/validations/shared_validations.rb @@ -131,7 +131,11 @@ module Validations::SharedValidations partner_numbers = (2..max_people).select { |n| person_is_partner?(record["relat#{n}"]) } if partner_numbers.count > 1 partner_numbers.each do |n| - record.errors.add "relat#{n}", I18n.t("validations.household.relat.one_partner") + if record.sales? + record.errors.add "relat#{n}", I18n.t("validations.sales.household.relat.one_partner") + else + record.errors.add "relat#{n}", I18n.t("validations.household.relat.one_partner") + end end end end diff --git a/app/services/bulk_upload/sales/year2024/row_parser.rb b/app/services/bulk_upload/sales/year2024/row_parser.rb index fbc99ba02..35984a045 100644 --- a/app/services/bulk_upload/sales/year2024/row_parser.rb +++ b/app/services/bulk_upload/sales/year2024/row_parser.rb @@ -1469,10 +1469,10 @@ private def validate_buyer1_economic_status if field_35 == 9 if field_31.present? && field_31.to_i >= 16 - errors.add(:field_35, I18n.t("validations.household.ecstat.buyer_cannot_be_over_16_and_child", buyer_index: "1")) - errors.add(:field_31, I18n.t("validations.household.ecstat.buyer_cannot_be_over_16_and_child", buyer_index: "1")) + errors.add(:field_35, I18n.t("validations.sales.household.ecstat.buyer_cannot_be_over_16_and_child", buyer_index: "1")) + errors.add(:field_31, I18n.t("validations.sales.household.ecstat.buyer_cannot_be_over_16_and_child", buyer_index: "1")) else - errors.add(:field_35, I18n.t("validations.household.ecstat.buyer_cannot_be_child", buyer_index: "1")) + errors.add(:field_35, I18n.t("validations.sales.household.ecstat1.buyer_cannot_be_child")) end end end @@ -1482,10 +1482,10 @@ private if field_42 == 9 if field_38.present? && field_38.to_i >= 16 - errors.add(:field_42, I18n.t("validations.household.ecstat.buyer_cannot_be_over_16_and_child", buyer_index: "2")) - errors.add(:field_38, I18n.t("validations.household.ecstat.buyer_cannot_be_over_16_and_child", buyer_index: "2")) + errors.add(:field_42, I18n.t("validations.sales.household.ecstat.buyer_cannot_be_over_16_and_child", buyer_index: "2")) + errors.add(:field_38, I18n.t("validations.sales.household.ecstat.buyer_cannot_be_over_16_and_child", buyer_index: "2")) else - errors.add(:field_42, I18n.t("validations.household.ecstat.buyer_cannot_be_child", buyer_index: "2")) + errors.add(:field_42, I18n.t("validations.sales.household.ecstat2.buyer_cannot_be_child")) end end end diff --git a/config/locales/en.yml b/config/locales/en.yml index b39183591..7cff7aae7 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -501,11 +501,8 @@ en: retired_female: "A female tenant who is retired must be 60 or over." retired_over_70: "Answer cannot be over 70 as person %{person_num} has economic status that is not ‘retired’." child_under_16_relat_lettings: "Answer cannot be under 16 as person %{person_num}'s relationship to the lead tenant is ‘partner’." - child_under_16_relat_sales: "Answer cannot be under 16 as person %{person_num}'s relationship to buyer 1 is ‘partner’." child_under_16_ecstat: "Answer cannot be under 16 as person %{person_num}’s working situation is not ‘child under 16’, ‘other’ or ‘prefers not to say’." child_over_16: "Answer cannot be over 16 as person’s %{person_num} working situation is ‘child under 16‘." - child_over_20: "Answer cannot be 20 or over as the relationship is ‘child’." - child_12_years_younger: "A child must be at least 12 years younger than their parent." not_student_16_19: "Answer cannot be between 16 and 19 as person %{person_num} is a child of the lead tenant but is not a full-time student." student_16_19: cannot_be_16_19: @@ -528,12 +525,8 @@ en: retired_female: "Answer cannot be ‘retired’ as the female tenant is under 60." not_child_16_19: cannot_be_student: "Person cannot be a student if they are aged 16-19 but are not a child." - buyer_cannot_be_child: "Buyer %{buyer_index} cannot have a working situation of child under 16." - buyer_cannot_be_over_16_and_child: "Buyer %{buyer_index}'s age cannot be 16 or over if their working situation is child under 16." relat: - child_under_16_sales: "Answer cannot be ‘partner’ as you told us person %{person_num}'s age is under 16." child_under_16_lettings: "Answer cannot be ‘partner’ as you told us person %{person_num}'s age is under 16." - child_over_20: "Answer cannot be ‘child’ if the person's age is 20 or over." one_partner: "Number of partners cannot be greater than 1." not_student_16_19: "Answer cannot be ‘child’ as you told us the person %{person_num} is between 16 and 19 and is not a full-time student." student_16_19: @@ -556,7 +549,6 @@ en: internal_transfer: "Answer cannot be %{prevten} as this tenancy is an internal transfer." la_general_needs: internal_transfer: "Answer cannot be a fixed-term or lifetime local authority general needs tenancy as it’s an internal transfer and a private registered provider is on the tenancy agreement." - invalid_for_discounted_sale: "Buyer 1’s previous tenure should be “local authority tenant” or “private registered provider or housing association tenant” for discounted sales." referral: secure_tenancy: "Answer must be internal transfer as this is a secure tenancy." rsnvac_non_temp: "Answer cannot be this source of referral as this is a re-let to tenant who occupied the same property as temporary accommodation." @@ -596,9 +588,6 @@ 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." - buylivein: - buyers_will_live_in_property_values_inconsistent_setup: "You have already told us that both buyer 1 and buyer 2 will not live in the property." - buyers_will_live_in_property_values_inconsistent: "You have already told us that the buyers will live in the property. Either buyer 1 or buyer 2 must live in the property." nationality: "Select a valid nationality." tenancy: diff --git a/config/locales/validations/sales/household.en.yml b/config/locales/validations/sales/household.en.yml new file mode 100644 index 000000000..c1f90971c --- /dev/null +++ b/config/locales/validations/sales/household.en.yml @@ -0,0 +1,47 @@ +en: + validations: + sales: + household: + buylivein: + buyers_will_live_in_property_values_inconsistent: "You have already told us that both buyer 1 and buyer 2 will not live in the property." + buy1livein: + buyers_will_live_in_property_values_inconsistent: "You have already told us that the buyers will live in the property. Either buyer 1 or buyer 2 must live in the property." + buy2livein: + buyers_will_live_in_property_values_inconsistent: "You have already told us that the buyers will live in the property. Either buyer 1 or buyer 2 must live in the property." + ownershipsch: + prevten_invalid_for_discounted_sale: "Buyer 1’s previous tenure should be “local authority tenant” or “private registered provider or housing association tenant” for discounted sales." + prevten: + prevten_invalid_for_discounted_sale: "Buyer 1’s previous tenure should be “local authority tenant” or “private registered provider or housing association tenant” for discounted sales." + age1: + child_12_years_younger: "A child must be at least 12 years younger than their parent." + ecstat1: + buyer_cannot_be_child: "Buyer 1 cannot have a working situation of child under 16." + ecstat2: + buyer_cannot_be_child: "Buyer 2 cannot have a working situation of child under 16." + age: + child_12_years_younger: "A child must be at least 12 years younger than their parent." + child_under_16: "Answer cannot be under 16 as person %{person_num}'s relationship to buyer 1 is ‘partner’." + child_under_16_ecstat: "Answer cannot be under 16 as person %{person_num}’s working situation is not ‘child under 16’, ‘other’ or ‘prefers not to say’." + child_over_16: "Answer cannot be over 16 as person’s %{person_num} working situation is ‘child under 16‘." + child_over_20: "Answer cannot be 20 or over as the relationship is ‘child’." + student_16_19: + cannot_be_16_19: + child_not_student: "Person cannot be aged 16-19 if they have relationship ‘child’ but are not a student." + must_be_16_19: "Person must be aged 16-19 if they are a student and have relationship ‘child’." + relat: + one_partner: "Number of partners cannot be greater than 1." + child_12_years_younger: "A child must be at least 12 years younger than their parent." + child_under_16: "Answer cannot be ‘partner’ as you told us person %{person_num}'s age is under 16." + child_over_20: "Answer cannot be ‘child’ if the person's age is 20 or over." + student_16_19: + cannot_be_child: + student_not_16_19: "Answer cannot be ‘child’ if the person is a student but not aged 16-19." + 16_19_not_student: "Answer cannot be ‘child’ if the person is aged 16-19 but not a student." + ecstat: + child_under_16: "Person %{person_num}’s working situation must be ‘child under 16’, ‘other’ or ‘prefers not to say’ as you told us they’re under 16." + child_over_16: "Answer cannot be ‘child under 16’ as you told us the person %{person_num} is older than 16." + student_16_19: + must_be_student: "Person must be a student if they are aged 16-19 and have relationship ‘child’." + cannot_be_student: + child_not_16_19: "Person cannot be a student if they are not aged 16-19 but have relationship ‘child’." + buyer_cannot_be_over_16_and_child: "Buyer %{buyer_index}'s age cannot be 16 or over if their working situation is child under 16." diff --git a/spec/models/validations/sales/household_validations_spec.rb b/spec/models/validations/sales/household_validations_spec.rb index 510681420..cffb42ab1 100644 --- a/spec/models/validations/sales/household_validations_spec.rb +++ b/spec/models/validations/sales/household_validations_spec.rb @@ -15,11 +15,11 @@ RSpec.describe Validations::Sales::HouseholdValidations do record.relat3 = "P" household_validator.validate_partner_count(record) expect(record.errors["relat2"]) - .to include(match I18n.t("validations.household.relat.one_partner")) + .to include(match I18n.t("validations.sales.household.relat.one_partner")) expect(record.errors["relat3"]) - .to include(match I18n.t("validations.household.relat.one_partner")) + .to include(match I18n.t("validations.sales.household.relat.one_partner")) expect(record.errors["relat4"]) - .not_to include(match I18n.t("validations.household.relat.one_partner")) + .not_to include(match I18n.t("validations.sales.household.relat.one_partner")) end it "expects that a tenant can have a partner" do @@ -47,9 +47,9 @@ RSpec.describe Validations::Sales::HouseholdValidations do record.relat2 = "P" household_validator.validate_person_age_matches_relationship(record) expect(record.errors["relat2"]) - .to include(match I18n.t("validations.household.relat.child_under_16_sales", person_num: 2)) + .to include(match I18n.t("validations.sales.household.relat.child_under_16", person_num: 2)) expect(record.errors["age2"]) - .to include(match I18n.t("validations.household.age.child_under_16_relat_sales", person_num: 2)) + .to include(match I18n.t("validations.sales.household.age.child_under_16", person_num: 2)) end end @@ -58,9 +58,9 @@ RSpec.describe Validations::Sales::HouseholdValidations do record.relat2 = "C" household_validator.validate_person_age_matches_relationship(record) expect(record.errors["relat2"]) - .to include(match I18n.t("validations.household.relat.child_over_20")) + .to include(match I18n.t("validations.sales.household.relat.child_over_20")) expect(record.errors["age2"]) - .to include(match I18n.t("validations.household.age.child_over_20")) + .to include(match I18n.t("validations.sales.household.age.child_over_20")) end end @@ -94,9 +94,9 @@ RSpec.describe Validations::Sales::HouseholdValidations do record.ecstat2 = 1 household_validator.validate_person_age_matches_economic_status(record) expect(record.errors["ecstat2"]) - .to include(match I18n.t("validations.household.ecstat.child_under_16", person_num: 2)) + .to include(match I18n.t("validations.sales.household.ecstat.child_under_16", person_num: 2)) expect(record.errors["age2"]) - .to include(match I18n.t("validations.household.age.child_under_16_ecstat", person_num: 2)) + .to include(match I18n.t("validations.sales.household.age.child_under_16_ecstat", person_num: 2)) end it "expects that person's economic status is Child" do @@ -112,9 +112,9 @@ RSpec.describe Validations::Sales::HouseholdValidations do record.ecstat2 = 9 household_validator.validate_person_age_matches_economic_status(record) expect(record.errors["ecstat2"]) - .to include(match I18n.t("validations.household.ecstat.child_over_16", person_num: 2)) + .to include(match I18n.t("validations.sales.household.ecstat.child_over_16", person_num: 2)) expect(record.errors["age2"]) - .to include(match I18n.t("validations.household.age.child_over_16", person_num: 2)) + .to include(match I18n.t("validations.sales.household.age.child_over_16", person_num: 2)) end end @@ -126,9 +126,9 @@ RSpec.describe Validations::Sales::HouseholdValidations do record.ecstat2 = 1 household_validator.validate_person_age_matches_economic_status(record) expect(record.errors["ecstat2"]) - .not_to include(match I18n.t("validations.household.ecstat.child_under_16", person_num: 2)) + .not_to include(match I18n.t("validations.sales.household.ecstat.child_under_16", person_num: 2)) expect(record.errors["age2"]) - .not_to include(match I18n.t("validations.household.age.child_under_16_ecstat", person_num: 2)) + .not_to include(match I18n.t("validations.sales.household.age.child_under_16_ecstat", person_num: 2)) end end end @@ -143,11 +143,11 @@ RSpec.describe Validations::Sales::HouseholdValidations do record.relat2 = "C" household_validator.validate_child_12_years_younger(record) expect(record.errors["age1"]) - .to include(match I18n.t("validations.household.age.child_12_years_younger", person_num: 2)) + .to include(match I18n.t("validations.sales.household.age.child_12_years_younger", person_num: 2)) expect(record.errors["age2"]) - .to include(match I18n.t("validations.household.age.child_12_years_younger", person_num: 2)) + .to include(match I18n.t("validations.sales.household.age.child_12_years_younger", person_num: 2)) expect(record.errors["relat2"]) - .to include(match I18n.t("validations.household.age.child_12_years_younger", person_num: 2)) + .to include(match I18n.t("validations.sales.household.age.child_12_years_younger", person_num: 2)) end it "expects the child is at least 12 years younger than buyer 1" do @@ -206,11 +206,11 @@ RSpec.describe Validations::Sales::HouseholdValidations do record.relat2 = "C" household_validator.validate_person_age_and_relationship_matches_economic_status(record) expect(record.errors["relat2"]) - .to include(match I18n.t("validations.household.relat.student_16_19.cannot_be_child.16_19_not_student")) + .to include(match I18n.t("validations.sales.household.relat.student_16_19.cannot_be_child.16_19_not_student")) expect(record.errors["age2"]) - .to include(match I18n.t("validations.household.age.student_16_19.cannot_be_16_19.child_not_student")) + .to include(match I18n.t("validations.sales.household.age.student_16_19.cannot_be_16_19.child_not_student")) expect(record.errors["ecstat2"]) - .to include(match I18n.t("validations.household.ecstat.student_16_19.must_be_student")) + .to include(match I18n.t("validations.sales.household.ecstat.student_16_19.must_be_student")) end it "adds errors for a person who is a child of the buyer and a student but not aged 16-19" do @@ -219,11 +219,11 @@ RSpec.describe Validations::Sales::HouseholdValidations do record.relat2 = "C" household_validator.validate_person_age_and_relationship_matches_economic_status(record) expect(record.errors["relat2"]) - .to include(match I18n.t("validations.household.relat.student_16_19.cannot_be_child.student_not_16_19")) + .to include(match I18n.t("validations.sales.household.relat.student_16_19.cannot_be_child.student_not_16_19")) expect(record.errors["age2"]) - .to include(match I18n.t("validations.household.age.student_16_19.must_be_16_19")) + .to include(match I18n.t("validations.sales.household.age.student_16_19.must_be_16_19")) expect(record.errors["ecstat2"]) - .to include(match I18n.t("validations.household.ecstat.student_16_19.cannot_be_student.child_not_16_19")) + .to include(match I18n.t("validations.sales.household.ecstat.student_16_19.cannot_be_student.child_not_16_19")) end end @@ -316,9 +316,9 @@ RSpec.describe Validations::Sales::HouseholdValidations do it "triggers a validation if buyer two will also not live in the property" do sales_log.buy2livein = 2 household_validator.validate_buyers_living_in_property(sales_log) - expect(sales_log.errors[:buylivein]).to include I18n.t("validations.household.buylivein.buyers_will_live_in_property_values_inconsistent_setup") - expect(sales_log.errors[:buy2livein]).to include I18n.t("validations.household.buylivein.buyers_will_live_in_property_values_inconsistent") - expect(sales_log.errors[:buy1livein]).to include I18n.t("validations.household.buylivein.buyers_will_live_in_property_values_inconsistent") + expect(sales_log.errors[:buylivein]).to include I18n.t("validations.sales.household.buylivein.buyers_will_live_in_property_values_inconsistent") + expect(sales_log.errors[:buy2livein]).to include I18n.t("validations.sales.household.buy2livein.buyers_will_live_in_property_values_inconsistent") + expect(sales_log.errors[:buy1livein]).to include I18n.t("validations.sales.household.buy1livein.buyers_will_live_in_property_values_inconsistent") end end end From 9bd3389c846243bf9ba1bb81334661f2c5ff53ba Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Tue, 22 Oct 2024 16:27:51 +0100 Subject: [PATCH 3/6] CLDC-3655 Extract property information validations (#2700) * Extract property information validations * Refactor possesive variable from errors --- app/models/form/sales/questions/uprn.rb | 2 +- app/models/form/sales/questions/uprn_known.rb | 2 +- .../validations/sales/property_validations.rb | 15 ++++++----- config/locales/en.yml | 6 ----- .../sales/property_information.en.yml | 27 +++++++++++++++++++ 5 files changed, 37 insertions(+), 15 deletions(-) create mode 100644 config/locales/validations/sales/property_information.en.yml diff --git a/app/models/form/sales/questions/uprn.rb b/app/models/form/sales/questions/uprn.rb index 75a41a0ec..94aaccb43 100644 --- a/app/models/form/sales/questions/uprn.rb +++ b/app/models/form/sales/questions/uprn.rb @@ -16,7 +16,7 @@ class Form::Sales::Questions::Uprn < ::Form::Question end def unanswered_error_message - I18n.t("validations.property.uprn.invalid") + I18n.t("validations.sales.property_information.uprn.invalid") end def get_extra_check_answer_value(log) diff --git a/app/models/form/sales/questions/uprn_known.rb b/app/models/form/sales/questions/uprn_known.rb index 44ce03cad..fdd0c9e9a 100644 --- a/app/models/form/sales/questions/uprn_known.rb +++ b/app/models/form/sales/questions/uprn_known.rb @@ -31,7 +31,7 @@ class Form::Sales::Questions::UprnKnown < ::Form::Question }.freeze def unanswered_error_message - I18n.t("validations.property.uprn_known.invalid") + I18n.t("validations.sales.property_information.uprn_known.invalid") end QUESTION_NUMBER_FROM_YEAR = { 2023 => 14, 2024 => 15 }.freeze diff --git a/app/models/validations/sales/property_validations.rb b/app/models/validations/sales/property_validations.rb index 50818c422..59d616c9a 100644 --- a/app/models/validations/sales/property_validations.rb +++ b/app/models/validations/sales/property_validations.rb @@ -4,10 +4,11 @@ module Validations::Sales::PropertyValidations return unless record.ppostcode_full.present? && record.postcode_full.present? if record.discounted_ownership_sale? && record.ppostcode_full != record.postcode_full - record.errors.add :postcode_full, I18n.t("validations.property.postcode.must_match_previous", buyer_possessive: record.joint_purchase? ? "Buyers’" : "Buyer’s") - record.errors.add :ppostcode_full, I18n.t("validations.property.postcode.must_match_previous", buyer_possessive: record.joint_purchase? ? "Buyers’" : "Buyer’s") - record.errors.add :ownershipsch, I18n.t("validations.property.postcode.must_match_previous", buyer_possessive: record.joint_purchase? ? "Buyers’" : "Buyer’s") - record.errors.add :uprn, I18n.t("validations.property.postcode.must_match_previous", buyer_possessive: record.joint_purchase? ? "Buyers’" : "Buyer’s") + joint_purchase_id = record.joint_purchase? ? "joint_purchase" : "not_joint_purchase" + record.errors.add :postcode_full, I18n.t("validations.sales.property_information.postcode_full.postcode_must_match_previous.#{joint_purchase_id}") + record.errors.add :ppostcode_full, I18n.t("validations.sales.property_information.ppostcode_full.postcode_must_match_previous.#{joint_purchase_id}") + record.errors.add :ownershipsch, I18n.t("validations.sales.property_information.ownershipsch.postcode_must_match_previous.#{joint_purchase_id}") + record.errors.add :uprn, I18n.t("validations.sales.property_information.uprn.postcode_must_match_previous.#{joint_purchase_id}") end end @@ -15,8 +16,8 @@ module Validations::Sales::PropertyValidations return unless record.proptype.present? && record.beds.present? if record.is_bedsit? && record.beds > 1 - record.errors.add :proptype, I18n.t("validations.property.proptype.bedsits_have_max_one_bedroom") - record.errors.add :beds, I18n.t("validations.property.beds.bedsits_have_max_one_bedroom") + record.errors.add :proptype, I18n.t("validations.sales.property_information.proptype.bedsits_have_max_one_bedroom") + record.errors.add :beds, I18n.t("validations.sales.property_information.beds.bedsits_have_max_one_bedroom") end end @@ -25,6 +26,6 @@ module Validations::Sales::PropertyValidations return if record.uprn.match?(/^[0-9]{1,12}$/) - record.errors.add :uprn, I18n.t("validations.property.uprn.invalid") + record.errors.add :uprn, I18n.t("validations.sales.property_information.uprn.invalid") end end diff --git a/config/locales/en.yml b/config/locales/en.yml index 7cff7aae7..ff73bfdcb 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -382,12 +382,6 @@ en: one_bedroom_bedsit: "A bedsit can only have one bedroom." one_seven_bedroom_shared: "A shared house must have 1 to 7 bedrooms." one_three_bedroom_single_tenant_shared: "A shared house with fewer than two tenants must have 1 to 3 bedrooms." - beds: - bedsits_have_max_one_bedroom: "Number of bedrooms must be 1 if the property is a bedsit." - proptype: - bedsits_have_max_one_bedroom: "Answer cannot be 'Bedsit' if the property has 2 or more bedrooms." - postcode: - must_match_previous: "%{buyer_possessive} last accommodation and discounted ownership postcodes must match." financial: tshortfall: diff --git a/config/locales/validations/sales/property_information.en.yml b/config/locales/validations/sales/property_information.en.yml new file mode 100644 index 000000000..e69f90edf --- /dev/null +++ b/config/locales/validations/sales/property_information.en.yml @@ -0,0 +1,27 @@ +en: + validations: + sales: + property_information: + postcode_full: + postcode_must_match_previous: + joint_purchase: "Buyers’ last accommodation and discounted ownership postcodes must match." + not_joint_purchase: "Buyer’s last accommodation and discounted ownership postcodes must match." + ppostcode_full: + postcode_must_match_previous: + joint_purchase: "Buyers’ last accommodation and discounted ownership postcodes must match." + not_joint_purchase: "Buyer’s last accommodation and discounted ownership postcodes must match." + ownershipsch: + postcode_must_match_previous: + joint_purchase: "Buyers’ last accommodation and discounted ownership postcodes must match." + not_joint_purchase: "Buyer’s last accommodation and discounted ownership postcodes must match." + uprn: + postcode_must_match_previous: + joint_purchase: "Buyers’ last accommodation and discounted ownership postcodes must match." + not_joint_purchase: "Buyer’s last accommodation and discounted ownership postcodes must match." + invalid: "UPRN must be 12 digits or less." + beds: + bedsits_have_max_one_bedroom: "Number of bedrooms must be 1 if the property is a bedsit." + proptype: + bedsits_have_max_one_bedroom: "Answer cannot be 'Bedsit' if the property has 2 or more bedrooms." + uprn_known: + invalid: "You must answer UPRN known?" From e71b2aeb9f687c9ac41d2b3e5993d5867a6f4690 Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Wed, 23 Oct 2024 10:21:41 +0100 Subject: [PATCH 4/6] CLDC-3638 Allow adding custom resources (#2690) * Stub some collection resource requests * Rebase changes * Add upload new resource page * Allow adding custom collection resources * Display and validate additional resources * Allow downloading additional collection resources * Allow updating additional resources * Display additional resources on the homepage * Add delete link to additional resources * Update hint text * Update tests * Allow unauthenticated users to donwload additional collection resources * Add some changes to copy and styling * Add content type to the files * Validate short display name at the same time as file --- .../collection_resources_controller.rb | 122 +++++-- app/controllers/start_controller.rb | 2 + app/helpers/collection_resources_helper.rb | 2 +- app/models/collection_resource.rb | 31 +- app/services/collection_resources_service.rb | 3 +- .../mandatory_collection_resources_service.rb | 1 + app/services/storage/local_disk_service.rb | 2 +- app/services/storage/s3_service.rb | 21 +- .../_collection_resource_summary_list.erb | 19 +- app/views/collection_resources/edit.html.erb | 21 +- app/views/collection_resources/index.html.erb | 4 +- app/views/collection_resources/new.html.erb | 36 ++ .../layouts/_collection_resources.html.erb | 4 +- config/locales/en.yml | 7 +- config/routes.rb | 8 +- spec/factories/collection_resource.rb | 10 + spec/features/collection_resources_spec.rb | 73 ++++ .../collection_resources_helper_spec.rb | 12 +- .../collection_resources_controller_spec.rb | 342 +++++++++++++++++- spec/requests/start_controller_spec.rb | 2 + .../collection_resources_service_spec.rb | 2 +- 21 files changed, 668 insertions(+), 56 deletions(-) create mode 100644 app/views/collection_resources/new.html.erb diff --git a/app/controllers/collection_resources_controller.rb b/app/controllers/collection_resources_controller.rb index ed080c749..5203ceea1 100644 --- a/app/controllers/collection_resources_controller.rb +++ b/app/controllers/collection_resources_controller.rb @@ -1,13 +1,15 @@ class CollectionResourcesController < ApplicationController include CollectionResourcesHelper - before_action :authenticate_user!, except: %i[download_mandatory_collection_resource] + before_action :authenticate_user!, except: %i[download_mandatory_collection_resource download_additional_collection_resource] def index render_not_found unless current_user.support? @mandatory_lettings_collection_resources_per_year = MandatoryCollectionResourcesService.generate_resources("lettings", editable_collection_resource_years) @mandatory_sales_collection_resources_per_year = MandatoryCollectionResourcesService.generate_resources("sales", editable_collection_resource_years) + @additional_lettings_collection_resources_per_year = CollectionResource.where(log_type: "lettings", mandatory: false).group_by(&:year) + @additional_sales_collection_resources_per_year = CollectionResource.where(log_type: "sales", mandatory: false).group_by(&:year) end def download_mandatory_collection_resource @@ -23,7 +25,16 @@ class CollectionResourcesController < ApplicationController download_resource(resource.download_filename) end - def edit + def download_additional_collection_resource + resource = CollectionResource.find_by(id: params[:collection_resource_id]) + + return render_not_found unless resource + return render_not_found unless resource_for_year_can_be_downloaded?(resource.year) + + download_resource(resource.download_filename) + end + + def edit_mandatory_collection_resource return render_not_found unless current_user.support? year = params[:year].to_i @@ -39,7 +50,18 @@ class CollectionResourcesController < ApplicationController render "collection_resources/edit" end - def update + def edit_additional_collection_resource + return render_not_found unless current_user.support? + + @collection_resource = CollectionResource.find_by(id: params[:collection_resource_id]) + + return render_not_found unless @collection_resource + return render_not_found unless resource_for_year_can_be_updated?(@collection_resource.year) + + render "collection_resources/edit" + end + + def update_mandatory_collection_resource return render_not_found unless current_user.support? year = resource_params[:year].to_i @@ -52,7 +74,8 @@ class CollectionResourcesController < ApplicationController @collection_resource = MandatoryCollectionResourcesService.generate_resource(log_type, year, resource_type) render_not_found unless @collection_resource - validate_file(file) + @collection_resource.file = file + @collection_resource.validate_attached_file return render "collection_resources/edit" if @collection_resource.errors.any? @@ -68,6 +91,36 @@ class CollectionResourcesController < ApplicationController redirect_to collection_resources_path end + def update_additional_collection_resource + return render_not_found unless current_user.support? + + @collection_resource = CollectionResource.find_by(id: params[:collection_resource_id]) + + return render_not_found unless @collection_resource + return render_not_found unless resource_for_year_can_be_updated?(@collection_resource.year) + + @collection_resource.file = resource_params[:file] + @collection_resource.validate_attached_file + @collection_resource.validate_short_display_name + return render "collection_resources/edit" if @collection_resource.errors.any? + + @collection_resource.short_display_name = resource_params[:short_display_name] + @collection_resource.download_filename = @collection_resource.file&.original_filename + @collection_resource.display_name = "#{@collection_resource.log_type} #{@collection_resource.short_display_name} (#{text_year_range_format(@collection_resource.year)})" + if @collection_resource.save + begin + CollectionResourcesService.new.upload_collection_resource(@collection_resource.download_filename, @collection_resource.file) + flash[:notice] = "The #{@collection_resource.log_type} #{text_year_range_format(@collection_resource.year)} #{@collection_resource.short_display_name.downcase} has been updated." + redirect_to collection_resources_path + rescue StandardError + @collection_resource.errors.add(:file, :error_uploading) + render "collection_resources/edit" + end + else + render "collection_resources/edit" + end + end + def confirm_mandatory_collection_resources_release return render_not_found unless current_user.support? @@ -91,10 +144,50 @@ class CollectionResourcesController < ApplicationController redirect_to collection_resources_path end + def new + return render_not_found unless current_user.support? + + year = params[:year].to_i + log_type = params[:log_type] + + return render_not_found unless editable_collection_resource_years.include?(year) + + @collection_resource = CollectionResource.new(year:, log_type:) + end + + def create + return render_not_found unless current_user.support? && editable_collection_resource_years.include?(resource_params[:year].to_i) + + @collection_resource = CollectionResource.new(resource_params) + @collection_resource.download_filename ||= @collection_resource.file&.original_filename + @collection_resource.display_name = "#{@collection_resource.log_type} #{@collection_resource.short_display_name} (#{text_year_range_format(@collection_resource.year)})" + + @collection_resource.validate_attached_file + @collection_resource.validate_short_display_name + return render "collection_resources/new" if @collection_resource.errors.any? + + if @collection_resource.save + begin + CollectionResourcesService.new.upload_collection_resource(@collection_resource.download_filename, @collection_resource.file) + flash[:notice] = if displayed_collection_resource_years.include?(@collection_resource.year) + "The #{@collection_resource.log_type} #{text_year_range_format(@collection_resource.year)} #{@collection_resource.short_display_name} is now available to users." + else + "The #{@collection_resource.log_type} #{text_year_range_format(@collection_resource.year)} #{@collection_resource.short_display_name} has been uploaded." + end + redirect_to collection_resources_path + rescue StandardError + @collection_resource.errors.add(:file, :error_uploading) + render "collection_resources/new" + end + else + render "collection_resources/new" + end + end + private def resource_params - params.require(:collection_resource).permit(:year, :log_type, :resource_type, :file) + params.require(:collection_resource).permit(:year, :log_type, :resource_type, :file, :mandatory, :short_display_name) end def download_resource(filename) @@ -113,23 +206,4 @@ private def resource_for_year_can_be_updated?(year) editable_collection_resource_years.include?(year) end - - def validate_file(file) - return @collection_resource.errors.add(:file, :blank) unless file - return @collection_resource.errors.add(:file, :above_100_mb) if file.size > 100.megabytes - - argv = %W[file --brief --mime-type -- #{file.path}] - output = `#{argv.shelljoin}` - - case @collection_resource.resource_type - when "paper_form" - unless output.match?(/application\/pdf/) - @collection_resource.errors.add(:file, :must_be_pdf) - end - when "bulk_upload_template", "bulk_upload_specification" - unless output.match?(/application\/vnd\.ms-excel|application\/vnd\.openxmlformats-officedocument\.spreadsheetml\.sheet/) - @collection_resource.errors.add(:file, :must_be_xlsx, resource: @collection_resource.short_display_name.downcase) - end - end - end end diff --git a/app/controllers/start_controller.rb b/app/controllers/start_controller.rb index 5bd49df3f..b65da4d44 100644 --- a/app/controllers/start_controller.rb +++ b/app/controllers/start_controller.rb @@ -4,6 +4,8 @@ class StartController < ApplicationController def index @mandatory_lettings_collection_resources_per_year = MandatoryCollectionResourcesService.generate_resources("lettings", displayed_collection_resource_years) @mandatory_sales_collection_resources_per_year = MandatoryCollectionResourcesService.generate_resources("sales", displayed_collection_resource_years) + @additional_lettings_collection_resources_per_year = CollectionResource.where(log_type: "lettings", mandatory: false, year: displayed_collection_resource_years).group_by(&:year) + @additional_sales_collection_resources_per_year = CollectionResource.where(log_type: "sales", mandatory: false, year: displayed_collection_resource_years).group_by(&:year) if current_user @homepage_presenter = HomepagePresenter.new(current_user) render "home/index" diff --git a/app/helpers/collection_resources_helper.rb b/app/helpers/collection_resources_helper.rb index 9deb13887..1a5887744 100644 --- a/app/helpers/collection_resources_helper.rb +++ b/app/helpers/collection_resources_helper.rb @@ -49,7 +49,7 @@ module CollectionResourcesHelper def document_list_component_items(resources) resources.map do |resource| { - name: "Download the #{resource.display_name}", + name: "Download the #{resource.display_name.downcase}", href: resource.download_path, metadata: file_type_size_and_pages(resource.download_filename), } diff --git a/app/models/collection_resource.rb b/app/models/collection_resource.rb index 81665ccf6..cc217b8cd 100644 --- a/app/models/collection_resource.rb +++ b/app/models/collection_resource.rb @@ -3,7 +3,36 @@ class CollectionResource < ApplicationRecord attr_accessor :file + validates :short_display_name, presence: true + def download_path - download_mandatory_collection_resource_path(log_type:, year:, resource_type:) + if mandatory + download_mandatory_collection_resource_path(log_type:, year:, resource_type:) + else + collection_resource_download_path(self) + end + end + + def validate_attached_file + return errors.add(:file, :blank) unless file + return errors.add(:file, :above_100_mb) if file.size > 100.megabytes + + argv = %W[file --brief --mime-type -- #{file.path}] + output = `#{argv.shelljoin}` + + case resource_type + when "paper_form" + unless output.match?(/application\/pdf/) + errors.add(:file, :must_be_pdf) + end + when "bulk_upload_template", "bulk_upload_specification" + unless output.match?(/application\/vnd\.ms-excel|application\/vnd\.openxmlformats-officedocument\.spreadsheetml\.sheet/) + errors.add(:file, :must_be_xlsx, resource: short_display_name.downcase) + end + end + end + + def validate_short_display_name + errors.add(:short_display_name, :blank) if short_display_name.blank? end end diff --git a/app/services/collection_resources_service.rb b/app/services/collection_resources_service.rb index 81ab08254..82247f7a0 100644 --- a/app/services/collection_resources_service.rb +++ b/app/services/collection_resources_service.rb @@ -24,6 +24,7 @@ class CollectionResourcesService end def upload_collection_resource(filename, file) - @storage_service.write_file(filename, file) + content_type = MiniMime.lookup_by_filename(filename)&.content_type + @storage_service.write_file(filename, file, content_type:) end end diff --git a/app/services/mandatory_collection_resources_service.rb b/app/services/mandatory_collection_resources_service.rb index 8b34153e9..397e4b5d0 100644 --- a/app/services/mandatory_collection_resources_service.rb +++ b/app/services/mandatory_collection_resources_service.rb @@ -27,6 +27,7 @@ class MandatoryCollectionResourcesService year:, log_type:, download_filename: download_filename(resource_type, year, log_type), + mandatory: true, ) end diff --git a/app/services/storage/local_disk_service.rb b/app/services/storage/local_disk_service.rb index bb5825340..ad3cc9608 100644 --- a/app/services/storage/local_disk_service.rb +++ b/app/services/storage/local_disk_service.rb @@ -19,7 +19,7 @@ module Storage File.open(path, "r") end - def write_file(filename, data) + def write_file(filename, data, _content_type: nil) path = Rails.root.join("tmp/storage", filename) FileUtils.mkdir_p(path.dirname) diff --git a/app/services/storage/s3_service.rb b/app/services/storage/s3_service.rb index 88199c0a0..2e8daa719 100644 --- a/app/services/storage/s3_service.rb +++ b/app/services/storage/s3_service.rb @@ -36,12 +36,21 @@ module Storage .body.read end - def write_file(file_name, data) - @client.put_object( - body: data, - bucket: @configuration.bucket_name, - key: file_name, - ) + def write_file(file_name, data, content_type: nil) + if content_type.nil? + @client.put_object( + body: data, + bucket: @configuration.bucket_name, + key: file_name, + ) + else + @client.put_object( + body: data, + bucket: @configuration.bucket_name, + key: file_name, + content_type:, + ) + end end def get_file_metadata(file_name) diff --git a/app/views/collection_resources/_collection_resource_summary_list.erb b/app/views/collection_resources/_collection_resource_summary_list.erb index 8ef588fd5..0e630f0aa 100644 --- a/app/views/collection_resources/_collection_resource_summary_list.erb +++ b/app/views/collection_resources/_collection_resource_summary_list.erb @@ -23,9 +23,26 @@ <% end %> <% end %> <% end %> + <% additional_resources&.each do |resource| %> + <% summary_list.with_row do |row| %> + <% row.with_key { resource.short_display_name } %> + <% row.with_value do %> + <%= render DocumentListComponent.new(items: document_list_edit_component_items([resource]), label: "") %> + <% end %> + <% row.with_action( + text: "Change", + href: collection_resource_edit_path(resource), + ) %> + <% row.with_action( + text: "Delete", + href: "/", + classes: "app-!-colour-red" + ) %> + <% end %> + <% end %> <% end %>
- <%= govuk_link_to "Add new #{mandatory_resources.first.log_type} #{text_year_range_format(mandatory_resources.first.year)} resource", href: "/" %> + <%= govuk_link_to "Add new #{mandatory_resources.first.log_type} #{text_year_range_format(mandatory_resources.first.year)} resource", href: new_collection_resource_path(year: mandatory_resources.first.year, log_type: mandatory_resources.first.log_type) %>
diff --git a/app/views/collection_resources/edit.html.erb b/app/views/collection_resources/edit.html.erb index e5b2a6d33..6100256c8 100644 --- a/app/views/collection_resources/edit.html.erb +++ b/app/views/collection_resources/edit.html.erb @@ -5,7 +5,7 @@
<% resource_exists = file_exists_on_s3?(@collection_resource.download_filename) %> - <%= form_with model: @collection_resource, url: update_mandatory_collection_resource_path, method: :patch do |f| %> + <%= form_with model: @collection_resource, url: @collection_resource.mandatory ? update_mandatory_collection_resource_path : collection_resource_update_path(@collection_resource), method: :patch do |f| %> <%= f.hidden_field :year %> <%= f.hidden_field :log_type %> <%= f.hidden_field :resource_type %> @@ -13,7 +13,7 @@ <%= f.govuk_error_summary %> <%= "#{@collection_resource.log_type.humanize} #{text_year_range_format(@collection_resource.year)}" %> -

<%= resource_exists ? "Change" : "Upload" %> the <%= @collection_resource.resource_type.humanize.downcase %>

+

<%= resource_exists ? "Change" : "Upload" %> the <%= @collection_resource.short_display_name.downcase %>

This file will be available for all users to download. @@ -21,9 +21,22 @@ <%= f.govuk_file_field :file, label: { text: "Upload file", size: "m" } %> + <% if resource_exists %> +

Current file: <%= govuk_link_to @collection_resource.download_filename, href: @collection_resource.download_path %>

+ <% end %> - <%= f.govuk_submit resource_exists ? "Save changes" : "Upload" %> - <%= govuk_button_link_to "Cancel", collection_resources_path, secondary: true %> + <% unless @collection_resource.mandatory %> + <%= f.govuk_text_field :short_display_name, + label: { text: "Resource type", size: "m" }, + hint: { text: safe_join(["This will be used in the download link on the homepage. Do not include the log type or collection year.", + content_tag(:br), + "For example, if you enter “bulk upload change log”, the download link will say “Download the #{@collection_resource.log_type} bulk upload change log (#{text_year_range_format(@collection_resource.year)})”."]) } %> + <% end %> + +
+ <%= f.govuk_submit resource_exists ? "Save changes" : "Upload" %> + <%= govuk_button_link_to "Cancel", collection_resources_path, secondary: true %> +
<% end %>
diff --git a/app/views/collection_resources/index.html.erb b/app/views/collection_resources/index.html.erb index f6785eb46..207a63991 100644 --- a/app/views/collection_resources/index.html.erb +++ b/app/views/collection_resources/index.html.erb @@ -18,12 +18,12 @@

Lettings <%= text_year_range_format(year) %>

- <%= render partial: "collection_resource_summary_list", locals: { mandatory_resources: } %> + <%= render partial: "collection_resource_summary_list", locals: { mandatory_resources:, additional_resources: @additional_lettings_collection_resources_per_year[year] } %> <% end %> <% @mandatory_sales_collection_resources_per_year.each do |year, mandatory_resources| %>

Sales <%= text_year_range_format(year) %>

- <%= render partial: "collection_resource_summary_list", locals: { mandatory_resources: } %> + <%= render partial: "collection_resource_summary_list", locals: { mandatory_resources:, additional_resources: @additional_sales_collection_resources_per_year[year] } %> <% end %> diff --git a/app/views/collection_resources/new.html.erb b/app/views/collection_resources/new.html.erb new file mode 100644 index 000000000..f66c56b94 --- /dev/null +++ b/app/views/collection_resources/new.html.erb @@ -0,0 +1,36 @@ +<% content_for :before_content do %> + <%= govuk_back_link href: collection_resources_path %> +<% end %> + +
+
+ <%= form_with model: @collection_resource, url: collection_resources_path, method: :post do |f| %> + <%= f.hidden_field :year %> + <%= f.hidden_field :log_type %> + <%= f.hidden_field :mandatory, value: false %> + + <%= f.govuk_error_summary %> + + <%= "#{@collection_resource.log_type.humanize} #{text_year_range_format(@collection_resource.year)}" %> +

Add a new collection resource

+ +

+ This file will be available for all users to download. +

+ + <%= f.govuk_file_field :file, + label: { text: "Upload file", size: "m" } %> + + <%= f.govuk_text_field :short_display_name, + label: { text: "Resource type", size: "m" }, + hint: { text: safe_join(["This will be used in the download link on the homepage. Do not include the log type or collection year.", + content_tag(:br), + "For example, if you enter “bulk upload change log”, the download link will say “Download the #{@collection_resource.log_type} bulk upload change log (#{text_year_range_format(@collection_resource.year)})”."]) } %> + +
+ <%= f.govuk_submit "Add resource" %> + <%= govuk_button_link_to "Cancel", collection_resources_path, secondary: true %> +
+ <% end %> +
+
diff --git a/app/views/layouts/_collection_resources.html.erb b/app/views/layouts/_collection_resources.html.erb index 935f1806c..07648e5c0 100644 --- a/app/views/layouts/_collection_resources.html.erb +++ b/app/views/layouts/_collection_resources.html.erb @@ -12,12 +12,12 @@ <%= govuk_tabs(title: "Collection resources", classes: %w[app-tab__small-headers]) do |c| %> <% @mandatory_lettings_collection_resources_per_year.each do |year, resources| %> <% c.with_tab(label: "Lettings #{year_range_format(year)}") do %> - <%= render DocumentListComponent.new(items: document_list_component_items(resources), label: "Lettings #{text_year_range_format(year)}") %> + <%= render DocumentListComponent.new(items: document_list_component_items(resources.concat(@additional_lettings_collection_resources_per_year[year] || [])), label: "Lettings #{text_year_range_format(year)}") %> <% end %> <% end %> <% @mandatory_sales_collection_resources_per_year.each do |year, resources| %> <% c.with_tab(label: "Sales #{year_range_format(year)}") do %> - <%= render DocumentListComponent.new(items: document_list_component_items(resources), label: "Sales #{text_year_range_format(year)}") %> + <%= render DocumentListComponent.new(items: document_list_component_items(resources.concat(@additional_sales_collection_resources_per_year[year] || [])), label: "Sales #{text_year_range_format(year)}") %> <% end %> <% end %> <% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index ff73bfdcb..d490fd8ae 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -122,7 +122,7 @@ en: file: error_uploading: There was an error uploading this file. blank: Select which file to upload. - above_100_mb: The file is above 100MB. + above_100_mb: File must be 100MB or less. must_be_pdf: The paper form must be a PDF. must_be_xlsx: The %{resource} must be a Microsoft Excel file. @@ -212,9 +212,12 @@ en: file: error_uploading: There was an error uploading this file. blank: Select which file to upload. - above_100_mb: The file is above 100MB. + above_100_mb: File must be 100MB or less. must_be_pdf: The paper form must be a PDF. must_be_xlsx: The %{resource} must be a Microsoft Excel file. + short_display_name: + blank: "You must answer resource type." + notification: logs_deleted: one: "%{count} log has been deleted." diff --git a/config/routes.rb b/config/routes.rb index db4b2a8a2..cbac4894a 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -42,13 +42,15 @@ Rails.application.routes.draw do get "collection-resources", to: "collection_resources#index" get "/collection-resources/:log_type/:year/:resource_type/download", to: "collection_resources#download_mandatory_collection_resource", as: :download_mandatory_collection_resource - get "/collection-resources/:log_type/:year/:resource_type/edit", to: "collection_resources#edit", as: :edit_mandatory_collection_resource - patch "/collection-resources", to: "collection_resources#update", as: :update_mandatory_collection_resource + get "/collection-resources/:log_type/:year/:resource_type/edit", to: "collection_resources#edit_mandatory_collection_resource", as: :edit_mandatory_collection_resource + patch "/collection-resources", to: "collection_resources#update_mandatory_collection_resource", as: :update_mandatory_collection_resource get "/collection-resources/:year/release", to: "collection_resources#confirm_mandatory_collection_resources_release", as: :confirm_mandatory_collection_resources_release patch "/collection-resources/:year/release", to: "collection_resources#release_mandatory_collection_resources", as: :release_mandatory_collection_resources resources :collection_resources, path: "/collection-resources" do - get "/download", to: "collection_resources#download_additional_collection_resource" # when we get to adding them + get "/download", to: "collection_resources#download_additional_collection_resource" + get "/edit", to: "collection_resources#edit_additional_collection_resource" + patch "/update", to: "collection_resources#update_additional_collection_resource" end get "clear-filters", to: "sessions#clear_filters" diff --git a/spec/factories/collection_resource.rb b/spec/factories/collection_resource.rb index c352b70a9..0282e33e3 100644 --- a/spec/factories/collection_resource.rb +++ b/spec/factories/collection_resource.rb @@ -6,5 +6,15 @@ FactoryBot.define do year { 2024 } log_type { "lettings" } download_filename { "24_25_lettings_paper_form.pdf" } + mandatory { true } + trait(:additional) do + resource_type { nil } + display_name { "lettings additional resource (2024 to 2025)" } + short_display_name { "additional resource" } + year { 2024 } + log_type { "lettings" } + download_filename { "additional.pdf" } + mandatory { false } + end end end diff --git a/spec/features/collection_resources_spec.rb b/spec/features/collection_resources_spec.rb index 643f3537e..ad9a370b6 100644 --- a/spec/features/collection_resources_spec.rb +++ b/spec/features/collection_resources_spec.rb @@ -5,6 +5,9 @@ RSpec.describe "Collection resources" do let(:collection_resources_service) { instance_double(CollectionResourcesService, file_exists_on_s3?: true) } before do + # rubocop:disable RSpec/AnyInstance + allow_any_instance_of(CollectionResourcesHelper).to receive(:editable_collection_resource_years).and_return([2024, 2025]) + # rubocop:enable RSpec/AnyInstance allow(CollectionResourcesService).to receive(:new).and_return(collection_resources_service) allow(collection_resources_service).to receive(:upload_collection_resource) allow(collection_resources_service).to receive(:get_file_metadata).and_return({ "content_type" => "application/pdf", "content_length" => 1000 }) @@ -165,4 +168,74 @@ RSpec.describe "Collection resources" do expect(page).to have_content("There was an error uploading this file.") end end + + context "when uploading an additional resource" do + it "allows valid files" do + expect(CollectionResource.count).to eq(0) + + visit(new_collection_resource_path(year: 2025, log_type: "sales")) + fill_in("collection_resource[short_display_name]", with: "some file") + attach_file "file", file_fixture("pdf_file.pdf") + + click_button("Add resource") + expect(collection_resources_service).to have_received(:upload_collection_resource).with("pdf_file.pdf", anything) + expect(CollectionResource.count).to eq(1) + expect(CollectionResource.first.year).to eq(2025) + expect(CollectionResource.first.log_type).to eq("sales") + expect(CollectionResource.first.resource_type).to be_nil + expect(CollectionResource.first.mandatory).to be_falsey + expect(CollectionResource.first.released_to_user).to be_nil + expect(CollectionResource.first.display_name).to eq("sales some file (2025 to 2026)") + expect(CollectionResource.first.short_display_name).to eq("some file") + expect(page).to have_content("The sales 2025 to 2026 some file has been uploaded.") + end + + it "validates file is attached" do + visit(new_collection_resource_path(year: 2025, log_type: "sales")) + + fill_in("collection_resource[short_display_name]", with: "some file") + click_button("Add resource") + expect(page).to have_content("Select which file to upload.") + end + + it "validates resource type is given" do + visit(new_collection_resource_path(year: 2025, log_type: "sales")) + + attach_file "file", file_fixture("pdf_file.pdf") + click_button("Add resource") + expect(page).to have_content("You must answer resource type.") + end + end + + context "when updating an additional resource" do + let!(:collection_resource) { create(:collection_resource, :additional, year: 2025, log_type: "sales") } + + it "only allows valid files" do + expect(CollectionResource.count).to eq(1) + + visit(collection_resource_edit_path(collection_resource)) + fill_in("collection_resource[short_display_name]", with: "some updated file") + attach_file "file", file_fixture("pdf_file.pdf") + + click_button("Save changes") + expect(collection_resources_service).to have_received(:upload_collection_resource).with("pdf_file.pdf", anything) + expect(CollectionResource.count).to eq(1) + expect(CollectionResource.first.year).to eq(2025) + expect(CollectionResource.first.log_type).to eq("sales") + expect(CollectionResource.first.resource_type).to be_nil + expect(CollectionResource.first.mandatory).to be_falsey + expect(CollectionResource.first.released_to_user).to be_nil + expect(CollectionResource.first.display_name).to eq("sales some updated file (2025 to 2026)") + expect(CollectionResource.first.short_display_name).to eq("some updated file") + expect(page).to have_content("The sales 2025 to 2026 some updated file has been updated.") + end + + it "validates file is attached" do + visit(collection_resource_edit_path(collection_resource)) + + fill_in("collection_resource[short_display_name]", with: "some file") + click_button("Save changes") + expect(page).to have_content("Select which file to upload.") + end + end end diff --git a/spec/helpers/collection_resources_helper_spec.rb b/spec/helpers/collection_resources_helper_spec.rb index 859d14cd1..9fc77a4c9 100644 --- a/spec/helpers/collection_resources_helper_spec.rb +++ b/spec/helpers/collection_resources_helper_spec.rb @@ -94,9 +94,9 @@ RSpec.describe CollectionResourcesHelper do context "and next year resources were manually released" do before do - CollectionResource.create!(year: 2025, resource_type: "paper_form", display_name: "lettings log for tenants (2025 to 2026)", download_filename: "file.pdf", mandatory: true, released_to_user: true) - CollectionResource.create!(year: 2025, resource_type: "bulk_upload_template", display_name: "bulk upload template (2025 to 2026)", download_filename: "file.xlsx", mandatory: true, released_to_user: true) - CollectionResource.create!(year: 2025, resource_type: "bulk_upload_specification", display_name: "sales log for tenants (2025 to 2026)", download_filename: "file.xlsx", mandatory: true, released_to_user: true) + create(:collection_resource, year: 2025, resource_type: "paper_form", display_name: "lettings log for tenants (2025 to 2026)", download_filename: "file.pdf", mandatory: true, released_to_user: true) + create(:collection_resource, year: 2025, resource_type: "bulk_upload_template", display_name: "bulk upload template (2025 to 2026)", download_filename: "file.xlsx", mandatory: true, released_to_user: true) + create(:collection_resource, year: 2025, resource_type: "bulk_upload_specification", display_name: "sales log for tenants (2025 to 2026)", download_filename: "file.xlsx", mandatory: true, released_to_user: true) end it "reutrns current and next years" do @@ -199,9 +199,9 @@ RSpec.describe CollectionResourcesHelper do context "and the resources have been manually released" do before do - CollectionResource.create!(year: 2025, resource_type: "paper_form", display_name: "lettings log for tenants (2025 to 2026)", download_filename: "file.pdf", mandatory: true, released_to_user: true) - CollectionResource.create!(year: 2025, resource_type: "bulk_upload_template", display_name: "bulk upload template (2025 to 2026)", download_filename: "file.xlsx", mandatory: true, released_to_user: true) - CollectionResource.create!(year: 2025, resource_type: "bulk_upload_specification", display_name: "sales log for tenants (2025 to 2026)", download_filename: "file.xlsx", mandatory: true, released_to_user: true) + create(:collection_resource, year: 2025, resource_type: "paper_form", display_name: "lettings log for tenants (2025 to 2026)", download_filename: "file.pdf", mandatory: true, released_to_user: true) + create(:collection_resource, year: 2025, resource_type: "bulk_upload_template", display_name: "bulk upload template (2025 to 2026)", download_filename: "file.xlsx", mandatory: true, released_to_user: true) + create(:collection_resource, year: 2025, resource_type: "bulk_upload_specification", display_name: "sales log for tenants (2025 to 2026)", download_filename: "file.xlsx", mandatory: true, released_to_user: true) end it "returns false" do diff --git a/spec/requests/collection_resources_controller_spec.rb b/spec/requests/collection_resources_controller_spec.rb index 7c9bb0f05..a6d88f911 100644 --- a/spec/requests/collection_resources_controller_spec.rb +++ b/spec/requests/collection_resources_controller_spec.rb @@ -62,7 +62,7 @@ RSpec.describe CollectionResourcesController, type: :request do expect(page).to have_content("Sales 2025 to 2026") end - it "displays mandatory filed" do + it "displays mandatory files" do get collection_resources_path expect(page).to have_content("Paper form") @@ -70,6 +70,15 @@ RSpec.describe CollectionResourcesController, type: :request do expect(page).to have_content("Bulk upload specification") end + it "allows uploading new resources" do + get collection_resources_path + + expect(page).to have_link("Add new sales 2024 to 2025 resource", href: new_collection_resource_path(year: 2024, log_type: "sales")) + expect(page).to have_link("Add new lettings 2024 to 2025 resource", href: new_collection_resource_path(year: 2024, log_type: "lettings")) + expect(page).to have_link("Add new sales 2025 to 2026 resource", href: new_collection_resource_path(year: 2025, log_type: "sales")) + expect(page).to have_link("Add new lettings 2025 to 2026 resource", href: new_collection_resource_path(year: 2025, log_type: "lettings")) + end + context "when files are on S3" do before do allow(storage_service).to receive(:file_exists?).and_return(true) @@ -113,6 +122,16 @@ RSpec.describe CollectionResourcesController, type: :request do expect(page).to have_content("The 2025 to 2026 collection resources are not yet available to users.") expect(page).to have_link("Release the 2025 to 2026 collection resources to users", href: confirm_mandatory_collection_resources_release_path(year: 2025)) end + + context "when there are additional resources" do + let!(:collection_resource) { create(:collection_resource, :additional, year: 2025, short_display_name: "additional resource", download_filename: "additional.pdf") } + + it "displays change links for additional resources" do + get collection_resources_path + + expect(page).to have_link("Change", href: collection_resource_edit_path(collection_resource)) + end + end end context "when files are not on S3" do @@ -147,6 +166,26 @@ RSpec.describe CollectionResourcesController, type: :request do expect(page).to have_content("Once you have uploaded all the required 2025 to 2026 collection resources, you will be able to release them to users.") end end + + context "when there are additional resources" do + let!(:collection_resource) { create(:collection_resource, :additional, year: 2025, short_display_name: "additional resource", download_filename: "additional.pdf") } + + before do + # rubocop:disable RSpec/AnyInstance + allow_any_instance_of(CollectionResourcesHelper).to receive(:editable_collection_resource_years).and_return([2025]) + # rubocop:enable RSpec/AnyInstance + create(:collection_resource, :additional, year: 2026, short_display_name: "additional resource 2") + end + + it "displays additional resources for editable years" do + get collection_resources_path + + expect(page).to have_content("additional resource") + expect(page).not_to have_content("additional resource 2") + expect(page).to have_link("additional.pdf", href: collection_resource_download_path(collection_resource)) + expect(page).to have_link("Delete") + end + end end end @@ -473,4 +512,305 @@ RSpec.describe CollectionResourcesController, type: :request do end end end + + describe "GET #new_collection_resource" do + context "when user is not signed in" do + it "redirects to the sign in page" do + get new_collection_resource_path(year: 2025, log_type: "sales") + expect(response).to redirect_to(new_user_session_path) + end + end + + context "when user is signed in as a data coordinator" do + let(:user) { create(:user, :data_coordinator) } + + before do + sign_in user + end + + it "returns page not found" do + get new_collection_resource_path(year: 2025, log_type: "sales") + expect(response).to have_http_status(:not_found) + end + end + + context "when user is signed in as a data provider" do + let(:user) { create(:user, :data_provider) } + + before do + sign_in user + end + + it "returns page not found" do + get new_collection_resource_path(year: 2025, log_type: "sales") + expect(response).to have_http_status(:not_found) + end + end + + context "when user is signed in as a support user" do + let(:user) { create(:user, :support) } + + before do + # rubocop:disable RSpec/AnyInstance + allow_any_instance_of(CollectionResourcesHelper).to receive(:editable_collection_resource_years).and_return([2025, 2026]) + # rubocop:enable RSpec/AnyInstance + allow(user).to receive(:need_two_factor_authentication?).and_return(false) + sign_in user + end + + it "displays new collection resource page content" do + get new_collection_resource_path(year: 2025, log_type: "sales") + + expect(page).to have_content("Sales 2025 to 2026") + expect(page).to have_content("Add a new collection resource") + expect(page).to have_content("Upload file") + expect(page).to have_button("Add resource") + expect(page).to have_link("Back", href: collection_resources_path) + expect(page).to have_link("Cancel", href: collection_resources_path) + end + end + end + + describe "POST #collection_resources" do + let(:some_file) { File.open(file_fixture("blank_bulk_upload_sales.csv")) } + let(:params) { { collection_resource: { year: 2025, log_type: "sales", file: some_file, display_name: "some file" } } } + + context "when user is not signed in" do + it "redirects to the sign in page" do + post collection_resources_path, params: params + expect(response).to redirect_to(new_user_session_path) + end + end + + context "when user is signed in as a data coordinator" do + let(:user) { create(:user, :data_coordinator) } + + before do + sign_in user + end + + it "returns page not found" do + post collection_resources_path, params: params + expect(response).to have_http_status(:not_found) + end + end + + context "when user is signed in as a data provider" do + let(:user) { create(:user, :data_provider) } + + before do + sign_in user + end + + it "returns page not found" do + post collection_resources_path, params: params + expect(response).to have_http_status(:not_found) + end + end + end + + describe "GET #download_additional_collection_resource" do + let(:collection_resource) { create(:collection_resource, :additional, year: 2025, short_display_name: "additional resource") } + + before do + # rubocop:disable RSpec/AnyInstance + allow_any_instance_of(CollectionResourcesHelper).to receive(:editable_collection_resource_years).and_return([2025, 2026]) + allow_any_instance_of(CollectionResourcesHelper).to receive(:displayed_collection_resource_years).and_return([2025]) + # rubocop:enable RSpec/AnyInstance + end + + context "when the user is not signed in" do + context "when the file exists on S3" do + before do + allow(storage_service).to receive(:get_file).and_return("file") + get collection_resource_download_path(collection_resource) + end + + it "downloads the file" do + expect(response.body).to eq("file") + end + end + end + + context "when user is signed in as a data coordinator" do + let(:user) { create(:user, :data_coordinator) } + + context "when the file exists on S3" do + before do + sign_in user + allow(storage_service).to receive(:get_file).and_return("file") + get collection_resource_download_path(collection_resource) + end + + it "downloads the file" do + expect(response.body).to eq("file") + end + end + + context "when the file does not exist on S3" do + before do + sign_in user + allow(storage_service).to receive(:get_file).and_return(nil) + get collection_resource_download_path(collection_resource) + end + + it "returns page not found" do + expect(response).to have_http_status(:not_found) + end + end + + context "when resource id is invalid" do + before do + sign_in user + allow(storage_service).to receive(:get_file).and_return(nil) + get collection_resource_download_path(collection_resource_id: "invalid") + end + + it "returns page not found" do + expect(response).to have_http_status(:not_found) + end + end + + context "when year not in displayed_collection_resource_years" do + let(:collection_resource) { create(:collection_resource, :additional, year: 2026, short_display_name: "additional resource") } + + before do + sign_in user + get collection_resource_download_path(collection_resource) + end + + it "returns page not found" do + expect(response).to have_http_status(:not_found) + end + end + end + + context "when user is signed in as a support user" do + let(:collection_resource) { create(:collection_resource, :additional, year: 2026, short_display_name: "additional resource") } + let(:user) { create(:user, :support) } + + context "when year is in editable_collection_resource_years but not in displayed_collection_resource_years" do + before do + allow(user).to receive(:need_two_factor_authentication?).and_return(false) + sign_in user + allow(storage_service).to receive(:get_file).and_return("file") + get collection_resource_download_path(collection_resource) + end + + it "downloads the file" do + expect(response.status).to eq(200) + expect(response.body).to eq("file") + end + end + end + end + + describe "GET #edit_additional_collection_resource" do + let(:collection_resource) { create(:collection_resource, :additional, year: 2025, log_type: "sales", short_display_name: "additional resource", download_filename: "additional.pdf") } + + context "when user is not signed in" do + it "redirects to the sign in page" do + get collection_resource_edit_path(collection_resource) + expect(response).to redirect_to(new_user_session_path) + end + end + + context "when user is signed in as a data coordinator" do + let(:user) { create(:user, :data_coordinator) } + + before do + sign_in user + end + + it "returns page not found" do + get collection_resource_edit_path(collection_resource) + expect(response).to have_http_status(:not_found) + end + end + + context "when user is signed in as a data provider" do + let(:user) { create(:user, :data_provider) } + + before do + sign_in user + end + + it "returns page not found" do + get collection_resource_edit_path(collection_resource) + expect(response).to have_http_status(:not_found) + end + end + + context "when user is signed in as a support user" do + let(:user) { create(:user, :support) } + + before do + allow(Time.zone).to receive(:today).and_return(Time.zone.local(2025, 1, 8)) + allow(user).to receive(:need_two_factor_authentication?).and_return(false) + sign_in user + end + + context "and the file exists on S3" do + before do + allow(storage_service).to receive(:file_exists?).and_return(true) + end + + it "displays update collection resources page content" do + get collection_resource_edit_path(collection_resource) + + expect(page).to have_content("Sales 2025 to 2026") + expect(page).to have_content("Change the additional resource") + expect(page).to have_content("This file will be available for all users to download.") + expect(page).to have_content("Upload file") + expect(page).to have_button("Save changes") + expect(page).to have_link("Back", href: collection_resources_path) + expect(page).to have_link("Cancel", href: collection_resources_path) + end + end + end + end + + describe "PATCH #update_additional_collection_resource" do + let(:some_file) { File.open(file_fixture("blank_bulk_upload_sales.csv")) } + let(:params) { { collection_resource: { short_display_name: "short name", file: some_file } } } + let(:collection_resource_service) { instance_double(CollectionResourcesService) } + let(:collection_resource) { create(:collection_resource, :additional, year: 2025, log_type: "sales", short_display_name: "additional resource", download_filename: "additional.pdf") } + + before do + allow(CollectionResourcesService).to receive(:new).and_return(collection_resource_service) + end + + context "when user is not signed in" do + it "redirects to the sign in page" do + patch collection_resource_update_path(collection_resource), params: params + expect(response).to redirect_to(new_user_session_path) + end + end + + context "when user is signed in as a data coordinator" do + let(:user) { create(:user, :data_coordinator) } + + before do + sign_in user + end + + it "returns page not found" do + patch collection_resource_update_path(collection_resource), params: params + expect(response).to have_http_status(:not_found) + end + end + + context "when user is signed in as a data provider" do + let(:user) { create(:user, :data_provider) } + + before do + sign_in user + end + + it "returns page not found" do + patch collection_resource_update_path(collection_resource), params: params + expect(response).to have_http_status(:not_found) + end + end + end end diff --git a/spec/requests/start_controller_spec.rb b/spec/requests/start_controller_spec.rb index db884d09f..b3846809c 100644 --- a/spec/requests/start_controller_spec.rb +++ b/spec/requests/start_controller_spec.rb @@ -324,6 +324,7 @@ RSpec.describe StartController, type: :request do context "and 2023 collection window is open for editing" do before do + create(:collection_resource, :additional, year: 2023, log_type: "sales", display_name: "sales additional resource (2023 to 2024)") allow(Time).to receive(:now).and_return(Time.zone.local(2024, 4, 1)) end @@ -337,6 +338,7 @@ RSpec.describe StartController, type: :request do expect(page).to have_content("Sales 23/24") expect(page).to have_content("Sales 2024 to 2025") expect(page).to have_content("Sales 2023 to 2024") + expect(page).to have_content("Download the sales additional resource (2023 to 2024)") end end diff --git a/spec/services/collection_resources_service_spec.rb b/spec/services/collection_resources_service_spec.rb index 3786b70c9..602e36c38 100644 --- a/spec/services/collection_resources_service_spec.rb +++ b/spec/services/collection_resources_service_spec.rb @@ -12,7 +12,7 @@ describe CollectionResourcesService do end it "calls write_file on S3 service" do - expect(storage_service).to receive(:write_file).with("2025_26_lettings_paper_form.pdf", some_file) + expect(storage_service).to receive(:write_file).with("2025_26_lettings_paper_form.pdf", some_file, content_type: "application/pdf") service.upload_collection_resource("2025_26_lettings_paper_form.pdf", some_file) end end From c45d370f732e792f52f86d528feafdbbde86dea4 Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Wed, 23 Oct 2024 12:52:05 +0100 Subject: [PATCH 5/6] CLDC-3649 Delete custom resource (#2693) * Stub some collection resource requests * Rebase changes * Add delete confirmation page * Allow deleting resources * lint * Return unauthorised status * Fix delete path from delete confirmation page * Change the ordering for collection resources * Do not create header for empty label * Add margin between sections --- .../document_list_component.html.erb | 4 +- .../collection_resources_controller.rb | 43 +++- app/controllers/start_controller.rb | 4 +- app/frontend/styles/_document-list.scss | 1 - app/models/collection_resource.rb | 7 + app/services/collection_resources_service.rb | 4 + app/services/storage/local_disk_service.rb | 6 + app/services/storage/s3_service.rb | 4 + app/services/storage/storage_service.rb | 8 + .../_collection_resource_summary_list.erb | 6 +- .../delete_confirmation.html.erb | 31 +++ app/views/collection_resources/index.html.erb | 9 +- config/routes.rb | 2 + db/migrate/20241011112158_add_discarded_at.rb | 5 + db/schema.rb | 3 +- .../collection_resources_controller_spec.rb | 191 ++++++++++++++---- 16 files changed, 270 insertions(+), 58 deletions(-) create mode 100644 app/views/collection_resources/delete_confirmation.html.erb create mode 100644 db/migrate/20241011112158_add_discarded_at.rb diff --git a/app/components/document_list_component.html.erb b/app/components/document_list_component.html.erb index 32b2d1562..f2587f10f 100644 --- a/app/components/document_list_component.html.erb +++ b/app/components/document_list_component.html.erb @@ -1,4 +1,6 @@ -

<%= label %>

+<% unless label.blank? %> +

<%= label %>

+<% end %>
<% items.each do |item| %>
diff --git a/app/controllers/collection_resources_controller.rb b/app/controllers/collection_resources_controller.rb index 5203ceea1..06988c88c 100644 --- a/app/controllers/collection_resources_controller.rb +++ b/app/controllers/collection_resources_controller.rb @@ -8,8 +8,8 @@ class CollectionResourcesController < ApplicationController @mandatory_lettings_collection_resources_per_year = MandatoryCollectionResourcesService.generate_resources("lettings", editable_collection_resource_years) @mandatory_sales_collection_resources_per_year = MandatoryCollectionResourcesService.generate_resources("sales", editable_collection_resource_years) - @additional_lettings_collection_resources_per_year = CollectionResource.where(log_type: "lettings", mandatory: false).group_by(&:year) - @additional_sales_collection_resources_per_year = CollectionResource.where(log_type: "sales", mandatory: false).group_by(&:year) + @additional_lettings_collection_resources_per_year = CollectionResource.visible.where(log_type: "lettings", mandatory: false).group_by(&:year) + @additional_sales_collection_resources_per_year = CollectionResource.visible.where(log_type: "sales", mandatory: false).group_by(&:year) end def download_mandatory_collection_resource @@ -35,7 +35,7 @@ class CollectionResourcesController < ApplicationController end def edit_mandatory_collection_resource - return render_not_found unless current_user.support? + return render_not_authorized unless current_user.support? year = params[:year].to_i resource_type = params[:resource_type] @@ -51,7 +51,7 @@ class CollectionResourcesController < ApplicationController end def edit_additional_collection_resource - return render_not_found unless current_user.support? + return render_not_authorized unless current_user.support? @collection_resource = CollectionResource.find_by(id: params[:collection_resource_id]) @@ -62,7 +62,7 @@ class CollectionResourcesController < ApplicationController end def update_mandatory_collection_resource - return render_not_found unless current_user.support? + return render_not_authorized unless current_user.support? year = resource_params[:year].to_i resource_type = resource_params[:resource_type] @@ -92,7 +92,7 @@ class CollectionResourcesController < ApplicationController end def update_additional_collection_resource - return render_not_found unless current_user.support? + return render_not_authorized unless current_user.support? @collection_resource = CollectionResource.find_by(id: params[:collection_resource_id]) @@ -122,7 +122,7 @@ class CollectionResourcesController < ApplicationController end def confirm_mandatory_collection_resources_release - return render_not_found unless current_user.support? + return render_not_authorized unless current_user.support? @year = params[:year].to_i @@ -132,7 +132,7 @@ class CollectionResourcesController < ApplicationController end def release_mandatory_collection_resources - return render_not_found unless current_user.support? + return render_not_authorized unless current_user.support? year = params[:year].to_i @@ -145,7 +145,7 @@ class CollectionResourcesController < ApplicationController end def new - return render_not_found unless current_user.support? + return render_not_authorized unless current_user.support? year = params[:year].to_i log_type = params[:log_type] @@ -156,7 +156,7 @@ class CollectionResourcesController < ApplicationController end def create - return render_not_found unless current_user.support? && editable_collection_resource_years.include?(resource_params[:year].to_i) + return render_not_authorized unless current_user.support? && editable_collection_resource_years.include?(resource_params[:year].to_i) @collection_resource = CollectionResource.new(resource_params) @collection_resource.download_filename ||= @collection_resource.file&.original_filename @@ -184,6 +184,29 @@ class CollectionResourcesController < ApplicationController end end + def delete_confirmation + return render_not_authorized unless current_user.support? + + @collection_resource = CollectionResource.find_by(id: params[:collection_resource_id]) + + return render_not_found unless @collection_resource + + render "collection_resources/delete_confirmation" + end + + def delete + return render_not_authorized unless current_user.support? + + @collection_resource = CollectionResource.find_by(id: params[:collection_resource_id]) + + return render_not_found unless @collection_resource + + @collection_resource.discard! + + flash[:notice] = "The #{@collection_resource.log_type} #{text_year_range_format(@collection_resource.year)} #{@collection_resource.short_display_name.downcase} has been deleted." + redirect_to collection_resources_path + end + private def resource_params diff --git a/app/controllers/start_controller.rb b/app/controllers/start_controller.rb index b65da4d44..d6df81c39 100644 --- a/app/controllers/start_controller.rb +++ b/app/controllers/start_controller.rb @@ -4,8 +4,8 @@ class StartController < ApplicationController def index @mandatory_lettings_collection_resources_per_year = MandatoryCollectionResourcesService.generate_resources("lettings", displayed_collection_resource_years) @mandatory_sales_collection_resources_per_year = MandatoryCollectionResourcesService.generate_resources("sales", displayed_collection_resource_years) - @additional_lettings_collection_resources_per_year = CollectionResource.where(log_type: "lettings", mandatory: false, year: displayed_collection_resource_years).group_by(&:year) - @additional_sales_collection_resources_per_year = CollectionResource.where(log_type: "sales", mandatory: false, year: displayed_collection_resource_years).group_by(&:year) + @additional_lettings_collection_resources_per_year = CollectionResource.visible.where(log_type: "lettings", mandatory: false, year: displayed_collection_resource_years).group_by(&:year) + @additional_sales_collection_resources_per_year = CollectionResource.visible.where(log_type: "sales", mandatory: false, year: displayed_collection_resource_years).group_by(&:year) if current_user @homepage_presenter = HomepagePresenter.new(current_user) render "home/index" diff --git a/app/frontend/styles/_document-list.scss b/app/frontend/styles/_document-list.scss index e8cef0625..a1f812551 100644 --- a/app/frontend/styles/_document-list.scss +++ b/app/frontend/styles/_document-list.scss @@ -1,5 +1,4 @@ .app-document-list { - margin-top: govuk-spacing(3); margin-bottom: govuk-spacing(6); } diff --git a/app/models/collection_resource.rb b/app/models/collection_resource.rb index cc217b8cd..fc5808cec 100644 --- a/app/models/collection_resource.rb +++ b/app/models/collection_resource.rb @@ -1,8 +1,10 @@ class CollectionResource < ApplicationRecord include Rails.application.routes.url_helpers + has_paper_trail attr_accessor :file + scope :visible, -> { where(discarded_at: nil) } validates :short_display_name, presence: true def download_path @@ -35,4 +37,9 @@ class CollectionResource < ApplicationRecord def validate_short_display_name errors.add(:short_display_name, :blank) if short_display_name.blank? end + + def discard! + CollectionResourcesService.new.delete_collection_resource(download_filename) + update!(discarded_at: Time.zone.now) + end end diff --git a/app/services/collection_resources_service.rb b/app/services/collection_resources_service.rb index 82247f7a0..f6762116a 100644 --- a/app/services/collection_resources_service.rb +++ b/app/services/collection_resources_service.rb @@ -27,4 +27,8 @@ class CollectionResourcesService content_type = MiniMime.lookup_by_filename(filename)&.content_type @storage_service.write_file(filename, file, content_type:) end + + def delete_collection_resource(filename) + @storage_service.delete_file(filename) + end end diff --git a/app/services/storage/local_disk_service.rb b/app/services/storage/local_disk_service.rb index ad3cc9608..228f0339e 100644 --- a/app/services/storage/local_disk_service.rb +++ b/app/services/storage/local_disk_service.rb @@ -43,5 +43,11 @@ module Storage File.exist?(path) end + + def delete_file(filename) + path = Rails.root.join("tmp/storage", filename) + + File.delete(path) + end end end diff --git a/app/services/storage/s3_service.rb b/app/services/storage/s3_service.rb index 2e8daa719..a6eef7d49 100644 --- a/app/services/storage/s3_service.rb +++ b/app/services/storage/s3_service.rb @@ -64,6 +64,10 @@ module Storage false end + def delete_file(file_name) + @client.delete_object(bucket: @configuration.bucket_name, key: file_name) + end + private def create_configuration diff --git a/app/services/storage/storage_service.rb b/app/services/storage/storage_service.rb index afb3d4a58..37d4cc0fd 100644 --- a/app/services/storage/storage_service.rb +++ b/app/services/storage/storage_service.rb @@ -15,5 +15,13 @@ module Storage def write_file(_file_name, _data) raise NotImplementedError end + + def get_file(_file_name, _data) + raise NotImplementedError + end + + def delete_file(_file_name, _data) + raise NotImplementedError + end end end diff --git a/app/views/collection_resources/_collection_resource_summary_list.erb b/app/views/collection_resources/_collection_resource_summary_list.erb index 0e630f0aa..581bb6303 100644 --- a/app/views/collection_resources/_collection_resource_summary_list.erb +++ b/app/views/collection_resources/_collection_resource_summary_list.erb @@ -35,15 +35,15 @@ ) %> <% row.with_action( text: "Delete", - href: "/", + href: collection_resource_delete_confirmation_path(resource), classes: "app-!-colour-red" ) %> <% end %> <% end %> <% end %> -
+
<%= govuk_link_to "Add new #{mandatory_resources.first.log_type} #{text_year_range_format(mandatory_resources.first.year)} resource", href: new_collection_resource_path(year: mandatory_resources.first.year, log_type: mandatory_resources.first.log_type) %>
-
+
diff --git a/app/views/collection_resources/delete_confirmation.html.erb b/app/views/collection_resources/delete_confirmation.html.erb new file mode 100644 index 000000000..9f861bc19 --- /dev/null +++ b/app/views/collection_resources/delete_confirmation.html.erb @@ -0,0 +1,31 @@ +<% content_for :before_content do %> + <% content_for :title, "Are you sure you want to delete the #{@collection_resource.short_display_name.downcase}?" %> + <%= govuk_back_link href: collection_resources_path %> +<% end %> + +
+
+ <%= "#{@collection_resource.log_type.humanize} #{text_year_range_format(@collection_resource.year)}" %> +

+ <%= content_for(:title) %> +

+

+ This file will no longer be available for users to download. +

+ + <%= govuk_warning_text(text: "You will not be able to undo this action.") %> + +
+ <%= govuk_button_to( + "Delete resource", + collection_resource_delete_path(@collection_resource), + method: :delete, + ) %> + <%= govuk_button_link_to( + "Cancel", + collection_resources_path, + secondary: true, + ) %> +
+
+
diff --git a/app/views/collection_resources/index.html.erb b/app/views/collection_resources/index.html.erb index 207a63991..ee5574f80 100644 --- a/app/views/collection_resources/index.html.erb +++ b/app/views/collection_resources/index.html.erb @@ -14,16 +14,13 @@ <% end %>

<%= title %>

-<% @mandatory_lettings_collection_resources_per_year.each do |year, mandatory_resources| %> +<% editable_collection_resource_years.each do |year| %>

Lettings <%= text_year_range_format(year) %>

- <%= render partial: "collection_resource_summary_list", locals: { mandatory_resources:, additional_resources: @additional_lettings_collection_resources_per_year[year] } %> -<% end %> - -<% @mandatory_sales_collection_resources_per_year.each do |year, mandatory_resources| %> + <%= render partial: "collection_resource_summary_list", locals: { mandatory_resources: @mandatory_lettings_collection_resources_per_year[year], additional_resources: @additional_lettings_collection_resources_per_year[year] } %>

Sales <%= text_year_range_format(year) %>

- <%= render partial: "collection_resource_summary_list", locals: { mandatory_resources:, additional_resources: @additional_sales_collection_resources_per_year[year] } %> + <%= render partial: "collection_resource_summary_list", locals: { mandatory_resources: @mandatory_sales_collection_resources_per_year[year], additional_resources: @additional_sales_collection_resources_per_year[year] } %> <% end %> diff --git a/config/routes.rb b/config/routes.rb index cbac4894a..6ac7b3f34 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -51,6 +51,8 @@ Rails.application.routes.draw do get "/download", to: "collection_resources#download_additional_collection_resource" get "/edit", to: "collection_resources#edit_additional_collection_resource" patch "/update", to: "collection_resources#update_additional_collection_resource" + get "/delete-confirmation", to: "collection_resources#delete_confirmation" + delete "/delete", to: "collection_resources#delete" end get "clear-filters", to: "sessions#clear_filters" diff --git a/db/migrate/20241011112158_add_discarded_at.rb b/db/migrate/20241011112158_add_discarded_at.rb new file mode 100644 index 000000000..d910b8797 --- /dev/null +++ b/db/migrate/20241011112158_add_discarded_at.rb @@ -0,0 +1,5 @@ +class AddDiscardedAt < ActiveRecord::Migration[7.0] + def change + add_column :collection_resources, :discarded_at, :datetime + end +end diff --git a/db/schema.rb b/db/schema.rb index 1f5cca5ca..84bf048af 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: 2024_10_08_100119) do +ActiveRecord::Schema[7.0].define(version: 2024_10_11_112158) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -61,6 +61,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_10_08_100119) do t.boolean "released_to_user" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.datetime "discarded_at" end create_table "csv_variable_definitions", force: :cascade do |t| diff --git a/spec/requests/collection_resources_controller_spec.rb b/spec/requests/collection_resources_controller_spec.rb index a6d88f911..eb6f56aed 100644 --- a/spec/requests/collection_resources_controller_spec.rb +++ b/spec/requests/collection_resources_controller_spec.rb @@ -2,7 +2,7 @@ require "rails_helper" RSpec.describe CollectionResourcesController, type: :request do let(:page) { Capybara::Node::Simple.new(response.body) } - let(:storage_service) { instance_double(Storage::S3Service, get_file_metadata: nil) } + let(:storage_service) { instance_double(Storage::S3Service, get_file_metadata: nil, delete_file: nil) } before do allow(Storage::S3Service).to receive(:new).and_return(storage_service) @@ -183,7 +183,7 @@ RSpec.describe CollectionResourcesController, type: :request do expect(page).to have_content("additional resource") expect(page).not_to have_content("additional resource 2") expect(page).to have_link("additional.pdf", href: collection_resource_download_path(collection_resource)) - expect(page).to have_link("Delete") + expect(page).to have_link("Delete", href: collection_resource_delete_confirmation_path(collection_resource)) end end end @@ -277,9 +277,9 @@ RSpec.describe CollectionResourcesController, type: :request do sign_in user end - it "returns page not found" do + it "returns page not authorised" do get edit_mandatory_collection_resource_path(year: 2024, log_type: "sales", resource_type: "bulk_upload_template") - expect(response).to have_http_status(:not_found) + expect(response).to have_http_status(:unauthorized) end end @@ -290,9 +290,9 @@ RSpec.describe CollectionResourcesController, type: :request do sign_in user end - it "returns page not found" do + it "returns page not authorised" do get edit_mandatory_collection_resource_path(year: 2024, log_type: "sales", resource_type: "bulk_upload_template") - expect(response).to have_http_status(:not_found) + expect(response).to have_http_status(:unauthorized) end end @@ -366,9 +366,9 @@ RSpec.describe CollectionResourcesController, type: :request do sign_in user end - it "returns page not found" do + it "returns page not authorised" do patch update_mandatory_collection_resource_path, params: params - expect(response).to have_http_status(:not_found) + expect(response).to have_http_status(:unauthorized) end end @@ -379,9 +379,9 @@ RSpec.describe CollectionResourcesController, type: :request do sign_in user end - it "returns page not found" do + it "returns page not authorised" do patch update_mandatory_collection_resource_path, params: params - expect(response).to have_http_status(:not_found) + expect(response).to have_http_status(:unauthorized) end end end @@ -401,9 +401,9 @@ RSpec.describe CollectionResourcesController, type: :request do sign_in user end - it "returns page not found" do + it "returns page not authorised" do get confirm_mandatory_collection_resources_release_path(year: 2025) - expect(response).to have_http_status(:not_found) + expect(response).to have_http_status(:unauthorized) end end @@ -414,9 +414,9 @@ RSpec.describe CollectionResourcesController, type: :request do sign_in user end - it "returns page not found" do + it "returns page not authorised" do get confirm_mandatory_collection_resources_release_path(year: 2025) - expect(response).to have_http_status(:not_found) + expect(response).to have_http_status(:unauthorized) end end @@ -466,9 +466,9 @@ RSpec.describe CollectionResourcesController, type: :request do sign_in user end - it "returns page not found" do + it "returns page not authorised" do patch release_mandatory_collection_resources_path(year: 2024) - expect(response).to have_http_status(:not_found) + expect(response).to have_http_status(:unauthorized) end end @@ -479,9 +479,9 @@ RSpec.describe CollectionResourcesController, type: :request do sign_in user end - it "returns page not found" do + it "returns page not authorised" do patch release_mandatory_collection_resources_path(year: 2024) - expect(response).to have_http_status(:not_found) + expect(response).to have_http_status(:unauthorized) end end @@ -528,9 +528,9 @@ RSpec.describe CollectionResourcesController, type: :request do sign_in user end - it "returns page not found" do + it "returns page not authorised" do get new_collection_resource_path(year: 2025, log_type: "sales") - expect(response).to have_http_status(:not_found) + expect(response).to have_http_status(:unauthorized) end end @@ -541,9 +541,9 @@ RSpec.describe CollectionResourcesController, type: :request do sign_in user end - it "returns page not found" do + it "returns page not authorised" do get new_collection_resource_path(year: 2025, log_type: "sales") - expect(response).to have_http_status(:not_found) + expect(response).to have_http_status(:unauthorized) end end @@ -589,9 +589,9 @@ RSpec.describe CollectionResourcesController, type: :request do sign_in user end - it "returns page not found" do + it "returns page not authorised" do post collection_resources_path, params: params - expect(response).to have_http_status(:not_found) + expect(response).to have_http_status(:unauthorized) end end @@ -602,9 +602,9 @@ RSpec.describe CollectionResourcesController, type: :request do sign_in user end - it "returns page not found" do + it "returns page not authorised" do post collection_resources_path, params: params - expect(response).to have_http_status(:not_found) + expect(response).to have_http_status(:unauthorized) end end end @@ -722,9 +722,9 @@ RSpec.describe CollectionResourcesController, type: :request do sign_in user end - it "returns page not found" do + it "returns page not authorised" do get collection_resource_edit_path(collection_resource) - expect(response).to have_http_status(:not_found) + expect(response).to have_http_status(:unauthorized) end end @@ -735,9 +735,9 @@ RSpec.describe CollectionResourcesController, type: :request do sign_in user end - it "returns page not found" do + it "returns page not authorised" do get collection_resource_edit_path(collection_resource) - expect(response).to have_http_status(:not_found) + expect(response).to have_http_status(:unauthorized) end end @@ -794,9 +794,9 @@ RSpec.describe CollectionResourcesController, type: :request do sign_in user end - it "returns page not found" do + it "returns page not authorised" do patch collection_resource_update_path(collection_resource), params: params - expect(response).to have_http_status(:not_found) + expect(response).to have_http_status(:unauthorized) end end @@ -807,9 +807,132 @@ RSpec.describe CollectionResourcesController, type: :request do sign_in user end - it "returns page not found" do + it "returns page not authorised" do patch collection_resource_update_path(collection_resource), params: params - expect(response).to have_http_status(:not_found) + expect(response).to have_http_status(:unauthorized) + end + end + end + + describe "GET #collection_resource_delete_confirmation" do + let(:collection_resource) { create(:collection_resource, :additional, year: 2025, log_type: "sales", short_display_name: "additional resource", download_filename: "additional.pdf") } + + context "when user is not signed in" do + it "redirects to the sign in page" do + get collection_resource_delete_confirmation_path(collection_resource) + expect(response).to redirect_to(new_user_session_path) + end + end + + context "when user is signed in as a data coordinator" do + let(:user) { create(:user, :data_coordinator) } + + before do + sign_in user + end + + it "returns page not authorised" do + get collection_resource_delete_confirmation_path(collection_resource) + expect(response).to have_http_status(:unauthorized) + end + end + + context "when user is signed in as a data provider" do + let(:user) { create(:user, :data_provider) } + + before do + sign_in user + end + + it "returns page not authorised" do + get collection_resource_delete_confirmation_path(collection_resource) + expect(response).to have_http_status(:unauthorized) + end + end + + context "when user is signed in as a support user" do + let(:user) { create(:user, :support) } + + before do + allow(Time.zone).to receive(:today).and_return(Time.zone.local(2025, 1, 8)) + allow(user).to receive(:need_two_factor_authentication?).and_return(false) + sign_in user + end + + context "and the file exists on S3" do + it "displays delete confirmation page content" do + get collection_resource_delete_confirmation_path(collection_resource) + + expect(page).to have_content("Sales 2025 to 2026") + expect(page).to have_content("Are you sure you want to delete the additional resource?") + expect(page).to have_content("This file will no longer be available for users to download.") + expect(page).to have_content("You will not be able to undo this action.") + expect(page).to have_button("Delete resource") + expect(page).to have_link("Back", href: collection_resources_path) + expect(page).to have_link("Cancel", href: collection_resources_path) + end + end + end + end + + describe "DELETE #collection_resource_delete" do + let!(:collection_resource) { create(:collection_resource, :additional, year: 2025, log_type: "sales", short_display_name: "additional resource", download_filename: "additional.pdf") } + + context "when user is not signed in" do + it "redirects to the sign in page" do + delete collection_resource_delete_path(collection_resource) + expect(response).to redirect_to(new_user_session_path) + end + end + + context "when user is signed in as a data coordinator" do + let(:user) { create(:user, :data_coordinator) } + + before do + sign_in user + end + + it "returns page not authorised" do + delete collection_resource_delete_path(collection_resource) + expect(response).to have_http_status(:unauthorized) + end + end + + context "when user is signed in as a data provider" do + let(:user) { create(:user, :data_provider) } + + before do + sign_in user + end + + it "returns page not authorised" do + delete collection_resource_delete_path(collection_resource) + expect(response).to have_http_status(:unauthorized) + end + end + + context "when user is signed in as a support user" do + let(:user) { create(:user, :support) } + + before do + allow(storage_service).to receive(:file_exists?).and_return(true) + allow(Time.zone).to receive(:today).and_return(Time.zone.local(2025, 1, 8)) + allow(user).to receive(:need_two_factor_authentication?).and_return(false) + sign_in user + end + + context "and the file exists on S3" do + it "displays delete confirmation page content" do + expect(CollectionResource.visible.count).to eq(1) + delete collection_resource_delete_path(collection_resource) + + expect(CollectionResource.count).to eq(1) + expect(CollectionResource.visible.count).to eq(0) + expect(response).to redirect_to(collection_resources_path) + expect(storage_service).to have_received(:delete_file).with(collection_resource.download_filename) + follow_redirect! + expect(page).to have_content("The sales 2025 to 2026 additional resource has been deleted.") + end end end end From 64caf018e0f0464ba1b77738df4c906ea10f5af0 Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Wed, 23 Oct 2024 13:27:23 +0100 Subject: [PATCH 6/6] CLDC-2413 Allow 4 digit year format for bulk uploads (#2715) * Allow 4 digit years for start date/sale date * Update other dates --- .../lettings/year2023/csv_parser.rb | 6 ++-- .../lettings/year2023/row_parser.rb | 25 ++++++-------- .../lettings/year2024/csv_parser.rb | 6 ++-- .../lettings/year2024/row_parser.rb | 25 ++++++-------- .../bulk_upload/sales/year2023/csv_parser.rb | 6 ++-- .../bulk_upload/sales/year2023/row_parser.rb | 13 ++++--- .../bulk_upload/sales/year2024/csv_parser.rb | 6 ++-- .../bulk_upload/sales/year2024/row_parser.rb | 13 ++++--- config/locales/en.yml | 4 +-- .../lettings/year2023/row_parser_spec.rb | 34 ++++++++++++++++--- .../lettings/year2024/row_parser_spec.rb | 34 ++++++++++++++++--- .../sales/year2023/row_parser_spec.rb | 19 ++++++++--- .../sales/year2024/row_parser_spec.rb | 20 ++++++++--- 13 files changed, 144 insertions(+), 67 deletions(-) diff --git a/app/services/bulk_upload/lettings/year2023/csv_parser.rb b/app/services/bulk_upload/lettings/year2023/csv_parser.rb index 64e27108f..96be2256d 100644 --- a/app/services/bulk_upload/lettings/year2023/csv_parser.rb +++ b/app/services/bulk_upload/lettings/year2023/csv_parser.rb @@ -109,9 +109,11 @@ private def first_record_start_date if with_headers? - Date.new(row_parsers.first.field_9.to_i + 2000, row_parsers.first.field_8.to_i, row_parsers.first.field_7.to_i) + year = row_parsers.first.field_9.to_s.strip.length.between?(1, 2) ? row_parsers.first.field_9.to_i + 2000 : row_parsers.first.field_9.to_i + Date.new(year, row_parsers.first.field_8.to_i, row_parsers.first.field_7.to_i) else - Date.new(rows.first[97].to_i + 2000, rows.first[96].to_i, rows.first[95].to_i) + year = rows.first[97].to_s.strip.length.between?(1, 2) ? rows.first[97].to_i + 2000 : rows.first[97].to_i + Date.new(year, rows.first[96].to_i, rows.first[95].to_i) end end end diff --git a/app/services/bulk_upload/lettings/year2023/row_parser.rb b/app/services/bulk_upload/lettings/year2023/row_parser.rb index 8c435ad24..59cf7ea34 100644 --- a/app/services/bulk_upload/lettings/year2023/row_parser.rb +++ b/app/services/bulk_upload/lettings/year2023/row_parser.rb @@ -323,8 +323,8 @@ class BulkUpload::Lettings::Year2023::RowParser category: :setup, }, format: { - with: /\A\d{2}\z/, - message: I18n.t("validations.setup.startdate.year_not_two_digits"), + with: /\A(\d{2}|\d{4})\z/, + message: I18n.t("validations.setup.startdate.year_not_two_or_four_digits"), category: :setup, unless: -> { field_9.blank? }, }, @@ -618,14 +618,6 @@ private end end - def start_date - return if field_7.blank? || field_8.blank? || field_9.blank? - - Date.parse("20#{field_9.to_s.rjust(2, '0')}-#{field_8}-#{field_7}") - rescue StandardError - nil - end - def validate_no_and_dont_know_disabled_needs_conjunction if field_87 == 1 && field_88 == 1 errors.add(:field_87, I18n.t("validations.household.housingneeds.no_and_dont_know_disabled_needs_conjunction")) @@ -736,9 +728,9 @@ private end def validate_relevant_collection_window - return if start_date.blank? || bulk_upload.form.blank? + return if startdate.blank? || bulk_upload.form.blank? - unless bulk_upload.form.valid_start_date_for_form?(start_date) + unless bulk_upload.form.valid_start_date_for_form?(startdate) errors.add(:field_7, I18n.t("validations.date.outside_collection_window", year_combo: bulk_upload.year_combo, start_year: bulk_upload.year, end_year: bulk_upload.end_year), category: :setup) errors.add(:field_8, I18n.t("validations.date.outside_collection_window", year_combo: bulk_upload.year_combo, start_year: bulk_upload.year, end_year: bulk_upload.end_year), category: :setup) errors.add(:field_9, I18n.t("validations.date.outside_collection_window", year_combo: bulk_upload.year_combo, start_year: bulk_upload.year, end_year: bulk_upload.end_year), category: :setup) @@ -1388,7 +1380,8 @@ private end def startdate - Date.new(field_9 + 2000, field_8, field_7) if field_9.present? && field_8.present? && field_7.present? + year = field_9.to_s.strip.length.between?(1, 2) ? field_9 + 2000 : field_9 + Date.new(year, field_8, field_7) if field_9.present? && field_8.present? && field_7.present? rescue Date::Error Date.new end @@ -1584,13 +1577,15 @@ private end def mrcdate - Date.new(field_38 + 2000, field_37, field_36) if field_38.present? && field_37.present? && field_36.present? + year = field_38.to_s.strip.length.between?(1, 2) ? field_38 + 2000 : field_38 + Date.new(year, field_37, field_36) if field_38.present? && field_37.present? && field_36.present? rescue Date::Error Date.new end def voiddate - Date.new(field_35 + 2000, field_34, field_33) if field_35.present? && field_34.present? && field_33.present? + year = field_35.to_s.strip.length.between?(1, 2) ? field_35 + 2000 : field_35 + Date.new(year, field_34, field_33) if field_35.present? && field_34.present? && field_33.present? rescue Date::Error Date.new end diff --git a/app/services/bulk_upload/lettings/year2024/csv_parser.rb b/app/services/bulk_upload/lettings/year2024/csv_parser.rb index d8d430755..22caeab02 100644 --- a/app/services/bulk_upload/lettings/year2024/csv_parser.rb +++ b/app/services/bulk_upload/lettings/year2024/csv_parser.rb @@ -112,9 +112,11 @@ private def first_record_start_date if with_headers? - Date.new(row_parsers.first.field_10.to_i + 2000, row_parsers.first.field_9.to_i, row_parsers.first.field_8.to_i) + year = row_parsers.first.field_10.to_s.strip.length.between?(1, 2) ? row_parsers.first.field_10.to_i + 2000 : row_parsers.first.field_10.to_i + Date.new(year, row_parsers.first.field_9.to_i, row_parsers.first.field_8.to_i) else - Date.new(rows.first[9].to_i + 2000, rows.first[8].to_i, rows.first[7].to_i) + year = rows.first[9].to_s.strip.length.between?(1, 2) ? rows.first[9].to_i + 2000 : rows.first[9].to_i + Date.new(year, rows.first[8].to_i, rows.first[7].to_i) end end end diff --git a/app/services/bulk_upload/lettings/year2024/row_parser.rb b/app/services/bulk_upload/lettings/year2024/row_parser.rb index a7eb96c61..312a5472a 100644 --- a/app/services/bulk_upload/lettings/year2024/row_parser.rb +++ b/app/services/bulk_upload/lettings/year2024/row_parser.rb @@ -324,8 +324,8 @@ class BulkUpload::Lettings::Year2024::RowParser category: :setup, }, format: { - with: /\A\d{2}\z/, - message: I18n.t("validations.setup.startdate.year_not_two_digits"), + with: /\A(\d{2}|\d{4})\z/, + message: I18n.t("validations.setup.startdate.year_not_two_or_four_digits"), category: :setup, unless: -> { field_10.blank? }, }, @@ -686,14 +686,6 @@ private end end - def start_date - return if field_8.blank? || field_9.blank? || field_10.blank? - - Date.parse("20#{field_10.to_s.rjust(2, '0')}-#{field_9}-#{field_8}") - rescue StandardError - nil - end - def validate_no_and_dont_know_disabled_needs_conjunction if field_83 == 1 && field_84 == 1 errors.add(:field_83, I18n.t("validations.household.housingneeds.no_and_dont_know_disabled_needs_conjunction")) @@ -790,9 +782,9 @@ private end def validate_relevant_collection_window - return if start_date.blank? || bulk_upload.form.blank? + return if startdate.blank? || bulk_upload.form.blank? - unless bulk_upload.form.valid_start_date_for_form?(start_date) + unless bulk_upload.form.valid_start_date_for_form?(startdate) errors.add(:field_8, I18n.t("validations.date.outside_collection_window", year_combo: bulk_upload.year_combo, start_year: bulk_upload.year, end_year: bulk_upload.end_year), category: :setup) errors.add(:field_9, I18n.t("validations.date.outside_collection_window", year_combo: bulk_upload.year_combo, start_year: bulk_upload.year, end_year: bulk_upload.end_year), category: :setup) errors.add(:field_10, I18n.t("validations.date.outside_collection_window", year_combo: bulk_upload.year_combo, start_year: bulk_upload.year, end_year: bulk_upload.end_year), category: :setup) @@ -1394,7 +1386,8 @@ private end def startdate - Date.new(field_10 + 2000, field_9, field_8) if field_10.present? && field_9.present? && field_8.present? + year = field_10.to_s.strip.length.between?(1, 2) ? field_10 + 2000 : field_10 + Date.new(year, field_9, field_8) if field_10.present? && field_9.present? && field_8.present? rescue Date::Error Date.new end @@ -1599,13 +1592,15 @@ private end def mrcdate - Date.new(field_35 + 2000, field_34, field_33) if field_35.present? && field_34.present? && field_33.present? + year = field_35.to_s.strip.length.between?(1, 2) ? field_35 + 2000 : field_35 + Date.new(year, field_34, field_33) if field_35.present? && field_34.present? && field_33.present? rescue Date::Error Date.new end def voiddate - Date.new(field_32 + 2000, field_31, field_30) if field_32.present? && field_31.present? && field_30.present? + year = field_32.to_s.strip.length.between?(1, 2) ? field_32 + 2000 : field_32 + Date.new(year, field_31, field_30) if field_32.present? && field_31.present? && field_30.present? rescue Date::Error Date.new end diff --git a/app/services/bulk_upload/sales/year2023/csv_parser.rb b/app/services/bulk_upload/sales/year2023/csv_parser.rb index ec7346f92..85d455d01 100644 --- a/app/services/bulk_upload/sales/year2023/csv_parser.rb +++ b/app/services/bulk_upload/sales/year2023/csv_parser.rb @@ -111,9 +111,11 @@ private def first_record_start_date if with_headers? - Date.new(row_parsers.first.field_5.to_i + 2000, row_parsers.first.field_4.to_i, row_parsers.first.field_3.to_i) + year = row_parsers.first.field_5.to_s.strip.length.between?(1, 2) ? row_parsers.first.field_5.to_i + 2000 : row_parsers.first.field_5.to_i + Date.new(year, row_parsers.first.field_4.to_i, row_parsers.first.field_3.to_i) else - Date.new(rows.first[3].to_i + 2000, rows.first[2].to_i, rows.first[1].to_i) + year = rows.first[3].to_s.strip.length.between?(1, 2) ? rows.first[3].to_i + 2000 : rows.first[3].to_i + Date.new(year, rows.first[2].to_i, rows.first[1].to_i) end end end diff --git a/app/services/bulk_upload/sales/year2023/row_parser.rb b/app/services/bulk_upload/sales/year2023/row_parser.rb index 6a18512aa..d87c94dce 100644 --- a/app/services/bulk_upload/sales/year2023/row_parser.rb +++ b/app/services/bulk_upload/sales/year2023/row_parser.rb @@ -328,8 +328,8 @@ class BulkUpload::Sales::Year2023::RowParser category: :setup, }, format: { - with: /\A\d{2}\z/, - message: I18n.t("validations.setup.saledate.year_not_two_digits"), + with: /\A(\d{2}|\d{4})\z/, + message: I18n.t("validations.setup.saledate.year_not_two_or_four_digits"), category: :setup, if: proc { field_5.present? }, }, on: :after_log @@ -954,19 +954,22 @@ private end def saledate - Date.new(field_5 + 2000, field_4, field_3) if field_5.present? && field_4.present? && field_3.present? + year = field_5.to_s.strip.length.between?(1, 2) ? field_5 + 2000 : field_5 + Date.new(year, field_4, field_3) if field_5.present? && field_4.present? && field_3.present? rescue Date::Error Date.new end def hodate - Date.new(field_97 + 2000, field_96, field_95) if field_97.present? && field_96.present? && field_95.present? + year = field_97.to_s.strip.length.between?(1, 2) ? field_97 + 2000 : field_97 + Date.new(year, field_96, field_95) if field_97.present? && field_96.present? && field_95.present? rescue Date::Error Date.new end def exdate - Date.new(field_94 + 2000, field_93, field_92) if field_94.present? && field_93.present? && field_92.present? + year = field_94.to_s.strip.length.between?(1, 2) ? field_94 + 2000 : field_94 + Date.new(year, field_93, field_92) if field_94.present? && field_93.present? && field_92.present? rescue Date::Error Date.new end diff --git a/app/services/bulk_upload/sales/year2024/csv_parser.rb b/app/services/bulk_upload/sales/year2024/csv_parser.rb index 9ba99c19b..4a3cb7ac9 100644 --- a/app/services/bulk_upload/sales/year2024/csv_parser.rb +++ b/app/services/bulk_upload/sales/year2024/csv_parser.rb @@ -114,9 +114,11 @@ private def first_record_start_date if with_headers? - Date.new(row_parsers.first.field_6.to_i + 2000, row_parsers.first.field_5.to_i, row_parsers.first.field_4.to_i) + year = row_parsers.first.field_6.to_s.strip.length.between?(1, 2) ? row_parsers.first.field_6.to_i + 2000 : row_parsers.first.field_6.to_i + Date.new(year, row_parsers.first.field_5.to_i, row_parsers.first.field_4.to_i) else - Date.new(rows.first[5].to_i + 2000, rows.first[4].to_i, rows.first[3].to_i) + year = rows.first[5].to_s.strip.length.between?(1, 2) ? rows.first[5].to_i + 2000 : rows.first[5].to_i + Date.new(year, rows.first[4].to_i, rows.first[3].to_i) end end end diff --git a/app/services/bulk_upload/sales/year2024/row_parser.rb b/app/services/bulk_upload/sales/year2024/row_parser.rb index 35984a045..7b72eef5c 100644 --- a/app/services/bulk_upload/sales/year2024/row_parser.rb +++ b/app/services/bulk_upload/sales/year2024/row_parser.rb @@ -320,8 +320,8 @@ class BulkUpload::Sales::Year2024::RowParser category: :setup, }, format: { - with: /\A\d{2}\z/, - message: I18n.t("validations.setup.saledate.year_not_two_digits"), + with: /\A(\d{2}|\d{4})\z/, + message: I18n.t("validations.setup.saledate.year_not_two_or_four_digits"), category: :setup, if: proc { field_6.present? }, }, on: :after_log @@ -994,19 +994,22 @@ private end def saledate - Date.new(field_6 + 2000, field_5, field_4) if field_6.present? && field_5.present? && field_4.present? + year = field_6.to_s.strip.length.between?(1, 2) ? field_6 + 2000 : field_6 + Date.new(year, field_5, field_4) if field_6.present? && field_5.present? && field_4.present? rescue Date::Error Date.new end def hodate - Date.new(field_96 + 2000, field_95, field_94) if field_96.present? && field_95.present? && field_94.present? + year = field_96.to_s.strip.length.between?(1, 2) ? field_96 + 2000 : field_96 + Date.new(year, field_95, field_94) if field_96.present? && field_95.present? && field_94.present? rescue Date::Error Date.new end def exdate - Date.new(field_93 + 2000, field_92, field_91) if field_93.present? && field_92.present? && field_91.present? + year = field_93.to_s.strip.length.between?(1, 2) ? field_93 + 2000 : field_93 + Date.new(year, field_92, field_91) if field_93.present? && field_92.present? && field_91.present? rescue Date::Error Date.new end diff --git a/config/locales/en.yml b/config/locales/en.yml index d490fd8ae..faced2c11 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -284,7 +284,7 @@ en: intermediate_rent_product_name: blank: "Enter name of other intermediate rent product." saledate: - year_not_two_digits: "Sale completion year must be 2 digits." + year_not_two_or_four_digits: "Sale completion year must be 2 or 4 digits." type: percentage_bought_must_be_at_least_threshold: "The minimum increase in equity while staircasing is %{threshold}% for this shared ownership type." @@ -297,7 +297,7 @@ en: before_scheme_end_date: "The tenancy start date must be before the end date for this supported housing scheme." after_void_date: "Enter a tenancy start date that is after the void date." after_major_repair_date: "Enter a tenancy start date that is after the major repair date." - year_not_two_digits: "Tenancy start year must be 2 digits." + year_not_two_or_four_digits: "Tenancy start year must be 2 or 4 digits." ten_years_after_void_date: "Enter a tenancy start date that is no more than 10 years after the void date." ten_years_after_mrc_date: "Enter a tenancy start date that is no more than 10 years after the major repairs completion date." invalid_merged_organisations_start_date: diff --git a/spec/services/bulk_upload/lettings/year2023/row_parser_spec.rb b/spec/services/bulk_upload/lettings/year2023/row_parser_spec.rb index 6d3feffa9..314db674d 100644 --- a/spec/services/bulk_upload/lettings/year2023/row_parser_spec.rb +++ b/spec/services/bulk_upload/lettings/year2023/row_parser_spec.rb @@ -1476,11 +1476,21 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do end context "when field_9 is 4 digits instead of 2" do - let(:attributes) { { bulk_upload:, field_9: "2022" } } + let(:attributes) { setup_section_params.merge({ bulk_upload:, field_9: "2023", field_8: "12", field_7: "1" }) } + + it "correctly sets the date" do + parser.valid? + expect(parser.errors[:field_9]).to be_empty + expect(parser.log.startdate).to eq(Date.new(2023, 12, 1)) + end + end + + context "when field_9 is not 4 or 2 digits" do + let(:attributes) { { bulk_upload:, field_9: "202" } } it "returns an error" do parser.valid? - expect(parser.errors[:field_9]).to include("Tenancy start year must be 2 digits.") + expect(parser.errors[:field_9]).to include("Tenancy start year must be 2 or 4 digits.") end end @@ -2546,6 +2556,14 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do end end + context "when valid (4 digit year)" do + let(:attributes) { { bulk_upload:, field_36: "13", field_37: "12", field_38: "2022" } } + + it "sets value given" do + expect(parser.log.mrcdate).to eq(Date.new(2022, 12, 13)) + end + end + context "when invalid" do let(:attributes) { { bulk_upload:, field_36: "13", field_37: "13", field_38: "22" } } @@ -2582,6 +2600,14 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do end end + context "when valid (4 digit year)" do + let(:attributes) { { bulk_upload:, field_33: "13", field_34: "12", field_35: "2022" } } + + it "sets value given" do + expect(parser.log.voiddate).to eq(Date.new(2022, 12, 13)) + end + end + context "when invalid" do let(:attributes) { { bulk_upload:, field_33: "13", field_34: "13", field_35: "22" } } @@ -2824,12 +2850,12 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do end end - describe "#start_date" do + describe "#startdate" do context "when year of 9 is passed to represent 2009" do let(:attributes) { { bulk_upload:, field_7: "1", field_8: "1", field_9: "9" } } it "uses the year 2009" do - expect(parser.send(:start_date)).to eql(Date.new(2009, 1, 1)) + expect(parser.send(:startdate)).to eql(Date.new(2009, 1, 1)) end end end 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 2e4c82635..d82effa0b 100644 --- a/spec/services/bulk_upload/lettings/year2024/row_parser_spec.rb +++ b/spec/services/bulk_upload/lettings/year2024/row_parser_spec.rb @@ -1303,11 +1303,21 @@ RSpec.describe BulkUpload::Lettings::Year2024::RowParser do end context "when field_10 is 4 digits instead of 2" do - let(:attributes) { { bulk_upload:, field_10: "2023" } } + let(:attributes) { setup_section_params.merge({ bulk_upload:, field_10: "2024", field_9: "4", field_8: "5" }) } + + it "correctly sets the date" do + parser.valid? + expect(parser.errors[:field_10]).to be_empty + expect(parser.log.startdate).to eq(Time.zone.local(2024, 4, 5)) + end + end + + context "when field_10 is not 4 or 2 digits" do + let(:attributes) { { bulk_upload:, field_10: "204" } } it "returns an error" do parser.valid? - expect(parser.errors[:field_10]).to include("Tenancy start year must be 2 digits.") + expect(parser.errors[:field_10]).to include("Tenancy start year must be 2 or 4 digits.") end end @@ -2659,6 +2669,14 @@ RSpec.describe BulkUpload::Lettings::Year2024::RowParser do end end + context "when valid (4 digit year)" do + let(:attributes) { { bulk_upload:, field_33: "13", field_34: "12", field_35: "2022" } } + + it "sets value given" do + expect(parser.log.mrcdate).to eq(Date.new(2022, 12, 13)) + end + end + context "when invalid" do let(:attributes) { { bulk_upload:, field_33: "13", field_34: "13", field_35: "22" } } @@ -2695,6 +2713,14 @@ RSpec.describe BulkUpload::Lettings::Year2024::RowParser do end end + context "when valid (4 digit year)" do + let(:attributes) { { bulk_upload:, field_30: "13", field_31: "12", field_32: "2022" } } + + it "sets value given" do + expect(parser.log.voiddate).to eq(Date.new(2022, 12, 13)) + end + end + context "when invalid" do let(:attributes) { { bulk_upload:, field_30: "13", field_31: "13", field_32: "22" } } @@ -2945,12 +2971,12 @@ RSpec.describe BulkUpload::Lettings::Year2024::RowParser do end end - describe "#start_date" do + describe "#startdate" do context "when year of 9 is passed to represent 2009" do let(:attributes) { { bulk_upload:, field_8: "1", field_9: "1", field_10: "9" } } it "uses the year 2009" do - expect(parser.send(:start_date)).to eql(Date.new(2009, 1, 1)) + expect(parser.send(:startdate)).to eql(Date.new(2009, 1, 1)) end end end diff --git a/spec/services/bulk_upload/sales/year2023/row_parser_spec.rb b/spec/services/bulk_upload/sales/year2023/row_parser_spec.rb index d5b6aa718..824754b25 100644 --- a/spec/services/bulk_upload/sales/year2023/row_parser_spec.rb +++ b/spec/services/bulk_upload/sales/year2023/row_parser_spec.rb @@ -88,10 +88,10 @@ RSpec.describe BulkUpload::Sales::Year2023::RowParser do field_90: "1", field_92: "30", field_93: "3", - field_94: "22", + field_94: "2022", field_95: "24", field_96: "3", - field_97: "22", + field_97: "2022", field_98: "3", field_99: "1", field_100: "1", @@ -594,10 +594,21 @@ RSpec.describe BulkUpload::Sales::Year2023::RowParser do end context "when field 5 is 4 digits instead of 2" do - let(:attributes) { setup_section_params.merge({ bulk_upload:, field_5: "2022" }) } + let(:attributes) { setup_section_params.merge({ bulk_upload:, field_5: "2023", field_4: "4", field_3: "3" }) } + + it "correctly sets the date" do + parser.valid? + expect(parser.errors.where(:field_5, category: :setup)).to be_empty + expect(parser.log.saledate).to eq(Time.zone.local(2023, 4, 3)) + end + end + + context "when field 5 is not 2 or 4 digits" do + let(:attributes) { setup_section_params.merge({ bulk_upload:, field_5: "202" }) } it "returns a setup error" do - expect(parser.errors.where(:field_5, category: :setup).map(&:message)).to include("Sale completion year must be 2 digits.") + parser.valid? + expect(parser.errors.where(:field_5, category: :setup).map(&:message)).to include("Sale completion year must be 2 or 4 digits.") end end diff --git a/spec/services/bulk_upload/sales/year2024/row_parser_spec.rb b/spec/services/bulk_upload/sales/year2024/row_parser_spec.rb index 3c4e71247..96914fc44 100644 --- a/spec/services/bulk_upload/sales/year2024/row_parser_spec.rb +++ b/spec/services/bulk_upload/sales/year2024/row_parser_spec.rb @@ -95,10 +95,10 @@ RSpec.describe BulkUpload::Sales::Year2024::RowParser do field_89: "1", field_91: "30", field_92: "3", - field_93: "22", + field_93: "2022", field_94: "24", field_95: "3", - field_96: "22", + field_96: "2022", field_97: "3", field_98: "1", field_99: "1", @@ -721,12 +721,22 @@ RSpec.describe BulkUpload::Sales::Year2024::RowParser do end end - context "when field 5 is 4 digits instead of 2" do - let(:attributes) { setup_section_params.merge({ bulk_upload:, field_6: "2023" }) } + context "when field 6 is 4 digits instead of 2" do + let(:attributes) { setup_section_params.merge({ bulk_upload:, field_6: "2024" }) } + + it "correctly sets the date" do + parser.valid? + expect(parser.errors.where(:field_6, category: :setup)).to be_empty + expect(parser.log.saledate).to eq(Time.zone.local(2024, 5, 1)) + end + end + + context "when field 5 is not 2 or 4 digits" do + let(:attributes) { setup_section_params.merge({ bulk_upload:, field_6: "202" }) } it "returns a setup error" do parser.valid? - expect(parser.errors.where(:field_6, category: :setup).map(&:message)).to include("Sale completion year must be 2 digits.") + expect(parser.errors.where(:field_6, category: :setup).map(&:message)).to include("Sale completion year must be 2 or 4 digits.") end end