From ade5eb314f7a799955d8e4daa1874d46a81ea96b Mon Sep 17 00:00:00 2001 From: Paul Robert Lloyd Date: Mon, 7 Feb 2022 17:17:09 +0000 Subject: [PATCH 01/11] Ensure error summary on question pages appears above page title --- app/views/form/page.html.erb | 37 ++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/app/views/form/page.html.erb b/app/views/form/page.html.erb index 29c784b43..893cd10dc 100644 --- a/app/views/form/page.html.erb +++ b/app/views/form/page.html.erb @@ -14,24 +14,25 @@
<%= turbo_frame_tag "case_log_form", target: "_top" do %> -
-
- <% if @page.header.present? %> -

- <% if !@page.hide_subsection_label %> - <%= @subsection.label %> - <% end %> - <%= @page.header %> -

- <% end %> - - <% if @page.description.present? %> -

<%= @page.description.html_safe %>

- <% end %> - - <%= form_with model: @case_log, url: form_case_log_path(@case_log), method: "post" do |f| %> + <%= form_with model: @case_log, url: form_case_log_path(@case_log), method: "post" do |f| %> +
+
<% remove_other_page_errors(@case_log, @page) %> <%= f.govuk_error_summary %> + + <% if @page.header.present? %> +

+ <% if !@page.hide_subsection_label %> + <%= @subsection.label %> + <% end %> + <%= @page.header %> +

+ <% end %> + + <% if @page.description.present? %> +

<%= @page.description.html_safe %>

+ <% end %> + <% @page.non_conditional_questions.map do |question| %>
<%= display_question_key_div(@page, question) %> > <% if question.read_only? %> @@ -47,7 +48,7 @@ <%= f.hidden_field :page, value: @page.id %> <%= f.govuk_submit "Save and continue" %> - <% end %> +
-
+ <% end %> <% end %> From 512c5fdad207ebc4057a41164dac6bbb7e0745eb Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Tue, 8 Feb 2022 10:59:55 +0000 Subject: [PATCH 02/11] Cldc 972 submitting lettings log (#279) * add validation for declaration on log submission * Only present next incomplete section link if such section exists --- app/controllers/form_controller.rb | 8 ++- app/models/bulk_upload.rb | 3 +- app/models/case_log.rb | 2 + app/models/form.rb | 7 ++- app/models/form/question.rb | 2 - .../validations/submission_validations.rb | 10 ++++ app/views/case_logs/edit.html.erb | 14 +++-- app/views/form/page.html.erb | 6 +- config/forms/2021_2022.json | 57 ++++--------------- config/locales/en.yml | 3 + db/migrate/20220207091117_add_declaration.rb | 7 +++ .../20220208101235_remove_gdpr_fields.rb | 15 +++++ db/schema.rb | 6 +- spec/factories/case_log.rb | 4 +- spec/features/form/check_answers_page_spec.rb | 2 +- spec/features/form/tasklist_page_spec.rb | 4 +- spec/features/form/validations_spec.rb | 31 ++++++++++ spec/fixtures/complete_case_log.json | 5 +- spec/fixtures/forms/2021_2022.json | 39 ++----------- spec/fixtures/forms/2022_2023.json | 39 +------------ spec/helpers/tasklist_helper_spec.rb | 4 +- spec/models/form/question_spec.rb | 12 ---- spec/models/form/subsection_spec.rb | 10 ---- spec/models/form_handler_spec.rb | 2 +- spec/models/form_spec.rb | 16 ++---- spec/requests/case_logs_controller_spec.rb | 4 +- 26 files changed, 133 insertions(+), 179 deletions(-) create mode 100644 app/models/validations/submission_validations.rb create mode 100644 db/migrate/20220207091117_add_declaration.rb create mode 100644 db/migrate/20220208101235_remove_gdpr_fields.rb diff --git a/app/controllers/form_controller.rb b/app/controllers/form_controller.rb index 573c8631a..bb99cd040 100644 --- a/app/controllers/form_controller.rb +++ b/app/controllers/form_controller.rb @@ -8,8 +8,12 @@ class FormController < ApplicationController @page = @case_log.form.get_page(params[:case_log][:page]) responses_for_page = responses_for_page(@page) if @case_log.update(responses_for_page) && @case_log.has_no_unresolved_soft_errors? - redirect_path = @case_log.form.next_page_redirect_path(@page, @case_log) - redirect_to(send(redirect_path, @case_log)) + if @case_log.form.is_last_question?(@page, @case_log.form.subsection_for_page(@page), @case_log) + redirect_to(case_logs_path) + else + redirect_path = @case_log.form.next_page_redirect_path(@page, @case_log) + redirect_to(send(redirect_path, @case_log)) + end else @subsection = @case_log.form.subsection_for_page(@page) render "form/page", status: :unprocessable_entity diff --git a/app/models/bulk_upload.rb b/app/models/bulk_upload.rb index 66d24bf8d..aeccfc4cd 100644 --- a/app/models/bulk_upload.rb +++ b/app/models/bulk_upload.rb @@ -196,8 +196,7 @@ class BulkUpload intermediate_rent_product_name: row[131], # data_protection: row[132], sale_or_letting: "letting", - gdpr_acceptance: 1, - gdpr_declined: 0, + declaration: 1, } end diff --git a/app/models/case_log.rb b/app/models/case_log.rb index c8922029a..1cb3a4cc9 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -7,6 +7,7 @@ class CaseLogValidator < ActiveModel::Validator include Validations::TenancyValidations include Validations::DateValidations include Validations::LocalAuthorityValidations + include Validations::SubmissionValidations def validate(record) validation_methods = public_methods.select { |method| method.starts_with?("validate_") } @@ -140,6 +141,7 @@ class CaseLog < ApplicationRecord enum is_carehome: POLAR, _suffix: true enum nocharge: POLAR, _suffix: true enum referral: REFERRAL, _suffix: true + enum declaration: POLAR, _suffix: true AUTOGENERATED_FIELDS = %w[id status created_at updated_at discarded_at].freeze OPTIONAL_FIELDS = %w[postcode_known la_known first_time_property_let_as_social_housing].freeze diff --git a/app/models/form.rb b/app/models/form.rb index f14adc9f8..f00507f50 100644 --- a/app/models/form.rb +++ b/app/models/form.rb @@ -79,7 +79,7 @@ class Form next_subsection_id_index = subsection_ids.index(subsection.id) + 1 next_subsection = get_subsection(subsection_ids[next_subsection_id_index]) - if next_subsection.id == "declaration" && case_log.status != "completed" + if subsection.id == "declaration" && case_log.status != "completed" next_subsection = get_subsection(subsection_ids[0]) end @@ -122,4 +122,9 @@ class Form def readonly_questions questions.select(&:read_only?) end + + def is_last_question?(page, subsection, case_log) + subsection_ids = subsections.map(&:id) + subsection.id == subsection_ids[subsection_ids.length - 1] && next_page(page, case_log) == :check_answers + end end diff --git a/app/models/form/question.rb b/app/models/form/question.rb index 5282bae71..3510c8c38 100644 --- a/app/models/form/question.rb +++ b/app/models/form/question.rb @@ -79,8 +79,6 @@ class Form::Question end def completed?(case_log) - # Special case as No is a valid answer but doesn't let you progress and use the service - return false if id == "gdpr_acceptance" && case_log[id] == "No" return answer_options.keys.any? { |key| case_log[key] == "Yes" } if type == "checkbox" case_log[id].present? || !case_log.respond_to?(id.to_sym) || has_inferred_display_value?(case_log) diff --git a/app/models/validations/submission_validations.rb b/app/models/validations/submission_validations.rb new file mode 100644 index 000000000..9f8bf76d0 --- /dev/null +++ b/app/models/validations/submission_validations.rb @@ -0,0 +1,10 @@ +module Validations::SubmissionValidations + # Validations methods need to be called 'validate_' to run on model save + # or 'validate_' to run on submit as well + + def validate_declaration(record) + if record.declaration == "No" + record.errors.add :declaration, I18n.t("validations.declaration.missing") + end + end +end diff --git a/app/views/case_logs/edit.html.erb b/app/views/case_logs/edit.html.erb index 6c8b38e93..5cd0de9a4 100644 --- a/app/views/case_logs/edit.html.erb +++ b/app/views/case_logs/edit.html.erb @@ -21,12 +21,14 @@ <% next_incomplete_section = get_next_incomplete_section(@case_log) %>

- > - Skip to next incomplete section: <%= next_incomplete_section.label %> - + <% if next_incomplete_section.present? %> + > + Skip to next incomplete section: <%= next_incomplete_section.label %> + + <% end %>

<%= render "tasklist" %>
diff --git a/app/views/form/page.html.erb b/app/views/form/page.html.erb index 893cd10dc..bf648599f 100644 --- a/app/views/form/page.html.erb +++ b/app/views/form/page.html.erb @@ -47,7 +47,11 @@ <% end %> <%= f.hidden_field :page, value: @page.id %> - <%= f.govuk_submit "Save and continue" %> + <% if @case_log.form.is_last_question?(@page, @subsection, @case_log) %> + <%= f.govuk_submit "Submit lettings log" %> + <%else %> + <%= f.govuk_submit "Save and continue" %> + <%end %>
<% end %> diff --git a/config/forms/2021_2022.json b/config/forms/2021_2022.json index 7ebdfdfef..9228e1eac 100644 --- a/config/forms/2021_2022.json +++ b/config/forms/2021_2022.json @@ -9,30 +9,6 @@ "setup": { "label": "Set up your lettings log", "pages": { - "gdpr_acceptance": { - "header": "", - "description": "", - "questions": { - "gdpr_acceptance": { - "check_answer_label": "Privacy notice seen", - "header": "Has the tenant or buyer seen the Department for Levelling Up, Housing and Communities (DLUHC) privacy notice?", - "hint_text": "You must show the privacy notice to the tenant or buyer before you can use this service.", - "type": "radio", - "answer_options": { - "0": "Yes", - "1": "No" - } - } - } - }, - "gdpr_declined": { - "hide_subsection_label": true, - "header": "You cannot use this service", - "hint_text": "", - "description": "We cannot accept data about a tenant or buyer unless they’ve seen the DLUHC privacy notice.

Go to your logs", - "questions": {}, - "depends_on": [{ "gdpr_acceptance": "No" }] - }, "organisation_details": { "header": "Organisation details", "description": "", @@ -57,8 +33,7 @@ "1": "B" } } - }, - "depends_on": [{ "gdpr_acceptance": "Yes" }] + } }, "renewal": { "header": "", @@ -74,10 +49,7 @@ "0": "No" } } - }, - "depends_on": [{ - "gdpr_acceptance": "Yes" - }] + } }, "startdate": { "header": "", @@ -89,10 +61,7 @@ "hint_text": "For example, 27 3 2021.", "type": "date" } - }, - "depends_on": [{ - "gdpr_acceptance": "Yes" - }] + } }, "about_this_letting": { "header": "Tell us about this letting", @@ -132,10 +101,7 @@ "0": "Supported housing" } } - }, - "depends_on": [{ - "gdpr_acceptance": "Yes" - }] + } }, "tenant_code": { "header": "", @@ -148,10 +114,7 @@ "type": "text", "width": 10 } - }, - "depends_on": [{ - "gdpr_acceptance": "Yes" - }] + } }, "property_reference": { "header": "", @@ -164,8 +127,7 @@ "type": "text", "width": 10 } - }, - "depends_on": [{ "gdpr_acceptance": "Yes" }] + } } } } @@ -3226,9 +3188,12 @@ "questions": { "declaration": { "check_answer_label": "", - "header": "What is the tenant code?", + "header": "Submit your lettings log ", "hint_text": "", - "type": "text" + "type": "checkbox", + "answer_options": { + "declaration": "The tenant has seen the Department for Levelling Up, Housing & Communities (DLUHC) privacy notice" + } } } } diff --git a/config/locales/en.yml b/config/locales/en.yml index 17e5ce3aa..c40da4bda 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -115,6 +115,9 @@ en: referral: rsnvac_non_temp: "Answer cannot be this source of referral as you already told us this is a re-let to tenant who occupied the same property as temporary accommodation" + declaration: + missing: "You must show the DLUHC privacy notice to the tenant before you can submit this log." + soft_validations: net_income: hint_text: "This is based on the tenant’s work situation: %{ecstat1}" diff --git a/db/migrate/20220207091117_add_declaration.rb b/db/migrate/20220207091117_add_declaration.rb new file mode 100644 index 000000000..f9b6be7e3 --- /dev/null +++ b/db/migrate/20220207091117_add_declaration.rb @@ -0,0 +1,7 @@ +class AddDeclaration < ActiveRecord::Migration[7.0] + def change + change_table :case_logs, bulk: true do |t| + t.column :declaration, :integer + end + end +end diff --git a/db/migrate/20220208101235_remove_gdpr_fields.rb b/db/migrate/20220208101235_remove_gdpr_fields.rb new file mode 100644 index 000000000..7272c483d --- /dev/null +++ b/db/migrate/20220208101235_remove_gdpr_fields.rb @@ -0,0 +1,15 @@ +class RemoveGdprFields < ActiveRecord::Migration[7.0] + def up + change_table :case_logs, bulk: true do |t| + t.remove :gdpr_declined + t.remove :gdpr_acceptance + end + end + + def down + change_table :case_logs, bulk: true do |t| + t.column :gdpr_declined, :string + t.column :gdpr_acceptance, :string + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 29e45f85d..9646f5029 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,8 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2022_02_07_1123100) do +ActiveRecord::Schema.define(version: 202202071123100) do + # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -129,8 +130,6 @@ ActiveRecord::Schema.define(version: 2022_02_07_1123100) do t.datetime "discarded_at" t.string "tenancyother" t.integer "override_net_income_validation" - t.string "gdpr_acceptance" - t.string "gdpr_declined" t.string "property_owner_organisation" t.string "property_manager_organisation" t.string "sale_or_letting" @@ -192,6 +191,7 @@ ActiveRecord::Schema.define(version: 2022_02_07_1123100) do t.decimal "tcharge", precision: 10, scale: 2 t.decimal "tshortfall", precision: 10, scale: 2 t.decimal "chcharge", precision: 10, scale: 2 + t.integer "declaration" t.index ["discarded_at"], name: "index_case_logs_on_discarded_at" t.index ["managing_organisation_id"], name: "index_case_logs_on_managing_organisation_id" t.index ["owning_organisation_id"], name: "index_case_logs_on_owning_organisation_id" diff --git a/spec/factories/case_log.rb b/spec/factories/case_log.rb index 0a73e7750..b3c952ded 100644 --- a/spec/factories/case_log.rb +++ b/spec/factories/case_log.rb @@ -3,7 +3,6 @@ FactoryBot.define do owning_organisation { FactoryBot.create(:organisation) } managing_organisation { FactoryBot.create(:organisation) } trait :about_completed do - gdpr_acceptance { "Yes" } renewal { "No" } needstype { 1 } rent_type { 1 } @@ -112,8 +111,6 @@ FactoryBot.define do tenancyother { nil } override_net_income_validation { nil } net_income_known { "Weekly" } - gdpr_acceptance { "Yes" } - gdpr_declined { "No" } property_owner_organisation { "Test" } property_manager_organisation { "Test" } renewal { 1 } @@ -152,6 +149,7 @@ FactoryBot.define do chcharge { 7 } letting_in_sheltered_accomodation { "No" } la_known { "Yes" } + declaration { "Yes" } end created_at { Time.zone.now } updated_at { Time.zone.now } diff --git a/spec/features/form/check_answers_page_spec.rb b/spec/features/form/check_answers_page_spec.rb index c51ddfa48..cadc0ec3e 100644 --- a/spec/features/form/check_answers_page_spec.rb +++ b/spec/features/form/check_answers_page_spec.rb @@ -209,7 +209,7 @@ RSpec.describe "Form Check Answers Page" do end it "they can click a button to cycle around to the next incomplete section" do - visit("/logs/#{cycle_sections_case_log.id}/setup/check-answers") + visit("/logs/#{cycle_sections_case_log.id}/declaration/check-answers") click_link("Save and go to next incomplete section") expect(page).to have_current_path("/logs/#{cycle_sections_case_log.id}/tenant-code") end diff --git a/spec/features/form/tasklist_page_spec.rb b/spec/features/form/tasklist_page_spec.rb index 12a6afd6f..ba731e05e 100644 --- a/spec/features/form/tasklist_page_spec.rb +++ b/spec/features/form/tasklist_page_spec.rb @@ -33,12 +33,12 @@ RSpec.describe "Task List" do it "shows the number of completed sections if no sections are completed" do visit("/logs/#{empty_case_log.id}") - expect(page).to have_content("You have completed 0 of 10 sections.") + expect(page).to have_content("You have completed 0 of 9 sections.") end it "shows the number of completed sections if one section is completed" do answer_all_questions_in_income_subsection(empty_case_log) visit("/logs/#{empty_case_log.id}") - expect(page).to have_content("You have completed 1 of 10 sections.") + expect(page).to have_content("You have completed 1 of 9 sections.") end end diff --git a/spec/features/form/validations_spec.rb b/spec/features/form/validations_spec.rb index 246f6af30..c11d238f3 100644 --- a/spec/features/form/validations_spec.rb +++ b/spec/features/form/validations_spec.rb @@ -25,6 +25,16 @@ RSpec.describe "validations" do managing_organisation: user.organisation, ) end + let(:completed_without_declaration) do + FactoryBot.create( + :case_log, + :completed, + owning_organisation: user.organisation, + managing_organisation: user.organisation, + status: 1, + declaration: nil, + ) + end let(:id) { case_log.id } describe "Question validation" do @@ -162,4 +172,25 @@ RSpec.describe "validations" do end end end + + describe "Submission validation" do + context "when tenant has not seen the privacy notice" do + it "shows a warning" do + visit("/logs/#{completed_without_declaration.id}/declaration") + expect(page).to have_current_path("/logs/#{completed_without_declaration.id}/declaration") + click_button("Submit lettings log") + expect(page).to have_content("You must show the DLUHC privacy notice to the tenant") + end + end + + context "when tenant has seen the privacy notice" do + it "lets submit the log" do + completed_without_declaration.update!({ declaration: "Yes" }) + visit("/logs/#{completed_without_declaration.id}/declaration") + expect(page).to have_current_path("/logs/#{completed_without_declaration.id}/declaration") + click_button("Submit lettings log") + expect(page).to have_current_path("/logs") + end + end + end end diff --git a/spec/fixtures/complete_case_log.json b/spec/fixtures/complete_case_log.json index 577523dc9..d3b9396bf 100644 --- a/spec/fixtures/complete_case_log.json +++ b/spec/fixtures/complete_case_log.json @@ -122,8 +122,6 @@ "rp_dontknow": "No", "discarded_at": "05/05/2020", "override_net_income_validation": "", - "gdpr_acceptance": "", - "gdpr_declined": "", "property_owner_organisation": "", "property_manager_organisation": "", "rent_type": "Social Rent", @@ -147,6 +145,7 @@ "household_charge": "Yes", "is_carehome": "Yes", "chcharge": "6", - "letting_in_sheltered_accomodation": "No" + "letting_in_sheltered_accomodation": "No", + "declaration": "Yes" } } diff --git a/spec/fixtures/forms/2021_2022.json b/spec/fixtures/forms/2021_2022.json index 3a322369e..12fb93b95 100644 --- a/spec/fixtures/forms/2021_2022.json +++ b/spec/fixtures/forms/2021_2022.json @@ -626,32 +626,6 @@ } } }, - "setup": { - "label": "Before you start", - "subsections": { - "setup": { - "label": "Set up your lettings log", - "pages": { - "gdpr_acceptance": { - "header": "", - "description": "", - "questions": { - "gdpr_acceptance": { - "check_answer_label": "Privacy notice seen", - "header": "Has the tenant or buyer seen the Department for Levelling Up, Housing and Communities (DLUHC) privacy notice?", - "hint_text": "You must show the privacy notice to the tenant or buyer before you can use this service.", - "type": "radio", - "answer_options": { - "0": "Yes", - "1": "No" - } - } - } - } - } - } - } - }, "submission": { "label": "Submission", "subsections": { @@ -661,19 +635,18 @@ "household_characteristics": "completed", "household_needs": "completed", "tenancy_information": "completed", - "property_information": "completed", - "income_and_benefits": "completed", - "rent_and_charges": "completed", - "local_authority": "completed" + "property_information": "completed" }], "pages": { "declaration": { "questions": { "declaration": { "check_answer_label": "", - "header": "What is the tenant code?", - "type": "text", - "width": 10 + "header": "Submit your lettings log ", + "type": "checkbox", + "answer_options": { + "declaration": "The tenant has seen the Department for Levelling Up, Housing & Communities (DLUHC) privacy notice" + } } } } diff --git a/spec/fixtures/forms/2022_2023.json b/spec/fixtures/forms/2022_2023.json index 68d1c1247..b599e3067 100644 --- a/spec/fixtures/forms/2022_2023.json +++ b/spec/fixtures/forms/2022_2023.json @@ -7,30 +7,6 @@ "setup": { "label": "Set up your lettings log", "pages": { - "gdpr_acceptance": { - "header": "", - "description": "", - "questions": { - "gdpr_acceptance": { - "check_answer_label": "Privacy notice seen", - "header": "Has the tenant or buyer seen the Department for Levelling Up, Housing and Communities (DLUHC) privacy notice?", - "hint_text": "You must show the privacy notice to the tenant or buyer before you can use this service.", - "type": "radio", - "answer_options": { - "0": "Yes", - "1": "No" - } - } - } - }, - "gdpr_declined": { - "header": "You cannot use this service", - "hint_text": "", - "description": "We cannot accept data about a tenant or buyer unless they’ve seen the DLUHC privacy notice.", - "questions": { - }, - "depends_on": [{ "gdpr_acceptance": "No" }] - }, "renewal": { "header": "", "description": "", @@ -45,10 +21,7 @@ "0": "No" } } - }, - "depends_on": [{ - "gdpr_acceptance": "Yes" - }] + } }, "startdate": { "header": "", @@ -60,10 +33,7 @@ "hint_text": "For example, 27 3 2007", "type": "date" } - }, - "depends_on": [{ - "gdpr_acceptance": "Yes" - }] + } }, "about_this_letting": { "header": "Tell us about this letting", @@ -101,10 +71,7 @@ "1": "General needs" } } - }, - "depends_on": [{ - "gdpr_acceptance": "Yes" - }] + } } } } diff --git a/spec/helpers/tasklist_helper_spec.rb b/spec/helpers/tasklist_helper_spec.rb index 578edf7c6..18b436a8a 100644 --- a/spec/helpers/tasklist_helper_spec.rb +++ b/spec/helpers/tasklist_helper_spec.rb @@ -22,7 +22,7 @@ RSpec.describe TasklistHelper do describe "get sections count" do it "returns the total of sections if no status is given" do - expect(get_subsections_count(empty_case_log)).to eq(10) + expect(get_subsections_count(empty_case_log)).to eq(9) end it "returns 0 sections for completed sections if no sections are completed" do @@ -30,7 +30,7 @@ RSpec.describe TasklistHelper do end it "returns the number of not started sections" do - expect(get_subsections_count(empty_case_log, :not_started)).to eq(9) + expect(get_subsections_count(empty_case_log, :not_started)).to eq(8) end it "returns the number of sections in progress" do diff --git a/spec/models/form/question_spec.rb b/spec/models/form/question_spec.rb index 2d166b73b..5d747311c 100644 --- a/spec/models/form/question_spec.rb +++ b/spec/models/form/question_spec.rb @@ -169,17 +169,5 @@ RSpec.describe Form::Question, type: :model do expect(question.completed?(case_log)).to be(true) end end - - context "when the gdpr acceptance is No" do - let(:section_id) { "setup" } - let(:subsection_id) { "setup" } - let(:page_id) { "gdpr_acceptance" } - let(:question_id) { "gdpr_acceptance" } - - it "returns false" do - case_log["gdpr_acceptance"] = "No" - expect(question.completed?(case_log)).to be(false) - end - end end end diff --git a/spec/models/form/subsection_spec.rb b/spec/models/form/subsection_spec.rb index f48c3ef4d..6bb2a36e2 100644 --- a/spec/models/form/subsection_spec.rb +++ b/spec/models/form/subsection_spec.rb @@ -77,16 +77,6 @@ RSpec.describe Form::Subsection, type: :model do end end - context "when the privacy notice has not been shown" do - let(:section_id) { "setup" } - let(:subsection_id) { "setup" } - let(:case_log) { FactoryBot.build(:case_log, :about_completed, gdpr_acceptance: "No") } - - it "does not mark the section as completed" do - expect(sub_section.status(case_log)).to eq(:in_progress) - end - end - context "with a completed case log" do let(:case_log) { FactoryBot.build(:case_log, :completed) } diff --git a/spec/models/form_handler_spec.rb b/spec/models/form_handler_spec.rb index 94a6e9f9f..932ebdc75 100644 --- a/spec/models/form_handler_spec.rb +++ b/spec/models/form_handler_spec.rb @@ -17,7 +17,7 @@ RSpec.describe FormHandler do form_handler = described_class.instance form = form_handler.get_form(test_form_name) expect(form).to be_a(Form) - expect(form.pages.count).to eq(29) + expect(form.pages.count).to eq(28) end end diff --git a/spec/models/form_spec.rb b/spec/models/form_spec.rb index 4ab1b6ada..93bca44c3 100644 --- a/spec/models/form_spec.rb +++ b/spec/models/form_spec.rb @@ -36,7 +36,7 @@ RSpec.describe Form, type: :model do describe "next_incomplete_section_redirect_path" do let(:case_log) { FactoryBot.build(:case_log, :in_progress) } let(:subsection) { form.get_subsection("household_characteristics") } - let(:later_subsection) { form.get_subsection("setup") } + let(:later_subsection) { form.get_subsection("declaration") } context "when a user is on the check answers page for a subsection" do def answer_household_needs(case_log) @@ -85,10 +85,6 @@ RSpec.describe Form, type: :model do case_log.mrcdate = Time.zone.parse("03/11/2019") end - def answer_local_gdpr_acceptance(case_log) - case_log.gdpr_acceptance = "Yes" - end - before do case_log.tenant_code = "123" case_log.age1 = 35 @@ -111,15 +107,14 @@ RSpec.describe Form, type: :model do expect(form.next_incomplete_section_redirect_path(subsection, case_log)).to eq("tenancy-code") end - it "returns the next incomplete section by cycling back around if next subsections are completed" do - answer_local_gdpr_acceptance(case_log) - expect(form.next_incomplete_section_redirect_path(later_subsection, case_log)).to eq("armed-forces") - end - it "returns the declaration section for a completed case log" do expect(form.next_incomplete_section_redirect_path(subsection, completed_case_log)).to eq("declaration") end + it "returns the next incomplete section by cycling back around if next subsections are completed" do + expect(form.next_incomplete_section_redirect_path(later_subsection, case_log)).to eq("armed-forces") + end + it "returns the declaration section if all sections are complete but the case log is in progress" do answer_household_needs(case_log) answer_tenancy_information(case_log) @@ -128,7 +123,6 @@ RSpec.describe Form, type: :model do answer_income_and_benefits(case_log) answer_rent_and_charges(case_log) answer_local_authority(case_log) - answer_local_gdpr_acceptance(case_log) expect(form.next_incomplete_section_redirect_path(subsection, case_log)).to eq("declaration") end diff --git a/spec/requests/case_logs_controller_spec.rb b/spec/requests/case_logs_controller_spec.rb index 9fef47719..09f14089e 100644 --- a/spec/requests/case_logs_controller_spec.rb +++ b/spec/requests/case_logs_controller_spec.rb @@ -200,7 +200,7 @@ RSpec.describe CaseLogsController, type: :request do end it "displays a section status for a case log" do - assert_select ".govuk-tag", text: /Not started/, count: 9 + assert_select ".govuk-tag", text: /Not started/, count: 8 assert_select ".govuk-tag", text: /Completed/, count: 0 assert_select ".govuk-tag", text: /Cannot start yet/, count: 1 end @@ -222,7 +222,7 @@ RSpec.describe CaseLogsController, type: :request do end it "displays a section status for a case log" do - assert_select ".govuk-tag", text: /Not started/, count: 8 + assert_select ".govuk-tag", text: /Not started/, count: 7 assert_select ".govuk-tag", text: /Completed/, count: 1 assert_select ".govuk-tag", text: /Cannot start yet/, count: 1 end 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 03/11] 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 From 7f4312860ce8d5834690e2daf3fdd78be25d740a Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Tue, 8 Feb 2022 14:42:07 +0000 Subject: [PATCH 04/11] Implement exclusive checkboxes (#281) * Implement exclusive checkboxes * Fix validation --- .../validations/household_validations.rb | 2 +- app/views/form/_checkbox_question.html.erb | 5 ++- spec/features/form/checkboxes_spec.rb | 41 +++++++++++++++++++ 3 files changed, 46 insertions(+), 2 deletions(-) create mode 100644 spec/features/form/checkboxes_spec.rb diff --git a/app/models/validations/household_validations.rb b/app/models/validations/household_validations.rb index 710332d8c..2487c9ca1 100644 --- a/app/models/validations/household_validations.rb +++ b/app/models/validations/household_validations.rb @@ -65,7 +65,7 @@ module Validations::HouseholdValidations if all_options.count("Yes") > 1 mobility_accessibility_options = [record.housingneeds_a, record.housingneeds_b, record.housingneeds_c] unless all_options.count("Yes") == 2 && record.housingneeds_f == "Yes" && mobility_accessibility_options.any? { |x| x == "Yes" } - record.errors.add :housingneeds_a, I18n.t("validations.household.housingneeds_a.one_or_two_choices") + record.errors.add :accessibility_requirements, I18n.t("validations.household.housingneeds_a.one_or_two_choices") end end end diff --git a/app/views/form/_checkbox_question.html.erb b/app/views/form/_checkbox_question.html.erb index 4ac23267b..444b0b58f 100644 --- a/app/views/form/_checkbox_question.html.erb +++ b/app/views/form/_checkbox_question.html.erb @@ -4,14 +4,17 @@ caption: caption(caption_text, page_header, conditional), legend: legend(question, page_header, conditional), hint: { text: question.hint_text&.html_safe } do %> + <% after_divider = false %> <% question.answer_options.map do |key, val| %> <% if key.starts_with?("divider") %> - <%= f.govuk_check_box_divider %> + <% after_divider = true %> + <%= f.govuk_check_box_divider %> <% else %> <%= f.govuk_check_box question.id, key, label: { text: val }, checked: @case_log[key] == "Yes", + exclusive: after_divider, **stimulus_html_attributes(question) %> <% end %> diff --git a/spec/features/form/checkboxes_spec.rb b/spec/features/form/checkboxes_spec.rb new file mode 100644 index 000000000..60ce22f14 --- /dev/null +++ b/spec/features/form/checkboxes_spec.rb @@ -0,0 +1,41 @@ +require "rails_helper" +require_relative "helpers" +require_relative "../../request_helper" + +RSpec.describe "Checkboxes" do + include Helpers + let(:user) { FactoryBot.create(:user) } + let(:case_log) do + FactoryBot.create( + :case_log, + :in_progress, + owning_organisation: user.organisation, + managing_organisation: user.organisation, + ) + end + let(:id) { case_log.id } + + before do + RequestHelper.stub_http_requests + sign_in user + end + + context "when exclusive checkbox is selected", js: true do + it "deselects all other checkboxes" do + visit("/logs/#{id}/accessibility-requirements") + page.check("case-log-accessibility-requirements-housingneeds-a-field", allow_label_click: true) + click_button("Save and continue") + + case_log.reload + expect(case_log["housingneeds_a"]).to eq("Yes") + + visit("/logs/#{id}/accessibility-requirements") + page.check("case-log-accessibility-requirements-housingneeds-h-field", allow_label_click: true) + click_button("Save and continue") + + case_log.reload + expect(case_log["housingneeds_a"]).to eq("No") + expect(case_log["housingneeds_h"]).to eq("Yes") + end + end +end From a3a7b9a713dbebcc61c93c10554196baec456b5f Mon Sep 17 00:00:00 2001 From: baarkerlounger <5101747+baarkerlounger@users.noreply.github.com> Date: Tue, 8 Feb 2022 16:19:37 +0000 Subject: [PATCH 05/11] Arbre now supports Rails 7 (#283) --- Gemfile | 1 - Gemfile.lock | 14 ++++---------- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/Gemfile b/Gemfile index b96ea93e1..019e0245a 100644 --- a/Gemfile +++ b/Gemfile @@ -27,7 +27,6 @@ gem "hotwire-rails" gem "discard" # Administration framework gem "activeadmin", git: "https://github.com/tagliala/activeadmin.git", branch: "feature/railties-7" -gem "arbre", github: "activeadmin/arbre" # Admin charts gem "chartkick" # Spreadsheet parsing diff --git a/Gemfile.lock b/Gemfile.lock index 06bea744e..78d641b68 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,11 +1,3 @@ -GIT - remote: https://github.com/activeadmin/arbre.git - revision: dcea09da31322f2bc9bdfc32eb14ec0aaf34ae92 - specs: - arbre (1.4.0) - activesupport (>= 3.0.0, < 7.1) - ruby2_keywords (>= 0.0.2, < 1.0) - GIT remote: https://github.com/baarkerlounger/devise.git revision: 6d0b6b52a9d0e87ae6d9f9acb562169751623078 @@ -31,7 +23,7 @@ GIT GIT remote: https://github.com/tagliala/activeadmin.git - revision: 5d6dfbe086331ce0538cc726abfe2eebe0260f02 + revision: 1515a40a1d407dafd34b104fabd8401e84baca94 branch: feature/railties-7 specs: activeadmin (2.9.0) @@ -114,6 +106,9 @@ GEM tzinfo (~> 2.0) addressable (2.8.0) public_suffix (>= 2.0.2, < 5.0) + arbre (1.5.0) + activesupport (>= 3.0.0, < 7.1) + ruby2_keywords (>= 0.0.2, < 1.0) ast (2.4.2) aws-eventstream (1.2.0) aws-partitions (1.553.0) @@ -454,7 +449,6 @@ PLATFORMS DEPENDENCIES activeadmin! - arbre! aws-sdk-s3 bootsnap (>= 1.4.4) byebug From 4fe1731705fd48d39917034e755bb98a0010cb1d Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Tue, 8 Feb 2022 16:38:29 +0000 Subject: [PATCH 06/11] CLDC-966-optional-setup-fields (#282) * Add tenant_code and propcode as optional fields and consider subsection completed it they are not filled in' --- app/models/case_log.rb | 1 - app/models/constants/case_log.rb | 2 ++ app/models/form/subsection.rb | 4 +++- spec/features/form/check_answers_page_spec.rb | 4 ++-- spec/fixtures/forms/2021_2022.json | 9 ++++++++ spec/helpers/check_answers_helper_spec.rb | 3 ++- spec/models/form/subsection_spec.rb | 22 ++++++++++++++----- spec/models/form_handler_spec.rb | 2 +- 8 files changed, 36 insertions(+), 11 deletions(-) diff --git a/app/models/case_log.rb b/app/models/case_log.rb index f3984e247..0261f7d6a 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -144,7 +144,6 @@ class CaseLog < ApplicationRecord enum declaration: POLAR, _suffix: true AUTOGENERATED_FIELDS = %w[id status created_at updated_at discarded_at].freeze - OPTIONAL_FIELDS = %w[postcode_known la_known first_time_property_let_as_social_housing].freeze def form FormHandler.instance.get_form(form_name) diff --git a/app/models/constants/case_log.rb b/app/models/constants/case_log.rb index 73cfc4a51..3a2593314 100644 --- a/app/models/constants/case_log.rb +++ b/app/models/constants/case_log.rb @@ -1123,4 +1123,6 @@ module Constants::CaseLog "Sheltered accomodation", "Home Office Asylum Support", "Other"].freeze + + OPTIONAL_FIELDS = %w[postcode_known la_known first_time_property_let_as_social_housing tenant_code propcode].freeze end diff --git a/app/models/form/subsection.rb b/app/models/form/subsection.rb index e3166a4e1..c0c330bec 100644 --- a/app/models/form/subsection.rb +++ b/app/models/form/subsection.rb @@ -1,4 +1,6 @@ class Form::Subsection + include Constants::CaseLog + attr_accessor :id, :label, :section, :pages, :depends_on, :form def initialize(id, hsh, section) @@ -30,7 +32,7 @@ class Form::Subsection return :cannot_start_yet end - qs = applicable_questions(case_log) + qs = applicable_questions(case_log).reject { |q| OPTIONAL_FIELDS.include?(q.id) } return :not_started if qs.all? { |question| case_log[question.id].blank? || question.read_only? } return :completed if qs.all? { |question| question.completed?(case_log) } diff --git a/spec/features/form/check_answers_page_spec.rb b/spec/features/form/check_answers_page_spec.rb index cadc0ec3e..e2250684f 100644 --- a/spec/features/form/check_answers_page_spec.rb +++ b/spec/features/form/check_answers_page_spec.rb @@ -38,7 +38,7 @@ RSpec.describe "Form Check Answers Page" do end context "when the user needs to check their answers for a subsection" do - let(:last_question_for_subsection) { "household-number-of-other-members" } + let(:last_question_for_subsection) { "propcode" } it "can be visited by URL" do visit("/logs/#{id}/#{subsection}/check-answers") @@ -46,7 +46,7 @@ RSpec.describe "Form Check Answers Page" do end it "redirects to the check answers page when answering the last question and clicking save and continue" do - fill_in_number_question(id, "other_hhmemb", 0, last_question_for_subsection) + fill_in_number_question(id, "propcode", 0, last_question_for_subsection) expect(page).to have_current_path("/logs/#{id}/#{subsection}/check-answers") end diff --git a/spec/fixtures/forms/2021_2022.json b/spec/fixtures/forms/2021_2022.json index e675ccea5..1498880fc 100644 --- a/spec/fixtures/forms/2021_2022.json +++ b/spec/fixtures/forms/2021_2022.json @@ -104,6 +104,15 @@ } } } + }, + "propcode": { + "questions": { + "propcode": { + "check_answer_label": "", + "header": "property reference?", + "type": "text" + } + } } } }, diff --git a/spec/helpers/check_answers_helper_spec.rb b/spec/helpers/check_answers_helper_spec.rb index 7778ed0d9..0d1a94d6d 100644 --- a/spec/helpers/check_answers_helper_spec.rb +++ b/spec/helpers/check_answers_helper_spec.rb @@ -9,7 +9,7 @@ RSpec.describe CheckAnswersHelper do context "when a section hasn't been completed yet" do it "returns that you have unanswered questions" do expect(display_answered_questions_summary(subsection, case_log)) - .to match(/You have answered 2 of 4 questions./) + .to match(/You have answered 2 of 5 questions./) end end @@ -17,6 +17,7 @@ RSpec.describe CheckAnswersHelper do it "returns that you have answered all the questions" do case_log.sex1 = "F" case_log.other_hhmemb = 0 + case_log.propcode = "123" expect(display_answered_questions_summary(subsection, case_log)) .to match(/You answered all the questions./) expect(display_answered_questions_summary(subsection, case_log)) diff --git a/spec/models/form/subsection_spec.rb b/spec/models/form/subsection_spec.rb index 6bb2a36e2..d086356f4 100644 --- a/spec/models/form/subsection_spec.rb +++ b/spec/models/form/subsection_spec.rb @@ -1,4 +1,5 @@ require "rails_helper" +require_relative "../../request_helper" RSpec.describe Form::Subsection, type: :model do subject(:sub_section) { described_class.new(subsection_id, subsection_definition, section) } @@ -11,6 +12,10 @@ RSpec.describe Form::Subsection, type: :model do let(:subsection_id) { "household_characteristics" } let(:subsection_definition) { section_definition["subsections"][subsection_id] } + before do + RequestHelper.stub_http_requests + end + it "has an id" do expect(sub_section.id).to eq(subsection_id) end @@ -20,12 +25,12 @@ RSpec.describe Form::Subsection, type: :model do end it "has pages" do - expected_pages = %w[tenant_code person_1_age person_1_gender household_number_of_other_members] + expected_pages = %w[tenant_code person_1_age person_1_gender household_number_of_other_members propcode] expect(sub_section.pages.map(&:id)).to eq(expected_pages) end it "has questions" do - expected_questions = %w[tenant_code age1 sex1 other_hhmemb relat2 age2 sex2 ecstat2] + expected_questions = %w[tenant_code age1 sex1 other_hhmemb relat2 age2 sex2 ecstat2 propcode] expect(sub_section.questions.map(&:id)).to eq(expected_questions) end @@ -53,9 +58,9 @@ RSpec.describe Form::Subsection, type: :model do end it "has question helpers for the number of applicable questions" do - expected_questions = %w[tenant_code age1 sex1 other_hhmemb] + expected_questions = %w[tenant_code age1 sex1 other_hhmemb propcode] expect(sub_section.applicable_questions(case_log).map(&:id)).to eq(expected_questions) - expect(sub_section.applicable_questions_count(case_log)).to eq(4) + expect(sub_section.applicable_questions_count(case_log)).to eq(5) end it "has question helpers for the number of answered questions" do @@ -72,18 +77,25 @@ RSpec.describe Form::Subsection, type: :model do end it "has a question helpers for the unanswered questions" do - expected_questions = %w[sex1 other_hhmemb] + expected_questions = %w[sex1 other_hhmemb propcode] expect(sub_section.unanswered_questions(case_log).map(&:id)).to eq(expected_questions) end end context "with a completed case log" do let(:case_log) { FactoryBot.build(:case_log, :completed) } + let(:case_log_too) { FactoryBot.build(:case_log, :in_progress) } it "has a status" do expect(sub_section.status(case_log)).to eq(:completed) end + it "has a status when optional fields are not filled" do + case_log.update!({ propcode: nil }) + case_log.reload + expect(sub_section.status(case_log)).to eq(:completed) + end + it "has status helpers" do expect(sub_section.is_incomplete?(case_log)).to be(false) expect(sub_section.is_started?(case_log)).to be(true) diff --git a/spec/models/form_handler_spec.rb b/spec/models/form_handler_spec.rb index 932ebdc75..94a6e9f9f 100644 --- a/spec/models/form_handler_spec.rb +++ b/spec/models/form_handler_spec.rb @@ -17,7 +17,7 @@ RSpec.describe FormHandler do form_handler = described_class.instance form = form_handler.get_form(test_form_name) expect(form).to be_a(Form) - expect(form.pages.count).to eq(28) + expect(form.pages.count).to eq(29) end end From dcc27cd492628f2081fef5edf19522e6a3016ea4 Mon Sep 17 00:00:00 2001 From: baarkerlounger Date: Wed, 9 Feb 2022 09:51:44 +0000 Subject: [PATCH 07/11] Bump rails --- Gemfile.lock | 108 +++++++++++++++++++++++++-------------------------- 1 file changed, 54 insertions(+), 54 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 78d641b68..ae61f693c 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -12,7 +12,7 @@ GIT GIT remote: https://github.com/baarkerlounger/two_factor_authentication.git - revision: 1fa214d18d311e019a343f836f2c591c0fa3d308 + revision: c2237dedb89b1fc53101cec536e57912049c5412 specs: two_factor_authentication (2.2.0) devise @@ -39,67 +39,67 @@ GIT GEM remote: https://rubygems.org/ specs: - actioncable (7.0.1) - actionpack (= 7.0.1) - activesupport (= 7.0.1) + actioncable (7.0.2) + actionpack (= 7.0.2) + activesupport (= 7.0.2) nio4r (~> 2.0) websocket-driver (>= 0.6.1) - actionmailbox (7.0.1) - actionpack (= 7.0.1) - activejob (= 7.0.1) - activerecord (= 7.0.1) - activestorage (= 7.0.1) - activesupport (= 7.0.1) + actionmailbox (7.0.2) + actionpack (= 7.0.2) + activejob (= 7.0.2) + activerecord (= 7.0.2) + activestorage (= 7.0.2) + activesupport (= 7.0.2) mail (>= 2.7.1) net-imap net-pop net-smtp - actionmailer (7.0.1) - actionpack (= 7.0.1) - actionview (= 7.0.1) - activejob (= 7.0.1) - activesupport (= 7.0.1) + actionmailer (7.0.2) + actionpack (= 7.0.2) + actionview (= 7.0.2) + activejob (= 7.0.2) + activesupport (= 7.0.2) mail (~> 2.5, >= 2.5.4) net-imap net-pop net-smtp rails-dom-testing (~> 2.0) - actionpack (7.0.1) - actionview (= 7.0.1) - activesupport (= 7.0.1) + actionpack (7.0.2) + actionview (= 7.0.2) + activesupport (= 7.0.2) rack (~> 2.0, >= 2.2.0) rack-test (>= 0.6.3) rails-dom-testing (~> 2.0) rails-html-sanitizer (~> 1.0, >= 1.2.0) - actiontext (7.0.1) - actionpack (= 7.0.1) - activerecord (= 7.0.1) - activestorage (= 7.0.1) - activesupport (= 7.0.1) + actiontext (7.0.2) + actionpack (= 7.0.2) + activerecord (= 7.0.2) + activestorage (= 7.0.2) + activesupport (= 7.0.2) globalid (>= 0.6.0) nokogiri (>= 1.8.5) - actionview (7.0.1) - activesupport (= 7.0.1) + actionview (7.0.2) + activesupport (= 7.0.2) builder (~> 3.1) erubi (~> 1.4) rails-dom-testing (~> 2.0) rails-html-sanitizer (~> 1.1, >= 1.2.0) - activejob (7.0.1) - activesupport (= 7.0.1) + activejob (7.0.2) + activesupport (= 7.0.2) globalid (>= 0.3.6) - activemodel (7.0.1) - activesupport (= 7.0.1) - activerecord (7.0.1) - activemodel (= 7.0.1) - activesupport (= 7.0.1) - activestorage (7.0.1) - actionpack (= 7.0.1) - activejob (= 7.0.1) - activerecord (= 7.0.1) - activesupport (= 7.0.1) + activemodel (7.0.2) + activesupport (= 7.0.2) + activerecord (7.0.2) + activemodel (= 7.0.2) + activesupport (= 7.0.2) + activestorage (7.0.2) + actionpack (= 7.0.2) + activejob (= 7.0.2) + activerecord (= 7.0.2) + activesupport (= 7.0.2) marcel (~> 1.0) mini_mime (>= 1.1.0) - activesupport (7.0.1) + activesupport (7.0.2) concurrent-ruby (~> 1.0, >= 1.0.2) i18n (>= 1.6, < 2) minitest (>= 5.1) @@ -289,28 +289,28 @@ GEM rack rack-test (1.1.0) rack (>= 1.0, < 3) - rails (7.0.1) - actioncable (= 7.0.1) - actionmailbox (= 7.0.1) - actionmailer (= 7.0.1) - actionpack (= 7.0.1) - actiontext (= 7.0.1) - actionview (= 7.0.1) - activejob (= 7.0.1) - activemodel (= 7.0.1) - activerecord (= 7.0.1) - activestorage (= 7.0.1) - activesupport (= 7.0.1) + rails (7.0.2) + actioncable (= 7.0.2) + actionmailbox (= 7.0.2) + actionmailer (= 7.0.2) + actionpack (= 7.0.2) + actiontext (= 7.0.2) + actionview (= 7.0.2) + activejob (= 7.0.2) + activemodel (= 7.0.2) + activerecord (= 7.0.2) + activestorage (= 7.0.2) + activesupport (= 7.0.2) bundler (>= 1.15.0) - railties (= 7.0.1) + railties (= 7.0.2) rails-dom-testing (2.0.3) activesupport (>= 4.2.0) nokogiri (>= 1.6) rails-html-sanitizer (1.4.2) loofah (~> 2.3) - railties (7.0.1) - actionpack (= 7.0.1) - activesupport (= 7.0.1) + railties (7.0.2) + actionpack (= 7.0.2) + activesupport (= 7.0.2) method_source rake (>= 12.2) thor (~> 1.0) From 8418e794263b274a0eb7ab03fbc92eacfa85381a Mon Sep 17 00:00:00 2001 From: baarkerlounger <5101747+baarkerlounger@users.noreply.github.com> Date: Wed, 9 Feb 2022 10:10:10 +0000 Subject: [PATCH 08/11] CLDC-961: Model data versioning (#277) * Replace Discard with Paper Trail * Track who created or changed records via UI * Track model updates by AdminUsers --- Gemfile | 6 +- Gemfile.lock | 13 +- app/concerns/admin/paper_trail.rb | 15 ++ app/controllers/application_controller.rb | 8 + app/controllers/case_logs_controller.rb | 2 +- app/models/admin_user.rb | 2 + app/models/case_log.rb | 4 +- app/models/organisation.rb | 3 + app/models/user.rb | 2 + config/initializers/active_admin.rb | 4 + db/migrate/20220207151239_create_versions.rb | 22 +++ ...1312_remove_discarded_at_from_case_logs.rb | 11 ++ db/schema.rb | 14 +- .../admin/admin_users_controller_spec.rb | 26 +++- .../admin/case_logs_controller_spec.rb | 48 +++++- .../admin/dashboard_controller_spec.rb | 4 +- .../admin/organisations_controller_spec.rb | 50 +++++- .../admin/users_controller_spec.rb | 24 ++- spec/factories/case_log.rb | 1 - spec/models/admin_user_spec.rb | 61 +++++--- spec/models/case_log_spec.rb | 13 ++ spec/models/organisation_spec.rb | 13 ++ spec/models/user_spec.rb | 13 ++ spec/requests/case_logs_controller_spec.rb | 145 ++++++++++-------- spec/requests/form_controller_spec.rb | 7 + .../requests/organisations_controller_spec.rb | 7 + spec/requests/users_controller_spec.rb | 7 + spec/support/controller_macros.rb | 8 - 28 files changed, 407 insertions(+), 126 deletions(-) create mode 100644 app/concerns/admin/paper_trail.rb create mode 100644 db/migrate/20220207151239_create_versions.rb create mode 100644 db/migrate/20220207151312_remove_discarded_at_from_case_logs.rb diff --git a/Gemfile b/Gemfile index 019e0245a..45832489b 100644 --- a/Gemfile +++ b/Gemfile @@ -23,8 +23,6 @@ gem "govuk_design_system_formbuilder" gem "notifications-ruby-client" # Turbo and Stimulus gem "hotwire-rails" -# Soft delete ActiveRecords objects -gem "discard" # Administration framework gem "activeadmin", git: "https://github.com/tagliala/activeadmin.git", branch: "feature/railties-7" # Admin charts @@ -47,6 +45,10 @@ gem "postcodes_io" gem "view_component" # Use the AWS S3 SDK as storage mechanism gem "aws-sdk-s3" +# Track changes to models for auditing or versioning. +gem "paper_trail" +# Store active record objects in version whodunnits +gem "paper_trail-globalid" group :development, :test do # Call 'byebug' anywhere in the code to stop execution and get a debugger console diff --git a/Gemfile.lock b/Gemfile.lock index ae61f693c..937bb3f80 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -156,8 +156,6 @@ GEM deep_merge (1.2.2) diff-lcs (1.5.0) digest (3.1.0) - discard (1.2.1) - activerecord (>= 4.2, < 8) docile (1.4.0) dotenv (2.7.6) dotenv-rails (2.7.6) @@ -266,6 +264,12 @@ GEM childprocess (>= 0.6.3, < 5) iniparse (~> 1.4) rexml (~> 3.2) + paper_trail (12.2.0) + activerecord (>= 5.2) + request_store (~> 1.1) + paper_trail-globalid (0.2.0) + globalid + paper_trail (>= 3.0.0) parallel (1.21.0) parser (3.1.0.0) ast (~> 2.4.1) @@ -326,6 +330,8 @@ GEM rb-inotify (0.10.1) ffi (~> 1.0) regexp_parser (2.2.0) + request_store (1.5.1) + rack (>= 1.4) responders (3.0.1) actionpack (>= 5.0) railties (>= 5.0) @@ -456,7 +462,6 @@ DEPENDENCIES capybara-lockstep chartkick devise! - discard dotenv-rails factory_bot_rails govuk-components @@ -466,6 +471,8 @@ DEPENDENCIES listen (~> 3.3) notifications-ruby-client overcommit (>= 0.37.0) + paper_trail + paper_trail-globalid pg (~> 1.1) postcodes_io pry-byebug diff --git a/app/concerns/admin/paper_trail.rb b/app/concerns/admin/paper_trail.rb new file mode 100644 index 000000000..5558d465b --- /dev/null +++ b/app/concerns/admin/paper_trail.rb @@ -0,0 +1,15 @@ +module Admin + module PaperTrail + extend ActiveSupport::Concern + + included do + before_action :set_paper_trail_whodunnit + end + + protected + + def user_for_paper_trail + current_admin_user + end + end +end diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 776259fab..78b367d80 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1,4 +1,6 @@ class ApplicationController < ActionController::Base + before_action :set_paper_trail_whodunnit + def render_not_found render "errors/not_found", status: :not_found end @@ -6,4 +8,10 @@ class ApplicationController < ActionController::Base def render_not_found_json(class_name, id) render json: { error: "#{class_name} #{id} not found" }, status: :not_found end + +protected + + def user_for_paper_trail + current_user + end end diff --git a/app/controllers/case_logs_controller.rb b/app/controllers/case_logs_controller.rb index e6a3a3ecd..8d6f75c5c 100644 --- a/app/controllers/case_logs_controller.rb +++ b/app/controllers/case_logs_controller.rb @@ -60,7 +60,7 @@ class CaseLogsController < ApplicationController def destroy if @case_log - if @case_log.discard + if @case_log.delete head :no_content else render json: { errors: @case_log.errors.messages }, status: :unprocessable_entity diff --git a/app/models/admin_user.rb b/app/models/admin_user.rb index 7fc666a7e..351bf834d 100644 --- a/app/models/admin_user.rb +++ b/app/models/admin_user.rb @@ -6,6 +6,8 @@ class AdminUser < ApplicationRecord has_one_time_password(encrypted: true) + has_paper_trail + validates :phone, presence: true, numericality: true MFA_SMS_TEMPLATE_ID = "bf309d93-804e-4f95-b1f4-bd513c48ecb0".freeze diff --git a/app/models/case_log.rb b/app/models/case_log.rb index 0261f7d6a..a4c331b4b 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -30,11 +30,11 @@ private end class CaseLog < ApplicationRecord - include Discard::Model include Validations::SoftValidations include Constants::CaseLog include Constants::IncomeRanges - default_scope -> { kept } + + has_paper_trail validates_with CaseLogValidator before_validation :process_postcode_changes!, if: :property_postcode_changed? diff --git a/app/models/organisation.rb b/app/models/organisation.rb index 6d31f8a94..ff85f55a8 100644 --- a/app/models/organisation.rb +++ b/app/models/organisation.rb @@ -3,7 +3,10 @@ class Organisation < ApplicationRecord has_many :owned_case_logs, class_name: "CaseLog", foreign_key: "owning_organisation_id" has_many :managed_case_logs, class_name: "CaseLog", foreign_key: "managing_organisation_id" + has_paper_trail + include Constants::Organisation + enum provider_type: PROVIDER_TYPE def case_logs diff --git a/app/models/user.rb b/app/models/user.rb index 8e18a55db..217b0a1ac 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -10,6 +10,8 @@ class User < ApplicationRecord has_many :owned_case_logs, through: :organisation has_many :managed_case_logs, through: :organisation + has_paper_trail + enum role: ROLES def case_logs diff --git a/config/initializers/active_admin.rb b/config/initializers/active_admin.rb index 47561059e..e8014c7e2 100644 --- a/config/initializers/active_admin.rb +++ b/config/initializers/active_admin.rb @@ -340,3 +340,7 @@ end Rails.application.config.after_initialize do ActiveAdmin.application.stylesheets.delete("active_admin/print.css") end + +Rails.application.config.after_initialize do + ActiveAdmin::BaseController.include Admin::PaperTrail +end diff --git a/db/migrate/20220207151239_create_versions.rb b/db/migrate/20220207151239_create_versions.rb new file mode 100644 index 000000000..2d69235bb --- /dev/null +++ b/db/migrate/20220207151239_create_versions.rb @@ -0,0 +1,22 @@ +# This migration creates the `versions` table, the only schema PT requires. +# All other migrations PT provides are optional. +class CreateVersions < ActiveRecord::Migration[7.0] + # The largest text column available in all supported RDBMS is + # 1024^3 - 1 bytes, roughly one gibibyte. We specify a size + # so that MySQL will use `longtext` instead of `text`. Otherwise, + # when serializing very large objects, `text` might not be big enough. + TEXT_BYTES = 1_073_741_823 + + def change + create_table :versions do |t| + t.string :item_type, null: false, limit: 191 + t.bigint :item_id, null: false + t.string :event, null: false + t.string :whodunnit + t.text :object, limit: TEXT_BYTES + + t.datetime :created_at + end + add_index :versions, %i[item_type item_id] + end +end diff --git a/db/migrate/20220207151312_remove_discarded_at_from_case_logs.rb b/db/migrate/20220207151312_remove_discarded_at_from_case_logs.rb new file mode 100644 index 000000000..00886f520 --- /dev/null +++ b/db/migrate/20220207151312_remove_discarded_at_from_case_logs.rb @@ -0,0 +1,11 @@ +class RemoveDiscardedAtFromCaseLogs < ActiveRecord::Migration[7.0] + def up + remove_index :case_logs, :discarded_at + remove_column :case_logs, :discarded_at + end + + def down + add_column :case_logs, :discarded_at, :datetime + add_index :case_logs, :discarded_at + end +end diff --git a/db/schema.rb b/db/schema.rb index 9646f5029..891c11299 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -93,6 +93,7 @@ ActiveRecord::Schema.define(version: 202202071123100) do t.integer "beds" t.integer "offered" t.integer "wchair" + t.integer "earnings" t.integer "incfreq" t.integer "benefits" t.integer "period" @@ -127,7 +128,6 @@ ActiveRecord::Schema.define(version: 202202071123100) do t.integer "rp_medwel" t.integer "rp_hardship" t.integer "rp_dontknow" - t.datetime "discarded_at" t.string "tenancyother" t.integer "override_net_income_validation" t.string "property_owner_organisation" @@ -182,7 +182,6 @@ ActiveRecord::Schema.define(version: 202202071123100) do t.integer "is_carehome" t.integer "letting_in_sheltered_accomodation" t.integer "household_charge" - t.integer "earnings" t.integer "referral" t.decimal "brent", precision: 10, scale: 2 t.decimal "scharge", precision: 10, scale: 2 @@ -192,7 +191,6 @@ ActiveRecord::Schema.define(version: 202202071123100) do t.decimal "tshortfall", precision: 10, scale: 2 t.decimal "chcharge", precision: 10, scale: 2 t.integer "declaration" - t.index ["discarded_at"], name: "index_case_logs_on_discarded_at" t.index ["managing_organisation_id"], name: "index_case_logs_on_managing_organisation_id" t.index ["owning_organisation_id"], name: "index_case_logs_on_owning_organisation_id" end @@ -252,4 +250,14 @@ ActiveRecord::Schema.define(version: 202202071123100) do t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true end + create_table "versions", force: :cascade do |t| + t.string "item_type", limit: 191, null: false + t.bigint "item_id", null: false + t.string "event", null: false + t.string "whodunnit" + t.text "object" + t.datetime "created_at", precision: 6 + t.index ["item_type", "item_id"], name: "index_versions_on_item_type_and_item_id" + end + end diff --git a/spec/controllers/admin/admin_users_controller_spec.rb b/spec/controllers/admin/admin_users_controller_spec.rb index e5977d830..871a198c3 100644 --- a/spec/controllers/admin/admin_users_controller_spec.rb +++ b/spec/controllers/admin/admin_users_controller_spec.rb @@ -6,8 +6,11 @@ describe Admin::AdminUsersController, type: :controller do let(:page) { Capybara::Node::Simple.new(response.body) } let(:resource_title) { "Admin Users" } let(:valid_session) { {} } + let(:signed_in_admin_user) { FactoryBot.create(:admin_user) } - login_admin_user + before do + sign_in signed_in_admin_user + end describe "Get admin users" do before do @@ -27,22 +30,30 @@ describe Admin::AdminUsersController, type: :controller do it "creates a new admin user" do expect { post :create, session: valid_session, params: params }.to change(AdminUser, :count).by(1) end + + it "tracks who created the record" do + post :create, session: valid_session, params: params + created_id = response.location.match(/[0-9]+/)[0] + whodunnit_actor = AdminUser.find_by(id: created_id).versions.last.actor + expect(whodunnit_actor).to be_a(AdminUser) + expect(whodunnit_actor.id).to eq(signed_in_admin_user.id) + end end describe "Update admin users" do - context "when editing the form" do + context "when viewing the form" do before do get :edit, session: valid_session, params: { id: AdminUser.first.id } end - it "shows an edit form" do + it "shows the correct fields" do expect(page).to have_field("admin_user_email") expect(page).to have_field("admin_user_password") expect(page).to have_field("admin_user_password_confirmation") end end - context "when updating the form" do + context "when updating an admin user" do let(:admin_user) { FactoryBot.create(:admin_user) } let(:email) { "new_email@example.com" } let(:params) { { id: admin_user.id, admin_user: { email: email } } } @@ -55,6 +66,13 @@ describe Admin::AdminUsersController, type: :controller do admin_user.reload expect(admin_user.email).to eq(email) end + + it "tracks who updated the record" do + admin_user.reload + whodunnit_actor = admin_user.versions.last.actor + expect(whodunnit_actor).to be_a(AdminUser) + expect(whodunnit_actor.id).to eq(signed_in_admin_user.id) + end end end end diff --git a/spec/controllers/admin/case_logs_controller_spec.rb b/spec/controllers/admin/case_logs_controller_spec.rb index 7a27443a1..1ff62e77b 100644 --- a/spec/controllers/admin/case_logs_controller_spec.rb +++ b/spec/controllers/admin/case_logs_controller_spec.rb @@ -5,14 +5,14 @@ require_relative "../../request_helper" describe Admin::CaseLogsController, type: :controller do before do RequestHelper.stub_http_requests + sign_in admin_user end render_views let(:page) { Capybara::Node::Simple.new(response.body) } let(:resource_title) { "Logs" } let(:valid_session) { {} } - - login_admin_user + let(:admin_user) { FactoryBot.create(:admin_user) } describe "Get case logs" do let!(:case_log) { FactoryBot.create(:case_log, :in_progress) } @@ -44,5 +44,49 @@ describe Admin::CaseLogsController, type: :controller do it "creates a new case log" do expect { post :create, session: valid_session, params: params }.to change(CaseLog, :count).by(1) end + + it "tracks who created the record" do + post :create, session: valid_session, params: params + created_id = response.location.match(/[0-9]+/)[0] + whodunnit_actor = CaseLog.find_by(id: created_id).versions.last.actor + expect(whodunnit_actor).to be_a(AdminUser) + expect(whodunnit_actor.id).to eq(admin_user.id) + end + end + + describe "Update case log" do + let!(:case_log) { FactoryBot.create(:case_log, :in_progress) } + + context "when viewing the edit form" do + before do + get :edit, session: valid_session, params: { id: case_log.id } + end + + it "has the correct fields" do + expect(page).to have_field("case_log_age1") + expect(page).to have_field("case_log_tenant_code") + end + end + + context "when updating the case_log" do + let(:tenant_code) { "New tenant code by Admin" } + let(:params) { { id: case_log.id, case_log: { tenant_code: tenant_code } } } + + before do + patch :update, session: valid_session, params: params + end + + it "updates the case log" do + case_log.reload + expect(case_log.tenant_code).to eq(tenant_code) + end + + it "tracks who updated the record" do + case_log.reload + whodunnit_actor = case_log.versions.last.actor + expect(whodunnit_actor).to be_a(AdminUser) + expect(whodunnit_actor.id).to eq(admin_user.id) + end + end end end diff --git a/spec/controllers/admin/dashboard_controller_spec.rb b/spec/controllers/admin/dashboard_controller_spec.rb index f23255201..aeba5aacf 100644 --- a/spec/controllers/admin/dashboard_controller_spec.rb +++ b/spec/controllers/admin/dashboard_controller_spec.rb @@ -5,14 +5,14 @@ require_relative "../../request_helper" describe Admin::DashboardController, type: :controller do before do RequestHelper.stub_http_requests + sign_in admin_user end render_views let(:page) { Capybara::Node::Simple.new(response.body) } let(:resource_title) { "Dashboard" } let(:valid_session) { {} } - - login_admin_user + let(:admin_user) { FactoryBot.create(:admin_user) } describe "Get case logs" do before do diff --git a/spec/controllers/admin/organisations_controller_spec.rb b/spec/controllers/admin/organisations_controller_spec.rb index 16dedcb69..014d13e8c 100644 --- a/spec/controllers/admin/organisations_controller_spec.rb +++ b/spec/controllers/admin/organisations_controller_spec.rb @@ -7,8 +7,11 @@ describe Admin::OrganisationsController, type: :controller do let(:resource_title) { "Organisations" } let(:valid_session) { {} } let!(:organisation) { FactoryBot.create(:organisation) } + let!(:admin_user) { FactoryBot.create(:admin_user) } - login_admin_user + before do + sign_in admin_user + end describe "Organisations" do before do @@ -22,23 +25,54 @@ describe Admin::OrganisationsController, type: :controller do end end - describe "Create admin users" do + describe "Create organisation" do let(:params) { { organisation: { name: "DLUHC" } } } it "creates a organisation" do expect { post :create, session: valid_session, params: params }.to change(Organisation, :count).by(1) end + + it "tracks who created the record" do + post :create, session: valid_session, params: params + created_id = response.location.match(/[0-9]+/)[0] + whodunnit_actor = Organisation.find_by(id: created_id).versions.last.actor + expect(whodunnit_actor).to be_a(AdminUser) + expect(whodunnit_actor.id).to eq(admin_user.id) + end end describe "Update organisation" do - before do - get :edit, session: valid_session, params: { id: organisation.id } + context "when viewing the edit form" do + before do + get :edit, session: valid_session, params: { id: organisation.id } + end + + it "has the correct fields" do + expect(page).to have_field("organisation_name") + expect(page).to have_field("organisation_provider_type") + expect(page).to have_field("organisation_phone") + end end - it "creates a new admin users" do - expect(page).to have_field("organisation_name") - expect(page).to have_field("organisation_provider_type") - expect(page).to have_field("organisation_phone") + context "when updating the organisation" do + let(:name) { "New Org Name by Admin" } + let(:params) { { id: organisation.id, organisation: { name: name } } } + + before do + patch :update, session: valid_session, params: params + end + + it "updates the organisation" do + organisation.reload + expect(organisation.name).to eq(name) + end + + it "tracks who updated the record" do + organisation.reload + whodunnit_actor = organisation.versions.last.actor + expect(whodunnit_actor).to be_a(AdminUser) + expect(whodunnit_actor.id).to eq(admin_user.id) + end end end end diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb index 78d97405b..9a7f7e264 100644 --- a/spec/controllers/admin/users_controller_spec.rb +++ b/spec/controllers/admin/users_controller_spec.rb @@ -8,8 +8,11 @@ describe Admin::UsersController, type: :controller do let(:page) { Capybara::Node::Simple.new(response.body) } let(:resource_title) { "Users" } let(:valid_session) { {} } + let!(:admin_user) { FactoryBot.create(:admin_user) } - login_admin_user + before do + sign_in admin_user + end describe "Get users" do before do @@ -39,15 +42,23 @@ describe Admin::UsersController, type: :controller do it "creates a new user" do expect { post :create, session: valid_session, params: params }.to change(User, :count).by(1) end + + it "tracks who created the record" do + post :create, session: valid_session, params: params + created_id = response.location.match(/[0-9]+/)[0] + whodunnit_actor = User.find_by(id: created_id).versions.last.actor + expect(whodunnit_actor).to be_a(AdminUser) + expect(whodunnit_actor.id).to eq(admin_user.id) + end end describe "Update users" do - context "when updating the form" do + context "when viewing the edit form" do before do get :edit, session: valid_session, params: { id: user.id } end - it "shows an edit form" do + it "has the correct fields" do expect(page).to have_field("user_email") expect(page).to have_field("user_name") expect(page).to have_field("user_organisation_id") @@ -69,6 +80,13 @@ describe Admin::UsersController, type: :controller do user.reload expect(user.name).to eq(name) end + + it "tracks who updated the record" do + user.reload + whodunnit_actor = user.versions.last.actor + expect(whodunnit_actor).to be_a(AdminUser) + expect(whodunnit_actor.id).to eq(admin_user.id) + end end end end diff --git a/spec/factories/case_log.rb b/spec/factories/case_log.rb index 71940ab99..4e59c1656 100644 --- a/spec/factories/case_log.rb +++ b/spec/factories/case_log.rb @@ -108,7 +108,6 @@ FactoryBot.define do rp_medwel { "No" } rp_hardship { "No" } rp_dontknow { "No" } - discarded_at { nil } tenancyother { nil } override_net_income_validation { nil } net_income_known { "Yes" } diff --git a/spec/models/admin_user_spec.rb b/spec/models/admin_user_spec.rb index a112bfb5b..ca25d39b8 100644 --- a/spec/models/admin_user_spec.rb +++ b/spec/models/admin_user_spec.rb @@ -20,33 +20,46 @@ RSpec.describe AdminUser, type: :model do ) }.to raise_error(ActiveRecord::RecordInvalid) end - end - it "requires an email" do - expect { - described_class.create!( - password: "password123", - phone: "075752137", - ) - }.to raise_error(ActiveRecord::RecordInvalid) - end + it "requires an email" do + expect { + described_class.create!( + password: "password123", + phone: "075752137", + ) + }.to raise_error(ActiveRecord::RecordInvalid) + end - it "requires a password" do - expect { - described_class.create!( - email: "admin_test@example.com", - phone: "075752137", - ) - }.to raise_error(ActiveRecord::RecordInvalid) + it "requires a password" do + expect { + described_class.create!( + email: "admin_test@example.com", + phone: "075752137", + ) + }.to raise_error(ActiveRecord::RecordInvalid) + end + + it "can be created" do + expect { + described_class.create!( + email: "admin_test@example.com", + password: "password123", + phone: "075752137", + ) + }.to change(described_class, :count).by(1) + end end - it "can be created" do - expect { - described_class.create!( - email: "admin_test@example.com", - password: "password123", - phone: "075752137", - ) - }.to change(described_class, :count).by(1) + describe "paper trail" do + let(:admin_user) { FactoryBot.create(:admin_user) } + + it "creates a record of changes to a log" do + expect { admin_user.update!(phone: "09673867853") }.to change(admin_user.versions, :count).by(1) + end + + it "allows case logs to be restored to a previous version" do + admin_user.update!(phone: "09673867853") + expect(admin_user.paper_trail.previous_version.phone).to eq("07563867654") + end end end diff --git a/spec/models/case_log_spec.rb b/spec/models/case_log_spec.rb index 8b704cf0e..6fb4f43be 100644 --- a/spec/models/case_log_spec.rb +++ b/spec/models/case_log_spec.rb @@ -1172,4 +1172,17 @@ RSpec.describe CaseLog do end end end + + describe "paper trail" do + let(:case_log) { FactoryBot.create(:case_log, :in_progress) } + + it "creates a record of changes to a log" do + expect { case_log.update!(age1: 64) }.to change(case_log.versions, :count).by(1) + end + + it "allows case logs to be restored to a previous version" do + case_log.update!(age1: 63) + expect(case_log.paper_trail.previous_version.age1).to eq(17) + end + end end diff --git a/spec/models/organisation_spec.rb b/spec/models/organisation_spec.rb index a1b4f011a..a8e2c32d3 100644 --- a/spec/models/organisation_spec.rb +++ b/spec/models/organisation_spec.rb @@ -54,4 +54,17 @@ RSpec.describe Organisation, type: :model do end end end + + describe "paper trail" do + let(:organisation) { FactoryBot.create(:organisation) } + + it "creates a record of changes to a log" do + expect { organisation.update!(name: "new test name") }.to change(organisation.versions, :count).by(1) + end + + it "allows case logs to be restored to a previous version" do + organisation.update!(name: "new test name") + expect(organisation.paper_trail.previous_version.name).to eq("DLUHC") + end + end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index f24280a5c..a84c2343f 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -52,4 +52,17 @@ RSpec.describe User, type: :model do expect(user.data_coordinator?).to be false end end + + describe "paper trail" do + let(:user) { FactoryBot.create(:user) } + + it "creates a record of changes to a log" do + expect { user.update!(name: "new test name") }.to change(user.versions, :count).by(1) + end + + it "allows case logs to be restored to a previous version" do + user.update!(name: "new test name") + expect(user.paper_trail.previous_version.name).to eq("Danny Rojas") + end + end end diff --git a/spec/requests/case_logs_controller_spec.rb b/spec/requests/case_logs_controller_spec.rb index 09f14089e..8294af0b3 100644 --- a/spec/requests/case_logs_controller_spec.rb +++ b/spec/requests/case_logs_controller_spec.rb @@ -34,84 +34,104 @@ RSpec.describe CaseLogsController, type: :request do let(:in_progress) { "in_progress" } let(:completed) { "completed" } - let(:params) do - { - "owning_organisation_id": owning_organisation.id, - "managing_organisation_id": managing_organisation.id, - "tenant_code": tenant_code, - "age1": age1, - "property_postcode": property_postcode, - "offered": offered, - } - end - - before do - post "/logs", headers: headers, params: params.to_json - end - - it "returns http success" do - expect(response).to have_http_status(:success) - end - - it "returns a serialized Case Log" do - json_response = JSON.parse(response.body) - expect(json_response.keys).to match_array(CaseLog.new.attributes.keys) - end + context "when API" do + let(:params) do + { + "owning_organisation_id": owning_organisation.id, + "managing_organisation_id": managing_organisation.id, + "tenant_code": tenant_code, + "age1": age1, + "property_postcode": property_postcode, + "offered": offered, + } + end - it "creates a case log with the values passed" do - json_response = JSON.parse(response.body) - expect(json_response["tenant_code"]).to eq(tenant_code) - expect(json_response["age1"]).to eq(age1) - expect(json_response["property_postcode"]).to eq(property_postcode) - end + before do + post "/logs", headers: headers, params: params.to_json + end - context "with invalid json parameters" do - let(:age1) { 2000 } - let(:offered) { 21 } + it "returns http success" do + expect(response).to have_http_status(:success) + end - it "validates case log parameters" do + it "returns a serialized Case Log" do json_response = JSON.parse(response.body) - expect(response).to have_http_status(:unprocessable_entity) - expect(json_response["errors"]).to match_array([["offered", [I18n.t("validations.property.offered.relet_number")]], ["age1", [I18n.t("validations.household.age.must_be_valid", lower_bound: 16)]]]) + expect(json_response.keys).to match_array(CaseLog.new.attributes.keys) end - end - context "with a partial case log submission" do - it "marks the record as in_progress" do + it "creates a case log with the values passed" do json_response = JSON.parse(response.body) - expect(json_response["status"]).to eq(in_progress) + expect(json_response["tenant_code"]).to eq(tenant_code) + expect(json_response["age1"]).to eq(age1) + expect(json_response["property_postcode"]).to eq(property_postcode) end - end - context "with a complete case log submission" do - let(:org_params) do - { - "case_log" => { - "owning_organisation_id" => owning_organisation.id, - "managing_organisation_id" => managing_organisation.id, - }, - } + context "with invalid json parameters" do + let(:age1) { 2000 } + let(:offered) { 21 } + + it "validates case log parameters" do + json_response = JSON.parse(response.body) + expect(response).to have_http_status(:unprocessable_entity) + expect(json_response["errors"]).to match_array([["offered", [I18n.t("validations.property.offered.relet_number")]], ["age1", [I18n.t("validations.household.age.must_be_valid", lower_bound: 16)]]]) + end end - let(:case_log_params) { JSON.parse(File.open("spec/fixtures/complete_case_log.json").read) } - let(:params) do - case_log_params.merge(org_params) { |_k, a_val, b_val| a_val.merge(b_val) } + + context "with a partial case log submission" do + it "marks the record as in_progress" do + json_response = JSON.parse(response.body) + expect(json_response["status"]).to eq(in_progress) + end end - it "marks the record as completed" do - json_response = JSON.parse(response.body) + context "with a complete case log submission" do + let(:org_params) do + { + "case_log" => { + "owning_organisation_id" => owning_organisation.id, + "managing_organisation_id" => managing_organisation.id, + }, + } + end + let(:case_log_params) { JSON.parse(File.open("spec/fixtures/complete_case_log.json").read) } + let(:params) do + case_log_params.merge(org_params) { |_k, a_val, b_val| a_val.merge(b_val) } + end + + it "marks the record as completed" do + json_response = JSON.parse(response.body) + + expect(json_response).not_to have_key("errors") + expect(json_response["status"]).to eq(completed) + end + end + + context "with a request containing invalid credentials" do + let(:basic_credentials) do + ActionController::HttpAuthentication::Basic.encode_credentials(api_username, "Oops") + end - expect(json_response).not_to have_key("errors") - expect(json_response["status"]).to eq(completed) + it "returns 401" do + expect(response).to have_http_status(:unauthorized) + end end end - context "with a request containing invalid credentials" do - let(:basic_credentials) do - ActionController::HttpAuthentication::Basic.encode_credentials(api_username, "Oops") + context "when UI" do + let(:user) { FactoryBot.create(:user) } + let(:headers) { { "Accept" => "text/html" } } + + before do + RequestHelper.stub_http_requests + sign_in user + post "/logs", headers: headers end - it "returns 401" do - expect(response).to have_http_status(:unauthorized) + it "tracks who created the record" do + created_id = response.location.match(/[0-9]+/)[0] + whodunnit_actor = CaseLog.find_by(id: created_id).versions.last.actor + expect(whodunnit_actor).to be_a(User) + expect(whodunnit_actor.id).to eq(user.id) end end end @@ -401,9 +421,8 @@ RSpec.describe CaseLogsController, type: :request do expect(response).to have_http_status(:success) end - it "soft deletes the case log" do + it "deletes the case log" do expect { CaseLog.find(id) }.to raise_error(ActiveRecord::RecordNotFound) - expect(CaseLog.with_discarded.find(id)).to be_a(CaseLog) end context "with an invalid case log id" do @@ -428,7 +447,7 @@ RSpec.describe CaseLogsController, type: :request do context "when a case log deletion fails" do before do allow(CaseLog).to receive(:find_by).and_return(case_log) - allow(case_log).to receive(:discard).and_return(false) + allow(case_log).to receive(:delete).and_return(false) delete "/logs/#{id}", headers: headers end diff --git a/spec/requests/form_controller_spec.rb b/spec/requests/form_controller_spec.rb index 3d77492b7..f4b49e129 100644 --- a/spec/requests/form_controller_spec.rb +++ b/spec/requests/form_controller_spec.rb @@ -155,6 +155,13 @@ RSpec.describe FormController, type: :request do expect(case_log.age1).to eq(answer) expect(case_log.age2).to be nil end + + it "tracks who updated the record" do + case_log.reload + whodunnit_actor = case_log.versions.last.actor + expect(whodunnit_actor).to be_a(User) + expect(whodunnit_actor.id).to eq(user.id) + end end end diff --git a/spec/requests/organisations_controller_spec.rb b/spec/requests/organisations_controller_spec.rb index 24ca82101..b92c8da53 100644 --- a/spec/requests/organisations_controller_spec.rb +++ b/spec/requests/organisations_controller_spec.rb @@ -185,6 +185,13 @@ RSpec.describe OrganisationsController, type: :request do follow_redirect! expect(page).to have_css(".govuk-notification-banner.govuk-notification-banner--success") end + + it "tracks who updated the record" do + organisation.reload + whodunnit_actor = organisation.versions.last.actor + expect(whodunnit_actor).to be_a(User) + expect(whodunnit_actor.id).to eq(user.id) + end end context "with an organisation that the user does not belong to" do diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 59997f3ce..66cbeb5c2 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -196,6 +196,13 @@ RSpec.describe UsersController, type: :request do user.reload expect(user.name).to eq(new_value) end + + it "tracks who updated the record" do + user.reload + whodunnit_actor = user.versions.last.actor + expect(whodunnit_actor).to be_a(User) + expect(whodunnit_actor.id).to eq(user.id) + end end context "when the update fails to persist" do diff --git a/spec/support/controller_macros.rb b/spec/support/controller_macros.rb index 2e21831dd..680663367 100644 --- a/spec/support/controller_macros.rb +++ b/spec/support/controller_macros.rb @@ -6,12 +6,4 @@ module ControllerMacros sign_in user end end - - def login_admin_user - before do - @request.env["devise.mapping"] = Devise.mappings[:admin_user] - admin_user = FactoryBot.create(:admin_user) - sign_in admin_user - end - end end From cd8ba346a203395c18aa99ad175d62a722b39110 Mon Sep 17 00:00:00 2001 From: baarkerlounger <5101747+baarkerlounger@users.noreply.github.com> Date: Wed, 9 Feb 2022 10:10:33 +0000 Subject: [PATCH 09/11] Time out logged in session after 1 day (#284) --- config/initializers/devise.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index de41a7b75..d377aacd0 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -189,7 +189,7 @@ Devise.setup do |config| # ==> Configuration for :timeoutable # The time you want to timeout the user session without activity. After this # time the user will be asked for credentials again. Default is 30 minutes. - # config.timeout_in = 30.minutes + config.timeout_in = 1.day # ==> Configuration for :lockable # Defines which strategy will be used to lock an account. From f0f50701353e6e6f5d7b2c545506792ea70e8303 Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Wed, 9 Feb 2022 10:19:57 +0000 Subject: [PATCH 10/11] Renewal inferred variables (#285) * Infer layear if it is a renewal * Infer underoccupation_benefitcap if it is a renewal * Infer homelessness if it is a renewal --- app/models/case_log.rb | 3 +++ spec/fixtures/complete_case_log.json | 7 +++---- spec/models/case_log_spec.rb | 29 ++++++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/app/models/case_log.rb b/app/models/case_log.rb index a4c331b4b..bfcf09913 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -252,6 +252,9 @@ private self.tcharge = brent.to_f + scharge.to_f + pscharge.to_f + supcharg.to_f self.has_benefits = get_has_benefits self.nocharge = household_charge == "Yes" ? "No" : "Yes" + self.layear = "Less than 1 year" if renewal == "Yes" + self.underoccupation_benefitcap = "No" if renewal == "Yes" && year == 2021 + self.homeless = "No" if renewal == "Yes" end def process_postcode_changes! diff --git a/spec/fixtures/complete_case_log.json b/spec/fixtures/complete_case_log.json index 268283c2a..a172080c3 100644 --- a/spec/fixtures/complete_case_log.json +++ b/spec/fixtures/complete_case_log.json @@ -40,7 +40,7 @@ "age8": 2, "sex8": "Prefer not to say", "ecstat8": "Child under 16", - "homeless": "Yes - other homelessness", + "homeless": "No", "reason": 1, "underoccupation_benefitcap": "No", "leftreg": "No - they left up to 5 years ago", @@ -90,8 +90,7 @@ "lawaitlist": "Less than 1 year", "prevloc": "Ashford", "previous_postcode": "SE2 6RT", - "reasonpref": "Yes", - "reasonable_preference_reason": "dummy", + "reasonpref": "No", "cbl": "Yes", "chr": "Yes", "cap": "No", @@ -116,7 +115,7 @@ "illness_type_9": "No", "illness_type_10": "No", "condition_effects_prefer_not_to_say": "Yes", - "rp_homeless": "Yes", + "rp_homeless": "No", "rp_insan_unsat": "No", "rp_medwel": "No", "rp_hardship": "No", diff --git a/spec/models/case_log_spec.rb b/spec/models/case_log_spec.rb index 6fb4f43be..cab941041 100644 --- a/spec/models/case_log_spec.rb +++ b/spec/models/case_log_spec.rb @@ -1153,6 +1153,35 @@ RSpec.describe CaseLog do record_from_db = ActiveRecord::Base.connection.execute("select has_benefits from case_logs where id=#{case_log.id}").to_a[0] expect(record_from_db["has_benefits"]).to eq("Yes") end + + context "when it is a renewal" do + let!(:case_log) do + described_class.create({ + managing_organisation: organisation, + owning_organisation: organisation, + renewal: "Yes", + year: 2021, + }) + end + + it "correctly derives and saves layear" do + record_from_db = ActiveRecord::Base.connection.execute("select layear from case_logs where id=#{case_log.id}").to_a[0] + expect(record_from_db["layear"]).to eq(2) + expect(case_log["layear"]).to eq("Less than 1 year") + end + + it "correctly derives and saves underoccupation_benefitcap if year is 2021" do + record_from_db = ActiveRecord::Base.connection.execute("select underoccupation_benefitcap from case_logs where id=#{case_log.id}").to_a[0] + expect(record_from_db["underoccupation_benefitcap"]).to eq(2) + expect(case_log["underoccupation_benefitcap"]).to eq("No") + end + + it "correctly derives and saves homeless" do + record_from_db = ActiveRecord::Base.connection.execute("select homeless from case_logs where id=#{case_log.id}").to_a[0] + expect(record_from_db["homeless"]).to eq(1) + expect(case_log["homeless"]).to eq("No") + end + end end describe "resetting invalidated fields" do From b553140d0dfaa1378a0bc87e7843318a98cc92ca Mon Sep 17 00:00:00 2001 From: baarkerlounger <5101747+baarkerlounger@users.noreply.github.com> Date: Wed, 9 Feb 2022 12:26:38 +0000 Subject: [PATCH 11/11] Production manifest (#286) * Update infra setup readme * Update production manifest --- infrastructure_setup.md | 31 ++++++++++++++++++++++++++++--- manifest.yml | 5 +++++ 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/infrastructure_setup.md b/infrastructure_setup.md index cd6b7f64c..1f7d7b422 100644 --- a/infrastructure_setup.md +++ b/infrastructure_setup.md @@ -1,10 +1,10 @@ # Staging 1. Login:\ -`cf login -a api.london.cloud.service.gov.uk -u ` + `cf login -a api.london.cloud.service.gov.uk -u ` 2. Set your deployment target (staging):\ -`cf target -o dluhc-core -s staging` + `cf target -o dluhc-core -s staging` 3. Create required Postgres and S3 bucket backing services (this will take ~15 mins to finish creating):\ `cf create-service postgres tiny-unencrypted-13 dluhc-core-staging-postgres` @@ -13,9 +13,34 @@ `cf create-service aws-s3-bucket default dluhc-core-staging-export-bucket` +4. Deploy manifest:\ + `cf push dluhc-core-staging --strategy rolling` + +5. Bind S3 services to app:\ `cf bind-service dluhc-core-staging dluhc-core-staging-import-bucket -c '{"permissions": "read-only"}'` `cf bind-service dluhc-core-staging dluhc-core-staging-export-bucket -c '{"permissions": "read-write"}'` + +# Production + +1. Login:\ + `cf login -a api.london.cloud.service.gov.uk -u ` + +2. Set your deployment target (production):\ + `cf target -o dluhc-core -s production` + +3. Create required Postgres and S3 bucket backing services (this will take ~15 mins to finish creating):\ + `cf create-service postgres small-ha-13 dluhc-core-production-postgres` + + `cf create-service aws-s3-bucket default dluhc-core-production-import-bucket` + + `cf create-service aws-s3-bucket default dluhc-core-production-export-bucket` + 4. Deploy manifest:\ -`cf push dluhc-core-staging --strategy rolling` + `cf push dluhc-core-production --strategy rolling` + +5. Bind S3 services to app:\ + `cf bind-service dluhc-core-production dluhc-core-production-import-bucket -c '{"permissions": "read-only"}'` + + `cf bind-service dluhc-core-production dluhc-core-production-export-bucket -c '{"permissions": "read-write"}'` diff --git a/manifest.yml b/manifest.yml index 8938a313c..a80004d41 100644 --- a/manifest.yml +++ b/manifest.yml @@ -20,6 +20,11 @@ applications: - name: dluhc-core-production <<: *defaults + processes: + - type: web + command: bundle exec rake cf:on_first_instance db:migrate && bin/rails server + instances: 4 + memory: 1G env: RAILS_ENV: production host: submit-social-housing-lettings-sales-data