From 5bc78a6a5a205844a7d35e40d5c46989fb1fb21c Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Fri, 3 Dec 2021 09:23:05 +0000 Subject: [PATCH 1/6] CLDC-498: property type validation (#142) * Update mappings based on content changes and add tests for property type validation * move out RENT_TYPE_MAPPING to constants file --- app/models/case_log.rb | 9 ----- app/models/constants/db_enums.rb | 37 ++++++++++++------- .../validations/household_validations.rb | 2 +- config/forms/2021_2022.json | 2 +- db/schema.rb | 2 +- spec/factories/case_log.rb | 2 +- spec/fixtures/complete_case_log.json | 2 +- spec/models/case_log_spec.rb | 24 ++++++++++-- 8 files changed, 49 insertions(+), 31 deletions(-) diff --git a/app/models/case_log.rb b/app/models/case_log.rb index 068b148fc..6809d8a9c 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -156,15 +156,6 @@ class CaseLog < ApplicationRecord private - RENT_TYPE_MAPPING = { - "Social Rent" => "Social Rent", - "Affordable Rent" => "Affordable Rent", - "London Affordable Rent" => "Affordable Rent", - "Rent To Buy" => "Intermediate Rent", - "London Living Rent" => "Intermediate Rent", - "Other Intermediate Rent Product" => "Intermediate Rent", - }.freeze - def update_status! self.status = if all_fields_completed? && errors.empty? "completed" diff --git a/app/models/constants/db_enums.rb b/app/models/constants/db_enums.rb index 00442cc38..e4c5ace27 100644 --- a/app/models/constants/db_enums.rb +++ b/app/models/constants/db_enums.rb @@ -672,8 +672,8 @@ module Constants::DbEnums }.freeze NEEDS_TYPE = { - "General Needs" => 1, - "Supported Housing" => 2, + "General needs" => 1, + "Supported housing" => 2, }.freeze ORG_TYPE = { @@ -682,17 +682,26 @@ module Constants::DbEnums }.freeze LET_TYPE = { - "Social Rent General Needs PRP" => 1, - "Social Rent Supported Housing PRP" => 2, - "Social Rent General Needs LA" => 3, - "Social Rent Supported Housing LA" => 4, - "Affordable Rent General Needs PRP" => 5, - "Affordable Rent Supported Housing PRP" => 6, - "Affordable Rent General Needs LA" => 7, - "Affordable Rent Supported Housing LA" => 8, - "Intermediate Rent General Needs PRP" => 9, - "Intermediate Rent Supported Housing PRP" => 10, - "Intermediate Rent General Needs LA" => 11, - "Intermediate Rent Supported Housing LA" => 12, + "Social Rent General needs PRP" => 1, + "Social Rent Supported housing PRP" => 2, + "Social Rent General needs LA" => 3, + "Social Rent Supported housing LA" => 4, + "Affordable Rent General needs PRP" => 5, + "Affordable Rent Supported housing PRP" => 6, + "Affordable Rent General needs LA" => 7, + "Affordable Rent Supported housing LA" => 8, + "Intermediate Rent General needs PRP" => 9, + "Intermediate Rent Supported housing PRP" => 10, + "Intermediate Rent General needs LA" => 11, + "Intermediate Rent Supported housing LA" => 12, + }.freeze + + RENT_TYPE_MAPPING = { + "Social rent" => "Social Rent", + "Affordable rent" => "Affordable Rent", + "London Affordable rent" => "Affordable Rent", + "Rent to buy" => "Intermediate Rent", + "London living rent" => "Intermediate Rent", + "Other intermediate rent product" => "Intermediate Rent", }.freeze end diff --git a/app/models/validations/household_validations.rb b/app/models/validations/household_validations.rb index de92df6e6..6f9ed06a3 100644 --- a/app/models/validations/household_validations.rb +++ b/app/models/validations/household_validations.rb @@ -76,7 +76,7 @@ module Validations::HouseholdValidations def validate_shared_housing_rooms(record) unless record.unittype_gn.nil? - if record.unittype_gn == "Bed-sit" && record.beds != 1 + if record.unittype_gn == "Bed-sit" && record.beds != 1 && record.beds.present? record.errors.add :unittype_gn, "A bedsit can only have one bedroom" end diff --git a/config/forms/2021_2022.json b/config/forms/2021_2022.json index d640825ae..d369344b1 100644 --- a/config/forms/2021_2022.json +++ b/config/forms/2021_2022.json @@ -1668,7 +1668,7 @@ "step": 1 } }, - "depends_on": { "needstype": "General Needs" } + "depends_on": { "needstype": "General needs" } }, "void_or_renewal_date": { "header": "", diff --git a/db/schema.rb b/db/schema.rb index fe6ed0955..255b55aa4 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -163,9 +163,9 @@ ActiveRecord::Schema.define(version: 2021_12_02_124802) do t.string "why_dont_you_know_la" t.integer "unitletas" t.integer "builtype" + t.datetime "property_void_date" t.bigint "owning_organisation_id" t.bigint "managing_organisation_id" - t.datetime "property_void_date" t.integer "renttype" t.integer "needstype" t.integer "lettype" diff --git a/spec/factories/case_log.rb b/spec/factories/case_log.rb index 80f05b7da..cca98c002 100644 --- a/spec/factories/case_log.rb +++ b/spec/factories/case_log.rb @@ -52,7 +52,7 @@ FactoryBot.define do startertenancy { "No" } tenancylength { 5 } tenancy { "Secure (including flexible)" } - lettype { "Affordable Rent General Needs LA" } + lettype { "Affordable Rent General needs LA" } landlord { "This landlord" } previous_postcode { "SE2 6RT" } rsnvac { "Tenant abandoned property" } diff --git a/spec/fixtures/complete_case_log.json b/spec/fixtures/complete_case_log.json index e70a7a2f0..b41538a8f 100644 --- a/spec/fixtures/complete_case_log.json +++ b/spec/fixtures/complete_case_log.json @@ -127,7 +127,7 @@ "sale_or_letting": "", "rent_type": "Social Rent", "intermediate_rent_product_name": "", - "needstype": "General Needs", + "needstype": "General needs", "sale_completion_date": "01/01/2020", "purchaser_code": "", "propcode": "123", diff --git a/spec/models/case_log_spec.rb b/spec/models/case_log_spec.rb index b94710b1b..28bbc877a 100644 --- a/spec/models/case_log_spec.rb +++ b/spec/models/case_log_spec.rb @@ -790,6 +790,24 @@ RSpec.describe Form, type: :model do }.not_to raise_error end end + + context "Validate type of unit" do + it "Cannot be bedsit if no of bedrooms is greater than 1" do + expect { + CaseLog.create!(unittype_gn: "Bed-sit", + beds: 2, + owning_organisation: owning_organisation, + managing_organisation: managing_organisation) + }.to raise_error(ActiveRecord::RecordInvalid) + + expect { + CaseLog.create!(unittype_gn: "Bed-sit", + beds: 1, + owning_organisation: owning_organisation, + managing_organisation: managing_organisation) + }.not_to raise_error + end + end end describe "status" do @@ -850,8 +868,8 @@ RSpec.describe Form, type: :model do # rubocop:enable Style/DateTime net_income_known: "Prefer not to say", other_hhmemb: 6, - rent_type: "London Living Rent", - needstype: "General Needs", + rent_type: "London living rent", + needstype: "General needs", }) end @@ -909,7 +927,7 @@ RSpec.describe Form, type: :model do case_log.reload record_from_db = ActiveRecord::Base.connection.execute("select lettype from case_logs where id=#{case_log.id}").to_a[0] - expect(case_log.lettype).to eq("Intermediate Rent General Needs PRP") + expect(case_log.lettype).to eq("Intermediate Rent General needs PRP") expect(record_from_db["lettype"]).to eq(9) end end From 19e5633e8bdcf3415cdf8561c2950d7e67988f26 Mon Sep 17 00:00:00 2001 From: baarkerlounger <5101747+baarkerlounger@users.noreply.github.com> Date: Fri, 3 Dec 2021 11:23:29 +0000 Subject: [PATCH 2/6] Domain specific constants (#144) --- app/models/case_log.rb | 2 +- app/models/constants/{db_enums.rb => case_log.rb} | 7 +------ app/models/constants/organisation.rb | 6 ++++++ app/models/organisation.rb | 2 +- 4 files changed, 9 insertions(+), 8 deletions(-) rename app/models/constants/{db_enums.rb => case_log.rb} (99%) create mode 100644 app/models/constants/organisation.rb diff --git a/app/models/case_log.rb b/app/models/case_log.rb index 6809d8a9c..08ec3ae3a 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -30,7 +30,7 @@ end class CaseLog < ApplicationRecord include Discard::Model include Validations::SoftValidations - include Constants::DbEnums + include Constants::CaseLog include Constants::IncomeRanges default_scope -> { kept } diff --git a/app/models/constants/db_enums.rb b/app/models/constants/case_log.rb similarity index 99% rename from app/models/constants/db_enums.rb rename to app/models/constants/case_log.rb index e4c5ace27..e7f1b84b2 100644 --- a/app/models/constants/db_enums.rb +++ b/app/models/constants/case_log.rb @@ -1,4 +1,4 @@ -module Constants::DbEnums +module Constants::CaseLog BENEFITCAP = { "Yes - benefit cap" => 5, "Yes - removal of the spare room subsidy" => 4, @@ -676,11 +676,6 @@ module Constants::DbEnums "Supported housing" => 2, }.freeze - ORG_TYPE = { - "LA" => 1, - "PRP" => 2, - }.freeze - LET_TYPE = { "Social Rent General needs PRP" => 1, "Social Rent Supported housing PRP" => 2, diff --git a/app/models/constants/organisation.rb b/app/models/constants/organisation.rb new file mode 100644 index 000000000..2a6501383 --- /dev/null +++ b/app/models/constants/organisation.rb @@ -0,0 +1,6 @@ +module Constants::Organisation + ORG_TYPE = { + "LA" => 1, + "PRP" => 2, + }.freeze +end diff --git a/app/models/organisation.rb b/app/models/organisation.rb index 84ab477bb..efd72bae5 100644 --- a/app/models/organisation.rb +++ b/app/models/organisation.rb @@ -3,7 +3,7 @@ class Organisation < ApplicationRecord has_many :owned_case_logs, class_name: "CaseLog", foreign_key: "owning_organisation_id" has_many :managed_case_logs, class_name: "CaseLog", foreign_key: "managing_organisation_id" - include Constants::DbEnums + include Constants::Organisation enum "Org type": ORG_TYPE, _suffix: true def case_logs From eee744adb1b76a5e71c8c0377997034380b6142b Mon Sep 17 00:00:00 2001 From: baarkerlounger Date: Fri, 3 Dec 2021 13:15:59 +0000 Subject: [PATCH 3/6] Don't need to lock turbo-rails version if you clear your cache --- Gemfile | 1 - Gemfile.lock | 11 +++++------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/Gemfile b/Gemfile index 4d8e8ea3c..9b9def7d3 100644 --- a/Gemfile +++ b/Gemfile @@ -34,7 +34,6 @@ gem "json-schema" # Authentication # Point at branch until devise is compatible with Turbo, see https://github.com/heartcombo/devise/pull/5340 gem "devise", github: "ghiculescu/devise", branch: "error-code-422" -gem "turbo-rails", "~> 0.8" gem "uk_postcode" gem "view_component" diff --git a/Gemfile.lock b/Gemfile.lock index 100e839eb..309b348a9 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -38,7 +38,7 @@ GIT GIT remote: https://github.com/rspec/rspec-rails.git - revision: 1fe6c2e8a56f46ae4c2f13ffa0733cce3b717c2d + revision: 5f54d780a3d8f21cfe4f424a7d0435be2e279356 branch: main specs: rspec-rails (5.1.0.pre) @@ -178,13 +178,13 @@ GEM formtastic (4.0.0) actionpack (>= 5.2.0) formtastic_i18n (0.7.0) - globalid (0.6.0) + globalid (1.0.0) activesupport (>= 5.0) govuk-components (2.1.4) activemodel (>= 6.0) railties (>= 6.0) view_component (~> 2.39.0) - govuk_design_system_formbuilder (2.7.6) + govuk_design_system_formbuilder (2.8.0) actionview (>= 6.0) activemodel (>= 6.0) activesupport (>= 6.0) @@ -249,7 +249,7 @@ GEM iniparse (~> 1.4) rexml (~> 3.2) parallel (1.21.0) - parser (3.0.2.0) + parser (3.0.3.1) ast (~> 2.4.1) pg (1.2.3) pry (0.13.1) @@ -377,7 +377,7 @@ GEM rails (>= 6.0.0) tzinfo (2.0.4) concurrent-ruby (~> 1.0) - uk_postcode (2.1.6) + uk_postcode (2.1.7) unicode-display_width (2.1.0) view_component (2.39.0) activesupport (>= 5.0.0, < 8.0) @@ -441,7 +441,6 @@ DEPENDENCIES scss_lint-govuk selenium-webdriver simplecov - turbo-rails (~> 0.8) tzinfo-data uk_postcode view_component From acb132da6ffe4b3ccf570ff10dfc0116074aa020 Mon Sep 17 00:00:00 2001 From: baarkerlounger Date: Fri, 3 Dec 2021 13:26:09 +0000 Subject: [PATCH 4/6] Gemfile comments --- Gemfile | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Gemfile b/Gemfile index 9b9def7d3..deaf820b7 100644 --- a/Gemfile +++ b/Gemfile @@ -15,9 +15,9 @@ gem "puma", "~> 5.0" gem "webpacker", "~> 5.0" # Reduces boot times through caching; required in config/boot.rb gem "bootsnap", ">= 1.4.4", require: false -# Gov.UK frontend components +# GOV UK frontend components gem "govuk-components" -# Gov.UK component form builder DSL +# GOV UK component form builder DSL gem "govuk_design_system_formbuilder" # Turbo & Stimulus gem "hotwire-rails" @@ -34,7 +34,9 @@ gem "json-schema" # Authentication # Point at branch until devise is compatible with Turbo, see https://github.com/heartcombo/devise/pull/5340 gem "devise", github: "ghiculescu/devise", branch: "error-code-422" +# UK postcode parsing and validation gem "uk_postcode" +# Use Ruby objects to build reusable markup. A React inspired evolution of the presenter pattern gem "view_component" group :development, :test do From d48a5d07dfcb7160872ff04ef1e50b1e76ac6a08 Mon Sep 17 00:00:00 2001 From: baarkerlounger <5101747+baarkerlounger@users.noreply.github.com> Date: Fri, 3 Dec 2021 14:33:28 +0000 Subject: [PATCH 5/6] CLDC-769: Scope authentication to the correct resources (#143) * Scope auth for org pages * Scope user methods * Refactor form UI controller * More tests for access * Consistently return not found for scoped auth * Add ADR * Authenticate bulk uploads * Authenticate soft validations controller * Scope bulk upload to user's org for now * Pass through current user * Update docs/adr/adr-012-controller-http-return-statuses.md Co-authored-by: Dushan <47317567+dushan-madetech@users.noreply.github.com> * Update spec/requests/case_log_controller_spec.rb Co-authored-by: Dushan <47317567+dushan-madetech@users.noreply.github.com> Co-authored-by: Dushan <47317567+dushan-madetech@users.noreply.github.com> --- app/controllers/bulk_upload_controller.rb | 6 +- app/controllers/case_logs_controller.rb | 94 +----- app/controllers/form_controller.rb | 83 ++++++ app/controllers/organisations_controller.rb | 9 +- .../soft_validations_controller.rb | 2 + app/controllers/users_controller.rb | 16 +- app/models/bulk_upload.rb | 6 +- config/routes.rb | 6 +- ...adr-012-controller-http-return-statuses.md | 15 + spec/controllers/case_logs_controller_spec.rb | 172 ----------- spec/requests/bulk_upload_controller_spec.rb | 91 ++++-- spec/requests/case_log_controller_spec.rb | 200 +++---------- spec/requests/form_controller_spec.rb | 278 ++++++++++++++++++ .../requests/organisations_controller_spec.rb | 233 ++++++++++----- .../soft_validations_controller_spec.rb | 57 ++-- spec/requests/user_controller_spec.rb | 129 +++++++- 16 files changed, 832 insertions(+), 565 deletions(-) create mode 100644 app/controllers/form_controller.rb create mode 100644 docs/adr/adr-012-controller-http-return-statuses.md delete mode 100644 spec/controllers/case_logs_controller_spec.rb create mode 100644 spec/requests/form_controller_spec.rb diff --git a/app/controllers/bulk_upload_controller.rb b/app/controllers/bulk_upload_controller.rb index ba552cb49..28fe7bbae 100644 --- a/app/controllers/bulk_upload_controller.rb +++ b/app/controllers/bulk_upload_controller.rb @@ -1,4 +1,6 @@ class BulkUploadController < ApplicationController + before_action :authenticate_user! + def show @bulk_upload = BulkUpload.new(nil, nil) render "case_logs/bulk_upload" @@ -8,7 +10,7 @@ class BulkUploadController < ApplicationController file = upload_params.tempfile content_type = upload_params.content_type @bulk_upload = BulkUpload.new(file, content_type) - @bulk_upload.process + @bulk_upload.process(current_user) if @bulk_upload.errors.present? render "case_logs/bulk_upload", status: :unprocessable_entity else @@ -16,6 +18,8 @@ class BulkUploadController < ApplicationController end end +private + def upload_params params.require("bulk_upload")["case_log_bulk_upload"] end diff --git a/app/controllers/case_logs_controller.rb b/app/controllers/case_logs_controller.rb index 069100299..77ccc3d6d 100644 --- a/app/controllers/case_logs_controller.rb +++ b/app/controllers/case_logs_controller.rb @@ -2,6 +2,7 @@ class CaseLogsController < ApplicationController skip_before_action :verify_authenticity_token, if: :json_api_request? before_action :authenticate, if: :json_api_request? before_action :authenticate_user!, unless: :json_api_request? + before_action :find_resource, except: %i[create index edit] def index @completed_case_logs = current_user.completed_case_logs @@ -23,11 +24,11 @@ class CaseLogsController < ApplicationController end def update - if (case_log = CaseLog.find_by(id: params[:id])) - if case_log.update(api_case_log_params) - render json: case_log, status: :ok + if @case_log + if @case_log.update(api_case_log_params) + render json: @case_log, status: :ok else - render json: { errors: case_log.errors.messages }, status: :unprocessable_entity + render json: { errors: @case_log.errors.messages }, status: :unprocessable_entity end else render_not_found_json("Case log", params[:id]) @@ -39,8 +40,8 @@ class CaseLogsController < ApplicationController # We don't have a dedicated non-editable show view format.html { edit } format.json do - if (case_log = CaseLog.find_by(id: params[:id])) - render json: case_log, status: :ok + if @case_log + render json: @case_log, status: :ok else render_not_found_json("Case log", params[:id]) end @@ -58,93 +59,22 @@ class CaseLogsController < ApplicationController end end - def submit_form - form = FormHandler.instance.get_form("2021_2022") - @case_log = current_user.case_logs.find_by(id: params[:id]) - if @case_log - page = form.get_page(params[:case_log][:page]) - responses_for_page = responses_for_page(page) - if @case_log.update(responses_for_page) && @case_log.has_no_unresolved_soft_errors? - redirect_path = form.next_page_redirect_path(page, @case_log) - redirect_to(send(redirect_path, @case_log)) - else - subsection = form.subsection_for_page(page) - render "form/page", locals: { form: form, page: page, subsection: subsection.label }, status: :unprocessable_entity - end - else - render_not_found_html - end - end - def destroy - if (case_log = CaseLog.find_by(id: params[:id])) - if case_log.discard + if @case_log + if @case_log.discard head :no_content else - render json: { errors: case_log.errors.messages }, status: :unprocessable_entity + render json: { errors: @case_log.errors.messages }, status: :unprocessable_entity end else render_not_found_json("Case log", params[:id]) end end - def check_answers - form = FormHandler.instance.get_form("2021_2022") - @case_log = current_user.case_logs.find_by(id: params[:case_log_id]) - if @case_log - current_url = request.env["PATH_INFO"] - subsection = form.get_subsection(current_url.split("/")[-2]) - render "form/check_answers", locals: { subsection: subsection, form: form } - else - render_not_found_html - end - end - - form = FormHandler.instance.get_form("2021_2022") - form.pages.map do |page| - define_method(page.id) do |_errors = {}| - @case_log = current_user.case_logs.find_by(id: params[:case_log_id]) - if @case_log - subsection = form.subsection_for_page(page) - render "form/page", locals: { form: form, page: page, subsection: subsection.label } - else - render_not_found_html - end - end - end - private API_ACTIONS = %w[create show update destroy].freeze - def responses_for_page(page) - page.expected_responses.each_with_object({}) do |question, result| - question_params = params["case_log"][question.id] - if question.type == "date" - day = params["case_log"]["#{question.id}(3i)"] - month = params["case_log"]["#{question.id}(2i)"] - year = params["case_log"]["#{question.id}(1i)"] - next unless [day, month, year].any?(&:present?) - - result[question.id] = if day.to_i.between?(1, 31) && month.to_i.between?(1, 12) && year.to_i.between?(2000, 2200) - Date.new(year.to_i, month.to_i, day.to_i) - else - Date.new(0, 1, 1) - end - end - next unless question_params - - if %w[checkbox validation_override].include?(question.type) - question.answer_options.keys.reject { |x| x.match(/divider/) }.each do |option| - result[option] = question_params.include?(option) ? 1 : 0 - end - else - result[question.id] = question_params - end - result - end - end - def json_api_request? API_ACTIONS.include?(request["action"]) && request.format.json? end @@ -173,4 +103,8 @@ private params.require(:case_log).permit(CaseLog.editable_fields) end + + def find_resource + @case_log = CaseLog.find_by(id: params[:id]) + end end diff --git a/app/controllers/form_controller.rb b/app/controllers/form_controller.rb new file mode 100644 index 000000000..93dc32e84 --- /dev/null +++ b/app/controllers/form_controller.rb @@ -0,0 +1,83 @@ +class FormController < ApplicationController + before_action :authenticate_user! + before_action :find_resource, only: [:submit_form] + before_action :find_resource_by_named_id, except: [:submit_form] + + def submit_form + form = FormHandler.instance.get_form("2021_2022") + if @case_log + page = form.get_page(params[:case_log][:page]) + responses_for_page = responses_for_page(page) + if @case_log.update(responses_for_page) && @case_log.has_no_unresolved_soft_errors? + redirect_path = form.next_page_redirect_path(page, @case_log) + redirect_to(send(redirect_path, @case_log)) + else + subsection = form.subsection_for_page(page) + render "form/page", locals: { form: form, page: page, subsection: subsection.label }, status: :unprocessable_entity + end + else + render_not_found_html + end + end + + def check_answers + form = FormHandler.instance.get_form("2021_2022") + if @case_log + current_url = request.env["PATH_INFO"] + subsection = form.get_subsection(current_url.split("/")[-2]) + render "form/check_answers", locals: { subsection: subsection, form: form } + else + render_not_found_html + end + end + + form = FormHandler.instance.get_form("2021_2022") + form.pages.map do |page| + define_method(page.id) do |_errors = {}| + if @case_log + subsection = form.subsection_for_page(page) + render "form/page", locals: { form: form, page: page, subsection: subsection.label } + else + render_not_found_html + end + end + end + +private + + def responses_for_page(page) + page.expected_responses.each_with_object({}) do |question, result| + question_params = params["case_log"][question.id] + if question.type == "date" + day = params["case_log"]["#{question.id}(3i)"] + month = params["case_log"]["#{question.id}(2i)"] + year = params["case_log"]["#{question.id}(1i)"] + next unless [day, month, year].any?(&:present?) + + result[question.id] = if day.to_i.between?(1, 31) && month.to_i.between?(1, 12) && year.to_i.between?(2000, 2200) + Date.new(year.to_i, month.to_i, day.to_i) + else + Date.new(0, 1, 1) + end + end + next unless question_params + + if %w[checkbox validation_override].include?(question.type) + question.answer_options.keys.reject { |x| x.match(/divider/) }.each do |option| + result[option] = question_params.include?(option) ? 1 : 0 + end + else + result[question.id] = question_params + end + result + end + end + + def find_resource + @case_log = current_user.case_logs.find_by(id: params[:id]) + end + + def find_resource_by_named_id + @case_log = current_user.case_logs.find_by(id: params[:case_log_id]) + end +end diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb index dcf1aa34a..a9081dd3f 100644 --- a/app/controllers/organisations_controller.rb +++ b/app/controllers/organisations_controller.rb @@ -1,6 +1,7 @@ class OrganisationsController < ApplicationController before_action :authenticate_user! - before_action :find_organisation + before_action :find_resource + before_action :authenticate_scope! def show redirect_to details_organisation_path(@organisation) @@ -20,7 +21,11 @@ class OrganisationsController < ApplicationController private - def find_organisation + def authenticate_scope! + head :not_found if current_user.organisation != @organisation + end + + def find_resource @organisation = Organisation.find(params[:id]) end end diff --git a/app/controllers/soft_validations_controller.rb b/app/controllers/soft_validations_controller.rb index 4f9881de6..0f0baf376 100644 --- a/app/controllers/soft_validations_controller.rb +++ b/app/controllers/soft_validations_controller.rb @@ -1,4 +1,6 @@ class SoftValidationsController < ApplicationController + before_action :authenticate_user! + def show @case_log = CaseLog.find(params[:case_log_id]) page_id = request.env["PATH_INFO"].split("/")[-2] diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 30afa501b..9ebdae068 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -2,12 +2,14 @@ class UsersController < ApplicationController include Devise::Controllers::SignInOut include Helpers::Email before_action :authenticate_user! + before_action :find_resource, except: %i[new create] + before_action :authenticate_scope!, except: %i[new create] def update - if current_user.update(user_params) - bypass_sign_in current_user + if @user.update(user_params) + bypass_sign_in @user flash[:notice] = I18n.t("devise.passwords.updated") - redirect_to user_path(current_user) + redirect_to user_path(@user) end end @@ -48,4 +50,12 @@ private def user_params params.require(:user).permit(:email, :name, :password, :role) end + + def find_resource + @user = User.find(params[:id]) + end + + def authenticate_scope! + head :not_found if current_user != @user + end end diff --git a/app/models/bulk_upload.rb b/app/models/bulk_upload.rb index 8bf0e2131..4eb558c10 100644 --- a/app/models/bulk_upload.rb +++ b/app/models/bulk_upload.rb @@ -15,7 +15,7 @@ class BulkUpload @content_type = content_type end - def process + def process(current_user) return unless valid_content_type? xlsx = Roo::Spreadsheet.open(@file, extension: :xlsx) @@ -30,8 +30,8 @@ class BulkUpload owning_organisation = Organisation.find(row[111]) managing_organisation = Organisation.find(row[113]) case_log = CaseLog.create!( - owning_organisation: owning_organisation, - managing_organisation: managing_organisation, + owning_organisation: current_user.organisation, + managing_organisation: current_user.organisation, ) map_row(row).each do |attr_key, attr_val| update = case_log.update(attr_key => attr_val) diff --git a/config/routes.rb b/config/routes.rb index 72e1ef208..0cc0065c0 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -38,16 +38,16 @@ Rails.application.routes.draw do end member do - post "form", to: "case_logs#submit_form" + post "form", to: "form#submit_form" end form.pages.map do |page| - get page.id.to_s.dasherize, to: "case_logs##{page.id}" + get page.id.to_s.dasherize, to: "form##{page.id}" get "#{page.id.to_s.dasherize}/soft-validations", to: "soft_validations#show" if page.has_soft_validations? end form.subsections.map do |subsection| - get "#{subsection.id.to_s.dasherize}/check-answers", to: "case_logs#check_answers" + get "#{subsection.id.to_s.dasherize}/check-answers", to: "form#check_answers" end end end diff --git a/docs/adr/adr-012-controller-http-return-statuses.md b/docs/adr/adr-012-controller-http-return-statuses.md new file mode 100644 index 000000000..d28ee39d8 --- /dev/null +++ b/docs/adr/adr-012-controller-http-return-statuses.md @@ -0,0 +1,15 @@ +### ADR - 012: Controller HTTP return statuses + +Controllers assess authentication by 3 criteria: + +1. Are you signed in at all? +2. Are you signed in and requesting an action that your role/user type has access to? +3. Are you signed in, requesting an action that your role/user type has access to and requesting a resource that your user has access to. + +When these aren't met they fail with the following response types: + +1. 401: Unauthorized. Redirect to sign-in page. +2. 401: Unauthorized +3. 404: Not found. + +This helps make it harder to determine whether a resource exists or not just by enumerating ids. diff --git a/spec/controllers/case_logs_controller_spec.rb b/spec/controllers/case_logs_controller_spec.rb deleted file mode 100644 index c2340d4ef..000000000 --- a/spec/controllers/case_logs_controller_spec.rb +++ /dev/null @@ -1,172 +0,0 @@ -require "rails_helper" - -RSpec.describe CaseLogsController, type: :controller do - let(:valid_session) { {} } - let(:user) { FactoryBot.create(:user) } - let(:case_log) do - FactoryBot.create( - :case_log, - owning_organisation: user.organisation, - managing_organisation: user.organisation, - ) - end - let(:id) { case_log.id } - - before do - sign_in user - end - - context "Collection routes" do - describe "GET #index" do - it "returns a success response" do - get :index, params: {}, session: valid_session - expect(response).to be_successful - end - end - - describe "Post #create" do - let(:owning_organisation) { FactoryBot.create(:organisation) } - let(:managing_organisation) { owning_organisation } - let(:params) do - { - "owning_organisation_id": owning_organisation.id, - "managing_organisation_id": managing_organisation.id, - } - end - - it "creates a new case log record" do - expect { - post :create, params: params, session: valid_session - }.to change(CaseLog, :count).by(1) - end - - it "redirects to that case log" do - post :create, params: params, session: valid_session - expect(response.status).to eq(302) - end - end - end - - context "Instance routes" do - describe "GET #show" do - it "returns a success response" do - get :show, params: { id: id } - expect(response).to be_successful - end - end - - describe "GET #edit" do - it "returns a success response" do - get :edit, params: { id: id } - expect(response).to be_successful - end - end - end - - describe "submit_form" do - let(:case_log_form_params) do - { accessibility_requirements: - %w[ housingneeds_a - housingneeds_b - housingneeds_c], - page: "accessibility_requirements" } - end - - let(:new_case_log_form_params) do - { - accessibility_requirements: %w[housingneeds_c], - page: "accessibility_requirements", - } - end - - it "sets checked items to true" do - post :submit_form, params: { id: id, case_log: case_log_form_params } - case_log.reload - - expect(case_log.housingneeds_a).to eq("Yes") - expect(case_log.housingneeds_b).to eq("Yes") - expect(case_log.housingneeds_c).to eq("Yes") - end - - it "sets previously submitted items to false when resubmitted with new values" do - post :submit_form, params: { id: id, case_log: new_case_log_form_params } - case_log.reload - - expect(case_log.housingneeds_a).to eq("No") - expect(case_log.housingneeds_b).to eq("No") - expect(case_log.housingneeds_c).to eq("Yes") - end - - context "given a page with checkbox and non-checkbox questions" do - let(:tenant_code) { "BZ355" } - let(:case_log_form_params) do - { accessibility_requirements: - %w[ housingneeds_a - housingneeds_b - housingneeds_c], - tenant_code: tenant_code, - page: "accessibility_requirements" } - end - let(:questions_for_page) do - [ - Form::Question.new( - "accessibility_requirements", - { - "type" => "checkbox", - "answer_options" => - { "housingneeds_a" => "Fully wheelchair accessible housing", - "housingneeds_b" => "Wheelchair access to essential rooms", - "housingneeds_c" => "Level access housing", - "housingneeds_f" => "Other disability requirements", - "housingneeds_g" => "No disability requirements", - "divider_a" => true, - "housingneeds_h" => "Do not know", - "divider_b" => true, - "accessibility_requirements_prefer_not_to_say" => "Prefer not to say" }, - }, nil - ), - Form::Question.new("tenant_code", { "type" => "text" }, nil), - ] - end - - it "updates both question fields" do - allow_any_instance_of(Form::Page).to receive(:expected_responses).and_return(questions_for_page) - post :submit_form, params: { id: id, case_log: case_log_form_params } - case_log.reload - - expect(case_log.housingneeds_a).to eq("Yes") - expect(case_log.housingneeds_b).to eq("Yes") - expect(case_log.housingneeds_c).to eq("Yes") - expect(case_log.tenant_code).to eq(tenant_code) - end - end - - context "conditional routing" do - before do - allow_any_instance_of(CaseLogValidator).to receive(:validate_pregnancy).and_return(true) - end - - let(:case_log_form_conditional_question_yes_params) do - { - preg_occ: "Yes", - page: "conditional_question", - } - end - - let(:case_log_form_conditional_question_no_params) do - { - preg_occ: "No", - page: "conditional_question", - } - end - - it "routes to the appropriate conditional page based on the question answer of the current page" do - post :submit_form, params: { id: id, case_log: case_log_form_conditional_question_yes_params } - expect(response).to redirect_to("/case-logs/#{id}/conditional-question-yes-page") - - post :submit_form, params: { id: id, case_log: case_log_form_conditional_question_no_params } - expect(response).to redirect_to("/case-logs/#{id}/conditional-question-no-page") - end - end - end -end diff --git a/spec/requests/bulk_upload_controller_spec.rb b/spec/requests/bulk_upload_controller_spec.rb index 52233906c..e19247162 100644 --- a/spec/requests/bulk_upload_controller_spec.rb +++ b/spec/requests/bulk_upload_controller_spec.rb @@ -2,62 +2,89 @@ require "rails_helper" RSpec.describe BulkUploadController, type: :request do let(:url) { "/case-logs/bulk-upload" } - let(:organisation) { FactoryBot.create(:organisation) } + let(:user) { FactoryBot.create(:user) } + let(:organisation) { user.organisation } before do allow(Organisation).to receive(:find).with(107_242).and_return(organisation) end - describe "GET #show" do - before do - get url, params: {} + context "a not signed in user" do + describe "GET #show" do + it "does not let you see the bulk upload page" do + get url, headers: headers, params: {} + expect(response).to redirect_to("/users/sign-in") + end end - it "returns a success response" do - expect(response).to be_successful - end + describe "POST #bulk upload" do + before do + @file = fixture_file_upload("2021_22_lettings_bulk_upload.xlsx", "application/vnd.ms-excel") + end - it "returns a page with a file upload form" do - expect(response.body).to match(/ "text/html" } } + context "case logs that are not owned or managed by your organisation" do + before do + sign_in user + get "/case-logs/#{unauthorized_case_log.id}", headers: headers, params: {} + end - context "case logs that are not owned or managed by your organisation" do - before do - sign_in user - get "/case-logs/#{unauthorized_case_log.id}/person-1-age", headers: headers, params: {} - end - - it "does not show form pages for case logs you don't have access to" do - expect(response).to have_http_status(:not_found) - end - end - end - - context "check answers pages" do - let(:headers) { { "Accept" => "text/html" } } - - context "case logs that are not owned or managed by your organisation" do - before do - sign_in user - get "/case-logs/#{unauthorized_case_log.id}/household-characteristics/check-answers", headers: headers, params: {} - end - - it "does not show a check answers for case logs you don't have access to" do - expect(response).to have_http_status(:not_found) + it "does not show the tasklist for case logs you don't have access to" do + expect(response).to have_http_status(:not_found) + end end end end @@ -414,87 +393,4 @@ RSpec.describe CaseLogsController, type: :request do end end end - - describe "Submit Form" do - let(:user) { FactoryBot.create(:user) } - let(:form) { Form.new("spec/fixtures/forms/test_form.json") } - let(:organisation) { user.organisation } - let(:case_log) do - FactoryBot.create( - :case_log, - owning_organisation: organisation, - managing_organisation: organisation, - ) - end - let(:page_id) { "person_1_age" } - let(:params) do - { - id: case_log.id, - case_log: { - page: page_id, - age1: answer, - }, - } - end - - before do - allow(FormHandler.instance).to receive(:get_form).and_return(form) - sign_in user - post "/case-logs/#{case_log.id}/form", params: params - end - - context "invalid answers" do - let(:answer) { 2000 } - - it "re-renders the same page with errors if validation fails" do - expect(response).to have_http_status(:unprocessable_entity) - end - end - - context "valid answers" do - let(:answer) { 20 } - - it "re-renders the same page with errors if validation fails" do - expect(response).to have_http_status(:redirect) - end - - let(:params) do - { - id: case_log.id, - case_log: { - page: page_id, - age1: answer, - age2: 2000, - }, - } - end - - it "only updates answers that apply to the page being submitted" do - case_log.reload - expect(case_log.age1).to eq(answer) - expect(case_log.age2).to be nil - end - end - - context "case logs that are not owned or managed by your organisation" do - let(:answer) { 25 } - let(:other_organisation) { FactoryBot.create(:organisation) } - let(:unauthorized_case_log) do - FactoryBot.create( - :case_log, - owning_organisation: other_organisation, - managing_organisation: other_organisation, - ) - end - - before do - sign_in user - post "/case-logs/#{unauthorized_case_log.id}/form", params: params - end - - it "does not let you post form answers to case logs you don't have access to" do - expect(response).to have_http_status(:not_found) - end - end - end end diff --git a/spec/requests/form_controller_spec.rb b/spec/requests/form_controller_spec.rb new file mode 100644 index 000000000..f88b1219b --- /dev/null +++ b/spec/requests/form_controller_spec.rb @@ -0,0 +1,278 @@ +require "rails_helper" + +RSpec.describe FormController, type: :request do + let(:user) { FactoryBot.create(:user) } + let(:organisation) { user.organisation } + let(:other_organisation) { FactoryBot.create(:organisation) } + let!(:case_log) do + FactoryBot.create( + :case_log, + owning_organisation: organisation, + managing_organisation: organisation, + ) + end + let!(:unauthorized_case_log) do + FactoryBot.create( + :case_log, + owning_organisation: other_organisation, + managing_organisation: other_organisation, + ) + end + let(:headers) { { "Accept" => "text/html" } } + + context "a not signed in user" do + describe "GET" do + it "does not let you get case logs pages you don't have access to" do + get "/case-logs/#{case_log.id}/person-1-age", headers: headers, params: {} + expect(response).to redirect_to("/users/sign-in") + end + + it "does not let you get case log check answer pages you don't have access to" do + get "/case-logs/#{case_log.id}/household-characteristics/check-answers", headers: headers, params: {} + expect(response).to redirect_to("/users/sign-in") + end + end + + describe "POST" do + it "does not let you post form answers to case logs you don't have access to" do + post "/case-logs/#{case_log.id}/form", params: {} + expect(response).to redirect_to("/users/sign-in") + end + end + end + + context "a signed in user" do + before do + sign_in user + end + + describe "GET" do + context "form pages" do + context "case logs that are not owned or managed by your organisation" do + it "does not show form pages for case logs you don't have access to" do + get "/case-logs/#{unauthorized_case_log.id}/person-1-age", headers: headers, params: {} + expect(response).to have_http_status(:not_found) + end + end + end + + context "check answers pages" do + context "case logs that are not owned or managed by your organisation" do + it "does not show a check answers for case logs you don't have access to" do + get "/case-logs/#{unauthorized_case_log.id}/household-characteristics/check-answers", headers: headers, params: {} + expect(response).to have_http_status(:not_found) + end + end + end + end + + describe "Submit Form" do + context "a form page" do + let(:user) { FactoryBot.create(:user) } + let(:form) { Form.new("spec/fixtures/forms/test_form.json") } + let(:organisation) { user.organisation } + let(:case_log) do + FactoryBot.create( + :case_log, + owning_organisation: organisation, + managing_organisation: organisation, + ) + end + let(:page_id) { "person_1_age" } + let(:params) do + { + id: case_log.id, + case_log: { + page: page_id, + age1: answer, + }, + } + end + + before do + allow(FormHandler.instance).to receive(:get_form).and_return(form) + post "/case-logs/#{case_log.id}/form", params: params + end + + context "invalid answers" do + let(:answer) { 2000 } + + it "re-renders the same page with errors if validation fails" do + expect(response).to have_http_status(:unprocessable_entity) + end + end + + context "valid answers" do + let(:answer) { 20 } + + it "re-renders the same page with errors if validation fails" do + expect(response).to have_http_status(:redirect) + end + + let(:params) do + { + id: case_log.id, + case_log: { + page: page_id, + age1: answer, + age2: 2000, + }, + } + end + + it "only updates answers that apply to the page being submitted" do + case_log.reload + expect(case_log.age1).to eq(answer) + expect(case_log.age2).to be nil + end + end + end + + context "checkbox questions" do + let(:case_log_form_params) do + { + id: case_log.id, + case_log: { + page: "accessibility_requirements", + accessibility_requirements: + %w[ housingneeds_a + housingneeds_b + housingneeds_c], + }, + } + end + + let(:new_case_log_form_params) do + { + id: case_log.id, + case_log: { + page: "accessibility_requirements", + accessibility_requirements: %w[housingneeds_c], + }, + } + end + + it "sets checked items to true" do + post "/case-logs/#{case_log.id}/form", params: case_log_form_params + case_log.reload + + expect(case_log.housingneeds_a).to eq("Yes") + expect(case_log.housingneeds_b).to eq("Yes") + expect(case_log.housingneeds_c).to eq("Yes") + end + + it "sets previously submitted items to false when resubmitted with new values" do + post "/case-logs/#{case_log.id}/form", params: new_case_log_form_params + case_log.reload + + expect(case_log.housingneeds_a).to eq("No") + expect(case_log.housingneeds_b).to eq("No") + expect(case_log.housingneeds_c).to eq("Yes") + end + + context "given a page with checkbox and non-checkbox questions" do + let(:tenant_code) { "BZ355" } + let(:case_log_form_params) do + { + id: case_log.id, + case_log: { + page: "accessibility_requirements", + accessibility_requirements: + %w[ housingneeds_a + housingneeds_b + housingneeds_c], + tenant_code: tenant_code, + }, + } + end + let(:questions_for_page) do + [ + Form::Question.new( + "accessibility_requirements", + { + "type" => "checkbox", + "answer_options" => + { "housingneeds_a" => "Fully wheelchair accessible housing", + "housingneeds_b" => "Wheelchair access to essential rooms", + "housingneeds_c" => "Level access housing", + "housingneeds_f" => "Other disability requirements", + "housingneeds_g" => "No disability requirements", + "divider_a" => true, + "housingneeds_h" => "Do not know", + "divider_b" => true, + "accessibility_requirements_prefer_not_to_say" => "Prefer not to say" }, + }, nil + ), + Form::Question.new("tenant_code", { "type" => "text" }, nil), + ] + end + + it "updates both question fields" do + allow_any_instance_of(Form::Page).to receive(:expected_responses).and_return(questions_for_page) + post "/case-logs/#{case_log.id}/form", params: case_log_form_params + case_log.reload + + expect(case_log.housingneeds_a).to eq("Yes") + expect(case_log.housingneeds_b).to eq("Yes") + expect(case_log.housingneeds_c).to eq("Yes") + expect(case_log.tenant_code).to eq(tenant_code) + end + end + end + + context "conditional routing" do + before do + allow_any_instance_of(CaseLogValidator).to receive(:validate_pregnancy).and_return(true) + end + + let(:case_log_form_conditional_question_yes_params) do + { + id: case_log.id, + case_log: { + page: "conditional_question", + preg_occ: "Yes", + }, + } + end + + let(:case_log_form_conditional_question_no_params) do + { + id: case_log.id, + case_log: { + page: "conditional_question", + preg_occ: "No", + }, + } + end + + it "routes to the appropriate conditional page based on the question answer of the current page" do + post "/case-logs/#{case_log.id}/form", params: case_log_form_conditional_question_yes_params + expect(response).to redirect_to("/case-logs/#{case_log.id}/conditional-question-yes-page") + + post "/case-logs/#{case_log.id}/form", params: case_log_form_conditional_question_no_params + expect(response).to redirect_to("/case-logs/#{case_log.id}/conditional-question-no-page") + end + end + + context "case logs that are not owned or managed by your organisation" do + let(:answer) { 25 } + let(:other_organisation) { FactoryBot.create(:organisation) } + let(:unauthorized_case_log) do + FactoryBot.create( + :case_log, + owning_organisation: other_organisation, + managing_organisation: other_organisation, + ) + end + + before do + post "/case-logs/#{unauthorized_case_log.id}/form", params: {} + end + + it "does not let you post form answers to case logs you don't have access to" do + expect(response).to have_http_status(:not_found) + end + end + end + end +end diff --git a/spec/requests/organisations_controller_spec.rb b/spec/requests/organisations_controller_spec.rb index be49b8c9d..d086452ac 100644 --- a/spec/requests/organisations_controller_spec.rb +++ b/spec/requests/organisations_controller_spec.rb @@ -2,110 +2,181 @@ require "rails_helper" RSpec.describe OrganisationsController, type: :request do let(:organisation) { user.organisation } + let(:unauthorised_organisation) { FactoryBot.create(:organisation) } let(:headers) { { "Accept" => "text/html" } } let(:page) { Capybara::Node::Simple.new(response.body) } + let(:user) { FactoryBot.create(:user, :data_coordinator) } - describe "#show" do - let(:user) { FactoryBot.create(:user, :data_coordinator) } - - before do - sign_in user - get "/organisations/#{organisation.id}", headers: headers, params: {} - end - - it "redirects to details" do - expect(response).to have_http_status(:redirect) - end - end - - context "As a data coordinator user" do - let(:user) { FactoryBot.create(:user, :data_coordinator) } - - context "details tab" do - before do - sign_in user - get "/organisations/#{organisation.id}/details", headers: headers, params: {} - end - - it "shows the tab navigation" do - expected_html = "