From 7d762bd4f3cdde6377ffee86f0af9e59b6b49669 Mon Sep 17 00:00:00 2001 From: Matthew Phelan Date: Thu, 30 Sep 2021 14:16:47 +0100 Subject: [PATCH 01/15] validation init --- app/controllers/case_logs_controller.rb | 8 +++++--- app/models/case_log.rb | 16 ++++++++++++++++ db/schema.rb | 2 +- 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/app/controllers/case_logs_controller.rb b/app/controllers/case_logs_controller.rb index 56919deda..de7501375 100644 --- a/app/controllers/case_logs_controller.rb +++ b/app/controllers/case_logs_controller.rb @@ -27,9 +27,11 @@ class CaseLogsController < ApplicationController previous_page = params[:previous_page] questions_for_page = form.questions_for_page(previous_page).keys answers_for_page = page_params(questions_for_page).select { |k, _v| questions_for_page.include?(k) } - @case_log.update!(answers_for_page) - next_page = form.next_page(previous_page) - redirect_to(send("case_log_#{next_page}_path", @case_log)) + if @case_log.valid? + @case_log.update!(answers_for_page) + next_page = form.next_page(previous_page) + redirect_to(send("case_log_#{next_page}_path", @case_log)) + end end form = Form.new(2021, 2022) diff --git a/app/models/case_log.rb b/app/models/case_log.rb index 3f860cc08..01092a240 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -1,3 +1,19 @@ +class CaseLogValidator < ActiveModel::Validator + def validate_tenant_age(record) + if record.tenant_age < 0 + record.errors.add :base, "Age needs to be above 0" + elsif record.tenant_age > 120 + record.errors.add :base, "Age needs to be below 120" + end + binding.pry + end + + def validate(record) + validate_tenant_age(record) + end +end + class CaseLog < ApplicationRecord enum status: { "in progress" => 0, "submitted" => 1 } + validates_with CaseLogValidator end diff --git a/db/schema.rb b/db/schema.rb index e3215a238..4dab74fee 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -27,7 +27,6 @@ ActiveRecord::Schema.define(version: 2021_09_24_143031) do t.string "previous_housing_situation" t.integer "prior_homelessness" t.string "armed_forces" - t.string "postcode" t.string "tenant_economic_status" t.integer "household_number_of_other_members" t.string "person_2_relationship" @@ -58,6 +57,7 @@ ActiveRecord::Schema.define(version: 2021_09_24_143031) do t.integer "person_8_age" t.string "person_8_gender" t.string "person_8_economic" + t.string "postcode" t.string "homelessness" t.string "last_settled_home" t.string "benefit_cap_spare_room_subsidy" From 380235a9ed7287b43818c57b40207147d57c79ea Mon Sep 17 00:00:00 2001 From: Matthew Phelan Date: Wed, 6 Oct 2021 08:08:07 +0100 Subject: [PATCH 02/15] error message passed through to front end --- app/controllers/case_logs_controller.rb | 12 +++++++++--- app/models/case_log.rb | 5 +++-- app/views/form/page.html.erb | 5 +++++ 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/app/controllers/case_logs_controller.rb b/app/controllers/case_logs_controller.rb index de7501375..edd0cc4be 100644 --- a/app/controllers/case_logs_controller.rb +++ b/app/controllers/case_logs_controller.rb @@ -24,21 +24,27 @@ class CaseLogsController < ApplicationController def next_page form = Form.new(2021, 2022) @case_log = CaseLog.find(params[:case_log_id]) + previous_page = params[:previous_page] questions_for_page = form.questions_for_page(previous_page).keys answers_for_page = page_params(questions_for_page).select { |k, _v| questions_for_page.include?(k) } - if @case_log.valid? + + @case_log_temp = CaseLog.new(answers_for_page) + if @case_log_temp.valid? @case_log.update!(answers_for_page) next_page = form.next_page(previous_page) redirect_to(send("case_log_#{next_page}_path", @case_log)) + else + @errors = @case_log_temp.errors.full_messages + redirect_to(send("case_log_#{previous_page}_path", @case_log, errors: @errors)) end end form = Form.new(2021, 2022) form.all_pages.map do |page_key, page_info| - define_method(page_key) do + define_method(page_key) do |errors = {}| @case_log = CaseLog.find(params[:case_log_id]) - render "form/page", locals: { case_log_id: @case_log.id, form: form, page_key: page_key, page_info: page_info } + render "form/page", locals: { case_log_id: @case_log.id, form: form, page_key: page_key, page_info: page_info, errors: errors } end end diff --git a/app/models/case_log.rb b/app/models/case_log.rb index 01092a240..d4321572c 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -5,11 +5,12 @@ class CaseLogValidator < ActiveModel::Validator elsif record.tenant_age > 120 record.errors.add :base, "Age needs to be below 120" end - binding.pry end def validate(record) - validate_tenant_age(record) + if record.tenant_age? + validate_tenant_age(record) + end end end diff --git a/app/views/form/page.html.erb b/app/views/form/page.html.erb index 997587d37..d6343d2cf 100644 --- a/app/views/form/page.html.erb +++ b/app/views/form/page.html.erb @@ -6,12 +6,17 @@ <% end %> <%= turbo_frame_tag "case_log_form", target: "_top" do %> + <% if page_info["header"].present? %>

<%= page_info["header"] %>

<% end %> <%= form_with action: '/case_logs', method: "next_page", builder: GOVUKDesignSystemFormBuilder::FormBuilder do |f| %> + <% if params[:errors].present? %> + <%= params[:errors] %> + <%= f.govuk_error_summary 'Uh-oh, spaghettios' %> + <% end %> <% page_info["questions"].map do |question_key, question| %> <%= render partial: "form/#{question["type"]}_question", locals: { question_key: question_key, question: question, f: f } %> From 4a8da65a4d0fcfe6e4dac6729fee2e1368457dd9 Mon Sep 17 00:00:00 2001 From: Matthew Phelan Date: Thu, 7 Oct 2021 12:25:27 +0100 Subject: [PATCH 03/15] Dynamically passing question to validator --- app/controllers/case_logs_controller.rb | 1 + app/models/case_log.rb | 23 ++++++++++++++++++----- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/app/controllers/case_logs_controller.rb b/app/controllers/case_logs_controller.rb index 40a88a8ca..a14adf2e9 100644 --- a/app/controllers/case_logs_controller.rb +++ b/app/controllers/case_logs_controller.rb @@ -26,6 +26,7 @@ class CaseLogsController < ApplicationController previous_page = params[:case_log][:previous_page] questions_for_page = form.questions_for_page(previous_page).keys answers_for_page = page_params(questions_for_page).select { |k, _v| questions_for_page.include?(k) } + @case_log.custom_validator_options = { previous_page: previous_page } if @case_log.update(answers_for_page) redirect_path = form.next_page_redirect_path(previous_page) redirect_to(send(redirect_path, @case_log)) diff --git a/app/models/case_log.rb b/app/models/case_log.rb index d4321572c..d56dba600 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -1,6 +1,9 @@ class CaseLogValidator < ActiveModel::Validator + def validate_tenant_age(record) - if record.tenant_age < 0 + if !record.tenant_age? + record.errors.add :base, "Tenant age can't be blank" + elsif record.tenant_age < 0 record.errors.add :base, "Age needs to be above 0" elsif record.tenant_age > 120 record.errors.add :base, "Age needs to be below 120" @@ -8,13 +11,23 @@ class CaseLogValidator < ActiveModel::Validator end def validate(record) - if record.tenant_age? - validate_tenant_age(record) + question_to_validate = options[:previous_page] + + if question_to_validate == "tenant_code" + if !record.tenant_code? + record.errors.add :base, "Tenant code can't be blank" + end + elsif question_to_validate == "tenant_age" + validate_tenant_age(record) end end end -class CaseLog < ApplicationRecord +class CaseLog < ApplicationRecord + validate :instance_validations + attr_accessor :custom_validator_options enum status: { "in progress" => 0, "submitted" => 1 } - validates_with CaseLogValidator + def instance_validations + validates_with CaseLogValidator, (custom_validator_options || {}) + end end From 47fa66cba3c6085bbf634bb86b6554d6e90a14bf Mon Sep 17 00:00:00 2001 From: Matthew Phelan Date: Thu, 7 Oct 2021 12:41:53 +0100 Subject: [PATCH 04/15] regex validation added --- app/models/case_log.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/models/case_log.rb b/app/models/case_log.rb index d56dba600..4933c7483 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -3,10 +3,8 @@ class CaseLogValidator < ActiveModel::Validator def validate_tenant_age(record) if !record.tenant_age? record.errors.add :base, "Tenant age can't be blank" - elsif record.tenant_age < 0 - record.errors.add :base, "Age needs to be above 0" - elsif record.tenant_age > 120 - record.errors.add :base, "Age needs to be below 120" + elsif !(record.tenant_age.to_s =~ /^[1-9][0-9]?$|^100$/) + record.errors.add :base, "Tenant age must be between 0 and 100" end end From 00569330486cbf3567c58c87f89ab2f8ceece3ea Mon Sep 17 00:00:00 2001 From: Matthew Phelan Date: Thu, 7 Oct 2021 12:52:27 +0100 Subject: [PATCH 05/15] regex added to form json --- app/controllers/case_logs_controller.rb | 2 +- app/models/case_log.rb | 9 +++++---- config/forms/2021_2022.json | 3 ++- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/app/controllers/case_logs_controller.rb b/app/controllers/case_logs_controller.rb index a14adf2e9..45973a8ba 100644 --- a/app/controllers/case_logs_controller.rb +++ b/app/controllers/case_logs_controller.rb @@ -26,7 +26,7 @@ class CaseLogsController < ApplicationController previous_page = params[:case_log][:previous_page] questions_for_page = form.questions_for_page(previous_page).keys answers_for_page = page_params(questions_for_page).select { |k, _v| questions_for_page.include?(k) } - @case_log.custom_validator_options = { previous_page: previous_page } + @case_log.custom_validator_options = { previous_page: previous_page, validation: form.questions_for_page(previous_page)[previous_page]["regex_validation"] } if @case_log.update(answers_for_page) redirect_path = form.next_page_redirect_path(previous_page) redirect_to(send(redirect_path, @case_log)) diff --git a/app/models/case_log.rb b/app/models/case_log.rb index 4933c7483..cbe687701 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -1,22 +1,23 @@ class CaseLogValidator < ActiveModel::Validator - def validate_tenant_age(record) + def validate_tenant_age(record, validation_regex) + regexp = Regexp.new validation_regex if !record.tenant_age? record.errors.add :base, "Tenant age can't be blank" - elsif !(record.tenant_age.to_s =~ /^[1-9][0-9]?$|^100$/) + elsif !(record.tenant_age.to_s =~ regexp) record.errors.add :base, "Tenant age must be between 0 and 100" end end def validate(record) question_to_validate = options[:previous_page] - + validation_regex = options[:validation] if question_to_validate == "tenant_code" if !record.tenant_code? record.errors.add :base, "Tenant code can't be blank" end elsif question_to_validate == "tenant_age" - validate_tenant_age(record) + validate_tenant_age(record, validation_regex) end end end diff --git a/config/forms/2021_2022.json b/config/forms/2021_2022.json index a5b3072ce..ef311aa6b 100644 --- a/config/forms/2021_2022.json +++ b/config/forms/2021_2022.json @@ -32,7 +32,8 @@ "type": "numeric", "min": 0, "max": 150, - "step": 1 + "step": 1, + "regex_validation" : "^[1-9][0-9]?$|^100$" } } }, From 88dbd595888e33e0892e10968d6675368e2073bb Mon Sep 17 00:00:00 2001 From: Matthew Phelan Date: Fri, 8 Oct 2021 14:38:49 +0100 Subject: [PATCH 06/15] base to question type --- app/models/case_log.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/case_log.rb b/app/models/case_log.rb index cbe687701..13c7a2f7a 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -3,9 +3,9 @@ class CaseLogValidator < ActiveModel::Validator def validate_tenant_age(record, validation_regex) regexp = Regexp.new validation_regex if !record.tenant_age? - record.errors.add :base, "Tenant age can't be blank" + record.errors.add :tenant_age, "Tenant age can't be blank" elsif !(record.tenant_age.to_s =~ regexp) - record.errors.add :base, "Tenant age must be between 0 and 100" + record.errors.add :tenant_age, "Tenant age must be between 0 and 100" end end @@ -14,7 +14,7 @@ class CaseLogValidator < ActiveModel::Validator validation_regex = options[:validation] if question_to_validate == "tenant_code" if !record.tenant_code? - record.errors.add :base, "Tenant code can't be blank" + record.errors.add :tenant_code, "Tenant code can't be blank" end elsif question_to_validate == "tenant_age" validate_tenant_age(record, validation_regex) From 301ae82a597bd7ddc003068429cfcb3d5918a633 Mon Sep 17 00:00:00 2001 From: Matthew Phelan Date: Mon, 11 Oct 2021 11:23:48 +0100 Subject: [PATCH 07/15] review comments update --- app/controllers/case_logs_controller.rb | 2 +- app/models/case_log.rb | 35 +++++++++++++++---------- config/forms/2021_2022.json | 3 +-- db/schema.rb | 1 + 4 files changed, 24 insertions(+), 17 deletions(-) diff --git a/app/controllers/case_logs_controller.rb b/app/controllers/case_logs_controller.rb index 515275f09..c4619b169 100644 --- a/app/controllers/case_logs_controller.rb +++ b/app/controllers/case_logs_controller.rb @@ -26,7 +26,7 @@ class CaseLogsController < ApplicationController previous_page = params[:case_log][:previous_page] questions_for_page = form.questions_for_page(previous_page).keys answers_for_page = page_params(questions_for_page).select { |k, _v| questions_for_page.include?(k) } - @case_log.custom_validator_options = { previous_page: previous_page, validation: form.questions_for_page(previous_page)[previous_page]["regex_validation"] } + @case_log.previous_page(previous_page) if @case_log.update(answers_for_page) redirect_path = form.next_page_redirect_path(previous_page) redirect_to(send(redirect_path, @case_log)) diff --git a/app/models/case_log.rb b/app/models/case_log.rb index 13c7a2f7a..5e4f99c48 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -1,32 +1,39 @@ class CaseLogValidator < ActiveModel::Validator - def validate_tenant_age(record, validation_regex) - regexp = Regexp.new validation_regex - if !record.tenant_age? + # Methods need to be named 'validate_' followed by field name + # this is how the metaprogramming of the method name being + # call in the validate method works. + + def validate_tenant_code(record) + if record.tenant_code.blank? + record.errors.add :tenant_code, "Tenant code can't be blank" + end + end + + def validate_tenant_age(record) + if record.tenant_age.blank? record.errors.add :tenant_age, "Tenant age can't be blank" - elsif !(record.tenant_age.to_s =~ regexp) + elsif !(record.tenant_age.to_s =~ /^[1-9][0-9]?$|^100$/) record.errors.add :tenant_age, "Tenant age must be between 0 and 100" end end def validate(record) question_to_validate = options[:previous_page] - validation_regex = options[:validation] - if question_to_validate == "tenant_code" - if !record.tenant_code? - record.errors.add :tenant_code, "Tenant code can't be blank" - end - elsif question_to_validate == "tenant_age" - validate_tenant_age(record, validation_regex) - end + public_send("validate_#{question_to_validate}", record) end end class CaseLog < ApplicationRecord validate :instance_validations - attr_accessor :custom_validator_options + @previous_page enum status: { "in progress" => 0, "submitted" => 1 } + + def previous_page(value) + @previous_page = value + end + def instance_validations - validates_with CaseLogValidator, (custom_validator_options || {}) + validates_with CaseLogValidator, ({ previous_page: @previous_page } || {}) end end diff --git a/config/forms/2021_2022.json b/config/forms/2021_2022.json index 1001224ad..eb91f81e3 100644 --- a/config/forms/2021_2022.json +++ b/config/forms/2021_2022.json @@ -32,8 +32,7 @@ "type": "numeric", "min": 0, "max": 150, - "step": 1, - "regex_validation" : "^[1-9][0-9]?$|^100$" + "step": 1 } } }, diff --git a/db/schema.rb b/db/schema.rb index 648ded320..706deaf3b 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -109,6 +109,7 @@ ActiveRecord::Schema.define(version: 2021_10_05_115813) do t.string "chr_letting" t.string "cap_letting" t.string "outstanding_rent_or_charges" + t.string "other_reason_for_leaving_last_settled_home" end end From a6feca49785a93b16a6593f20cdc3ad7c09ea009 Mon Sep 17 00:00:00 2001 From: Matthew Phelan Date: Mon, 11 Oct 2021 11:44:43 +0100 Subject: [PATCH 08/15] previous_page null check --- app/models/case_log.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/models/case_log.rb b/app/models/case_log.rb index 5e4f99c48..f6bf7639e 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -20,7 +20,9 @@ class CaseLogValidator < ActiveModel::Validator def validate(record) question_to_validate = options[:previous_page] - public_send("validate_#{question_to_validate}", record) + if question_to_validate.present? + public_send("validate_#{question_to_validate}", record) + end end end From 55dedc1781676528f3d28b572f6c9cb0cf2df2f2 Mon Sep 17 00:00:00 2001 From: Matthew Phelan Date: Mon, 11 Oct 2021 13:43:34 +0100 Subject: [PATCH 09/15] valid method name check --- app/models/case_log.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/case_log.rb b/app/models/case_log.rb index f6bf7639e..d81b83f2c 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -20,7 +20,7 @@ class CaseLogValidator < ActiveModel::Validator def validate(record) question_to_validate = options[:previous_page] - if question_to_validate.present? + if respond_to?("validate_#{question_to_validate}") public_send("validate_#{question_to_validate}", record) end end From dc3e13cbb01ee5cbef3d1d770cafbb19518b6312 Mon Sep 17 00:00:00 2001 From: Matthew Phelan Date: Tue, 12 Oct 2021 09:48:55 +0100 Subject: [PATCH 10/15] Attr writter added --- app/controllers/case_logs_controller.rb | 2 +- app/models/case_log.rb | 6 +----- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/app/controllers/case_logs_controller.rb b/app/controllers/case_logs_controller.rb index c4619b169..93cc367db 100644 --- a/app/controllers/case_logs_controller.rb +++ b/app/controllers/case_logs_controller.rb @@ -26,7 +26,7 @@ class CaseLogsController < ApplicationController previous_page = params[:case_log][:previous_page] questions_for_page = form.questions_for_page(previous_page).keys answers_for_page = page_params(questions_for_page).select { |k, _v| questions_for_page.include?(k) } - @case_log.previous_page(previous_page) + @case_log.previous_page = previous_page if @case_log.update(answers_for_page) redirect_path = form.next_page_redirect_path(previous_page) redirect_to(send(redirect_path, @case_log)) diff --git a/app/models/case_log.rb b/app/models/case_log.rb index d81b83f2c..bb8dbe54c 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -28,13 +28,9 @@ end class CaseLog < ApplicationRecord validate :instance_validations - @previous_page + attr_writer :previous_page enum status: { "in progress" => 0, "submitted" => 1 } - def previous_page(value) - @previous_page = value - end - def instance_validations validates_with CaseLogValidator, ({ previous_page: @previous_page } || {}) end From 541b850015775ed34554b703a93ed56ed0126920 Mon Sep 17 00:00:00 2001 From: baarkerlounger Date: Tue, 12 Oct 2021 09:57:37 +0100 Subject: [PATCH 11/15] Linting --- app/controllers/case_logs_controller.rb | 2 +- app/models/case_log.rb | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/controllers/case_logs_controller.rb b/app/controllers/case_logs_controller.rb index 050542551..f039e2d56 100644 --- a/app/controllers/case_logs_controller.rb +++ b/app/controllers/case_logs_controller.rb @@ -45,7 +45,7 @@ class CaseLogsController < ApplicationController form = Form.new(2021, 2022) form.all_pages.map do |page_key, page_info| - define_method(page_key) do |errors = {}| + define_method(page_key) do |_errors = {}| @case_log = CaseLog.find(params[:case_log_id]) render "form/page", locals: { form: form, page_key: page_key, page_info: page_info } end diff --git a/app/models/case_log.rb b/app/models/case_log.rb index bb8dbe54c..aa17eddf6 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -1,7 +1,6 @@ class CaseLogValidator < ActiveModel::Validator - # Methods need to be named 'validate_' followed by field name - # this is how the metaprogramming of the method name being + # this is how the metaprogramming of the method name being # call in the validate method works. def validate_tenant_code(record) @@ -13,7 +12,7 @@ class CaseLogValidator < ActiveModel::Validator def validate_tenant_age(record) if record.tenant_age.blank? record.errors.add :tenant_age, "Tenant age can't be blank" - elsif !(record.tenant_age.to_s =~ /^[1-9][0-9]?$|^100$/) + elsif !/^[1-9][0-9]?$|^100$/.match?(record.tenant_age.to_s) record.errors.add :tenant_age, "Tenant age must be between 0 and 100" end end @@ -26,9 +25,10 @@ class CaseLogValidator < ActiveModel::Validator end end -class CaseLog < ApplicationRecord +class CaseLog < ApplicationRecord validate :instance_validations attr_writer :previous_page + enum status: { "in progress" => 0, "submitted" => 1 } def instance_validations From 07c24913a9e413eef708a2df84c1ba2bf4a8c3a9 Mon Sep 17 00:00:00 2001 From: baarkerlounger Date: Tue, 12 Oct 2021 10:01:14 +0100 Subject: [PATCH 12/15] Fix migration --- db/migrate/20211011115946_rename_economic_status_fields.rb | 2 +- db/schema.rb | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/db/migrate/20211011115946_rename_economic_status_fields.rb b/db/migrate/20211011115946_rename_economic_status_fields.rb index b7c6a94ab..295980bae 100644 --- a/db/migrate/20211011115946_rename_economic_status_fields.rb +++ b/db/migrate/20211011115946_rename_economic_status_fields.rb @@ -8,7 +8,7 @@ class RenameEconomicStatusFields < ActiveRecord::Migration[6.1] t.rename :person_6_economic, :person_6_economic_status t.rename :person_7_economic, :person_7_economic_status t.rename :person_8_economic, :person_8_economic_status - t.rename :postcode, :property_postcode + t.remove :postcode end end end diff --git a/db/schema.rb b/db/schema.rb index 539a5d32c..08f44eb0a 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -57,7 +57,6 @@ ActiveRecord::Schema.define(version: 2021_10_11_115946) do t.integer "person_8_age" t.string "person_8_gender" t.string "person_8_economic_status" - t.string "postcode" t.string "homelessness" t.string "reason_for_leaving_last_settled_home" t.string "benefit_cap_spare_room_subsidy" @@ -102,6 +101,7 @@ ActiveRecord::Schema.define(version: 2021_10_11_115946) do t.string "time_lived_in_la" t.string "time_on_la_waiting_list" t.string "previous_la" + t.string "property_postcode" t.string "reasonable_preference" t.string "reasonable_preference_reason" t.string "cbl_letting" @@ -132,7 +132,6 @@ ActiveRecord::Schema.define(version: 2021_10_11_115946) do t.boolean "reasonable_preference_reason_medical_grounds" t.boolean "reasonable_preference_reason_avoid_hardship" t.boolean "reasonable_preference_reason_do_not_know" - t.string "property_postcode" end end From 4886a3ceb50c578bcf121259a808d9e4137951ee Mon Sep 17 00:00:00 2001 From: baarkerlounger Date: Tue, 12 Oct 2021 10:01:42 +0100 Subject: [PATCH 13/15] Make it reversible --- db/migrate/20211011115946_rename_economic_status_fields.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/migrate/20211011115946_rename_economic_status_fields.rb b/db/migrate/20211011115946_rename_economic_status_fields.rb index 295980bae..a9c4f1e72 100644 --- a/db/migrate/20211011115946_rename_economic_status_fields.rb +++ b/db/migrate/20211011115946_rename_economic_status_fields.rb @@ -8,7 +8,7 @@ class RenameEconomicStatusFields < ActiveRecord::Migration[6.1] t.rename :person_6_economic, :person_6_economic_status t.rename :person_7_economic, :person_7_economic_status t.rename :person_8_economic, :person_8_economic_status - t.remove :postcode + t.remove :postcode, :string end end end From bb4ef5e16803e8ae90cb8becf2b8369b8b629a7e Mon Sep 17 00:00:00 2001 From: baarkerlounger Date: Tue, 12 Oct 2021 10:22:40 +0100 Subject: [PATCH 14/15] Make the migration actually reversible --- ...11011115946_rename_economic_status_fields.rb | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/db/migrate/20211011115946_rename_economic_status_fields.rb b/db/migrate/20211011115946_rename_economic_status_fields.rb index a9c4f1e72..6862554bb 100644 --- a/db/migrate/20211011115946_rename_economic_status_fields.rb +++ b/db/migrate/20211011115946_rename_economic_status_fields.rb @@ -1,5 +1,5 @@ class RenameEconomicStatusFields < ActiveRecord::Migration[6.1] - def change + def up change_table :case_logs, bulk: true do |t| t.rename :person_2_economic, :person_2_economic_status t.rename :person_3_economic, :person_3_economic_status @@ -8,7 +8,20 @@ class RenameEconomicStatusFields < ActiveRecord::Migration[6.1] t.rename :person_6_economic, :person_6_economic_status t.rename :person_7_economic, :person_7_economic_status t.rename :person_8_economic, :person_8_economic_status - t.remove :postcode, :string end + remove_column :case_logs, :postcode + end + + def down + change_table :case_logs, bulk: true do |t| + t.rename :person_2_economic_status, :person_2_economic + t.rename :person_3_economic_status, :person_3_economic + t.rename :person_4_economic_status, :person_4_economic + t.rename :person_5_economic_status, :person_5_economic + t.rename :person_6_economic_status, :person_6_economic + t.rename :person_7_economic_status, :person_7_economic + t.rename :person_8_economic_status, :person_8_economic + end + add_column :case_logs, :postcode, :string end end From 8e93fb976ded0742e34822a91e0c0fe19b892f07 Mon Sep 17 00:00:00 2001 From: Daniel Baark <5101747+baarkerlounger@users.noreply.github.com> Date: Tue, 12 Oct 2021 10:47:33 +0100 Subject: [PATCH 15/15] =?UTF-8?q?Navigate=20straight=20to=20check=20answer?= =?UTF-8?q?s=20page=20if=20a=20section=20has=20been=20started=E2=80=A6=20(?= =?UTF-8?q?#41)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Navigate straight to check answers page if a section has been started already * Code review suggestion * Remove Form config specific test --- app/helpers/tasklist_helper.rb | 14 +++++++ app/views/case_logs/_tasklist.html.erb | 7 ++-- spec/features/case_log_spec.rb | 12 ------ spec/helpers/tasklist_helper_spec.rb | 51 ++++++++++++++------------ 4 files changed, 45 insertions(+), 39 deletions(-) diff --git a/app/helpers/tasklist_helper.rb b/app/helpers/tasklist_helper.rb index 4c831029c..549ef2d22 100644 --- a/app/helpers/tasklist_helper.rb +++ b/app/helpers/tasklist_helper.rb @@ -36,6 +36,15 @@ module TasklistHelper subsections.count { |subsection| get_subsection_status(subsection, case_log, form.questions_for_subsection(subsection).keys) == status } end + def get_first_page_or_check_answers(subsection, case_log, form, questions) + path = if is_started?(subsection, case_log, questions) + "case_log_#{subsection}_check_answers_path" + else + "case_log_#{form.first_page_for_subsection(subsection)}_path" + end + send(path, case_log) + end + private def all_questions_completed(case_log) @@ -46,4 +55,9 @@ private status = get_subsection_status(subsection, case_log, questions) %i[not_started in_progress].include?(status) end + + def is_started?(subsection, case_log, questions) + status = get_subsection_status(subsection, case_log, questions) + %i[in_progress completed].include?(status) + end end diff --git a/app/views/case_logs/_tasklist.html.erb b/app/views/case_logs/_tasklist.html.erb index dbbeb6321..fdf4c5aa9 100644 --- a/app/views/case_logs/_tasklist.html.erb +++ b/app/views/case_logs/_tasklist.html.erb @@ -9,9 +9,10 @@
    <% section_value["subsections"].map do |subsection_key, subsection_value| %>
  • > - <% first_page = @form.first_page_for_subsection(subsection_key) %> - <%= link_to subsection_value["label"], send("case_log_#{first_page}_path", @case_log), class: "task-name govuk-link" %> - <% subsection_status=get_subsection_status(subsection_key, @case_log, @form.questions_for_subsection(subsection_key).keys) %> + <% questions_for_subsection = @form.questions_for_subsection(subsection_key).keys %> + <% next_page_path = get_first_page_or_check_answers(subsection_key, @case_log, @form, questions_for_subsection) %> + <%= link_to subsection_value["label"], next_page_path, class: "task-name govuk-link" %> + <% subsection_status = get_subsection_status(subsection_key, @case_log, questions_for_subsection) %> <%= TasklistHelper::STATUSES[subsection_status] %> diff --git a/spec/features/case_log_spec.rb b/spec/features/case_log_spec.rb index b83ccb932..d3962ad2c 100644 --- a/spec/features/case_log_spec.rb +++ b/spec/features/case_log_spec.rb @@ -83,18 +83,6 @@ RSpec.describe "Test Features" do end end - it "displays the household questions when you click into that section" do - visit("/case_logs/#{id}") - click_link("Household characteristics") - expect(page).to have_field("case-log-tenant-code-field") - click_button("Save and continue") - expect(page).to have_field("case-log-tenant-age-field") - click_button("Save and continue") - expect(page).to have_field("case-log-tenant-gender-male-field") - visit page.driver.request.env["HTTP_REFERER"] - expect(page).to have_field("case-log-tenant-age-field") - end - describe "form questions" do let(:case_log_with_checkbox_questions_answered) do FactoryBot.create( diff --git a/spec/helpers/tasklist_helper_spec.rb b/spec/helpers/tasklist_helper_spec.rb index f7b2a6534..7078159cd 100644 --- a/spec/helpers/tasklist_helper_spec.rb +++ b/spec/helpers/tasklist_helper_spec.rb @@ -1,12 +1,15 @@ require "rails_helper" RSpec.describe TasklistHelper do + let!(:empty_case_log) { FactoryBot.create(:case_log) } + let!(:case_log) { FactoryBot.create(:case_log, :in_progress) } + let(:form) { Form.new(2021, 2022) } + describe "get subsection status" do - @form = Form.new(2021, 2022) - income_and_benefits_questions = @form.questions_for_subsection("income_and_benefits").keys - declaration_questions = @form.questions_for_subsection("declaration").keys - local_authority_questions = @form.questions_for_subsection("local_authority").keys - let!(:case_log) { FactoryBot.create(:case_log) } + let(:section) { "income_and_benefits" } + let(:income_and_benefits_questions) { form.questions_for_subsection("income_and_benefits").keys } + let(:declaration_questions) { form.questions_for_subsection("declaration").keys } + let(:local_authority_questions) { form.questions_for_subsection("local_authority").keys } it "returns not started if none of the questions in the subsection are answered" do status = get_subsection_status("income_and_benefits", case_log, income_and_benefits_questions) @@ -38,47 +41,47 @@ RSpec.describe TasklistHelper do end describe "get next incomplete section" do - let!(:case_log) { FactoryBot.create(:case_log) } - it "returns the first subsection name if it is not completed" do - @form = Form.new(2021, 2022) - expect(get_next_incomplete_section(@form, case_log)).to eq("household_characteristics") + expect(get_next_incomplete_section(form, case_log)).to eq("household_characteristics") end it "returns the first subsection name if it is partially completed" do - @form = Form.new(2021, 2022) case_log["tenant_code"] = 123 - expect(get_next_incomplete_section(@form, case_log)).to eq("household_characteristics") + expect(get_next_incomplete_section(form, case_log)).to eq("household_characteristics") end end describe "get sections count" do - let!(:empty_case_log) { FactoryBot.create(:case_log) } - let!(:case_log) { FactoryBot.create(:case_log, :in_progress) } - it "returns the total of sections if no status is given" do - @form = Form.new(2021, 2022) - expect(get_sections_count(@form, empty_case_log)).to eq(9) + expect(get_sections_count(form, empty_case_log)).to eq(9) end it "returns 0 sections for completed sections if no sections are completed" do - @form = Form.new(2021, 2022) - expect(get_sections_count(@form, empty_case_log, :completed)).to eq(0) + expect(get_sections_count(form, empty_case_log, :completed)).to eq(0) end it "returns the number of not started sections" do - @form = Form.new(2021, 2022) - expect(get_sections_count(@form, empty_case_log, :not_started)).to eq(8) + expect(get_sections_count(form, empty_case_log, :not_started)).to eq(8) end it "returns the number of sections in progress" do - @form = Form.new(2021, 2022) - expect(get_sections_count(@form, case_log, :in_progress)).to eq(3) + expect(get_sections_count(form, case_log, :in_progress)).to eq(3) end it "returns 0 for invalid state" do - @form = Form.new(2021, 2022) - expect(get_sections_count(@form, case_log, :fake)).to eq(0) + expect(get_sections_count(form, case_log, :fake)).to eq(0) + end + end + + describe "get_first_page_or_check_answers" do + let(:household_characteristics_questions) { form.questions_for_subsection("household_characteristics").keys } + + it "returns the check answers page path if the section has been started already" do + expect(get_first_page_or_check_answers("household_characteristics", case_log, form, household_characteristics_questions)).to match(/check_answers/) + end + + it "returns the first question page path for the section if it has not been started yet" do + expect(get_first_page_or_check_answers("household_characteristics", empty_case_log, form, household_characteristics_questions)).to match(/tenant_code/) end end end