From 7f5fc281e4daa8c31665fcd00785fb57a79e1b0f Mon Sep 17 00:00:00 2001 From: Theodor Vararu Date: Tue, 5 Sep 2023 11:11:53 +0100 Subject: [PATCH 1/4] Define steps in ConsentForm Replace the `on:` context based validation with conditional validation that relies on specifying a `form_step`. This also makes the validations fire off for previous steps, and at the end for the final `record` step. Unifies the `*_params` methods in the EditController into one `update_params` method. `required_for_step?` takes an optional `exact` parameter, for transient steps like the `school` one that ask a question but don't actually save a field to the database (and shouldn't be re-validated on other steps). Overall approach based on: https://www.joshmcarthur.com/2014/12/23/rails-multistep-forms.html --- .../consent_forms/edit_controller.rb | 39 +++++------ app/controllers/consent_forms_controller.rb | 3 +- app/models/consent_form.rb | 65 +++++++++++++------ 3 files changed, 62 insertions(+), 45 deletions(-) diff --git a/app/controllers/consent_forms/edit_controller.rb b/app/controllers/consent_forms/edit_controller.rb index 49ba6ea28..224e07182 100644 --- a/app/controllers/consent_forms/edit_controller.rb +++ b/app/controllers/consent_forms/edit_controller.rb @@ -4,7 +4,7 @@ class ConsentForms::EditController < ConsentForms::BaseController layout "two_thirds" - steps :name, :date_of_birth, :school + steps(*ConsentForm.form_steps) before_action :set_session before_action :set_consent_form @@ -15,14 +15,10 @@ def show end def update + @consent_form.assign_attributes(update_params) + case current_step - when :name - @consent_form.assign_attributes(name_params) - when :date_of_birth - @consent_form.assign_attributes(date_of_birth_params) when :school - @consent_form.assign_attributes(school_params) - if @consent_form.is_this_their_school == "no" return( redirect_to session_consent_form_cannot_consent_path( @@ -33,7 +29,7 @@ def update end end - render_wizard @consent_form, context: current_step + render_wizard @consent_form end private @@ -46,20 +42,17 @@ def finish_wizard_path session_consent_form_confirm_path(@session, @consent_form) end - def name_params - params.fetch(:consent_form, {}).permit( - %i[first_name last_name use_common_name common_name] - ) - end - - def date_of_birth_params - params.fetch(:consent_form, {}).permit( - %i[date_of_birth(3i) date_of_birth(2i) date_of_birth(1i)] - ) - end - - def school_params - params.fetch(:consent_form, {}).permit(%i[is_this_their_school]) + def update_params + permitted_attributes = { + name: %i[first_name last_name use_common_name common_name], + date_of_birth: %i[date_of_birth(3i) date_of_birth(2i) date_of_birth(1i)], + school: %i[is_this_their_school] + }.fetch(current_step) + + params + .fetch(:consent_form, {}) + .permit(permitted_attributes) + .merge(form_step: current_step) end def set_session @@ -77,7 +70,7 @@ def validate_params DateParamsValidator.new( field_name: :date_of_birth, object: @consent_form, - params: date_of_birth_params + params: update_params ) unless validator.date_params_valid? diff --git a/app/controllers/consent_forms_controller.rb b/app/controllers/consent_forms_controller.rb index 0d270cc01..5cb4b5e8b 100644 --- a/app/controllers/consent_forms_controller.rb +++ b/app/controllers/consent_forms_controller.rb @@ -8,7 +8,8 @@ def start end def create - consent_form = @session.consent_forms.create! + consent_form = @session.consent_forms.new + consent_form.save!(validate: false) redirect_to session_consent_form_edit_path(@session, consent_form, :name) end diff --git a/app/models/consent_form.rb b/app/models/consent_form.rb index 56b27a0b2..eac4f0eb3 100644 --- a/app/models/consent_form.rb +++ b/app/models/consent_form.rb @@ -23,34 +23,57 @@ # class ConsentForm < ApplicationRecord - attr_accessor :is_this_their_school + cattr_accessor :form_steps do + %i[name date_of_birth school] + end + + attr_accessor :form_step, :is_this_their_school audited belongs_to :session - validates :first_name, presence: true, on: :name - validates :last_name, presence: true, on: :name - validates :use_common_name, inclusion: { in: [true, false] }, on: :name - validates :common_name, presence: true, on: :name, if: :use_common_name? - - validates :date_of_birth, - presence: true, - comparison: { - less_than: Time.zone.today, - greater_than_or_equal_to: 22.years.ago.to_date, - less_than_or_equal_to: 3.years.ago.to_date - }, - on: :date_of_birth - - validates :is_this_their_school, - presence: true, - inclusion: { - in: %w[yes no] - }, - on: :school + with_options if: -> { required_for_step?(:name) } do + validates :first_name, presence: true + validates :last_name, presence: true + validates :use_common_name, inclusion: { in: [true, false] } + validates :common_name, presence: true, if: :use_common_name? + end + + with_options if: -> { required_for_step?(:date_of_birth) } do + validates :date_of_birth, + presence: true, + comparison: { + less_than: Time.zone.today, + greater_than_or_equal_to: 22.years.ago.to_date, + less_than_or_equal_to: 3.years.ago.to_date + } + end + + with_options if: -> { required_for_step?(:school, exact: true) } do + validates :is_this_their_school, + presence: true, + inclusion: { + in: %w[yes no] + } + end def full_name [first_name, last_name].join(" ") end + + private + + def required_for_step?(step, exact: false) + # Exact means that the form_step must match the step + return false if exact && form_step != step + + # All fields are required if no form_step is set + return true if form_step.nil? + + # Otherwise, all fields from previous and current steps are required + return true if form_steps.index(step) <= form_steps.index(form_step) + + false + end end From 99a774da7db8e066f24b3be4ac7998e9eb49165a Mon Sep 17 00:00:00 2001 From: Theodor Vararu Date: Tue, 5 Sep 2023 16:31:02 +0100 Subject: [PATCH 2/4] Wrap ConsentForm validations with on: :update option [-w] Allows us to use .create! as before. --- app/controllers/consent_forms_controller.rb | 3 +- app/models/consent_form.rb | 44 +++++++++++---------- 2 files changed, 24 insertions(+), 23 deletions(-) diff --git a/app/controllers/consent_forms_controller.rb b/app/controllers/consent_forms_controller.rb index 5cb4b5e8b..0d270cc01 100644 --- a/app/controllers/consent_forms_controller.rb +++ b/app/controllers/consent_forms_controller.rb @@ -8,8 +8,7 @@ def start end def create - consent_form = @session.consent_forms.new - consent_form.save!(validate: false) + consent_form = @session.consent_forms.create! redirect_to session_consent_form_edit_path(@session, consent_form, :name) end diff --git a/app/models/consent_form.rb b/app/models/consent_form.rb index eac4f0eb3..78ece2977 100644 --- a/app/models/consent_form.rb +++ b/app/models/consent_form.rb @@ -33,29 +33,31 @@ class ConsentForm < ApplicationRecord belongs_to :session - with_options if: -> { required_for_step?(:name) } do - validates :first_name, presence: true - validates :last_name, presence: true - validates :use_common_name, inclusion: { in: [true, false] } - validates :common_name, presence: true, if: :use_common_name? - end + with_options on: :update do + with_options if: -> { required_for_step?(:name) } do + validates :first_name, presence: true + validates :last_name, presence: true + validates :use_common_name, inclusion: { in: [true, false] } + validates :common_name, presence: true, if: :use_common_name? + end - with_options if: -> { required_for_step?(:date_of_birth) } do - validates :date_of_birth, - presence: true, - comparison: { - less_than: Time.zone.today, - greater_than_or_equal_to: 22.years.ago.to_date, - less_than_or_equal_to: 3.years.ago.to_date - } - end + with_options if: -> { required_for_step?(:date_of_birth) } do + validates :date_of_birth, + presence: true, + comparison: { + less_than: Time.zone.today, + greater_than_or_equal_to: 22.years.ago.to_date, + less_than_or_equal_to: 3.years.ago.to_date + } + end - with_options if: -> { required_for_step?(:school, exact: true) } do - validates :is_this_their_school, - presence: true, - inclusion: { - in: %w[yes no] - } + with_options if: -> { required_for_step?(:school, exact: true) } do + validates :is_this_their_school, + presence: true, + inclusion: { + in: %w[yes no] + } + end end def full_name From edef7a5a872c0c2eef8aeb6d181cf8ced6894076 Mon Sep 17 00:00:00 2001 From: Theodor Vararu Date: Wed, 6 Sep 2023 09:14:06 +0100 Subject: [PATCH 3/4] Add shoulda-matchers test dependency Allows us to use a more terse syntax for testing validations. --- Gemfile | 1 + Gemfile.lock | 3 +++ spec/rails_helper.rb | 7 +++++++ 3 files changed, 11 insertions(+) diff --git a/Gemfile b/Gemfile index 7094b8a57..77af9e869 100644 --- a/Gemfile +++ b/Gemfile @@ -59,5 +59,6 @@ group :test do gem "capybara" gem "cuprite" gem "rspec" + gem "shoulda-matchers" gem "webmock" end diff --git a/Gemfile.lock b/Gemfile.lock index 66c2a26e9..149bbda56 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -472,6 +472,8 @@ GEM sentry-ruby (~> 5.10.0) sentry-ruby (5.10.0) concurrent-ruby (~> 1.0, >= 1.0.2) + shoulda-matchers (5.3.0) + activesupport (>= 5.2.0) silencer (2.0.0) solargraph (0.49.0) backport (~> 1.2) @@ -586,6 +588,7 @@ DEPENDENCIES rubocop-govuk sentry-rails sentry-ruby + shoulda-matchers silencer solargraph solargraph-rails diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index e77ae17dd..77279a92b 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -21,6 +21,13 @@ end Capybara.javascript_driver = :cuprite_custom +Shoulda::Matchers.configure do |config| + config.integrate do |with| + with.test_framework :rspec + with.library :rails + end +end + # Requires supporting ruby files with custom matchers and macros, etc, in # spec/support/ and its subdirectories. Files matching `spec/**/*_spec.rb` are # run as spec files by default. This means that files in spec/support that end From cacaf8909b25aff36006e817ec802676de3d2b55 Mon Sep 17 00:00:00 2001 From: Theodor Vararu Date: Wed, 6 Sep 2023 09:30:13 +0100 Subject: [PATCH 4/4] Add validation tests for ConsentForm --- spec/factories/consent_forms.rb | 4 ++ spec/models/consent_form_spec.rb | 66 ++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+) create mode 100644 spec/factories/consent_forms.rb create mode 100644 spec/models/consent_form_spec.rb diff --git a/spec/factories/consent_forms.rb b/spec/factories/consent_forms.rb new file mode 100644 index 000000000..1209aa3e4 --- /dev/null +++ b/spec/factories/consent_forms.rb @@ -0,0 +1,4 @@ +FactoryBot.define do + factory :consent_form do + end +end diff --git a/spec/models/consent_form_spec.rb b/spec/models/consent_form_spec.rb new file mode 100644 index 000000000..a3a3c5b61 --- /dev/null +++ b/spec/models/consent_form_spec.rb @@ -0,0 +1,66 @@ +require "rails_helper" + +RSpec.describe ConsentForm, type: :model do + describe "Validations" do + let(:form_step) { nil } + let(:use_common_name) { false } + subject { build(:consent_form, form_step:, use_common_name:) } + + it "validates all fields when no form_step is set" do + expect(subject).to validate_presence_of(:first_name).on(:update) + expect(subject).to validate_presence_of(:last_name).on(:update) + expect(subject).to validate_presence_of(:date_of_birth).on(:update) + end + + context "when form_step is :name" do + let(:form_step) { :name } + + it { should validate_presence_of(:first_name).on(:update) } + it { should validate_presence_of(:last_name).on(:update) } + + context "when use_common_name is true" do + let(:use_common_name) { true } + + it { should validate_presence_of(:common_name).on(:update) } + end + end + + context "when form_step is :date_of_birth" do + let(:form_step) { :date_of_birth } + + context "runs validations from previous steps" do + it { should validate_presence_of(:first_name).on(:update) } + end + + it { should validate_presence_of(:date_of_birth).on(:update) } + # it { should validate_comparison_of(:date_of_birth) + # .is_less_than(Time.zone.today) + # .is_greater_than_or_equal_to(22.years.ago.to_date) + # .is_less_than_or_equal_to(3.years.ago.to_date) + # .on(:update) } + end + + context "when form_step is :school" do + let(:form_step) { :school } + + context "runs validations from previous steps" do + it { should validate_presence_of(:first_name).on(:update) } + it { should validate_presence_of(:date_of_birth).on(:update) } + end + + it { should validate_presence_of(:is_this_their_school).on(:update) } + it do + should validate_inclusion_of(:is_this_their_school).in_array( + %w[yes no] + ).on(:update) + end + end + end + + describe "#full_name" do + it "returns the full name as a string" do + consent_form = build(:consent_form, first_name: "John", last_name: "Doe") + expect(consent_form.full_name).to eq("John Doe") + end + end +end