From c7db5102f6ee17d32af4bf5087d976e054ff83dc Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Tue, 2 May 2023 09:50:14 +0100 Subject: [PATCH 1/3] CLDC-2294 Add authorization to bulk upload results (#1591) # Context - https://digital.dclg.gov.uk/jira/browse/CLDC-2294 - When bulk upload returns a results page it would be useful if colleagues of the uploader can see this page to help fix errors - It would also be useful if support users can see these reports to help diagnose bulk upload errors # Changes - Added `pundit` gem to handle authorization - Bulk upload results previously only accessible to the bulk uploader. Now they can be seen by users in the same org as the uploader and also support users --- Gemfile | 3 ++ Gemfile.lock | 3 ++ app/controllers/application_controller.rb | 8 ++++ ...bulk_upload_lettings_results_controller.rb | 8 +++- .../bulk_upload_sales_results_controller.rb | 10 ++++- app/policies/bulk_upload_policy.rb | 26 ++++++++++++ .../application_controller_spec.rb | 23 +++++++++++ ...upload_lettings_results_controller_spec.rb | 13 ++++++ ...lk_upload_sales_results_controller_spec.rb | 33 +++++++++++++++ spec/policies/bulk_upload_policy_spec.rb | 39 ++++++++++++++++++ spec/rails_helper.rb | 1 + ...upload_lettings_results_controller_spec.rb | 41 ++++++++++++++++++- 12 files changed, 203 insertions(+), 5 deletions(-) create mode 100644 app/policies/bulk_upload_policy.rb create mode 100644 spec/controllers/application_controller_spec.rb create mode 100644 spec/controllers/bulk_upload_sales_results_controller_spec.rb create mode 100644 spec/policies/bulk_upload_policy_spec.rb diff --git a/Gemfile b/Gemfile index f23a4db6f..5322a25b2 100644 --- a/Gemfile +++ b/Gemfile @@ -48,6 +48,9 @@ gem "aws-sdk-s3" gem "paper_trail" # Store active record objects in version whodunnits gem "paper_trail-globalid" + +gem "pundit" + # Request rate limiting gem "rack", ">= 2.2.6.3" gem "rack-attack" diff --git a/Gemfile.lock b/Gemfile.lock index 78fe1e157..98267f775 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -277,6 +277,8 @@ GEM public_suffix (5.0.1) puma (5.6.5) nio4r (~> 2.0) + pundit (2.3.0) + activesupport (>= 3.0.0) raabro (1.4.0) racc (1.6.2) rack (2.2.6.4) @@ -478,6 +480,7 @@ DEPENDENCIES propshaft pry-byebug puma (~> 5.0) + pundit rack (>= 2.2.6.3) rack-attack rack-mini-profiler (~> 2.0) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 5685796af..ff085e6dc 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1,10 +1,18 @@ class ApplicationController < ActionController::Base + include Pundit::Authorization + + rescue_from Pundit::NotAuthorizedError, with: :render_not_authorized + before_action :set_paper_trail_whodunnit def render_not_found render "errors/not_found", status: :not_found end + def render_not_authorized + render "errors/not_found", status: :unauthorized + end + def render_not_found_json(class_name, id) render json: { error: "#{class_name} #{id} not found" }, status: :not_found end diff --git a/app/controllers/bulk_upload_lettings_results_controller.rb b/app/controllers/bulk_upload_lettings_results_controller.rb index 6f92c0375..90008bcd2 100644 --- a/app/controllers/bulk_upload_lettings_results_controller.rb +++ b/app/controllers/bulk_upload_lettings_results_controller.rb @@ -4,7 +4,9 @@ class BulkUploadLettingsResultsController < ApplicationController rescue_from ActiveRecord::RecordNotFound, with: :render_not_found def show - @bulk_upload = current_user.bulk_uploads.lettings.find(params[:id]) + @bulk_upload = BulkUpload.lettings.find(params[:id]) + + authorize @bulk_upload end def resume @@ -20,7 +22,9 @@ class BulkUploadLettingsResultsController < ApplicationController end def summary - @bulk_upload = current_user.bulk_uploads.lettings.find(params[:id]) + @bulk_upload = BulkUpload.lettings.find(params[:id]) + + authorize @bulk_upload end private diff --git a/app/controllers/bulk_upload_sales_results_controller.rb b/app/controllers/bulk_upload_sales_results_controller.rb index c70907bbe..feb7b3e06 100644 --- a/app/controllers/bulk_upload_sales_results_controller.rb +++ b/app/controllers/bulk_upload_sales_results_controller.rb @@ -4,7 +4,9 @@ class BulkUploadSalesResultsController < ApplicationController rescue_from ActiveRecord::RecordNotFound, with: :render_not_found def show - @bulk_upload = current_user.bulk_uploads.sales.find(params[:id]) + @bulk_upload = BulkUpload.sales.find(params[:id]) + + authorize @bulk_upload end def resume @@ -20,9 +22,13 @@ class BulkUploadSalesResultsController < ApplicationController end def summary - @bulk_upload = current_user.bulk_uploads.sales.find(params[:id]) + @bulk_upload = BulkUpload.sales.find(params[:id]) + + authorize @bulk_upload end +private + def reset_logs_filters session["logs_filters"] = {}.to_json end diff --git a/app/policies/bulk_upload_policy.rb b/app/policies/bulk_upload_policy.rb new file mode 100644 index 000000000..8c609e1d8 --- /dev/null +++ b/app/policies/bulk_upload_policy.rb @@ -0,0 +1,26 @@ +class BulkUploadPolicy + attr_reader :user, :bulk_upload + + def initialize(user, bulk_upload) + @user = user + @bulk_upload = bulk_upload + end + + def summary? + owner? || same_org? || user.support? + end + + def show? + owner? || same_org? || user.support? + end + +private + + def owner? + bulk_upload.user == user + end + + def same_org? + bulk_upload.user.organisation.users.include?(user) + end +end diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb new file mode 100644 index 000000000..f120eb02a --- /dev/null +++ b/spec/controllers/application_controller_spec.rb @@ -0,0 +1,23 @@ +require "rails_helper" + +RSpec.describe ApplicationController do + describe "when Pundit::NotAuthorizedError raised" do + render_views + + controller do + def index + raise Pundit::NotAuthorizedError, "error goes here" + end + end + + it "returns status 401 unauthorized" do + get :index + expect(response).to be_unauthorized + end + + it "renders page not found" do + get :index + expect(response.body).to have_content("Page not found") + end + end +end diff --git a/spec/controllers/bulk_upload_lettings_results_controller_spec.rb b/spec/controllers/bulk_upload_lettings_results_controller_spec.rb index 7fb50a76a..c49511425 100644 --- a/spec/controllers/bulk_upload_lettings_results_controller_spec.rb +++ b/spec/controllers/bulk_upload_lettings_results_controller_spec.rb @@ -5,6 +5,19 @@ RSpec.describe BulkUploadLettingsResultsController do sign_in user end + describe "#show" do + let(:user) { create(:user) } + let(:bulk_upload) { create(:bulk_upload, :lettings, user:) } + + it "passes thru pundit" do + allow(controller).to receive(:authorize) + + get :show, params: { id: bulk_upload.id } + + expect(controller).to have_received(:authorize) + end + end + describe "GET #resume /lettings-logs/bulk-upload-results/:ID/resume" do let(:user) { create(:user) } let(:bulk_upload) { create(:bulk_upload, :lettings, user:) } diff --git a/spec/controllers/bulk_upload_sales_results_controller_spec.rb b/spec/controllers/bulk_upload_sales_results_controller_spec.rb new file mode 100644 index 000000000..e2cc46291 --- /dev/null +++ b/spec/controllers/bulk_upload_sales_results_controller_spec.rb @@ -0,0 +1,33 @@ +require "rails_helper" + +RSpec.describe BulkUploadSalesResultsController do + before do + sign_in user + end + + describe "#show" do + let(:user) { create(:user) } + let(:bulk_upload) { create(:bulk_upload, :sales, user:) } + + it "passes thru pundit" do + allow(controller).to receive(:authorize) + + get :show, params: { id: bulk_upload.id } + + expect(controller).to have_received(:authorize) + end + end + + describe "#summary" do + let(:user) { create(:user) } + let(:bulk_upload) { create(:bulk_upload, :sales, user:) } + + it "passes thru pundit" do + allow(controller).to receive(:authorize) + + get :summary, params: { id: bulk_upload.id } + + expect(controller).to have_received(:authorize) + end + end +end diff --git a/spec/policies/bulk_upload_policy_spec.rb b/spec/policies/bulk_upload_policy_spec.rb new file mode 100644 index 000000000..a99af5584 --- /dev/null +++ b/spec/policies/bulk_upload_policy_spec.rb @@ -0,0 +1,39 @@ +require "rails_helper" + +RSpec.describe BulkUploadPolicy do + subject(:policy) { described_class } + + permissions :summary?, :show? do + it "grants access to owner" do + user = build(:user) + bulk_upload = build(:bulk_upload, user:) + + expect(policy).to permit(user, bulk_upload) + end + + it "grants access to user from same org as uploader" do + user = create(:user) + organisation = user.organisation + other_user = create(:user, organisation:) + bulk_upload = create(:bulk_upload, user:) + + expect(policy).to permit(other_user, bulk_upload) + end + + it "grants access to support" do + user = create(:user) + support_user = create(:user, :support) + bulk_upload = create(:bulk_upload, user:) + + expect(policy).to permit(support_user, bulk_upload) + end + + it "denies access to random users" do + user = create(:user) + other_user = create(:user) + bulk_upload = create(:bulk_upload, user:) + + expect(policy).not_to permit(other_user, bulk_upload) + end + end +end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 8e175ccf6..36cf81b99 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -9,6 +9,7 @@ require "capybara/rspec" require "capybara-screenshot/rspec" require "selenium-webdriver" require "view_component/test_helpers" +require "pundit/rspec" Capybara.register_driver :headless do |app| options = Selenium::WebDriver::Firefox::Options.new diff --git a/spec/requests/bulk_upload_lettings_results_controller_spec.rb b/spec/requests/bulk_upload_lettings_results_controller_spec.rb index c15fef5b9..91d7a0742 100644 --- a/spec/requests/bulk_upload_lettings_results_controller_spec.rb +++ b/spec/requests/bulk_upload_lettings_results_controller_spec.rb @@ -2,11 +2,13 @@ require "rails_helper" RSpec.describe BulkUploadLettingsResultsController, type: :request do let(:user) { create(:user) } + let(:support_user) { create(:user, :support) } let(:bulk_upload) { create(:bulk_upload, :lettings, user:, bulk_upload_errors:) } let(:bulk_upload_errors) { create_list(:bulk_upload_error, 2) } + let(:viewing_user) { user } before do - sign_in user + sign_in viewing_user end describe "GET /lettings-logs/bulk-upload-results/:ID/summary" do @@ -22,6 +24,43 @@ RSpec.describe BulkUploadLettingsResultsController, type: :request do expect(response.body).to include(bulk_upload.filename) end + + context "when viewed by support user" do + before do + allow(support_user).to receive(:need_two_factor_authentication?).and_return(false) + end + + let(:viewing_user) { support_user } + + it "is accessible" do + get "/lettings-logs/bulk-upload-results/#{bulk_upload.id}/summary" + + expect(response).to be_successful + expect(response.body).to include(bulk_upload.filename) + end + end + + context "when viewed by some other random user" do + let(:other_user) { create(:user) } + let(:viewing_user) { other_user } + + it "is not accessible" do + get "/lettings-logs/bulk-upload-results/#{bulk_upload.id}/summary" + expect(response).to be_unauthorized + end + end + + context "when viewed by another user in the same org" do + let(:other_user) { create(:user, organisation: user.organisation) } + let(:viewing_user) { other_user } + + it "is accessible" do + get "/lettings-logs/bulk-upload-results/#{bulk_upload.id}/summary" + + expect(response).to be_successful + expect(response.body).to include(bulk_upload.filename) + end + end end describe "GET /lettings-logs/bulk-upload-results/:ID" do From d610a770248e708a049308ab7b3f6b889c53f3f0 Mon Sep 17 00:00:00 2001 From: James Rose Date: Wed, 3 May 2023 15:30:36 +0100 Subject: [PATCH 2/3] Bump sidekiq from 7.0.5 to 7.0.8 (#1601) --- Gemfile.lock | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 98267f775..3ebae16b2 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -135,7 +135,7 @@ GEM childprocess (4.1.0) coderay (1.1.3) concurrent-ruby (1.2.2) - connection_pool (2.3.0) + connection_pool (2.4.0) crack (0.4.5) rexml crass (1.0.6) @@ -281,7 +281,7 @@ GEM activesupport (>= 3.0.0) raabro (1.4.0) racc (1.6.2) - rack (2.2.6.4) + rack (2.2.7) rack-attack (6.6.1) rack (>= 1.0, < 3) rack-mini-profiler (2.3.4) @@ -322,7 +322,7 @@ GEM ffi (~> 1.0) redcarpet (3.6.0) redis (4.8.1) - redis-client (0.12.2) + redis-client (0.14.1) connection_pool regexp_parser (2.7.0) request_store (1.5.1) @@ -392,11 +392,11 @@ GEM sentry-ruby (~> 5.8.0) sentry-ruby (5.8.0) concurrent-ruby (~> 1.0, >= 1.0.2) - sidekiq (7.0.5) + sidekiq (7.1.0) concurrent-ruby (< 2) connection_pool (>= 2.3.0) rack (>= 2.2.4) - redis-client (>= 0.11.0) + redis-client (>= 0.14.0) sidekiq-cron (1.9.1) fugit (~> 1.8) sidekiq (>= 4.2.1) From 899c9995d92b39add18f5ca583c72d46b080d245 Mon Sep 17 00:00:00 2001 From: Arthur Campbell <51094020+arfacamble@users.noreply.github.com> Date: Thu, 4 May 2023 10:25:53 +0100 Subject: [PATCH 3/3] CLDC-2163 add validations on whether buyers are living in the property (#1512) * add several methods to the sales log to allow subsequent work to be human readable * add a validation to prevent an inconsistent combination of values and tests for this validation * handle derivation of values around buyers living in the property - derive values where appropriate - clear these values when the derived state no longer holds - update the routing for pages holding questions about whether particular buyers will live in the property to reflect when they are derived - test the deriving and associated clearing of values - update tests on page routing and sales log factory * update a page routing condition for human readability using an existing method and update test to reflect this change * show related method in diff * minor amendments after tech review * simplify reset_derived_questions after tech review * refactor on deriving and clearing invalid derived values on sales log * correct linter complaints * remove comment * add validation to one more field with a new error message as it is in fact possible to tirgger the validation in the setup section --- .../derived_variables/sales_log_variables.rb | 69 ++++++++++++++++++- .../sales/pages/buyer1_live_in_property.rb | 12 ++++ .../sales/pages/buyer2_live_in_property.rb | 14 +++- app/models/form/sales/pages/buyer_company.rb | 2 +- app/models/sales_log.rb | 26 +++++-- .../sales/household_validations.rb | 10 +++ config/locales/en.yml | 3 + .../pages/buyer1_live_in_property_spec.rb | 21 +++++- .../pages/buyer2_live_in_property_spec.rb | 14 +++- .../form/sales/pages/buyer_company_spec.rb | 4 +- spec/models/sales_log_spec.rb | 64 +++++++++++++++++ .../sales/household_validations_spec.rb | 57 +++++++++++++++ 12 files changed, 279 insertions(+), 17 deletions(-) diff --git a/app/models/derived_variables/sales_log_variables.rb b/app/models/derived_variables/sales_log_variables.rb index b4ff1a671..9151a72f5 100644 --- a/app/models/derived_variables/sales_log_variables.rb +++ b/app/models/derived_variables/sales_log_variables.rb @@ -1,5 +1,7 @@ module DerivedVariables::SalesLogVariables def set_derived_fields! + reset_invalidated_derived_values! + self.ethnic = 17 if ethnic_refused? self.mscharge = nil if no_monthly_leasehold_charges? if exdate.present? @@ -13,9 +15,6 @@ module DerivedVariables::SalesLogVariables self.hoyear = hodate.year end self.deposit = value if outright_sale? && mortgage_not_used? - if mortgage_not_used? - self.mortgage = 0 - end self.pcode1, self.pcode2 = postcode_full.split(" ") if postcode_full.present? self.totchild = total_child self.totadult = total_adult + total_elder @@ -30,10 +29,74 @@ module DerivedVariables::SalesLogVariables self.uprn = nil self.uprn_known = 0 end + + set_encoded_derived_values! end private + DEPENDENCIES = [ + { + conditions: { + buylivein: 2, + }, + derived_values: { + buy1livein: 2, + }, + }, + { + conditions: { + buylivein: 2, + jointpur: 1, + }, + derived_values: { + buy1livein: 2, + buy2livein: 2, + }, + }, + { + conditions: { + buylivein: 1, + jointpur: 2, + }, + derived_values: { + buy1livein: 1, + }, + }, + { + conditions: { + mortgageused: 2, + }, + derived_values: { + mortgage: 0, + }, + }, + ].freeze + + def reset_invalidated_derived_values! + DEPENDENCIES.each do |dependency| + any_conditions_changed = dependency[:conditions].any? { |attribute, _value| send("#{attribute}_changed?") } + next unless any_conditions_changed + + previously_in_derived_state = dependency[:conditions].all? { |attribute, value| send("#{attribute}_was") == value } + next unless previously_in_derived_state + + dependency[:derived_values].each do |derived_attribute, _derived_value| + Rails.logger.debug("Cleared derived #{derived_attribute} value") + send("#{derived_attribute}=", nil) + end + end + end + + def set_encoded_derived_values! + DEPENDENCIES.each do |dependency| + derivation_applies = dependency[:conditions].all? { |attribute, value| send(attribute) == value } + if derivation_applies + dependency[:derived_values].each { |attribute, value| send("#{attribute}=", value) } + end + end + end + def number_of_household_members return unless hholdcount.present? && jointpur.present? diff --git a/app/models/form/sales/pages/buyer1_live_in_property.rb b/app/models/form/sales/pages/buyer1_live_in_property.rb index 17877ee7b..8ee63d26c 100644 --- a/app/models/form/sales/pages/buyer1_live_in_property.rb +++ b/app/models/form/sales/pages/buyer1_live_in_property.rb @@ -5,9 +5,21 @@ class Form::Sales::Pages::Buyer1LiveInProperty < ::Form::Page @depends_on = [ { "buyer_has_seen_privacy_notice?" => true, + "outright_sale?" => false, }, { "buyer_not_interviewed?" => true, + "outright_sale?" => false, + }, + { + "buyer_has_seen_privacy_notice?" => true, + "joint_purchase?" => true, + "buyers_will_live_in?" => true, + }, + { + "buyer_not_interviewed?" => true, + "joint_purchase?" => true, + "buyers_will_live_in?" => true, }, ] end diff --git a/app/models/form/sales/pages/buyer2_live_in_property.rb b/app/models/form/sales/pages/buyer2_live_in_property.rb index 193f434f8..a16638c0d 100644 --- a/app/models/form/sales/pages/buyer2_live_in_property.rb +++ b/app/models/form/sales/pages/buyer2_live_in_property.rb @@ -4,12 +4,24 @@ class Form::Sales::Pages::Buyer2LiveInProperty < ::Form::Page @id = "buyer_2_live_in_property" @depends_on = [ { - "joint_purchase?" => true, "buyer_has_seen_privacy_notice?" => true, + "outright_sale?" => false, + "joint_purchase?" => true, + }, + { + "buyer_not_interviewed?" => true, + "outright_sale?" => false, + "joint_purchase?" => true, }, { + "buyer_has_seen_privacy_notice?" => true, "joint_purchase?" => true, + "buyers_will_live_in?" => true, + }, + { "buyer_not_interviewed?" => true, + "joint_purchase?" => true, + "buyers_will_live_in?" => true, }, ] end diff --git a/app/models/form/sales/pages/buyer_company.rb b/app/models/form/sales/pages/buyer_company.rb index b9f628f31..dbe1afcec 100644 --- a/app/models/form/sales/pages/buyer_company.rb +++ b/app/models/form/sales/pages/buyer_company.rb @@ -3,7 +3,7 @@ class Form::Sales::Pages::BuyerCompany < ::Form::Page super @id = "buyer_company" @depends_on = [{ - "ownershipsch" => 3, + "outright_sale?" => true, }] end diff --git a/app/models/sales_log.rb b/app/models/sales_log.rb index 06d1d0f5c..411977ba4 100644 --- a/app/models/sales_log.rb +++ b/app/models/sales_log.rb @@ -26,7 +26,6 @@ class SalesLog < Log validates_with SalesLogValidator before_validation :recalculate_start_year!, if: :saledate_changed? - before_validation :reset_invalidated_dependent_fields! before_validation :process_postcode_changes!, if: :postcode_full_changed? before_validation :process_previous_postcode_changes!, if: :ppostcode_full_changed? before_validation :reset_location_fields!, unless: :postcode_known? @@ -161,10 +160,26 @@ class SalesLog < Log inc1mort == 1 end + def buyers_will_live_in? + buylivein == 1 + end + + def buyers_will_not_live_in? + buylivein == 2 + end + def buyer_two_will_live_in_property? buy2livein == 1 end + def buyer_two_will_not_live_in_property? + buy2livein == 2 + end + + def buyer_one_will_not_live_in_property? + buy1livein == 2 + end + def buyer_two_not_already_living_in_property? buy2living == 2 end @@ -352,12 +367,9 @@ class SalesLog < Log def ownership_scheme case ownershipsch - when 1 - "shared ownership" - when 2 - "discounted ownership" - when 3 - "outright sale" + when 1 then "shared ownership" + when 2 then "discounted ownership" + when 3 then "outright sale" end end end diff --git a/app/models/validations/sales/household_validations.rb b/app/models/validations/sales/household_validations.rb index 5d4552eb3..a0202ce89 100644 --- a/app/models/validations/sales/household_validations.rb +++ b/app/models/validations/sales/household_validations.rb @@ -20,6 +20,16 @@ module Validations::Sales::HouseholdValidations end end + def validate_buyers_living_in_property(record) + return unless record.form.start_date.year >= 2023 + + if record.buyers_will_live_in? && record.buyer_one_will_not_live_in_property? && record.buyer_two_will_not_live_in_property? + record.errors.add :buylivein, I18n.t("validations.household.buylivein.buyers_will_live_in_property_values_inconsistent_setup") + record.errors.add :buy1livein, I18n.t("validations.household.buylivein.buyers_will_live_in_property_values_inconsistent") + record.errors.add :buy2livein, I18n.t("validations.household.buylivein.buyers_will_live_in_property_values_inconsistent") + end + end + private def validate_person_age_matches_relationship(record, person_num) diff --git a/config/locales/en.yml b/config/locales/en.yml index a798a258f..5a3189f03 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -442,6 +442,9 @@ en: no_choices: "You cannot answer this question as you told us nobody in the household has a physical or mental health condition (or other illness) expected to last 12 months or more" postcode: discounted_ownership: "Last settled accommodation and discounted ownership property postcodes must match" + buylivein: + buyers_will_live_in_property_values_inconsistent_setup: "You have already told us that both buyer 1 and buyer 2 will not live in the property" + buyers_will_live_in_property_values_inconsistent: "You have already told us that the buyers will live in the property. Either buyer 1 or buyer 2 must live in the property" tenancy: length: diff --git a/spec/models/form/sales/pages/buyer1_live_in_property_spec.rb b/spec/models/form/sales/pages/buyer1_live_in_property_spec.rb index 27a00d36a..b42dfeef0 100644 --- a/spec/models/form/sales/pages/buyer1_live_in_property_spec.rb +++ b/spec/models/form/sales/pages/buyer1_live_in_property_spec.rb @@ -28,6 +28,25 @@ RSpec.describe Form::Sales::Pages::Buyer1LiveInProperty, type: :model do end it "has correct depends_on" do - expect(page.depends_on).to eq([{ "buyer_has_seen_privacy_notice?" => true }, { "buyer_not_interviewed?" => true }]) + expect(page.depends_on).to eq([ + { + "buyer_has_seen_privacy_notice?" => true, + "outright_sale?" => false, + }, + { + "buyer_not_interviewed?" => true, + "outright_sale?" => false, + }, + { + "buyer_has_seen_privacy_notice?" => true, + "joint_purchase?" => true, + "buyers_will_live_in?" => true, + }, + { + "buyer_not_interviewed?" => true, + "joint_purchase?" => true, + "buyers_will_live_in?" => true, + }, + ]) end end diff --git a/spec/models/form/sales/pages/buyer2_live_in_property_spec.rb b/spec/models/form/sales/pages/buyer2_live_in_property_spec.rb index bb6ca4b13..bbb54613a 100644 --- a/spec/models/form/sales/pages/buyer2_live_in_property_spec.rb +++ b/spec/models/form/sales/pages/buyer2_live_in_property_spec.rb @@ -30,12 +30,24 @@ RSpec.describe Form::Sales::Pages::Buyer2LiveInProperty, type: :model do it "has correct depends_on" do expect(page.depends_on).to eq([ { - "joint_purchase?" => true, "buyer_has_seen_privacy_notice?" => true, + "outright_sale?" => false, + "joint_purchase?" => true, + }, + { + "buyer_not_interviewed?" => true, + "outright_sale?" => false, + "joint_purchase?" => true, }, { + "buyer_has_seen_privacy_notice?" => true, "joint_purchase?" => true, + "buyers_will_live_in?" => true, + }, + { "buyer_not_interviewed?" => true, + "joint_purchase?" => true, + "buyers_will_live_in?" => true, }, ]) end diff --git a/spec/models/form/sales/pages/buyer_company_spec.rb b/spec/models/form/sales/pages/buyer_company_spec.rb index 2551f8700..ca78d9204 100644 --- a/spec/models/form/sales/pages/buyer_company_spec.rb +++ b/spec/models/form/sales/pages/buyer_company_spec.rb @@ -28,8 +28,6 @@ RSpec.describe Form::Sales::Pages::BuyerCompany, type: :model do end it "has correct depends_on" do - expect(page.depends_on).to eq([{ - "ownershipsch" => 3, - }]) + expect(page.depends_on).to eq([{ "outright_sale?" => true }]) end end diff --git a/spec/models/sales_log_spec.rb b/spec/models/sales_log_spec.rb index 310fb48d1..bc0a96150 100644 --- a/spec/models/sales_log_spec.rb +++ b/spec/models/sales_log_spec.rb @@ -220,6 +220,70 @@ RSpec.describe SalesLog, type: :model do record_from_db = ActiveRecord::Base.connection.execute("select mortgage from sales_logs where id=#{sales_log.id}").to_a[0] expect(record_from_db["mortgage"]).to eq(0.0) end + + it "clears mortgage value if mortgage used is changed from no to yes" do + # to avoid log failing validations when mortgage value is removed: + new_grant_value = sales_log.grant + sales_log.mortgage + sales_log.update!(mortgageused: 2, grant: new_grant_value) + sales_log.update!(mortgageused: 1) + record_from_db = ActiveRecord::Base.connection.execute("select mortgage from sales_logs where id=#{sales_log.id}").to_a[0] + expect(record_from_db["mortgage"]).to eq(nil) + end + + context "when outright sale and buyers will live in the property" do + let(:sales_log) { create(:sales_log, :outright_sale_setup_complete, buylivein: 1, jointpur:) } + + context "and the sale is not a joint purchase" do + let(:jointpur) { 2 } + + it "derives that buyer 1 will live in the property" do + expect(sales_log.buy1livein).to be 1 + end + + it "does not derive a value for whether buyer 2 will live in the property" do + expect(sales_log.buy2livein).to be nil + end + + it "clears that buyer 1 will live in the property if joint purchase is updated" do + sales_log.update!(jointpur: 1) + expect(sales_log.buy1livein).to be nil + end + end + + context "and the sale is a joint purchase" do + let(:jointpur) { 1 } + + it "does not derive values for whether buyer 1 or buyer 2 will live in the property" do + expect(sales_log.buy1livein).to be nil + expect(sales_log.buy2livein).to be nil + end + end + end + + context "when outright sale and buyers will not live in the property" do + let(:sales_log) { create(:sales_log, :outright_sale_setup_complete, buylivein: 2, jointpur:) } + + context "and the sale is not a joint purchase" do + let(:jointpur) { 2 } + + it "derives that buyer 1 will not live in the property" do + expect(sales_log.buy1livein).to be 2 + end + + it "does not derive a value for whether buyer 2 will live in the property" do + expect(sales_log.buy2livein).to be nil + end + end + + context "and the sale is a joint purchase" do + let(:jointpur) { 1 } + + it "derives that neither buyer 1 nor buyer 2 will live in the property" do + expect(sales_log.buy1livein).to be 2 + expect(sales_log.buy2livein).to be 2 + end + end + end end context "when saving addresses" do diff --git a/spec/models/validations/sales/household_validations_spec.rb b/spec/models/validations/sales/household_validations_spec.rb index 953f27c1d..925ac1814 100644 --- a/spec/models/validations/sales/household_validations_spec.rb +++ b/spec/models/validations/sales/household_validations_spec.rb @@ -201,4 +201,61 @@ RSpec.describe Validations::Sales::HouseholdValidations do end end end + + describe "validating fields about buyers living in the property" do + let(:sales_log) { FactoryBot.create(:sales_log, :outright_sale_setup_complete, noint: 1, companybuy: 2, buylivein:, jointpur:, jointmore:, buy1livein:) } + + context "when buyers will live in the property and the sale is a joint purchase" do + let(:buylivein) { 1 } + let(:jointpur) { 1 } + let(:jointmore) { 2 } + + context "and buyer one will live in the property" do + let(:buy1livein) { 1 } + + it "does not add validations regardless of whether buyer two will live in the property" do + sales_log.buy2livein = 1 + household_validator.validate_buyers_living_in_property(sales_log) + expect(sales_log.errors).to be_empty + sales_log.buy2livein = 2 + household_validator.validate_buyers_living_in_property(sales_log) + expect(sales_log.errors).to be_empty + end + end + + context "and buyer one will not live in the property" do + let(:buy1livein) { 2 } + + it "does not add validations if buyer two will live in the property or if we do not yet know" do + sales_log.buy2livein = 1 + household_validator.validate_buyers_living_in_property(sales_log) + expect(sales_log.errors).to be_empty + sales_log.buy2livein = nil + household_validator.validate_buyers_living_in_property(sales_log) + expect(sales_log.errors).to be_empty + end + + it "triggers a validation if buyer two will also not live in the property" do + sales_log.buy2livein = 2 + household_validator.validate_buyers_living_in_property(sales_log) + expect(sales_log.errors[:buylivein]).to include I18n.t("validations.household.buylivein.buyers_will_live_in_property_values_inconsistent_setup") + expect(sales_log.errors[:buy2livein]).to include I18n.t("validations.household.buylivein.buyers_will_live_in_property_values_inconsistent") + expect(sales_log.errors[:buy1livein]).to include I18n.t("validations.household.buylivein.buyers_will_live_in_property_values_inconsistent") + end + end + + context "and we don't know whether buyer one will live in the property" do + let(:buy1livein) { nil } + + it "does not add validations regardless of whether buyer two will live in the property" do + sales_log.buy2livein = 1 + household_validator.validate_buyers_living_in_property(sales_log) + expect(sales_log.errors).to be_empty + sales_log.buy2livein = 2 + household_validator.validate_buyers_living_in_property(sales_log) + expect(sales_log.errors).to be_empty + end + end + end + end end