From 297ba50e78f610ae5e9333cd553f8b60503a85f9 Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Wed, 14 Dec 2022 12:43:35 +0000 Subject: [PATCH] Validate the user belongs to either the managing or owning organisation (#1055) * Validate that the user belongs to either the managing or owning organisation * do not reset created_by and remove user_organisation_chosen? * Do not default managing organisation to owning organisation * validate user belongs to organisation * update tests to specify created_by * clear create_by for support users * refactor * typo * rebase lint * Add `updated_by` to logs to track who was the last person to update a log - Reset `created_by` automatically when a form is updated and the owner does not belong to the managing or owning organisation * move reset_invalidated_dependent_fields!, update schema file and fix tests Co-authored-by: James Rose --- app/controllers/form_controller.rb | 2 +- .../lettings_log_variables.rb | 2 - app/models/lettings_log.rb | 1 - app/models/log.rb | 17 +- app/models/validations/setup_validations.rb | 9 + app/services/csv/lettings_log_csv_service.rb | 2 +- config/locales/en.yml | 6 + .../20221207141947_add_updated_by_to_logs.rb | 10 + db/schema.rb | 4 + spec/features/form/check_answers_page_spec.rb | 18 +- spec/features/form/checkboxes_spec.rb | 3 +- .../form/conditional_questions_spec.rb | 6 +- spec/features/form/page_routing_spec.rb | 4 +- .../form/progressive_total_field_spec.rb | 3 +- spec/features/form/saving_data_spec.rb | 6 +- spec/features/form/validations_spec.rb | 12 +- spec/features/organisation_spec.rb | 4 +- .../fixtures/files/lettings_logs_download.csv | 4 +- .../interruption_screen_helper_spec.rb | 3 +- spec/jobs/email_csv_job_spec.rb | 5 +- .../lettings/questions/created_by_id_spec.rb | 1 + spec/models/lettings_log_spec.rb | 15 +- spec/models/organisation_spec.rb | 4 +- spec/models/user_spec.rb | 3 +- .../validations/setup_validations_spec.rb | 60 ++++++ spec/requests/form_controller_spec.rb | 185 ++++++++++++++++-- .../requests/lettings_logs_controller_spec.rb | 28 ++- .../requests/organisations_controller_spec.rb | 4 +- .../csv/lettings_log_csv_service_spec.rb | 1 + 29 files changed, 315 insertions(+), 107 deletions(-) create mode 100644 db/migrate/20221207141947_add_updated_by_to_logs.rb diff --git a/app/controllers/form_controller.rb b/app/controllers/form_controller.rb index 19d47f8e3..b0468c875 100644 --- a/app/controllers/form_controller.rb +++ b/app/controllers/form_controller.rb @@ -10,7 +10,7 @@ class FormController < ApplicationController responses_for_page = responses_for_page(@page) mandatory_questions_with_no_response = mandatory_questions_with_no_response(responses_for_page) - if mandatory_questions_with_no_response.empty? && @log.update(responses_for_page) + if mandatory_questions_with_no_response.empty? && @log.update(responses_for_page.merge(updated_by: current_user)) session[:errors] = session[:fields] = nil redirect_to(successful_redirect_path) else diff --git a/app/models/derived_variables/lettings_log_variables.rb b/app/models/derived_variables/lettings_log_variables.rb index 1b1817ec3..96b937ab4 100644 --- a/app/models/derived_variables/lettings_log_variables.rb +++ b/app/models/derived_variables/lettings_log_variables.rb @@ -9,8 +9,6 @@ module DerivedVariables::LettingsLogVariables end def set_derived_fields! - # TODO: Remove once we support parent/child relationships - self.managing_organisation_id ||= owning_organisation_id if rsnvac.present? self.newprop = has_first_let_vacancy_reason? ? 1 : 2 end diff --git a/app/models/lettings_log.rb b/app/models/lettings_log.rb index 46d8b0759..14d0ead2c 100644 --- a/app/models/lettings_log.rb +++ b/app/models/lettings_log.rb @@ -570,7 +570,6 @@ private super reset_invalid_unresolved_log_fields! - reset_created_by reset_scheme reset_derived_questions end diff --git a/app/models/log.rb b/app/models/log.rb index e3a85e9ee..c09cdd46a 100644 --- a/app/models/log.rb +++ b/app/models/log.rb @@ -4,6 +4,7 @@ class Log < ApplicationRecord belongs_to :owning_organisation, class_name: "Organisation", optional: true belongs_to :managing_organisation, class_name: "Organisation", optional: true belongs_to :created_by, class_name: "User", optional: true + belongs_to :updated_by, class_name: "User", optional: true before_save :update_status! STATUS = { "not_started" => 0, "in_progress" => 1, "completed" => 2 }.freeze @@ -72,15 +73,19 @@ private subsection_statuses.all? { |status| not_started_statuses.include?(status) } end - def reset_created_by - return unless created_by && owning_organisation - - self.created_by = nil if created_by.organisation != owning_organisation - end - def reset_invalidated_dependent_fields! return unless form form.reset_not_routed_questions(self) + + reset_created_by! + end + + def reset_created_by! + return unless updated_by&.support? + return if owning_organisation.blank? || managing_organisation.blank? || created_by.blank? + return if created_by&.organisation == managing_organisation || created_by&.organisation == owning_organisation + + update!(created_by: nil) end end diff --git a/app/models/validations/setup_validations.rb b/app/models/validations/setup_validations.rb index c1bdc3a71..93105f9ea 100644 --- a/app/models/validations/setup_validations.rb +++ b/app/models/validations/setup_validations.rb @@ -16,6 +16,15 @@ module Validations::SetupValidations scheme_during_startdate_validation(record, :scheme_id) end + def validate_organisation(record) + created_by, managing_organisation, owning_organisation = record.values_at("created_by", "managing_organisation", "owning_organisation") + unless [created_by, managing_organisation, owning_organisation].any?(&:blank?) || created_by.organisation == managing_organisation || created_by.organisation == owning_organisation + record.errors.add :created_by, I18n.t("validations.setup.created_by.invalid") + record.errors.add :owning_organisation_id, I18n.t("validations.setup.owning_organisation.invalid") + record.errors.add :managing_organisation_id, I18n.t("validations.setup.managing_organisation.invalid") + end + end + private def intermediate_product_rent_type?(record) diff --git a/app/services/csv/lettings_log_csv_service.rb b/app/services/csv/lettings_log_csv_service.rb index 67078fb03..d084bfaa1 100644 --- a/app/services/csv/lettings_log_csv_service.rb +++ b/app/services/csv/lettings_log_csv_service.rb @@ -1,6 +1,6 @@ module Csv class LettingsLogCsvService - CSV_FIELDS_TO_OMIT = %w[hhmemb net_income_value_check first_time_property_let_as_social_housing renttype needstype postcode_known is_la_inferred totchild totelder totadult net_income_known is_carehome previous_la_known is_previous_la_inferred age1_known age2_known age3_known age4_known age5_known age6_known age7_known age8_known letting_allocation_unknown details_known_2 details_known_3 details_known_4 details_known_5 details_known_6 details_known_7 details_known_8 rent_type_detail wrent wscharge wpschrge wsupchrg wtcharge wtshortfall rent_value_check old_form_id old_id retirement_value_check tshortfall_known pregnancy_value_check hhtype new_old vacdays la prevloc unresolved].freeze + CSV_FIELDS_TO_OMIT = %w[hhmemb net_income_value_check first_time_property_let_as_social_housing renttype needstype postcode_known is_la_inferred totchild totelder totadult net_income_known is_carehome previous_la_known is_previous_la_inferred age1_known age2_known age3_known age4_known age5_known age6_known age7_known age8_known letting_allocation_unknown details_known_2 details_known_3 details_known_4 details_known_5 details_known_6 details_known_7 details_known_8 rent_type_detail wrent wscharge wpschrge wsupchrg wtcharge wtshortfall rent_value_check old_form_id old_id retirement_value_check tshortfall_known pregnancy_value_check hhtype new_old vacdays la prevloc unresolved updated_by_id].freeze def initialize(user) @user = user diff --git a/config/locales/en.yml b/config/locales/en.yml index 6215ef93c..ba9ffac32 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -157,6 +157,12 @@ en: deactivated: "%{name} was deactivated on %{date} and was not available on the day you entered" reactivating_soon: "The scheme %{name} is not available until %{date}. Select another scheme or edit the tenancy start date" activating_soon: "%{name} is not available until %{date}. Enter a tenancy start date after %{date}" + owning_organisation: + invalid: "Please select owning organisation or managing organisation that you belong to" + managing_organisation: + invalid: "Please select owning organisation or managing organisation that you belong to" + created_by: + invalid: "Please select owning organisation or managing organisation that you belong to" property: mrcdate: diff --git a/db/migrate/20221207141947_add_updated_by_to_logs.rb b/db/migrate/20221207141947_add_updated_by_to_logs.rb new file mode 100644 index 000000000..9bc80c526 --- /dev/null +++ b/db/migrate/20221207141947_add_updated_by_to_logs.rb @@ -0,0 +1,10 @@ +class AddUpdatedByToLogs < ActiveRecord::Migration[7.0] + def change + change_table :lettings_logs, bulk: true do |t| + t.belongs_to :updated_by, class_name: "User" + end + change_table :sales_logs, bulk: true do |t| + t.belongs_to :updated_by, class_name: "User" + end + end +end diff --git a/db/schema.rb b/db/schema.rb index ac78ab8fc..e32a01805 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -250,12 +250,14 @@ ActiveRecord::Schema[7.0].define(version: 2022_12_13_085819) do t.integer "housingneeds_type" t.integer "housingneeds_other" t.boolean "unresolved" + t.bigint "updated_by_id" t.index ["created_by_id"], name: "index_lettings_logs_on_created_by_id" t.index ["location_id"], name: "index_lettings_logs_on_location_id" t.index ["managing_organisation_id"], name: "index_lettings_logs_on_managing_organisation_id" t.index ["old_id"], name: "index_lettings_logs_on_old_id", unique: true t.index ["owning_organisation_id"], name: "index_lettings_logs_on_owning_organisation_id" t.index ["scheme_id"], name: "index_lettings_logs_on_scheme_id" + t.index ["updated_by_id"], name: "index_lettings_logs_on_updated_by_id" end create_table "location_deactivation_periods", force: :cascade do |t| @@ -403,6 +405,7 @@ ActiveRecord::Schema[7.0].define(version: 2022_12_13_085819) do t.integer "savings" t.integer "prevown" t.string "sex3" + t.bigint "updated_by_id" t.integer "details_known_1" t.integer "income1_value_check" t.integer "mortgage" @@ -411,6 +414,7 @@ ActiveRecord::Schema[7.0].define(version: 2022_12_13_085819) do t.index ["created_by_id"], name: "index_sales_logs_on_created_by_id" t.index ["managing_organisation_id"], name: "index_sales_logs_on_managing_organisation_id" t.index ["owning_organisation_id"], name: "index_sales_logs_on_owning_organisation_id" + t.index ["updated_by_id"], name: "index_sales_logs_on_updated_by_id" end create_table "scheme_deactivation_periods", force: :cascade do |t| diff --git a/spec/features/form/check_answers_page_spec.rb b/spec/features/form/check_answers_page_spec.rb index 4ea6a905f..7834e0dad 100644 --- a/spec/features/form/check_answers_page_spec.rb +++ b/spec/features/form/check_answers_page_spec.rb @@ -13,8 +13,7 @@ RSpec.describe "Form Check Answers Page" do FactoryBot.create( :lettings_log, :in_progress, - owning_organisation: user.organisation, - managing_organisation: user.organisation, + created_by: user, needstype: 2, scheme:, location:, @@ -26,8 +25,7 @@ RSpec.describe "Form Check Answers Page" do previous_la_known: 1, prevloc: "E09000033", is_previous_la_inferred: false, - owning_organisation: user.organisation, - managing_organisation: user.organisation, + created_by: user, ) end let(:completed_lettings_log) do @@ -36,6 +34,7 @@ RSpec.describe "Form Check Answers Page" do :completed, owning_organisation: user.organisation, managing_organisation: user.organisation, + created_by: user, startdate: Time.zone.local(2021, 5, 1), ) end @@ -190,8 +189,7 @@ RSpec.describe "Form Check Answers Page" do FactoryBot.create( :lettings_log, :in_progress, - owning_organisation: user.organisation, - managing_organisation: user.organisation, + created_by: user, tenancycode: "123", age1: 35, sex1: "M", @@ -203,8 +201,7 @@ RSpec.describe "Form Check Answers Page" do FactoryBot.create( :lettings_log, :in_progress, - owning_organisation: user.organisation, - managing_organisation: user.organisation, + created_by: user, tenancycode: "123", age1: 35, sex1: "M", @@ -218,8 +215,7 @@ RSpec.describe "Form Check Answers Page" do FactoryBot.create( :lettings_log, :in_progress, - owning_organisation: user.organisation, - managing_organisation: user.organisation, + created_by: user, tenancycode: "123", age1: 35, sex1: "M", @@ -236,8 +232,6 @@ RSpec.describe "Form Check Answers Page" do FactoryBot.create( :lettings_log, :in_progress, - owning_organisation: user.organisation, - managing_organisation: user.organisation, created_by: user, needstype: 1, tenancycode: nil, diff --git a/spec/features/form/checkboxes_spec.rb b/spec/features/form/checkboxes_spec.rb index d3e3671eb..8f3f4ad65 100644 --- a/spec/features/form/checkboxes_spec.rb +++ b/spec/features/form/checkboxes_spec.rb @@ -9,8 +9,7 @@ RSpec.describe "Checkboxes" do FactoryBot.create( :lettings_log, :in_progress, - owning_organisation: user.organisation, - managing_organisation: user.organisation, + created_by: user, ) end let(:id) { lettings_log.id } diff --git a/spec/features/form/conditional_questions_spec.rb b/spec/features/form/conditional_questions_spec.rb index 07a660321..4fc9387da 100644 --- a/spec/features/form/conditional_questions_spec.rb +++ b/spec/features/form/conditional_questions_spec.rb @@ -8,16 +8,14 @@ RSpec.describe "Form Conditional Questions" do FactoryBot.create( :lettings_log, :in_progress, - owning_organisation: user.organisation, - managing_organisation: user.organisation, + created_by: user, ) end let(:sales_log) do FactoryBot.create( :sales_log, :completed, - owning_organisation: user.organisation, - managing_organisation: user.organisation, + created_by: user, ) end let(:id) { lettings_log.id } diff --git a/spec/features/form/page_routing_spec.rb b/spec/features/form/page_routing_spec.rb index b60c2dd03..eb4f6acc2 100644 --- a/spec/features/form/page_routing_spec.rb +++ b/spec/features/form/page_routing_spec.rb @@ -8,8 +8,7 @@ RSpec.describe "Form Page Routing" do FactoryBot.create( :lettings_log, :in_progress, - owning_organisation: user.organisation, - managing_organisation: user.organisation, + created_by: user, ) end let(:id) { lettings_log.id } @@ -120,6 +119,7 @@ RSpec.describe "Form Page Routing" do :lettings_log, owning_organisation: user.organisation, managing_organisation: user.organisation, + created_by: user, needstype: 2, ) end diff --git a/spec/features/form/progressive_total_field_spec.rb b/spec/features/form/progressive_total_field_spec.rb index 77edfbedd..e5959d16d 100644 --- a/spec/features/form/progressive_total_field_spec.rb +++ b/spec/features/form/progressive_total_field_spec.rb @@ -8,8 +8,7 @@ RSpec.describe "Accessible Automcomplete" do FactoryBot.create( :lettings_log, :in_progress, - owning_organisation: user.organisation, - managing_organisation: user.organisation, + created_by: user, ) end diff --git a/spec/features/form/saving_data_spec.rb b/spec/features/form/saving_data_spec.rb index a0889e9a4..7a7996457 100644 --- a/spec/features/form/saving_data_spec.rb +++ b/spec/features/form/saving_data_spec.rb @@ -8,8 +8,7 @@ RSpec.describe "Form Saving Data" do FactoryBot.create( :lettings_log, :in_progress, - owning_organisation: user.organisation, - managing_organisation: user.organisation, + created_by: user, ) end let(:id) { lettings_log.id } @@ -17,8 +16,7 @@ RSpec.describe "Form Saving Data" do FactoryBot.create( :lettings_log, :in_progress, housingneeds_a: 1, - owning_organisation: user.organisation, - managing_organisation: user.organisation + created_by: user ) end let(:question_answers) do diff --git a/spec/features/form/validations_spec.rb b/spec/features/form/validations_spec.rb index 9df179937..24b2816ce 100644 --- a/spec/features/form/validations_spec.rb +++ b/spec/features/form/validations_spec.rb @@ -8,24 +8,21 @@ RSpec.describe "validations" do FactoryBot.create( :lettings_log, :in_progress, - owning_organisation: user.organisation, - managing_organisation: user.organisation, + created_by: user, renewal: 0, ) end let(:empty_lettings_log) do FactoryBot.create( :lettings_log, - owning_organisation: user.organisation, - managing_organisation: user.organisation, + created_by: user, ) end let(:completed_without_declaration) do FactoryBot.create( :lettings_log, :completed, - owning_organisation: user.organisation, - managing_organisation: user.organisation, + created_by: user, status: 1, declaration: nil, startdate: Time.zone.local(2021, 5, 1), @@ -124,8 +121,7 @@ RSpec.describe "validations" do :lettings_log, :in_progress, ecstat1: 1, - owning_organisation: user.organisation, - managing_organisation: user.organisation, + created_by: user, ) end let(:income_over_soft_limit) { 750 } diff --git a/spec/features/organisation_spec.rb b/spec/features/organisation_spec.rb index 09d6130e4..9d2478d9f 100644 --- a/spec/features/organisation_spec.rb +++ b/spec/features/organisation_spec.rb @@ -136,8 +136,8 @@ RSpec.describe "User Features" do context "when viewing lettings logs for specific organisation" do let(:first_log) { organisation.lettings_logs.first } - let!(:log_to_search) { FactoryBot.create(:lettings_log, owning_organisation: user.organisation, managing_organisation_id: organisation.id) } - let!(:other_logs) { FactoryBot.create_list(:lettings_log, 4, owning_organisation_id: organisation.id, managing_organisation_id: organisation.id) } + let!(:log_to_search) { FactoryBot.create(:lettings_log, created_by: user) } + let!(:other_logs) { FactoryBot.create_list(:lettings_log, 4, created_by: user) } let(:number_of_lettings_logs) { LettingsLog.count } before do diff --git a/spec/fixtures/files/lettings_logs_download.csv b/spec/fixtures/files/lettings_logs_download.csv index 26ee815d1..7600f0aad 100644 --- a/spec/fixtures/files/lettings_logs_download.csv +++ b/spec/fixtures/files/lettings_logs_download.csv @@ -1,2 +1,2 @@ -id,status,created_at,updated_at,created_by_name,is_dpo,owning_organisation_name,managing_organisation_name,collection_start_year,needstype,renewal,startdate,rent_type_detail,irproduct_other,tenancycode,propcode,age1,sex1,ecstat1,hhmemb,relat2,age2,sex2,retirement_value_check,ecstat2,armedforces,leftreg,illness,housingneeds_a,housingneeds_b,housingneeds_c,housingneeds_h,is_previous_la_inferred,prevloc_label,prevloc,illness_type_1,illness_type_2,is_la_inferred,la_label,la,postcode_known,postcode_full,previous_la_known,wchair,preg_occ,cbl,earnings,incfreq,net_income_value_check,benefits,hb,period,brent,scharge,pscharge,supcharg,tcharge,offered,layear,ppostcode_full,mrcdate,declaration,ethnic,national,prevten,age3,sex3,ecstat3,age4,sex4,ecstat4,age5,sex5,ecstat5,age6,sex6,ecstat6,age7,sex7,ecstat7,age8,sex8,ecstat8,homeless,underoccupation_benefitcap,reservist,startertenancy,tenancylength,tenancy,rsnvac,unittype_gn,beds,waityear,reasonpref,chr,cap,reasonother,housingneeds_f,housingneeds_g,illness_type_3,illness_type_4,illness_type_8,illness_type_5,illness_type_6,illness_type_7,illness_type_9,illness_type_10,rp_homeless,rp_insan_unsat,rp_medwel,rp_hardship,rp_dontknow,tenancyother,property_owner_organisation,property_manager_organisation,purchaser_code,reason,majorrepairs,hbrentshortfall,property_relet,incref,first_time_property_let_as_social_housing,unitletas,builtype,voiddate,renttype,lettype,totchild,totelder,totadult,net_income_known,nocharge,is_carehome,household_charge,referral,tshortfall,chcharge,ppcodenk,age1_known,age2_known,age3_known,age4_known,age5_known,age6_known,age7_known,age8_known,ethnic_group,letting_allocation_unknown,details_known_2,details_known_3,details_known_4,details_known_5,details_known_6,details_known_7,details_known_8,has_benefits,wrent,wscharge,wpschrge,wsupchrg,wtcharge,wtshortfall,refused,housingneeds,wchchrg,newprop,relat3,relat4,relat5,relat6,relat7,relat8,rent_value_check,old_form_id,lar,irproduct,old_id,joint,illness_type_0,tshortfall_known,sheltered,pregnancy_value_check,hhtype,new_old,vacdays,major_repairs_date_value_check,void_date_value_check,housingneeds_type,housingneeds_other,unresolved,unittype_sh,scheme_code,scheme_service_name,scheme_sensitive,scheme_type,scheme_registered_under_care_act,scheme_owning_organisation_name,scheme_managing_organisation_name,scheme_primary_client_group,scheme_has_other_client_group,scheme_secondary_client_group,scheme_support_type,scheme_intended_stay,scheme_created_at,location_code,location_postcode,location_name,location_units,location_type_of_unit,location_mobility_type,location_admin_district,location_startdate -{id},in_progress,2022-02-08 16:52:15 +0000,2022-02-08 16:52:15 +0000,Danny Rojas,No,DLUHC,DLUHC,2021,Supported housing,,2 October 2021,London Affordable Rent,,,,,,,,,,,,,,,,,,,,No,,,,,No,Westminster,E09000033,,SE1 1TE,,2,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,2,8,0,0,0,,0,,,,,,,,,,,,,,,,,,,,,,,,0,,,,,,,0,,,,,,,,,,,,,,,,,,,,9,1,,,,,,,6,{scheme_code},{scheme_service_name},{scheme_sensitive},Missing,No,DLUHC,DLUHC,{scheme_primary_client_group},,{scheme_secondary_client_group},{scheme_support_type},{scheme_intended_stay},2021-04-01 00:00:00 +0100,{location_code},SE1 1TE,Downing Street,20,Bungalow,Fitted with equipment and adaptations,Westminster,{location_startdate} +id,status,created_at,updated_at,created_by_name,is_dpo,owning_organisation_name,managing_organisation_name,collection_start_year,needstype,renewal,startdate,rent_type_detail,irproduct_other,tenancycode,propcode,age1,sex1,ecstat1,hhmemb,relat2,age2,sex2,retirement_value_check,ecstat2,armedforces,leftreg,illness,housingneeds_a,housingneeds_b,housingneeds_c,housingneeds_h,is_previous_la_inferred,prevloc_label,prevloc,illness_type_1,illness_type_2,is_la_inferred,la_label,la,postcode_known,postcode_full,previous_la_known,wchair,preg_occ,cbl,earnings,incfreq,net_income_value_check,benefits,hb,period,brent,scharge,pscharge,supcharg,tcharge,offered,layear,ppostcode_full,mrcdate,declaration,ethnic,national,prevten,age3,sex3,ecstat3,age4,sex4,ecstat4,age5,sex5,ecstat5,age6,sex6,ecstat6,age7,sex7,ecstat7,age8,sex8,ecstat8,homeless,underoccupation_benefitcap,reservist,startertenancy,tenancylength,tenancy,rsnvac,unittype_gn,beds,waityear,reasonpref,chr,cap,reasonother,housingneeds_f,housingneeds_g,illness_type_3,illness_type_4,illness_type_8,illness_type_5,illness_type_6,illness_type_7,illness_type_9,illness_type_10,rp_homeless,rp_insan_unsat,rp_medwel,rp_hardship,rp_dontknow,tenancyother,property_owner_organisation,property_manager_organisation,purchaser_code,reason,majorrepairs,hbrentshortfall,property_relet,incref,first_time_property_let_as_social_housing,unitletas,builtype,voiddate,renttype,lettype,totchild,totelder,totadult,net_income_known,nocharge,is_carehome,household_charge,referral,tshortfall,chcharge,ppcodenk,age1_known,age2_known,age3_known,age4_known,age5_known,age6_known,age7_known,age8_known,ethnic_group,letting_allocation_unknown,details_known_2,details_known_3,details_known_4,details_known_5,details_known_6,details_known_7,details_known_8,has_benefits,wrent,wscharge,wpschrge,wsupchrg,wtcharge,wtshortfall,refused,housingneeds,wchchrg,newprop,relat3,relat4,relat5,relat6,relat7,relat8,rent_value_check,old_form_id,lar,irproduct,old_id,joint,illness_type_0,tshortfall_known,sheltered,pregnancy_value_check,hhtype,new_old,vacdays,major_repairs_date_value_check,void_date_value_check,housingneeds_type,housingneeds_other,unresolved,updated_by_id,unittype_sh,scheme_code,scheme_service_name,scheme_sensitive,scheme_type,scheme_registered_under_care_act,scheme_owning_organisation_name,scheme_managing_organisation_name,scheme_primary_client_group,scheme_has_other_client_group,scheme_secondary_client_group,scheme_support_type,scheme_intended_stay,scheme_created_at,location_code,location_postcode,location_name,location_units,location_type_of_unit,location_mobility_type,location_admin_district,location_startdate +{id},in_progress,2022-02-08 16:52:15 +0000,2022-02-08 16:52:15 +0000,Danny Rojas,No,DLUHC,DLUHC,2021,Supported housing,,2 October 2021,London Affordable Rent,,,,,,,,,,,,,,,,,,,,No,,,,,No,Westminster,E09000033,,SE1 1TE,,2,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,2,8,0,0,0,,0,,,,,,,,,,,,,,,,,,,,,,,,0,,,,,,,0,,,,,,,,,,,,,,,,,,,,9,1,,,,,,,,6,{scheme_code},{scheme_service_name},{scheme_sensitive},Missing,No,DLUHC,DLUHC,{scheme_primary_client_group},,{scheme_secondary_client_group},{scheme_support_type},{scheme_intended_stay},2021-04-01 00:00:00 +0100,{location_code},SE1 1TE,Downing Street,20,Bungalow,Fitted with equipment and adaptations,Westminster,{location_startdate} diff --git a/spec/helpers/interruption_screen_helper_spec.rb b/spec/helpers/interruption_screen_helper_spec.rb index 4984a0450..8153b9d7b 100644 --- a/spec/helpers/interruption_screen_helper_spec.rb +++ b/spec/helpers/interruption_screen_helper_spec.rb @@ -12,8 +12,7 @@ RSpec.describe InterruptionScreenHelper do ecstat1: 1, earnings: 750, incfreq: 1, - owning_organisation: user.organisation, - managing_organisation: user.organisation, + created_by: user, ) end diff --git a/spec/jobs/email_csv_job_spec.rb b/spec/jobs/email_csv_job_spec.rb index f4939b855..5ff0b3355 100644 --- a/spec/jobs/email_csv_job_spec.rb +++ b/spec/jobs/email_csv_job_spec.rb @@ -19,8 +19,7 @@ describe EmailCsvJob do let!(:lettings_log) do FactoryBot.create( :lettings_log, - owning_organisation: organisation, - managing_organisation: organisation, + created_by: user, ecstat1: 1, ) end @@ -31,8 +30,6 @@ describe EmailCsvJob do before do FactoryBot.create(:lettings_log, :completed, - owning_organisation: organisation, - managing_organisation: organisation, created_by: user, startdate: Time.zone.local(2021, 5, 1)) diff --git a/spec/models/form/lettings/questions/created_by_id_spec.rb b/spec/models/form/lettings/questions/created_by_id_spec.rb index 84ee4156a..b1ee1de49 100644 --- a/spec/models/form/lettings/questions/created_by_id_spec.rb +++ b/spec/models/form/lettings/questions/created_by_id_spec.rb @@ -71,6 +71,7 @@ RSpec.describe Form::Lettings::Questions::CreatedById, type: :model do let(:lettings_log) do create( :lettings_log, + created_by: user_2, owning_organisation: user_2.organisation, managing_organisation: user_3.organisation, ) diff --git a/spec/models/lettings_log_spec.rb b/spec/models/lettings_log_spec.rb index a4704a12c..67db22f46 100644 --- a/spec/models/lettings_log_spec.rb +++ b/spec/models/lettings_log_spec.rb @@ -2,9 +2,9 @@ require "rails_helper" require "shared/shared_examples_for_derived_fields" RSpec.describe LettingsLog do - let(:owning_organisation) { FactoryBot.create(:organisation) } let(:different_managing_organisation) { FactoryBot.create(:organisation) } let(:created_by_user) { FactoryBot.create(:user) } + let(:owning_organisation) { created_by_user.organisation } let(:fake_2021_2022_form) { Form.new("spec/fixtures/forms/2021_2022.json") } before do @@ -2013,11 +2013,6 @@ RSpec.describe LettingsLog do let(:lettings_log) { FactoryBot.create(:lettings_log, created_by: created_by_user) } let(:organisation_2) { FactoryBot.create(:organisation) } - it "clears the created by user set" do - expect { lettings_log.update!(owning_organisation: organisation_2) } - .to change { lettings_log.reload.created_by }.from(created_by_user).to(nil) - end - context "when the organisation selected doesn't match the scheme set" do let(:scheme) { FactoryBot.create(:scheme, owning_organisation: created_by_user.organisation) } let(:location) { FactoryBot.create(:location, scheme:) } @@ -2377,10 +2372,10 @@ RSpec.describe LettingsLog do let(:organisation_3) { FactoryBot.create(:organisation) } before do - FactoryBot.create(:lettings_log, :in_progress, owning_organisation: organisation_1, managing_organisation: organisation_1) - FactoryBot.create(:lettings_log, :completed, owning_organisation: organisation_1, managing_organisation: organisation_2) - FactoryBot.create(:lettings_log, :completed, owning_organisation: organisation_2, managing_organisation: organisation_1) - FactoryBot.create(:lettings_log, :completed, owning_organisation: organisation_2, managing_organisation: organisation_2) + FactoryBot.create(:lettings_log, :in_progress, owning_organisation: organisation_1, managing_organisation: organisation_1, created_by: nil) + FactoryBot.create(:lettings_log, :completed, owning_organisation: organisation_1, managing_organisation: organisation_2, created_by: nil) + FactoryBot.create(:lettings_log, :completed, owning_organisation: organisation_2, managing_organisation: organisation_1, created_by: nil) + FactoryBot.create(:lettings_log, :completed, owning_organisation: organisation_2, managing_organisation: organisation_2, created_by: nil) end it "filters by given organisation id" do diff --git a/spec/models/organisation_spec.rb b/spec/models/organisation_spec.rb index 1dd61f4b2..c5787f9e0 100644 --- a/spec/models/organisation_spec.rb +++ b/spec/models/organisation_spec.rb @@ -151,7 +151,6 @@ RSpec.describe Organisation, type: :model do FactoryBot.create( :lettings_log, :completed, - owning_organisation: organisation, managing_organisation: other_organisation, created_by: user, ) @@ -159,8 +158,7 @@ RSpec.describe Organisation, type: :model do let!(:managed_lettings_log) do FactoryBot.create( :lettings_log, - owning_organisation: other_organisation, - managing_organisation: organisation, + created_by: user, ) end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index bd6555348..fd132a8ef 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -8,7 +8,6 @@ RSpec.describe User, type: :model do FactoryBot.create( :lettings_log, :completed, - owning_organisation: user.organisation, managing_organisation: other_organisation, created_by: user, ) @@ -16,8 +15,8 @@ RSpec.describe User, type: :model do let!(:managed_lettings_log) do FactoryBot.create( :lettings_log, + created_by: user, owning_organisation: other_organisation, - managing_organisation: user.organisation, ) end diff --git a/spec/models/validations/setup_validations_spec.rb b/spec/models/validations/setup_validations_spec.rb index df21a4dc4..701e410ad 100644 --- a/spec/models/validations/setup_validations_spec.rb +++ b/spec/models/validations/setup_validations_spec.rb @@ -224,4 +224,64 @@ RSpec.describe Validations::SetupValidations do end end end + + describe "#validate_organisation" do + let(:user) { FactoryBot.create(:user) } + let(:other_organisation) { FactoryBot.create(:organisation) } + + it "validates if neither managing nor owning organisation is the same as created_by user organisation" do + record.created_by = user + record.owning_organisation = other_organisation + record.managing_organisation = other_organisation + + setup_validator.validate_organisation(record) + expect(record.errors["created_by"]).to include(I18n.t("validations.setup.created_by.invalid")) + expect(record.errors["owning_organisation_id"]).to include(I18n.t("validations.setup.owning_organisation.invalid")) + expect(record.errors["managing_organisation_id"]).to include(I18n.t("validations.setup.managing_organisation.invalid")) + end + + it "doesn not validate if either managing or owning organisation is the same as current user organisation" do + record.created_by = user + record.owning_organisation = user.organisation + record.managing_organisation = other_organisation + + setup_validator.validate_organisation(record) + expect(record.errors["created_by"]).to be_empty + expect(record.errors["owning_organisation_id"]).to be_empty + expect(record.errors["managing_organisation_id"]).to be_empty + end + + it "does not validate if current user is missing" do + record.created_by = nil + record.owning_organisation = other_organisation + record.managing_organisation = other_organisation + + setup_validator.validate_organisation(record) + expect(record.errors["created_by"]).to be_empty + expect(record.errors["owning_organisation_id"]).to be_empty + expect(record.errors["managing_organisation_id"]).to be_empty + end + + it "does not validate if managing organisation is missing" do + record.created_by = user + record.owning_organisation = other_organisation + record.managing_organisation = nil + + setup_validator.validate_organisation(record) + expect(record.errors["created_by"]).to be_empty + expect(record.errors["owning_organisation_id"]).to be_empty + expect(record.errors["managing_organisation_id"]).to be_empty + end + + it "does not validate if owning organisation is missing" do + record.created_by = user + record.owning_organisation = nil + record.managing_organisation = other_organisation + + setup_validator.validate_organisation(record) + expect(record.errors["created_by"]).to be_empty + expect(record.errors["owning_organisation_id"]).to be_empty + expect(record.errors["managing_organisation_id"]).to be_empty + end + end end diff --git a/spec/requests/form_controller_spec.rb b/spec/requests/form_controller_spec.rb index cb5523b8e..eec493ede 100644 --- a/spec/requests/form_controller_spec.rb +++ b/spec/requests/form_controller_spec.rb @@ -4,12 +4,12 @@ RSpec.describe FormController, type: :request do let(:page) { Capybara::Node::Simple.new(response.body) } let(:user) { create(:user) } let(:organisation) { user.organisation } - let(:other_organisation) { create(:organisation) } + let(:other_user) { create(:user) } + let(:other_organisation) { other_user.organisation } let!(:unauthorized_lettings_log) do create( :lettings_log, - owning_organisation: other_organisation, - managing_organisation: other_organisation, + created_by: other_user, ) end let(:setup_complete_lettings_log) do @@ -18,16 +18,14 @@ RSpec.describe FormController, type: :request do :about_completed, status: 1, startdate: Time.zone.local(2021, 10, 10), - owning_organisation: organisation, - managing_organisation: organisation, + created_by: user, ) end let(:completed_lettings_log) do create( :lettings_log, :completed, - owning_organisation: organisation, - managing_organisation: organisation, + created_by: user, startdate: Time.zone.local(2021, 5, 1), ) end @@ -43,8 +41,7 @@ RSpec.describe FormController, type: :request do let!(:lettings_log) do create( :lettings_log, - owning_organisation: organisation, - managing_organisation: organisation, + created_by: user, ) end @@ -68,12 +65,134 @@ RSpec.describe FormController, type: :request do end end + context "when signed in as a support user" do + let!(:lettings_log) do + create( + :lettings_log, + created_by: user, + ) + end + let(:page) { Capybara::Node::Simple.new(response.body) } + let(:managing_organisation) { create(:organisation) } + let(:managing_organisation_too) { create(:organisation) } + let(:housing_provider) { create(:organisation) } + let(:support_user) { create(:user, :support) } + + before do + organisation.housing_providers << housing_provider + organisation.managing_agents << managing_organisation + organisation.managing_agents << managing_organisation_too + organisation.reload + allow(support_user).to receive(:need_two_factor_authentication?).and_return(false) + sign_in support_user + end + + context "with invalid organisation answers" do + let(:params) do + { + id: lettings_log.id, + lettings_log: { + page: "managing_organisation", + managing_organisation_id: other_organisation.id, + }, + } + end + + before do + lettings_log.update!(owning_organisation: housing_provider, created_by: user, managing_organisation: organisation) + lettings_log.reload + end + + it "resets created by and renders the next page" do + post "/lettings-logs/#{lettings_log.id}/form", params: params + expect(response).to redirect_to("/lettings-logs/#{lettings_log.id}/created-by") + follow_redirect! + lettings_log.reload + expect(lettings_log.created_by).to eq(nil) + end + end + + context "with valid owning organisation" do + let(:params) do + { + id: lettings_log.id, + lettings_log: { + page: "managing_organisation", + managing_organisation_id: other_organisation.id, + }, + } + end + + before do + lettings_log.update!(owning_organisation: organisation, created_by: user, managing_organisation: organisation) + lettings_log.reload + end + + it "does not reset created by" do + post "/lettings-logs/#{lettings_log.id}/form", params: params + expect(response).to redirect_to("/lettings-logs/#{lettings_log.id}/created-by") + follow_redirect! + lettings_log.reload + expect(lettings_log.created_by).to eq(user) + end + end + + context "with valid managing organisation" do + let(:params) do + { + id: lettings_log.id, + lettings_log: { + page: "housing_provider", + owning_organisation_id: housing_provider.id, + }, + } + end + + before do + lettings_log.update!(owning_organisation: organisation, created_by: user, managing_organisation: organisation) + lettings_log.reload + end + + it "does not reset created by" do + post "/lettings-logs/#{lettings_log.id}/form", params: params + expect(response).to redirect_to("/lettings-logs/#{lettings_log.id}/managing-organisation") + follow_redirect! + lettings_log.reload + expect(lettings_log.created_by).to eq(user) + end + end + + context "with only adding the housing provider" do + let(:params) do + { + id: lettings_log.id, + lettings_log: { + page: "housing_provider", + owning_organisation_id: housing_provider.id, + }, + } + end + + before do + lettings_log.update!(owning_organisation: nil, created_by: user, managing_organisation: nil) + lettings_log.reload + end + + it "does not reset created by" do + post "/lettings-logs/#{lettings_log.id}/form", params: params + expect(response).to redirect_to("/lettings-logs/#{lettings_log.id}/managing-organisation") + follow_redirect! + lettings_log.reload + expect(lettings_log.created_by).to eq(user) + end + end + end + context "when a user is signed in" do let!(:lettings_log) do create( :lettings_log, - owning_organisation: organisation, - managing_organisation: organisation, + created_by: user, ) end @@ -156,8 +275,7 @@ RSpec.describe FormController, type: :request do create( :lettings_log, startdate: Time.zone.local(2022, 12, 1), - owning_organisation: organisation, - managing_organisation: organisation, + created_by: user, ) end let(:headers) { { "Accept" => "text/html" } } @@ -220,12 +338,12 @@ RSpec.describe FormController, type: :request do describe "Submit Form" do context "with a form page" do let(:user) { create(:user) } + let(:support_user) { FactoryBot.create(:user, :support) } let(:organisation) { user.organisation } let(:lettings_log) do create( :lettings_log, - owning_organisation: organisation, - managing_organisation: organisation, + created_by: user, ) end let(:page_id) { "person_1_age" } @@ -298,6 +416,38 @@ RSpec.describe FormController, type: :request do end end + context "with invalid organisation answers" do + let(:page) { Capybara::Node::Simple.new(response.body) } + let(:managing_organisation) { create(:organisation) } + let(:managing_organisation_too) { create(:organisation) } + let(:housing_provider) { create(:organisation) } + let(:params) do + { + id: lettings_log.id, + lettings_log: { + page: "managing_organisation", + managing_organisation_id: other_organisation.id, + }, + } + end + + before do + organisation.housing_providers << housing_provider + organisation.managing_agents << managing_organisation + organisation.managing_agents << managing_organisation_too + organisation.reload + lettings_log.update!(owning_organisation: housing_provider, created_by: user, managing_organisation: organisation) + lettings_log.reload + end + + it "re-renders the same page with errors if validation fails" do + post "/lettings-logs/#{lettings_log.id}/form", params: params + expect(response).to redirect_to("/lettings-logs/#{lettings_log.id}/managing-organisation") + follow_redirect! + expect(page).to have_content("There is a problem") + end + end + context "with valid answers" do let(:answer) { 20 } let(:params) do @@ -534,12 +684,11 @@ RSpec.describe FormController, type: :request do context "with lettings logs that are not owned or managed by your organisation" do let(:answer) { 25 } - let(:other_organisation) { create(:organisation) } + let(:other_user) { create(:user) } let(:unauthorized_lettings_log) do create( :lettings_log, - owning_organisation: other_organisation, - managing_organisation: other_organisation, + created_by: other_user, ) end diff --git a/spec/requests/lettings_logs_controller_spec.rb b/spec/requests/lettings_logs_controller_spec.rb index 0cee5d7b3..862f82fa1 100644 --- a/spec/requests/lettings_logs_controller_spec.rb +++ b/spec/requests/lettings_logs_controller_spec.rb @@ -150,20 +150,19 @@ RSpec.describe LettingsLogsController, type: :request do let(:page) { Capybara::Node::Simple.new(response.body) } let(:user) { FactoryBot.create(:user) } let(:organisation) { user.organisation } - let(:other_organisation) { FactoryBot.create(:organisation) } + let(:other_user) { FactoryBot.create(:user) } + let(:other_organisation) { other_user.organisation } let!(:lettings_log) do FactoryBot.create( :lettings_log, - owning_organisation: organisation, - managing_organisation: organisation, + created_by: user, tenancycode: "LC783", ) end let!(:unauthorized_lettings_log) do FactoryBot.create( :lettings_log, - owning_organisation: other_organisation, - managing_organisation: other_organisation, + created_by: other_user, tenancycode: "UA984", ) end @@ -266,9 +265,8 @@ RSpec.describe LettingsLogsController, type: :request do context "with year filter" do let!(:lettings_log_2021) do FactoryBot.create(:lettings_log, :in_progress, - owning_organisation: organisation, - startdate: Time.zone.local(2022, 3, 1), - managing_organisation: organisation) + created_by: user, + startdate: Time.zone.local(2022, 3, 1)) end let!(:lettings_log_2022) do lettings_log = FactoryBot.build(:lettings_log, :completed, @@ -567,7 +565,7 @@ RSpec.describe LettingsLogsController, type: :request do context "when there are more than 20 logs" do before do - FactoryBot.create_list(:lettings_log, 25, owning_organisation: organisation, managing_organisation: organisation) + FactoryBot.create_list(:lettings_log, 25, created_by: user) end context "when on the first page" do @@ -699,8 +697,7 @@ RSpec.describe LettingsLogsController, type: :request do FactoryBot.create( :lettings_log, :conditional_section_complete, - owning_organisation: organisation, - managing_organisation: organisation, + created_by: user, ) end @@ -756,8 +753,7 @@ RSpec.describe LettingsLogsController, type: :request do context "when accessing the check answers page" do let(:postcode_lettings_log) do FactoryBot.create(:lettings_log, - owning_organisation: organisation, - managing_organisation: organisation, + created_by: user, postcode_known: "No") end let(:id) { postcode_lettings_log.id } @@ -771,8 +767,7 @@ RSpec.describe LettingsLogsController, type: :request do it "shows the inferred la" do lettings_log = FactoryBot.create(:lettings_log, - owning_organisation: organisation, - managing_organisation: organisation, + created_by: user, postcode_known: 1, postcode_full: "PO5 3TE") id = lettings_log.id @@ -1096,8 +1091,7 @@ RSpec.describe LettingsLogsController, type: :request do let!(:lettings_log) do FactoryBot.create( :lettings_log, - owning_organisation:, - managing_organisation: owning_organisation, + created_by: user, ecstat1: 1, ) end diff --git a/spec/requests/organisations_controller_spec.rb b/spec/requests/organisations_controller_spec.rb index 797b7600e..ab5dcb6af 100644 --- a/spec/requests/organisations_controller_spec.rb +++ b/spec/requests/organisations_controller_spec.rb @@ -590,8 +590,8 @@ RSpec.describe OrganisationsController, type: :request do let(:number_of_org2_lettings_logs) { 4 } before do - FactoryBot.create_list(:lettings_log, number_of_org1_lettings_logs, owning_organisation_id: organisation.id, managing_organisation_id: organisation.id) - FactoryBot.create_list(:lettings_log, number_of_org2_lettings_logs, owning_organisation_id: unauthorised_organisation.id, managing_organisation_id: unauthorised_organisation.id) + FactoryBot.create_list(:lettings_log, number_of_org1_lettings_logs, created_by: user) + FactoryBot.create_list(:lettings_log, number_of_org2_lettings_logs, created_by: nil, owning_organisation_id: unauthorised_organisation.id, managing_organisation_id: unauthorised_organisation.id) get "/organisations/#{organisation.id}/lettings-logs", headers:, params: {} end diff --git a/spec/services/csv/lettings_log_csv_service_spec.rb b/spec/services/csv/lettings_log_csv_service_spec.rb index 9b9c7ff87..e68be40d7 100644 --- a/spec/services/csv/lettings_log_csv_service_spec.rb +++ b/spec/services/csv/lettings_log_csv_service_spec.rb @@ -201,6 +201,7 @@ RSpec.describe Csv::LettingsLogCsvService do new_old vacdays unresolved + updated_by_id unittype_sh scheme_code scheme_service_name