From 07f345df71c02504f2a6ca9d58e414ae3cbb9e79 Mon Sep 17 00:00:00 2001 From: natdeanlewissoftwire <94526761+natdeanlewissoftwire@users.noreply.github.com> Date: Wed, 14 Dec 2022 09:33:39 +0000 Subject: [PATCH 1/3] CLDC-1730-stock-owning-validation (#1073) * feat: add error message if org doesn't own stock * feat: move org relationship errors to model * feat: use correct model for form * tests: update params in tests * feat: add specific fields for parent/child errors * test: fix params in failing tests * refactor: unrelated linting * feat: simplify controller behaviour further * test: address failing tests * feat: add copy to en.yml * feat: add updated validation message and rename related housing provider -> stock owner --- .../organisation_relationships_controller.rb | 115 ++++++++---------- app/helpers/tasklist_helper.rb | 2 +- app/models/organisation_relationship.rb | 13 ++ app/views/devise/unlocks/new.html.erb | 2 +- ...ated_organisation_select_question.html.erb | 2 +- .../add_housing_provider.html.erb | 3 +- .../add_managing_agent.html.erb | 3 +- .../managing_agents.html.erb | 2 +- config/locales/en.yml | 7 ++ db/schema.rb | 2 +- ...anisation_relationships_controller_spec.rb | 16 +-- 11 files changed, 88 insertions(+), 79 deletions(-) diff --git a/app/controllers/organisation_relationships_controller.rb b/app/controllers/organisation_relationships_controller.rb index e7a6a7856..9aa93c37c 100644 --- a/app/controllers/organisation_relationships_controller.rb +++ b/app/controllers/organisation_relationships_controller.rb @@ -5,13 +5,19 @@ class OrganisationRelationshipsController < ApplicationController before_action :authenticate_user! before_action :authenticate_scope! + before_action :organisations + before_action :target_organisation, only: %i[ + remove_housing_provider + remove_managing_agent + delete_housing_provider + delete_managing_agent + ] + def housing_providers housing_providers = organisation.housing_providers unpaginated_filtered_housing_providers = filtered_collection(housing_providers, search_term) - organisations = Organisation.where.not(id: @organisation.id).pluck(:id, :name) - respond_to :html + @pagy, @housing_providers = pagy(unpaginated_filtered_housing_providers) - @organisations = organisations @searched = search_term.presence @total_count = housing_providers.size end @@ -19,107 +25,84 @@ class OrganisationRelationshipsController < ApplicationController def managing_agents managing_agents = organisation.managing_agents unpaginated_filtered_managing_agents = filtered_collection(managing_agents, search_term) - organisations = Organisation.where.not(id: @organisation.id).pluck(:id, :name) - respond_to :html + @pagy, @managing_agents = pagy(unpaginated_filtered_managing_agents) - @organisations = organisations @searched = search_term.presence @total_count = managing_agents.size end def add_housing_provider - @organisations = Organisation.where.not(id: @organisation.id).pluck(:id, :name) - respond_to :html + @organisation_relationship = organisation.parent_organisation_relationships.new end def add_managing_agent - @organisations = Organisation.where.not(id: @organisation.id).pluck(:id, :name) - respond_to :html + @organisation_relationship = organisation.child_organisation_relationships.new end def create_housing_provider - child_organisation = @organisation - if params[:organisation][:related_organisation_id].empty? - @organisation.errors.add :related_organisation_id, "You must choose a housing provider" - @organisations = Organisation.where.not(id: child_organisation.id).pluck(:id, :name) - render "organisation_relationships/add_housing_provider" - return + @organisation_relationship = organisation.parent_organisation_relationships.new(organisation_relationship_params) + if @organisation_relationship.save(context: :housing_provider) + flash[:notice] = "#{@organisation_relationship.parent_organisation.name} is now one of #{current_user.data_coordinator? ? 'your' : "this organisation's"} housing providers" + redirect_to housing_providers_organisation_path else - parent_organisation = related_organisation - if OrganisationRelationship.exists?(child_organisation:, parent_organisation:) - @organisation.errors.add :related_organisation_id, "You have already added this housing provider" - @organisations = Organisation.where.not(id: child_organisation.id).pluck(:id, :name) - render "organisation_relationships/add_housing_provider" - return - end + @organisations = Organisation.where.not(id: organisation.id).pluck(:id, :name) + render "organisation_relationships/add_housing_provider", status: :unprocessable_entity end - create!(child_organisation:, parent_organisation:) - flash[:notice] = "#{related_organisation.name} is now one of #{current_user.data_coordinator? ? 'your' : "this organisation's"} housing providers" - redirect_to housing_providers_organisation_path end def create_managing_agent - parent_organisation = @organisation - if params[:organisation][:related_organisation_id].empty? - @organisation.errors.add :related_organisation_id, "You must choose a managing agent" - @organisations = Organisation.where.not(id: parent_organisation.id).pluck(:id, :name) - render "organisation_relationships/add_managing_agent" - return + @organisation_relationship = organisation.child_organisation_relationships.new(organisation_relationship_params) + if @organisation_relationship.save + flash[:notice] = "#{@organisation_relationship.child_organisation.name} is now one of #{current_user.data_coordinator? ? 'your' : "this organisation's"} managing agents" + redirect_to managing_agents_organisation_path else - child_organisation = related_organisation - if OrganisationRelationship.exists?(child_organisation:, parent_organisation:) - @organisation.errors.add :related_organisation_id, "You have already added this managing agent" - @organisations = Organisation.where.not(id: parent_organisation.id).pluck(:id, :name) - render "organisation_relationships/add_managing_agent" - return - end + @organisations = Organisation.where.not(id: organisation.id).pluck(:id, :name) + render "organisation_relationships/add_managing_agent", status: :unprocessable_entity end - create!(child_organisation:, parent_organisation:) - flash[:notice] = "#{related_organisation.name} is now one of #{current_user.data_coordinator? ? 'your' : "this organisation's"} managing agents" - redirect_to managing_agents_organisation_path end - def remove_housing_provider - @target_organisation_id = target_organisation.id - end + def remove_housing_provider; end def delete_housing_provider - relationship = OrganisationRelationship.find_by!( - child_organisation: @organisation, + OrganisationRelationship.find_by!( + child_organisation: organisation, parent_organisation: target_organisation, - ) - relationship.destroy! + ).destroy! flash[:notice] = "#{target_organisation.name} is no longer one of #{current_user.data_coordinator? ? 'your' : "this organisation's"} housing providers" redirect_to housing_providers_organisation_path end - def remove_managing_agent - @target_organisation_id = target_organisation.id - end + def remove_managing_agent; end def delete_managing_agent - relationship = OrganisationRelationship.find_by!( - parent_organisation: @organisation, + OrganisationRelationship.find_by!( + parent_organisation: organisation, child_organisation: target_organisation, - ) - relationship.destroy! + ).destroy! flash[:notice] = "#{target_organisation.name} is no longer one of #{current_user.data_coordinator? ? 'your' : "this organisation's"} managing agents" redirect_to managing_agents_organisation_path end private - def create!(child_organisation:, parent_organisation:) - @resource = OrganisationRelationship.new(child_organisation:, parent_organisation:) - @resource.save! + def organisation + @organisation ||= if current_user.support? + Organisation.find(params[:id]) + else + current_user.organisation + end end - def organisation - @organisation ||= Organisation.find(params[:id]) + def organisations + @organisations ||= Organisation.where.not(id: organisation.id).pluck(:id, :name) + end + + def parent_organisation + @parent_organisation ||= Organisation.find(params[:organisation_relationship][:parent_organisation_id]) end - def related_organisation - @related_organisation ||= Organisation.find(params[:organisation][:related_organisation_id]) + def child_organisation + @child_organisation ||= Organisation.find(params[:organisation_relationship][:child_organisation_id]) end def target_organisation @@ -130,8 +113,12 @@ private params["search"] end + def organisation_relationship_params + params.require(:organisation_relationship).permit(:parent_organisation_id, :child_organisation_id) + end + def authenticate_scope! - if current_user.organisation != organisation && !current_user.support? + if current_user.organisation != Organisation.find(params[:id]) && !current_user.support? render_not_found end end diff --git a/app/helpers/tasklist_helper.rb b/app/helpers/tasklist_helper.rb index 05c84d1b7..f297de2ff 100644 --- a/app/helpers/tasklist_helper.rb +++ b/app/helpers/tasklist_helper.rb @@ -39,7 +39,7 @@ module TasklistHelper def review_log_text(log) if log.collection_period_open? - "You can #{govuk_link_to 'review and make changes to this log', review_lettings_log_path(log)} until #{(log.form.end_date).to_formatted_s(:govuk_date)}.".html_safe + "You can #{govuk_link_to 'review and make changes to this log', review_lettings_log_path(log)} until #{log.form.end_date.to_formatted_s(:govuk_date)}.".html_safe else "This log is from the #{log.form.start_date.year}/#{log.form.start_date.year + 1} collection window, which is now closed." end diff --git a/app/models/organisation_relationship.rb b/app/models/organisation_relationship.rb index 034fc5d0e..7f3a85cae 100644 --- a/app/models/organisation_relationship.rb +++ b/app/models/organisation_relationship.rb @@ -1,4 +1,17 @@ class OrganisationRelationship < ApplicationRecord belongs_to :child_organisation, class_name: "Organisation" belongs_to :parent_organisation, class_name: "Organisation" + validates :parent_organisation_id, presence: { message: I18n.t("validations.organisation.housing_provider.blank") } + validates :child_organisation_id, presence: { message: I18n.t("validations.organisation.managing_agent.blank") } + validates :parent_organisation_id, uniqueness: { scope: :child_organisation_id, message: I18n.t("validations.organisation.housing_provider.already_added") } + validates :child_organisation_id, uniqueness: { scope: :parent_organisation_id, message: I18n.t("validations.organisation.managing_agent.already_added") } + validate :validate_housing_provider_owns_stock, on: :housing_provider + +private + + def validate_housing_provider_owns_stock + if parent_organisation_id.present? && !parent_organisation.holds_own_stock + errors.add :parent_organisation_id, I18n.t("validations.organisation.housing_provider.does_not_own_stock") + end + end end diff --git a/app/views/devise/unlocks/new.html.erb b/app/views/devise/unlocks/new.html.erb index 4f0de4c0b..e5ad2b7b0 100644 --- a/app/views/devise/unlocks/new.html.erb +++ b/app/views/devise/unlocks/new.html.erb @@ -1,7 +1,7 @@
This organisation does not currently have any managing agents.
<% end %> <% else %> - <%= render partial: "organisations/headings", locals: { main: "This organisation managing agents", sub: current_user.organisation.name } %> + <%= render partial: "organisations/headings", locals: { main: "Your managing agents", sub: current_user.organisation.name } %>A managing agent can submit logs for this organisation.
<% if @total_count == 0 %>This organisation does not currently have any managing agents.
diff --git a/config/locales/en.yml b/config/locales/en.yml index 05a828dea..fae17aa0f 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -116,6 +116,13 @@ en: organisation: name_missing: "Enter the name of the organisation" provider_type_missing: "Select the organisation type" + housing_provider: + blank: "You must choose a stock owner" + already_added: "You have already added this stock owner" + does_not_own_stock: "You can only add stock owners who own stock, which this organisation does not." + managing_agent: + blank: "You must choose a managing agent" + already_added: "You have already added this managing agent" not_answered: "You must answer %{question}" other_field_missing: "If %{main_field_label} is other then %{other_field_label} must be provided" diff --git a/db/schema.rb b/db/schema.rb index f16ab6d9d..9beffb241 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -399,9 +399,9 @@ ActiveRecord::Schema[7.0].define(version: 2022_12_12_161657) do t.integer "inc1mort" t.integer "income2" t.integer "income2nk" - t.integer "prevown" t.integer "savingsnk" t.integer "savings" + t.integer "prevown" t.string "sex3" t.integer "details_known_1" t.index ["created_by_id"], name: "index_sales_logs_on_created_by_id" diff --git a/spec/requests/organisation_relationships_controller_spec.rb b/spec/requests/organisation_relationships_controller_spec.rb index 99c57a01a..47593d1c0 100644 --- a/spec/requests/organisation_relationships_controller_spec.rb +++ b/spec/requests/organisation_relationships_controller_spec.rb @@ -139,8 +139,8 @@ RSpec.describe OrganisationRelationshipsController, type: :request do let(:params) do { - "organisation": { - "related_organisation_id": housing_provider.id, + "organisation_relationship": { + "parent_organisation_id": housing_provider.id, }, } end @@ -167,8 +167,8 @@ RSpec.describe OrganisationRelationshipsController, type: :request do let(:params) do { - "organisation": { - "related_organisation_id": managing_agent.id, + "organisation_relationship": { + "child_organisation_id": managing_agent.id, }, } end @@ -368,8 +368,8 @@ RSpec.describe OrganisationRelationshipsController, type: :request do let(:params) do { - "organisation": { - "related_organisation_id": housing_provider.id, + "organisation_relationship": { + "parent_organisation_id": housing_provider.id, }, } end @@ -396,8 +396,8 @@ RSpec.describe OrganisationRelationshipsController, type: :request do let(:params) do { - "organisation": { - "related_organisation_id": managing_agent.id, + "organisation_relationship": { + "child_organisation_id": managing_agent.id, }, } end From 1ceaf7b00059393ab3d0fdeceeb3b29f033eaaf5 Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Wed, 14 Dec 2022 09:42:47 +0000 Subject: [PATCH 2/3] Update rails-html-sanitizer gem because of a known vulnerability (#1087) --- Gemfile.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 8323c7e1b..896f87f91 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -199,7 +199,7 @@ GEM listen (3.7.1) rb-fsevent (~> 0.10, >= 0.10.3) rb-inotify (~> 0.9, >= 0.9.10) - loofah (2.19.0) + loofah (2.19.1) crass (~> 1.0.2) nokogiri (>= 1.5.9) mail (2.7.1) @@ -294,8 +294,8 @@ GEM rails-dom-testing (2.0.3) activesupport (>= 4.2.0) nokogiri (>= 1.6) - rails-html-sanitizer (1.4.3) - loofah (~> 2.3) + rails-html-sanitizer (1.4.4) + loofah (~> 2.19, >= 2.19.1) railties (7.0.4) actionpack (= 7.0.4) activesupport (= 7.0.4) From 514552934d3d86bdd2ac3e40a0be87b40487aa60 Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Wed, 14 Dec 2022 09:58:11 +0000 Subject: [PATCH 3/3] Sales financial validations (#1079) * Include shared validations in sales log model and run validate_numeric_min_max method * Add non london hard max income validation * Add london income validation * Remove child vlidation because buyer cannot be a child * Add income1_value_check column * Add buyer_1_income_value_check question and page * Update income1_under_soft_min? * Add mortgage and value check column * Add mortgage value check page * Update mortgage_over_soft_max? in soft validateions * Remove unused error message --- .../sales/pages/buyer1_income_value_check.rb | 21 ++ .../form/sales/pages/mortgage_value_check.rb | 21 ++ .../form/sales/questions/buyer1_income.rb | 1 + .../questions/buyer1_income_value_check.rb | 25 +++ .../sales/questions/mortgage_value_check.rb | 25 +++ .../income_benefits_and_savings.rb | 2 + app/models/sales_log.rb | 51 +++++ .../sales/financial_validations.rb | 14 ++ .../validations/sales/soft_validations.rb | 23 ++ config/locales/en.yml | 2 + ...2161657_add_details_known1_to_sales_log.rb | 6 +- ...213085819_add_mortgage_and_value_checks.rb | 10 + db/schema.rb | 6 +- spec/factories/sales_log.rb | 2 + .../pages/buyer1_income_value_check_spec.rb | 33 +++ .../sales/pages/mortgage_value_check_spec.rb | 33 +++ .../sales/questions/buyer1_income_spec.rb | 4 + .../buyer1_income_value_check_spec.rb | 61 ++++++ .../questions/mortgage_value_check_spec.rb | 61 ++++++ .../income_benefits_and_savings_spec.rb | 2 + spec/models/form_handler_spec.rb | 4 +- .../sales/financial_validations_spec.rb | 56 +++++ .../sales/soft_validations_spec.rb | 204 ++++++++++++++++++ 23 files changed, 662 insertions(+), 5 deletions(-) create mode 100644 app/models/form/sales/pages/buyer1_income_value_check.rb create mode 100644 app/models/form/sales/pages/mortgage_value_check.rb create mode 100644 app/models/form/sales/questions/buyer1_income_value_check.rb create mode 100644 app/models/form/sales/questions/mortgage_value_check.rb create mode 100644 app/models/validations/sales/financial_validations.rb create mode 100644 app/models/validations/sales/soft_validations.rb create mode 100644 db/migrate/20221213085819_add_mortgage_and_value_checks.rb create mode 100644 spec/models/form/sales/pages/buyer1_income_value_check_spec.rb create mode 100644 spec/models/form/sales/pages/mortgage_value_check_spec.rb create mode 100644 spec/models/form/sales/questions/buyer1_income_value_check_spec.rb create mode 100644 spec/models/form/sales/questions/mortgage_value_check_spec.rb create mode 100644 spec/models/validations/sales/financial_validations_spec.rb create mode 100644 spec/models/validations/sales/soft_validations_spec.rb diff --git a/app/models/form/sales/pages/buyer1_income_value_check.rb b/app/models/form/sales/pages/buyer1_income_value_check.rb new file mode 100644 index 000000000..243388ed9 --- /dev/null +++ b/app/models/form/sales/pages/buyer1_income_value_check.rb @@ -0,0 +1,21 @@ +class Form::Sales::Pages::Buyer1IncomeValueCheck < ::Form::Page + def initialize(id, hsh, subsection) + super + @id = "buyer_1_income_value_check" + @header = "" + @description = "" + @subsection = subsection + @depends_on = [ + { + "income1_under_soft_min?" => true, + }, + ] + @informative_text = {} + end + + def questions + @questions ||= [ + Form::Sales::Questions::Buyer1IncomeValueCheck.new(nil, nil, self), + ] + end +end diff --git a/app/models/form/sales/pages/mortgage_value_check.rb b/app/models/form/sales/pages/mortgage_value_check.rb new file mode 100644 index 000000000..0495f4f4f --- /dev/null +++ b/app/models/form/sales/pages/mortgage_value_check.rb @@ -0,0 +1,21 @@ +class Form::Sales::Pages::MortgageValueCheck < ::Form::Page + def initialize(id, hsh, subsection) + super + @id = "mortgage_value_check" + @header = "" + @description = "" + @subsection = subsection + @depends_on = [ + { + "mortgage_over_soft_max?" => true, + }, + ] + @informative_text = {} + end + + def questions + @questions ||= [ + Form::Sales::Questions::MortgageValueCheck.new(nil, nil, self), + ] + end +end diff --git a/app/models/form/sales/questions/buyer1_income.rb b/app/models/form/sales/questions/buyer1_income.rb index 2b80aec5a..aa4a9df09 100644 --- a/app/models/form/sales/questions/buyer1_income.rb +++ b/app/models/form/sales/questions/buyer1_income.rb @@ -7,6 +7,7 @@ class Form::Sales::Questions::Buyer1Income < ::Form::Question @type = "numeric" @page = page @min = 0 + @max = 999_999 @step = 1 @width = 5 @prefix = "£" diff --git a/app/models/form/sales/questions/buyer1_income_value_check.rb b/app/models/form/sales/questions/buyer1_income_value_check.rb new file mode 100644 index 000000000..9b0f21660 --- /dev/null +++ b/app/models/form/sales/questions/buyer1_income_value_check.rb @@ -0,0 +1,25 @@ +class Form::Sales::Questions::Buyer1IncomeValueCheck < ::Form::Question + def initialize(id, hsh, page) + super + @id = "income1_value_check" + @check_answer_label = "Income confirmation" + @header = "Are you sure this income is correct?" + @type = "interruption_screen" + @answer_options = { + "0" => { "value" => "Yes" }, + "1" => { "value" => "No" }, + } + @hidden_in_check_answers = { + "depends_on" => [ + { + "income1_value_check" => 0, + }, + { + "income1_value_check" => 1, + }, + ], + } + @check_answers_card_number = 1 + @page = page + end +end diff --git a/app/models/form/sales/questions/mortgage_value_check.rb b/app/models/form/sales/questions/mortgage_value_check.rb new file mode 100644 index 000000000..9d063f428 --- /dev/null +++ b/app/models/form/sales/questions/mortgage_value_check.rb @@ -0,0 +1,25 @@ +class Form::Sales::Questions::MortgageValueCheck < ::Form::Question + def initialize(id, hsh, page) + super + @id = "mortgage_value_check" + @check_answer_label = "Mortgage confirmation" + @header = "Are you sure that the mortgage is more than 5 times the income used for the mortgage application?" + @type = "interruption_screen" + @answer_options = { + "0" => { "value" => "Yes" }, + "1" => { "value" => "No" }, + } + @hidden_in_check_answers = { + "depends_on" => [ + { + "mortgage_value_check" => 0, + }, + { + "mortgage_value_check" => 1, + }, + ], + } + @check_answers_card_number = 1 + @page = page + end +end diff --git a/app/models/form/sales/subsections/income_benefits_and_savings.rb b/app/models/form/sales/subsections/income_benefits_and_savings.rb index 74a07231b..447aba589 100644 --- a/app/models/form/sales/subsections/income_benefits_and_savings.rb +++ b/app/models/form/sales/subsections/income_benefits_and_savings.rb @@ -10,8 +10,10 @@ class Form::Sales::Subsections::IncomeBenefitsAndSavings < ::Form::Subsection def pages @pages ||= [ Form::Sales::Pages::Buyer1Income.new(nil, nil, self), + Form::Sales::Pages::Buyer1IncomeValueCheck.new(nil, nil, self), Form::Sales::Pages::Buyer1Mortgage.new(nil, nil, self), Form::Sales::Pages::Buyer2Income.new(nil, nil, self), + Form::Sales::Pages::MortgageValueCheck.new(nil, nil, self), Form::Sales::Pages::Savings.new(nil, nil, self), Form::Sales::Pages::PreviousOwnership.new(nil, nil, self), ] diff --git a/app/models/sales_log.rb b/app/models/sales_log.rb index 5a2e045bc..d3066974f 100644 --- a/app/models/sales_log.rb +++ b/app/models/sales_log.rb @@ -1,5 +1,7 @@ class SalesLogValidator < ActiveModel::Validator include Validations::Sales::HouseholdValidations + include Validations::SharedValidations + include Validations::Sales::FinancialValidations def validate(record) validation_methods = public_methods.select { |method| method.starts_with?("validate_") } @@ -9,6 +11,7 @@ end class SalesLog < Log include DerivedVariables::SalesLogVariables + include Validations::Sales::SoftValidations self.inheritance_column = :_type_disabled @@ -60,4 +63,52 @@ class SalesLog < Log def unresolved false end + + LONDON_BOROUGHS = %w[ + E09000001 + E09000033 + E09000020 + E09000013 + E09000032 + E09000022 + E09000028 + E09000030 + E09000012 + E09000019 + E09000007 + E09000005 + E09000009 + E09000018 + E09000027 + E09000021 + E09000024 + E09000029 + E09000008 + E09000006 + E09000023 + E09000011 + E09000004 + E09000016 + E09000002 + E09000026 + E09000025 + E09000031 + E09000014 + E09000010 + E09000003 + E09000015 + E09000017 + ].freeze + + def london_property? + la && LONDON_BOROUGHS.include?(la) + end + + def income1_used_for_mortgage? + inc1mort == 1 + end + + def income2_used_for_mortgage? + inc2mort == 1 + end end diff --git a/app/models/validations/sales/financial_validations.rb b/app/models/validations/sales/financial_validations.rb new file mode 100644 index 000000000..04f6a1d06 --- /dev/null +++ b/app/models/validations/sales/financial_validations.rb @@ -0,0 +1,14 @@ +module Validations::Sales::FinancialValidations + # Validations methods need to be called 'validate_