From 6360c56da5945c9649632450e138c9f79960c41f Mon Sep 17 00:00:00 2001 From: Dushan <47317567+dushan-madetech@users.noreply.github.com> Date: Tue, 8 Feb 2022 11:22:54 +0000 Subject: [PATCH] CLDC-933 update household income questions (#273) * Change net income known question * earnings and frequency on same page * check answers changes and other fixes * some fixes * delete unnecessary test * fix failing spec * Refactor answer label Co-authored-by: baarkerlounger * test and lint fixes * Method args * Fix specs * Rubocop * Incfreq doesn't have it's own check answers display * Add suffix to actual form * JSON linting * Conditional suffix only applies to check answers * Validate that earnings and incfreq must be provided together * Rubocop * Fix spec * Fix page view specs * form fixes * update error messages Co-authored-by: baarkerlounger Co-authored-by: baarkerlounger --- app/helpers/check_answers_helper.rb | 8 +- app/models/case_log.rb | 17 +--- app/models/constants/case_log.rb | 9 +- app/models/form.rb | 4 + app/models/form/question.rb | 33 ++++++- .../validations/financial_validations.rb | 18 +++- app/views/form/_check_answers_table.html.erb | 2 +- app/views/form/_numeric_question.html.erb | 2 +- config/forms/2021_2022.json | 93 ++++++------------- config/locales/en.yml | 2 + spec/factories/case_log.rb | 3 +- spec/fixtures/complete_case_log.json | 3 +- spec/fixtures/forms/2021_2022.json | 6 +- spec/models/case_log_spec.rb | 11 --- spec/models/form/question_spec.rb | 28 +++++- .../validations/financial_validations_spec.rb | 24 +++++ .../validations/property_validations_spec.rb | 1 - spec/views/form/page_view_spec.rb | 21 +++++ 18 files changed, 163 insertions(+), 122 deletions(-) create mode 100644 spec/models/validations/financial_validations_spec.rb diff --git a/app/helpers/check_answers_helper.rb b/app/helpers/check_answers_helper.rb index 6797b00af..afa95069b 100644 --- a/app/helpers/check_answers_helper.rb +++ b/app/helpers/check_answers_helper.rb @@ -12,12 +12,6 @@ module CheckAnswersHelper end def get_answer_label(question, case_log) - answer = question.prefix == "£" ? ActionController::Base.helpers.number_to_currency(question.answer_label(case_log), delimiter: ",", format: "%n") : question.answer_label(case_log) - - if answer.present? - [question.prefix, answer, question.suffix].join("") - else - "You didn’t answer this question".html_safe - end + question.answer_label(case_log).presence || "You didn’t answer this question".html_safe end end diff --git a/app/models/case_log.rb b/app/models/case_log.rb index 1cb3a4cc9..f3984e247 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -243,22 +243,7 @@ private self.month = startdate.month self.year = startdate.year end - case net_income_known - when "Weekly" - self.incfreq = "Weekly" - self.incref = nil - when "Monthly" - self.incfreq = "Monthly" - self.incref = nil - when "Annually" - self.incfreq = "Yearly" - self.incref = nil - when "Tenant prefers not to say" - self.incref = 1 - self.incfreq = nil - when "Don’t know" - self.incfreq = nil - end + self.incref = 1 if net_income_known == "Tenant prefers not to say" self.hhmemb = other_hhmemb + 1 if other_hhmemb.present? self.renttype = RENT_TYPE_MAPPING[rent_type] self.lettype = "#{renttype} #{needstype} #{owning_organisation[:provider_type]}" if renttype.present? && needstype.present? && owning_organisation[:provider_type].present? diff --git a/app/models/constants/case_log.rb b/app/models/constants/case_log.rb index 712aff8d0..73cfc4a51 100644 --- a/app/models/constants/case_log.rb +++ b/app/models/constants/case_log.rb @@ -1071,11 +1071,10 @@ module Constants::CaseLog }.freeze NET_INCOME_KNOWN = { - "Weekly" => 0, - "Monthly" => 1, - "Annually" => 2, - "Tenant prefers not to say" => 3, - "Don’t know" => 4, + "Yes" => 0, + "No" => 1, + "Tenant prefers not to say" => 2, + "Don’t know" => 3, }.freeze HAS_BENEFITS_OPTIONS = ["Housing benefit", diff --git a/app/models/form.rb b/app/models/form.rb index f00507f50..d21728def 100644 --- a/app/models/form.rb +++ b/app/models/form.rb @@ -24,6 +24,10 @@ class Form pages.find { |p| p.id == id.to_s.underscore } end + def get_question(id) + questions.find { |q| q.id == id.to_s.underscore } + end + def subsection_for_page(page) subsections.find { |s| s.pages.find { |p| p.id == page.id } } end diff --git a/app/models/form/question.rb b/app/models/form/question.rb index 3510c8c38..24575a374 100644 --- a/app/models/form/question.rb +++ b/app/models/form/question.rb @@ -38,7 +38,10 @@ class Form::Question return checkbox_answer_label(case_log) if type == "checkbox" return case_log[id]&.to_formatted_s(:govuk_date).to_s if type == "date" - return case_log[id].to_s if case_log[id].present? + answer = case_log[id].to_s if case_log[id].present? + answer_label = [prefix, format_value(answer), suffix_label(case_log)].join("") if answer + + return answer_label if answer_label has_inferred_check_answers_value?(case_log) ? inferred_check_answers_value["value"] : "" end @@ -96,6 +99,28 @@ private answer.join(", ") end + def format_value(answer_label) + prefix == "£" ? ActionController::Base.helpers.number_to_currency(answer_label, delimiter: ",", format: "%n") : answer_label + end + + def suffix_label(case_log) + return "" unless suffix + return suffix if suffix.is_a?(String) + + label = "" + + suffix.each do |s| + condition = s["depends_on"] + next unless condition + + answer = case_log.send(condition.keys.first) + if answer == condition.values.first + label = ANSWER_SUFFIX_LABELS.key?(answer.to_sym) ? ANSWER_SUFFIX_LABELS[answer.to_sym] : answer + end + end + label + end + def conditional_on @conditional_on ||= form.conditional_question_conditions.select do |condition| condition[:to] == id @@ -118,4 +143,10 @@ private def enabled_inferred_answers(inferred_answers, case_log) inferred_answers.filter { |_key, value| value.all? { |condition_key, condition_value| case_log[condition_key] == condition_value } } end + + ANSWER_SUFFIX_LABELS = { + "Weekly": " every week", + "Monthly": " every month", + "Yearly": " every year", + }.freeze end diff --git a/app/models/validations/financial_validations.rb b/app/models/validations/financial_validations.rb index 048957acf..a06328c25 100644 --- a/app/models/validations/financial_validations.rb +++ b/app/models/validations/financial_validations.rb @@ -21,14 +21,22 @@ module Validations::FinancialValidations end def validate_net_income(record) - return unless record.ecstat1 && record.weekly_net_income + if record.ecstat1 && record.weekly_net_income + if record.weekly_net_income > record.applicable_income_range.hard_max + record.errors.add :earnings, I18n.t("validations.financial.earnings.under_hard_max", hard_max: record.applicable_income_range.hard_max) + end + + if record.weekly_net_income < record.applicable_income_range.hard_min + record.errors.add :earnings, I18n.t("validations.financial.earnings.over_hard_min", hard_min: record.applicable_income_range.hard_min) + end + end - if record.weekly_net_income > record.applicable_income_range.hard_max - record.errors.add :earnings, I18n.t("validations.financial.earnings.under_hard_max", hard_max: record.applicable_income_range.hard_max) + if record.earnings.present? && record.incfreq.blank? + record.errors.add :incfreq, I18n.t("validations.financial.earnings.freq_missing") end - if record.weekly_net_income < record.applicable_income_range.hard_min - record.errors.add :earnings, I18n.t("validations.financial.earnings.over_hard_min", hard_min: record.applicable_income_range.hard_min) + if record.incfreq.present? && record.earnings.blank? + record.errors.add :earnings, I18n.t("validations.financial.earnings.earnings_missing") end end diff --git a/app/views/form/_check_answers_table.html.erb b/app/views/form/_check_answers_table.html.erb index 0ba792f1f..4a2b7183a 100644 --- a/app/views/form/_check_answers_table.html.erb +++ b/app/views/form/_check_answers_table.html.erb @@ -1,6 +1,6 @@
- <%= question.check_answer_label.to_s.present? ? question.check_answer_label.to_s : question.header.to_s %> + <%= question.check_answer_label.to_s.present? ? question.check_answer_label.to_s : question.header.to_s %>
<%= get_answer_label(question, @case_log) %>
diff --git a/app/views/form/_numeric_question.html.erb b/app/views/form/_numeric_question.html.erb index 606d2294c..1e9702310 100644 --- a/app/views/form/_numeric_question.html.erb +++ b/app/views/form/_numeric_question.html.erb @@ -7,6 +7,6 @@ min: question.min, max: question.max, step: question.step, width: question.width, :readonly => question.read_only?, prefix_text: question.prefix.to_s, - suffix_text: question.suffix.to_s, + suffix_text: question.suffix.is_a?(String) ? question.suffix : nil, **stimulus_html_attributes(question) %> diff --git a/config/forms/2021_2022.json b/config/forms/2021_2022.json index 9228e1eac..33fdaa6e0 100644 --- a/config/forms/2021_2022.json +++ b/config/forms/2021_2022.json @@ -2292,95 +2292,56 @@ "depends_on": [{ "setup": "completed" }], "pages": { "net_income_known": { - "header": "Household’s combined income", + "header": "", "description": "", "questions": { "net_income_known": { - "check_answer_label": "How often household receives income", - "header": "How often does the household receive income?", - "guidance_partial": "what_counts_as_income", + "check_answer_label": "Do you know the household’s combined income?", + "header": "Do you know the household’s combined income?", "hint_text": "", "type": "radio", "answer_options": { - "0": "Weekly", - "1": "Monthly", - "2": "Annually", + "0": "Yes", + "1": "No", "divider_a": true, - "3": "Don’t know", - "4": "Tenant prefers not to say" + "2": "Don’t know", + "3": "Tenant prefers not to say" } } } }, - "weekly_net_income": { - "depends_on": [{ "net_income_known": "Weekly" }], - "header": "", + "net_income": { + "depends_on": [{ "net_income_known": "Yes" }], + "header": "Household’s combined income after tax", "description": "", "questions": { "earnings": { "check_answer_label": "Total household income", - "header": "How much income does the household have in total every week?", + "header": "How much income does the household have in total?", + "guidance_partial": "what_counts_as_income", "hint_text": "", "type": "numeric", "min": 0, "step": "1", "width": 5, "prefix": "£", - "suffix": " every week" - } - }, - "soft_validations": { - "override_net_income_validation": { - "check_answer_label": "Net income confirmed?", - "type": "validation_override", - "answer_options": { - "override_net_income_validation": "Yes" - } - } - } - }, - "monthly_net_income": { - "depends_on": [{ "net_income_known": "Monthly" }], - "header": "", - "description": "", - "questions": { - "earnings": { - "check_answer_label": "Total household income", - "header": "How much income does the household have in total every month?", + "suffix": [ + { "label": "every week", "depends_on" : { "incfreq": "Weekly" } }, + { "label": "every month", "depends_on" : { "incfreq": "Monthly" } }, + { "label": "every month", "depends_on" : { "incfreq": "Yearly" } } + ] + }, + "incfreq": { + "check_answer_label": "How often does the household receive this amount?", + "header": "How often does the household receive this amount?", "hint_text": "", - "type": "numeric", - "min": 0, - "step": "1", - "width": 5, - "prefix": "£", - "suffix": " every month" - } - }, - "soft_validations": { - "override_net_income_validation": { - "check_answer_label": "Net income confirmed?", - "type": "validation_override", + "type": "radio", "answer_options": { - "override_net_income_validation": "Yes" - } - } - } - }, - "yearly_net_income": { - "depends_on": [{ "net_income_known": "Annually" }], - "header": "", - "description": "", - "questions": { - "earnings": { - "check_answer_label": "Total household income", - "header": "How much income does the household have in total every year?", - "hint_text": "", - "type": "numeric", - "min": 0, - "step": "1", - "width": 5, - "prefix": "£", - "suffix": " every year" + "0": "Weekly", + "1": "Monthly", + "2": "Yearly" + }, + "hidden_in_check_answers": true } }, "soft_validations": { diff --git a/config/locales/en.yml b/config/locales/en.yml index c40da4bda..f4d8cda63 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -68,6 +68,8 @@ en: earnings: under_hard_max: "Net income cannot be greater than %{hard_max} given the tenant’s working situation" over_hard_min: "Net income cannot be less than %{hard_min} given the tenant’s working situation" + freq_missing: "Select how often the household receives income" + earnings_missing: "Enter how much income the household has in total" household: reasonpref: diff --git a/spec/factories/case_log.rb b/spec/factories/case_log.rb index b3c952ded..71940ab99 100644 --- a/spec/factories/case_log.rb +++ b/spec/factories/case_log.rb @@ -69,6 +69,7 @@ FactoryBot.define do offered { 2 } wchair { "Yes" } earnings { 68 } + incfreq { "Weekly" } benefits { "Some" } period { "Every 2 weeks" } brent { 200 } @@ -110,7 +111,7 @@ FactoryBot.define do discarded_at { nil } tenancyother { nil } override_net_income_validation { nil } - net_income_known { "Weekly" } + net_income_known { "Yes" } property_owner_organisation { "Test" } property_manager_organisation { "Test" } renewal { 1 } diff --git a/spec/fixtures/complete_case_log.json b/spec/fixtures/complete_case_log.json index d3b9396bf..268283c2a 100644 --- a/spec/fixtures/complete_case_log.json +++ b/spec/fixtures/complete_case_log.json @@ -74,8 +74,9 @@ "mrcyear": 2020, "offered": 2, "wchair": "Yes", - "net_income_known": "Weekly", + "net_income_known": "Yes", "earnings": 150, + "incfreq": "Weekly", "benefits": "Some", "hb": "Universal Credit with housing element (excluding housing benefit)", "period": "Every 2 weeks", diff --git a/spec/fixtures/forms/2021_2022.json b/spec/fixtures/forms/2021_2022.json index 12fb93b95..e675ccea5 100644 --- a/spec/fixtures/forms/2021_2022.json +++ b/spec/fixtures/forms/2021_2022.json @@ -358,7 +358,11 @@ "step": 1, "width": 5, "prefix": "£", - "suffix": "incfreq" + "suffix": [ + { "label": "every week", "depends_on" : { "incfreq": "Weekly" } }, + { "label": "every month", "depends_on" : { "incfreq": "Monthly" } }, + { "label": "every month", "depends_on" : { "incfreq": "Yearly" } } + ] }, "incfreq": { "check_answer_label": "Income Frequency", diff --git a/spec/models/case_log_spec.rb b/spec/models/case_log_spec.rb index 82744db29..8b704cf0e 100644 --- a/spec/models/case_log_spec.rb +++ b/spec/models/case_log_spec.rb @@ -1092,17 +1092,6 @@ RSpec.describe CaseLog do end end - context "when saving net_income" do - it "infers the income frequency" do - case_log.update!(net_income_known: "Weekly") - expect(case_log.reload.incfreq).to eq("Weekly") - case_log.update!(net_income_known: "Monthly") - expect(case_log.reload.incfreq).to eq("Monthly") - case_log.update!(net_income_known: "Annually") - expect(case_log.reload.incfreq).to eq("Yearly") - end - end - context "when saving rent and charges" do let!(:case_log) do described_class.create({ diff --git a/spec/models/form/question_spec.rb b/spec/models/form/question_spec.rb index 5d747311c..7fe6cff28 100644 --- a/spec/models/form/question_spec.rb +++ b/spec/models/form/question_spec.rb @@ -99,16 +99,17 @@ RSpec.describe Form::Question, type: :model do context "with a case log" do let(:case_log) { FactoryBot.build(:case_log, :in_progress) } + let(:question_id) { "incfreq" } it "has an answer label" do - case_log.earnings = 100 - expect(question.answer_label(case_log)).to eq("100") + case_log.incfreq = "Weekly" + expect(question.answer_label(case_log)).to eq("Weekly") end it "has an update answer link text helper" do - expect(question.update_answer_link_name(case_log)).to eq("Answer income") - case_log[question_id] = 5 - expect(question.update_answer_link_name(case_log)).to eq("Change income") + expect(question.update_answer_link_name(case_log)).to match(/Answer/) + case_log["incfreq"] = "Weekly" + expect(question.update_answer_link_name(case_log)).to match(/Change/) end context "when type is date" do @@ -155,6 +156,23 @@ RSpec.describe Form::Question, type: :model do expect(question.enabled?(case_log)).to be true end end + + context "when answers have a suffix dependent on another answer" do + let(:section_id) { "rent_and_charges" } + let(:subsection_id) { "income_and_benefits" } + let(:page_id) { "net_income" } + let(:question_id) { "earnings" } + + it "displays the correct label for given suffix and answer the suffix depends on" do + case_log.incfreq = "Weekly" + case_log.earnings = 500 + expect(question.answer_label(case_log)).to eq("£500.00 every week") + case_log.incfreq = "Monthly" + expect(question.answer_label(case_log)).to eq("£500.00 every month") + case_log.incfreq = "Yearly" + expect(question.answer_label(case_log)).to eq("£500.00 every year") + end + end end describe ".completed?" do diff --git a/spec/models/validations/financial_validations_spec.rb b/spec/models/validations/financial_validations_spec.rb new file mode 100644 index 000000000..1cbd1cde1 --- /dev/null +++ b/spec/models/validations/financial_validations_spec.rb @@ -0,0 +1,24 @@ +require "rails_helper" + +RSpec.describe Validations::FinancialValidations do + subject(:financial_validator) { validator_class.new } + + let(:validator_class) { Class.new { include Validations::FinancialValidations } } + let(:record) { FactoryBot.create(:case_log) } + + describe "earnings and income frequency" do + it "when earnings are provided it validates that income frequency must be provided" do + record.earnings = 500 + record.incfreq = nil + financial_validator.validate_net_income(record) + expect(record.errors["incfreq"]).to include(match I18n.t("validations.financial.earnings.freq_missing")) + end + + it "when income frequency is provided it validates that earnings must be provided" do + record.earnings = nil + record.incfreq = "Weekly" + financial_validator.validate_net_income(record) + expect(record.errors["earnings"]).to include(match I18n.t("validations.financial.earnings.earnings_missing")) + end + end +end diff --git a/spec/models/validations/property_validations_spec.rb b/spec/models/validations/property_validations_spec.rb index 15e981036..003208cf4 100644 --- a/spec/models/validations/property_validations_spec.rb +++ b/spec/models/validations/property_validations_spec.rb @@ -1,5 +1,4 @@ require "rails_helper" -require_relative "../../request_helper" RSpec.describe Validations::PropertyValidations do subject(:property_validator) { property_validator_class.new } diff --git a/spec/views/form/page_view_spec.rb b/spec/views/form/page_view_spec.rb index e75f650cf..d10b435cd 100644 --- a/spec/views/form/page_view_spec.rb +++ b/spec/views/form/page_view_spec.rb @@ -70,6 +70,27 @@ RSpec.describe "form/page" do expect(rendered).to match(/govuk-input__suffix/) expect(rendered).to match("every week") end + + context "when the suffix is conditional and not a string" do + let(:question_attributes) do + { + type: "numeric", + prefix: "£", + suffix: [ + { "label": "every week", "depends_on": { "incfreq": "Weekly" } }, + { "label": "every month", "depends_on": { "incfreq": "Monthly" } }, + { "label": "every month", "depends_on": { "incfreq": "Yearly" } }, + ], + } + end + + it "does not render the suffix" do + expect(rendered).not_to match(/govuk-input__suffix/) + expect(rendered).not_to match("every week") + expect(rendered).not_to match("every month") + expect(rendered).not_to match("every year") + end + end end context "with a question containing extra guidance" do