From 2549be58b463304b8802105629f546e9515dde18 Mon Sep 17 00:00:00 2001 From: Giuseppe Lobraico Date: Thu, 9 May 2024 22:57:13 +0100 Subject: [PATCH 1/8] Make sure permitted params fetch from the model name param key This is a further abstraction; as a future step, the `model_name` should probably be more generic (e.g. using `Form`) --- app/forms/form.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/forms/form.rb b/app/forms/form.rb index 4ec6fa113f..bfbceef7be 100644 --- a/app/forms/form.rb +++ b/app/forms/form.rb @@ -53,7 +53,7 @@ def i18n_errors_path(msg, args = {}) end def permitted_params - @permitted_params ||= params.fetch(:claim, {}).permit(*permitted_attributes) + @permitted_params ||= params.fetch(model_name.param_key, {}).permit(*permitted_attributes) end private From f22b2c42188e2b2ea82ef8b7c9dd568d46eadc83 Mon Sep 17 00:00:00 2001 From: Giuseppe Lobraico Date: Thu, 9 May 2024 23:01:12 +0100 Subject: [PATCH 2/8] Always return `true` when calling `Form#persisted?` This is another step to avoid coupling a generic form object to a specific AR model. As a reminder, we keep `#persisted?` so Rails can make an informed decision to use PATCH instead of POST when submitting a form. By definition, our form objects are and will always be persisted "somewhere" (most likely not the DB in the future) --- app/forms/form.rb | 6 ++++-- spec/forms/form_spec.rb | 7 +------ 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/app/forms/form.rb b/app/forms/form.rb index bfbceef7be..77980e241f 100644 --- a/app/forms/form.rb +++ b/app/forms/form.rb @@ -9,8 +9,6 @@ class Form attr_accessor :journey_session attr_accessor :params - delegate :persisted?, to: :claim - def self.model_name Claim.model_name end @@ -56,6 +54,10 @@ def permitted_params @permitted_params ||= params.fetch(model_name.param_key, {}).permit(*permitted_attributes) end + def persisted? + true + end + private def permitted_attributes diff --git a/spec/forms/form_spec.rb b/spec/forms/form_spec.rb index 601a8fdd74..f564a7d126 100644 --- a/spec/forms/form_spec.rb +++ b/spec/forms/form_spec.rb @@ -139,12 +139,7 @@ def initialize(claim, journey_session) end describe "#persisted?" do - before do - allow(claim).to receive(:persisted?) - form.persisted? - end - - it { expect(claim).to have_received(:persisted?) } + it { expect(form.persisted?).to eq(true) } end describe "#update!" do From 216fdaee904a77bedc0ae4c459509282fb52ed39 Mon Sep 17 00:00:00 2001 From: Giuseppe Lobraico Date: Thu, 9 May 2024 23:53:30 +0100 Subject: [PATCH 3/8] Implement form objects for reminders slugs (ECP/LUPP) In this commit: - Organise forms by controller name, as slugs can be repeated on different controllers for the same journey (and map to a different form and view) - Use a generic and more abstract "form" model name for the new forms (it should probably be done for all the forms, but the changes would pollute this commit) - Add new reminders forms and unit tests --- app/forms/email_verification_form.rb | 6 ++- .../reminders/email_verification_form.rb | 13 +++++ .../reminders/personal_details_form.rb | 33 ++++++++++++ .../additional_payments_for_teaching.rb | 40 ++++++++------ app/models/journeys/base.rb | 40 +++++++------- .../teacher_student_loan_reimbursement.rb | 26 ++++----- config/locales/en.yml | 11 ++++ .../reminders/email_verification_form_spec.rb | 43 +++++++++++++++ .../reminders/personal_details_form_spec.rb | 54 +++++++++++++++++++ 9 files changed, 217 insertions(+), 49 deletions(-) create mode 100644 app/forms/journeys/additional_payments_for_teaching/reminders/email_verification_form.rb create mode 100644 app/forms/journeys/additional_payments_for_teaching/reminders/personal_details_form.rb create mode 100644 spec/forms/journeys/additional_payments_for_teaching/reminders/email_verification_form_spec.rb create mode 100644 spec/forms/journeys/additional_payments_for_teaching/reminders/personal_details_form_spec.rb diff --git a/app/forms/email_verification_form.rb b/app/forms/email_verification_form.rb index 0b6f80b422..e00ce99617 100644 --- a/app/forms/email_verification_form.rb +++ b/app/forms/email_verification_form.rb @@ -18,10 +18,14 @@ def save private + def sent_one_time_password_at + claim.sent_one_time_password_at + end + def otp_validate otp = OneTimePassword::Validator.new( one_time_password, - claim.sent_one_time_password_at + sent_one_time_password_at ) errors.add(:one_time_password, otp.warning) unless otp.valid? diff --git a/app/forms/journeys/additional_payments_for_teaching/reminders/email_verification_form.rb b/app/forms/journeys/additional_payments_for_teaching/reminders/email_verification_form.rb new file mode 100644 index 0000000000..4547022507 --- /dev/null +++ b/app/forms/journeys/additional_payments_for_teaching/reminders/email_verification_form.rb @@ -0,0 +1,13 @@ +module Journeys + module AdditionalPaymentsForTeaching + module Reminders + class EmailVerificationForm < ::EmailVerificationForm + attribute :sent_one_time_password_at + + def self.model_name + ActiveModel::Name.new(Form) + end + end + end + end +end diff --git a/app/forms/journeys/additional_payments_for_teaching/reminders/personal_details_form.rb b/app/forms/journeys/additional_payments_for_teaching/reminders/personal_details_form.rb new file mode 100644 index 0000000000..c993759f91 --- /dev/null +++ b/app/forms/journeys/additional_payments_for_teaching/reminders/personal_details_form.rb @@ -0,0 +1,33 @@ +module Journeys + module AdditionalPaymentsForTeaching + module Reminders + class PersonalDetailsForm < Form + attribute :full_name + attribute :email_address + + validates :full_name, presence: {message: i18n_error_message(:"full_name.blank")} + validates :full_name, length: {maximum: 100, message: i18n_error_message(:"full_name.length")} + + validates :email_address, presence: {message: i18n_error_message(:"email_address.blank")} + validates :email_address, format: {with: Rails.application.config.email_regexp, message: i18n_error_message(:"email_address.invalid")}, + length: {maximum: 256, message: i18n_error_message(:"email_address.length")}, if: -> { email_address.present? } + + def self.model_name + ActiveModel::Name.new(Form) + end + + def save + return false unless valid? + + update!(attributes) + end + + private + + def i18n_form_namespace + "reminders.#{super}" + end + end + end + end +end diff --git a/app/models/journeys/additional_payments_for_teaching.rb b/app/models/journeys/additional_payments_for_teaching.rb index 4c007a293c..6f71d4ecad 100644 --- a/app/models/journeys/additional_payments_for_teaching.rb +++ b/app/models/journeys/additional_payments_for_teaching.rb @@ -10,23 +10,29 @@ module AdditionalPaymentsForTeaching I18N_NAMESPACE = "additional_payments" POLICIES = [Policies::EarlyCareerPayments, Policies::LevellingUpPremiumPayments] FORMS = { - "induction-completed" => InductionCompletedForm, - "itt-year" => IttAcademicYearForm, - "nqt-in-academic-year-after-itt" => NqtInAcademicYearAfterIttForm, - "eligible-degree-subject" => EligibleDegreeSubjectForm, - "supply-teacher" => SupplyTeacherForm, - "poor-performance" => PoorPerformanceForm, - "entire-term-contract" => EntireTermContractForm, - "employed-directly" => EmployedDirectlyForm, - "qualification" => QualificationForm, - "qualification-details" => QualificationDetailsForm, - "eligible-itt-subject" => EligibleIttSubjectForm, - "teaching-subject-now" => TeachingSubjectNowForm, - "eligibility-confirmed" => EligibilityConfirmedForm, - "correct-school" => CorrectSchoolForm, - "reset-claim" => ResetClaimForm, - "postcode-search" => PostcodeSearchForm, - "select-home-address" => SelectHomeAddressForm + "claims" => { + "induction-completed" => InductionCompletedForm, + "itt-year" => IttAcademicYearForm, + "nqt-in-academic-year-after-itt" => NqtInAcademicYearAfterIttForm, + "eligible-degree-subject" => EligibleDegreeSubjectForm, + "supply-teacher" => SupplyTeacherForm, + "poor-performance" => PoorPerformanceForm, + "entire-term-contract" => EntireTermContractForm, + "employed-directly" => EmployedDirectlyForm, + "qualification" => QualificationForm, + "qualification-details" => QualificationDetailsForm, + "eligible-itt-subject" => EligibleIttSubjectForm, + "teaching-subject-now" => TeachingSubjectNowForm, + "eligibility-confirmed" => EligibilityConfirmedForm, + "correct-school" => CorrectSchoolForm, + "reset-claim" => ResetClaimForm, + "postcode-search" => PostcodeSearchForm, + "select-home-address" => SelectHomeAddressForm + }, + "reminders" => { + "personal-details" => Reminders::PersonalDetailsForm, + "email-verification" => Reminders::EmailVerificationForm + } }.freeze end end diff --git a/app/models/journeys/base.rb b/app/models/journeys/base.rb index 85ca0ae71a..421894d4c7 100644 --- a/app/models/journeys/base.rb +++ b/app/models/journeys/base.rb @@ -1,24 +1,26 @@ module Journeys module Base SHARED_FORMS = { - "sign-in-or-continue" => SignInOrContinueForm, - "current-school" => CurrentSchoolForm, - "gender" => GenderForm, - "personal-details" => PersonalDetailsForm, - "select-email" => SelectEmailForm, - "provide-mobile-number" => ProvideMobileNumberForm, - "select-mobile" => SelectMobileForm, - "email-address" => EmailAddressForm, - "email-verification" => EmailVerificationForm, - "mobile-number" => MobileNumberForm, - "mobile-verification" => MobileVerificationForm, - "bank-or-building-society" => BankOrBuildingSocietyForm, - "personal-bank-account" => BankDetailsForm, - "building-society-account" => BankDetailsForm, - "teacher-reference-number" => TeacherReferenceNumberForm, - "address" => AddressForm, - "select-home-address" => SelectHomeAddressForm - } + "claims" => { + "sign-in-or-continue" => SignInOrContinueForm, + "current-school" => CurrentSchoolForm, + "gender" => GenderForm, + "personal-details" => PersonalDetailsForm, + "select-email" => SelectEmailForm, + "provide-mobile-number" => ProvideMobileNumberForm, + "select-mobile" => SelectMobileForm, + "email-address" => EmailAddressForm, + "email-verification" => EmailVerificationForm, + "mobile-number" => MobileNumberForm, + "mobile-verification" => MobileVerificationForm, + "bank-or-building-society" => BankOrBuildingSocietyForm, + "personal-bank-account" => BankDetailsForm, + "building-society-account" => BankDetailsForm, + "teacher-reference-number" => TeacherReferenceNumberForm, + "address" => AddressForm, + "select-home-address" => SelectHomeAddressForm + } + }.freeze def configuration Configuration.find(self::ROUTING_NAME) @@ -33,7 +35,7 @@ def slug_sequence end def form(claim:, journey_session:, params:) - form = SHARED_FORMS.merge(forms)[params[:slug]] + form = SHARED_FORMS.deep_merge(forms).dig(params[:controller].split("/").last, params[:slug]) form&.new(journey: self, journey_session:, claim:, params:) end diff --git a/app/models/journeys/teacher_student_loan_reimbursement.rb b/app/models/journeys/teacher_student_loan_reimbursement.rb index 5240e94500..cf683369d0 100644 --- a/app/models/journeys/teacher_student_loan_reimbursement.rb +++ b/app/models/journeys/teacher_student_loan_reimbursement.rb @@ -11,17 +11,19 @@ module TeacherStudentLoanReimbursement POLICIES = [Policies::StudentLoans] FORMS = { - "claim-school" => ClaimSchoolForm, - "qualification-details" => QualificationDetailsForm, - "qts-year" => QtsYearForm, - "subjects-taught" => SubjectsTaughtForm, - "still-teaching" => StillTeachingForm, - "leadership-position" => LeadershipPositionForm, - "mostly-performed-leadership-duties" => MostlyPerformedLeadershipDutiesForm, - "reset-claim" => ResetClaimForm, - "postcode-search" => PostcodeSearchForm, - "select-claim-school" => SelectClaimSchoolForm, - "select-home-address" => SelectHomeAddressForm - } + "claims" => { + "claim-school" => ClaimSchoolForm, + "qualification-details" => QualificationDetailsForm, + "qts-year" => QtsYearForm, + "subjects-taught" => SubjectsTaughtForm, + "still-teaching" => StillTeachingForm, + "leadership-position" => LeadershipPositionForm, + "mostly-performed-leadership-duties" => MostlyPerformedLeadershipDutiesForm, + "reset-claim" => ResetClaimForm, + "postcode-search" => PostcodeSearchForm, + "select-claim-school" => SelectClaimSchoolForm, + "select-home-address" => SelectHomeAddressForm + } + }.freeze end end diff --git a/config/locales/en.yml b/config/locales/en.yml index fc485e70d9..6ed37aadaf 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -535,6 +535,17 @@ en: errors: blank: Select an additional payment inclusion: Select a valid additional payment + reminders: + personal_details: + errors: + full_name: + blank: Enter full name + length: Full name must be 100 characters or less + email_address: + blank: Enter an email address + invalid: Enter an email address in the correct format, like name@example.com + length: Email address must be 256 characters or less + unauthorised: Only authorised email addresses can be used when using a team-only API key check_your_answers: part_one: primary_heading: Check your answers diff --git a/spec/forms/journeys/additional_payments_for_teaching/reminders/email_verification_form_spec.rb b/spec/forms/journeys/additional_payments_for_teaching/reminders/email_verification_form_spec.rb new file mode 100644 index 0000000000..06f591106e --- /dev/null +++ b/spec/forms/journeys/additional_payments_for_teaching/reminders/email_verification_form_spec.rb @@ -0,0 +1,43 @@ +require "rails_helper" + +RSpec.describe Journeys::AdditionalPaymentsForTeaching::Reminders::EmailVerificationForm do + subject(:form) { described_class.new(claim: form_data_object, journey:, params:) } + + let(:journey) { Journeys::AdditionalPaymentsForTeaching } + let(:form_data_object) { Reminder.new } + let(:slug) { "email-verification" } + let(:params) { ActionController::Parameters.new({slug:, form: form_params}) } + let(:form_params) { {one_time_password: "123456"} } + + it { is_expected.to be_a(EmailVerificationForm) } + + describe ".model_name" do + it { expect(form.model_name).to eq(ActiveModel::Name.new(Form)) } + end + + describe "#save" do + subject(:save) { form.save } + + before do + allow(form).to receive(:update!).and_return(true) + end + + context "valid params" do + let(:form_params) { {"one_time_password" => OneTimePassword::Generator.new.code, "sent_one_time_password_at" => Time.now} } + + it "saves the attributes" do + expect(save).to eq(true) + expect(form).to have_received(:update!).with(email_verified: true) + end + end + + context "invalid params" do + let(:form_params) { {"one_time_password" => OneTimePassword::Generator.new.code, "sent_one_time_password_at" => ""} } + + it "does not save the attributes" do + expect(save).to eq(false) + expect(form).not_to have_received(:update!) + end + end + end +end diff --git a/spec/forms/journeys/additional_payments_for_teaching/reminders/personal_details_form_spec.rb b/spec/forms/journeys/additional_payments_for_teaching/reminders/personal_details_form_spec.rb new file mode 100644 index 0000000000..734f17c1e0 --- /dev/null +++ b/spec/forms/journeys/additional_payments_for_teaching/reminders/personal_details_form_spec.rb @@ -0,0 +1,54 @@ +require "rails_helper" + +RSpec.describe Journeys::AdditionalPaymentsForTeaching::Reminders::PersonalDetailsForm, type: :model do + subject(:form) { described_class.new(claim: form_data_object, journey:, params:) } + + let(:journey) { Journeys::AdditionalPaymentsForTeaching } + let(:form_data_object) { Reminder.new } + let(:slug) { "personal-details" } + let(:params) { ActionController::Parameters.new({slug:, form: form_params}) } + let(:form_params) { {full_name: "John Doe"} } + + it { is_expected.to be_a(Form) } + + describe "validations" do + it { is_expected.to validate_presence_of(:full_name).with_message(form.i18n_errors_path(:"full_name.blank")) } + it { is_expected.to validate_length_of(:full_name).is_at_most(100).with_message(form.i18n_errors_path(:"full_name.length")) } + + it { is_expected.to validate_presence_of(:email_address).with_message(form.i18n_errors_path(:"email_address.blank")) } + it { is_expected.to validate_length_of(:email_address).is_at_most(256).with_message(form.i18n_errors_path(:"email_address.length")) } + + it { is_expected.to allow_value("valid@email.com").for(:email_address) } + it { is_expected.not_to allow_value("in valid@email.com").for(:email_address) } + end + + describe ".model_name" do + it { expect(form.model_name).to eq(ActiveModel::Name.new(Form)) } + end + + describe "#save" do + subject(:save) { form.save } + + before do + allow(form).to receive(:update!).and_return(true) + end + + context "valid params" do + let(:form_params) { {"full_name" => "John Doe", "email_address" => "john.doe@email.com"} } + + it "saves the attributes" do + expect(save).to eq(true) + expect(form).to have_received(:update!).with(form_params) + end + end + + context "invalid params" do + let(:form_params) { {"full_name" => "John Doe", "email_address" => ""} } + + it "does not save the attributes" do + expect(save).to eq(false) + expect(form).not_to have_received(:update!) + end + end + end +end From 514e53e8656da7b7cd70b3e848ad442ed151e93e Mon Sep 17 00:00:00 2001 From: Giuseppe Lobraico Date: Thu, 9 May 2024 23:55:11 +0100 Subject: [PATCH 4/8] Introduce a form concern to handle form submissions See comments in the code for more details --- app/controllers/concerns/form_submittable.rb | 167 +++++++++++++++++ spec/requests/form_submittable_spec.rb | 186 +++++++++++++++++++ 2 files changed, 353 insertions(+) create mode 100644 app/controllers/concerns/form_submittable.rb create mode 100644 spec/requests/form_submittable_spec.rb diff --git a/app/controllers/concerns/form_submittable.rb b/app/controllers/concerns/form_submittable.rb new file mode 100644 index 0000000000..5ea2dbdcb2 --- /dev/null +++ b/app/controllers/concerns/form_submittable.rb @@ -0,0 +1,167 @@ +module FormSubmittable + extend ActiveSupport::Concern + + # + # This concern provides a way of handling form submissions for a generic slug sequence. + # + # The `new`, `show`, `create`, and `update` actions can be overridden only if necessary, + # but it's discouraged as you could easily break the rendering cycle and callback chain. + # If you need to override actions, you are probably not dealing with a form-based page sequence. + # + # The average use case will most likely only require to override slug-specific callbacks + # that are used to execute custom logic before, after, or around certain actions. + # In some cases, you may not need to define any callbacks at all. + # + # Default behaviour summary for each action: + # + # controller#new: `redirect_to_first_slug` + # controller#show: `before_show` -> `render_template_for_current_slug` + # controller#update: `before_update` -> `handle_form_submission` -> + # -> `form#save` succeded? -> + # -> `execute_callback_if_exists(:after_form_save_success)` OR `redirect_to_next_slug` + # -> `form#save failed?` -> + # -> `execute_callback_if_exists(:after_form_save_failure)` OR `render_template_for_current_slug + # controller#create: same as controller#update + # + # When including this concern make sure that these methods are accessible in the controller: + # + # - `slugs`, `current_slug`, `next_slug` (normally from `PageSequence`, or overridden) + # - `journey` (from `PartOfJourneyConcern`) + # - `current_data_object`, required to load the form object + # - any slug-specific callbacks (via a separate mixin) + # + # Important: `current_slug` is used to generate callbacks and assumed safe to use, i.e. derived + # from constrained, validated or sanitised user input. + # + # Important: If there are other callbacks in the controller, you should be including this concern + # **after** all the callbacks are defined. Placing this at the top is most likely a bad idea. + # In most cases, form submission callbacks are meant to kick in last. + # + # See the implementation of `_set_slug_specific_callbacks` below for more details. + + included do + before_action :_set_slug_specific_callbacks, only: [:show, :update, :create] + before_action :before_show, only: :show + before_action :before_update, only: [:update, :create] + before_action :load_form_if_exists, only: [:show, :update, :create] + around_action :handle_form_submission, only: [:update, :create] + + def current_data_object + # This is the instance of the main resource handled by the form object, + # and should always be defined in the controller. + nil + end + + def new + redirect_to_first_slug + end + + def show + render_template_for_current_slug + end + + def create + # Note: if implemented, this action will be yielded at the end of `handle_form_submission` + end + + def update + # Note: if implemented, this action will be yielded at the end of `handle_form_submission` + end + + private + + # + # Slug-specific callbacks are generated and executed around `show`, `update`, `create` actions, + # For example, for the "personal-details" slug, the following callbacks are available: + # + # `personal_details_before_show` + # `personal_details_before_update` + # `personal_details_after_form_save_success` (*) + # `personal_details_after_form_save_failure` (*) + # + # Ensure that the callbacks are implemented only where really needed. + # Consider organizing the callback methods in one mixin per journey and controller. + # + # (*) If you need to define these callbacks, the default rendering behaviour will not be + # followed, so you'll have to explicitly define what to do next (render/redirect_to). + + def _set_slug_specific_callbacks + %i[before_show before_update after_form_save_success after_form_save_failure].each do |callback_name| + self.class.send(:define_method, callback_name) do + execute_callback_if_exists(callback_name) + end + end + end + + def redirect_to_slug(slug) + raise NoMethodError, "End of sequence: you must define #{current_slug.underscore}_after_form_save_success" unless next_slug + raise NoMethodError, "Missing path helper for resource: \"#{path_helper_resource}\"; try overriding it with #path_helper_resource" unless respond_to?(:"#{path_helper_resource}_path") + + redirect_to send(:"#{path_helper_resource}_path", current_journey_routing_name, slug) + end + + def redirect_to_next_slug + redirect_to_slug(next_slug) + end + + def redirect_to_first_slug + redirect_to_slug(first_slug) + end + + def path_helper_resource + controller_name.singularize + end + + def render_template_for_current_slug + render current_template + end + + def current_template + current_slug.underscore + end + + def slugs + journey.slug_sequence::SLUGS + end + + def first_slug + slugs.first.to_sym + end + + def execute_callback_if_exists(callback_name) + callback_name = :"#{current_slug.underscore}_#{callback_name}" + if respond_to?(callback_name) + log_event(callback_name) { send(callback_name) } + return true + end + false + end + + def handle_form_submission + log_event(__method__) + + if @form.present? + if @form.save + return if execute_callback_if_exists(:after_form_save_success) + redirect_to_next_slug + else + return if execute_callback_if_exists(:after_form_save_failure) + render_template_for_current_slug + end + else + redirect_to_next_slug + end + + yield + end + + def log_event(callback_name) + logger.info "Executing callback ##{callback_name}" + yield if block_given? + end + + def load_form_if_exists + @form ||= journey.form(claim: current_data_object, journey_session:, params:) + end + end +end diff --git a/spec/requests/form_submittable_spec.rb b/spec/requests/form_submittable_spec.rb new file mode 100644 index 0000000000..6609ff5770 --- /dev/null +++ b/spec/requests/form_submittable_spec.rb @@ -0,0 +1,186 @@ +require "rails_helper" + +class TestDummyController < BasePublicController + include PartOfClaimJourney + include FormSubmittable + + # Overriding template for current slug to bypass view search + def render_template_for_current_slug + render plain: "Rendered template for current slug: #{current_slug}" + end + + skip_before_action :send_unstarted_claimants_to_the_start + + def slugs + %w[first-slug second-slug] + end + + def next_slug + slugs[current_slug_index + 1] + end + + def current_slug + slugs[current_slug_index] + end + + def current_slug_index + slugs.index(params[:slug]) || 0 + end +end + +class TestDummyForm < Form + def save + end +end + +RSpec.describe FormSubmittable, type: :request do + before do + Rails.application.routes.draw do + scope path: ":journey", constraints: {journey: "additional-payments"} do + get "/claim", to: "test_dummy#new" + get "/:slug", as: :test_dummy, to: "test_dummy#show" + post "/:slug", to: "test_dummy#create", as: :test_dummies + patch "/:slug", to: "test_dummy#update" + end + end + end + + after { Rails.application.reload_routes! } + + before { create(:journey_configuration, :additional_payments) } + + shared_context :define_filter do |filter_name| + before { define_filter(filter_name) } + after { remove_filter(filter_name) } + + def define_filter(filter_name) + TestDummyController.class_eval do + define_method(filter_name) do + render plain: "Triggered: `#{filter_name}` filter" + end + end + end + + def remove_filter(filter_name) + TestDummyController.class_eval do + remove_method(filter_name) if method_defined?(filter_name) + end + end + end + + describe "GET #new" do + it "redirects to the first slug" do + get "/additional-payments/claim" + expect(response).to redirect_to("/additional-payments/first-slug") + end + end + + describe "GET #show" do + context "when the `{current_slug}_before_show` filter is defined" do + include_context :define_filter, :first_slug_before_show + + it "executes the filter" do + get "/additional-payments/first-slug" + expect(response.body).to include("Triggered: `first_slug_before_show` filter") + end + end + + context "when the `{current_slug}_before_show` filter is not defined" do + it "renders the template for the current slug" do + get "/additional-payments/first-slug" + expect(response.body).to include("Rendered template for current slug: first-slug") + end + end + end + + shared_examples :form_submission do + def submit(slug) + send(method, slug, params: {}) + end + + context "when a form object is not present for the current slug" do + context "when the `{current_slug}_before_update` filter is not defined" do + it "redirects to the next slug" do + submit "/additional-payments/first-slug" + expect(response).to redirect_to("/additional-payments/second-slug") + end + end + + context "when the `{current_slug}_before_update` filter is defined" do + include_context :define_filter, :first_slug_before_update + + it "executes the filter" do + submit "/additional-payments/first-slug" + expect(response.body).to include("Triggered: `first_slug_before_update` filter") + end + end + end + + context "when a form object is present for the current slug" do + before do + stub_const("Journeys::AdditionalPaymentsForTeaching::FORMS", + {"test_dummy" => {"first-slug" => TestDummyForm, "second-slug" => TestDummyForm}}) + end + + context "when the form save succeeds" do + before do + allow_any_instance_of(TestDummyForm).to receive(:save).and_return(true) + end + + context "when the `{current_slug}_after_form_save_success` filter is defined" do + include_context :define_filter, :first_slug_after_form_save_success + + it "executes the filter" do + submit "/additional-payments/first-slug" + expect(response.body).to include("Triggered: `first_slug_after_form_save_success` filter") + end + end + + context "when the `{current_slug}_after_form_save_success` filter is not defined" do + it "redirects to the next slug" do + submit "/additional-payments/first-slug" + expect(response).to redirect_to("/additional-payments/second-slug") + end + end + + context "when it's the end of the sequence" do + it { expect { submit "/additional-payments/second-slug" }.to raise_error(NoMethodError, /End of sequence/) } + end + end + + context "when the form save fails" do + before do + allow_any_instance_of(TestDummyForm).to receive(:save).and_return(false) + end + + context "when the `{current_slug}_after_form_save_failure` filter is defined" do + include_context :define_filter, :first_slug_after_form_save_failure + + it "executes the filter" do + submit "/additional-payments/first-slug" + expect(response.body).to include("Triggered: `first_slug_after_form_save_failure` filter") + end + end + + context "when the `{current_slug}_after_form_save_failure` filter is not defined" do + it "renders to template for the current slug" do + submit "/additional-payments/second-slug" + expect(response.body).to include("Rendered template for current slug: second-slug") + end + end + end + end + end + + describe "POST #create" do + let(:method) { :post } + + it_behaves_like :form_submission + end + + describe "PATCH #update" do + let(:method) { :patch } + + it_behaves_like :form_submission + end +end From 28baad97a55073c3bd651ad5d2be992d2447adb8 Mon Sep 17 00:00:00 2001 From: Giuseppe Lobraico Date: Thu, 9 May 2024 23:57:05 +0100 Subject: [PATCH 5/8] Fix reminders routes There should be a POST for every slug; default to the first one when not specified. The `new_reminder` path should probably be a more generic `/reminder`. --- config/routes.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/routes.rb b/config/routes.rb index 5cae21cc67..c0ae7ecca6 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -59,8 +59,8 @@ def matches?(request) end scope constraints: {journey: "additional-payments"} do - get "reminders/personal-details", as: :new_reminder, to: "journeys/additional_payments_for_teaching/reminders#new" - post "reminders/personal-details", as: :reminders, to: "journeys/additional_payments_for_teaching/reminders#create" + get "reminder", as: :new_reminder, to: "journeys/additional_payments_for_teaching/reminders#new" + post "reminders/:slug", constraints: {slug: %r{#{Journeys::AdditionalPaymentsForTeaching::SlugSequence::REMINDER_SLUGS.join("|")}}}, defaults: {slug: "personal-details"}, as: :reminders, to: "journeys/additional_payments_for_teaching/reminders#create" resources :reminders, only: [:show, :update], param: :slug, constraints: {slug: %r{#{Journeys::AdditionalPaymentsForTeaching::SlugSequence::REMINDER_SLUGS.join("|")}}}, controller: "journeys/additional_payments_for_teaching/reminders" end From bfa5708519221e568e8abfcff2fc3a01f77b2311 Mon Sep 17 00:00:00 2001 From: Giuseppe Lobraico Date: Thu, 9 May 2024 23:57:59 +0100 Subject: [PATCH 6/8] Refactor reminders controller to use form objects and filters (ECP/LUPP) --- .../reminders_controller.rb | 113 +++--------------- .../reminders_form_callbacks.rb | 83 +++++++++++++ .../reminders/_one_time_password.html.erb | 18 +-- .../reminders/email_verification.html.erb | 4 +- .../reminders/personal_details.html.erb | 24 ++-- spec/features/reminders_spec.rb | 4 +- ...teacher_subjourney_for_lup_schools_spec.rb | 4 +- .../reminders/email_verification_form_spec.rb | 3 +- .../reminders/personal_details_form_spec.rb | 3 +- spec/requests/reminders_spec.rb | 16 ++- 10 files changed, 138 insertions(+), 134 deletions(-) create mode 100644 app/controllers/journeys/additional_payments_for_teaching/reminders_form_callbacks.rb diff --git a/app/controllers/journeys/additional_payments_for_teaching/reminders_controller.rb b/app/controllers/journeys/additional_payments_for_teaching/reminders_controller.rb index ffc8761722..94ce376e3c 100644 --- a/app/controllers/journeys/additional_payments_for_teaching/reminders_controller.rb +++ b/app/controllers/journeys/additional_payments_for_teaching/reminders_controller.rb @@ -1,59 +1,21 @@ module Journeys module AdditionalPaymentsForTeaching class RemindersController < BasePublicController - helper_method :current_reminder - after_action :reminder_set_email, :clear_sessions, only: [:show] - - def new - # Skip the OTP process if the current_claim already has email_verified - # - transfer the email_verified state to the reminder (done in #current_reminder) - # - jump straight to reminder set - if current_reminder.email_verified? && current_reminder.save - redirect_to reminder_path(current_journey_routing_name, "set") - return - end + include PartOfClaimJourney - render "reminders/#{first_template_in_sequence}" - end - - def create - current_reminder.attributes = reminder_params - - begin - one_time_password - rescue Notifications::Client::BadRequestError => e - if notify_email_error?(e.message) - render "reminders/#{first_template_in_sequence}" - return - else - raise - end - end + after_action :clear_sessions, only: :show + helper_method :current_reminder - if current_reminder.save(context: current_slug.to_sym) - session[:reminder_id] = current_reminder.to_param - redirect_to reminder_path(current_journey_routing_name, next_slug) - else - render "reminders/#{first_template_in_sequence}" - end - end + include FormSubmittable + include RemindersFormCallbacks - def show - render "reminders/#{current_template}" - end + private - def update - current_reminder.attributes = reminder_params - one_time_password - if current_reminder.save(context: current_slug.to_sym) - redirect_to reminder_path(current_journey_routing_name, next_slug) - else - show - end + # Wrapping `current_reminder` with an abstract method that is fed to the form object. + def current_data_object + current_reminder end - private - def claim_from_session return unless session.key?(:claim_id) || session.key?(:submitted_claim_id) @@ -69,18 +31,6 @@ def slugs journey.slug_sequence::REMINDER_SLUGS end - def first_template_in_sequence - slugs.first.underscore - end - - def current_template - current_slug.underscore - end - - def next_template - next_slug.underscore - end - def next_slug slugs[current_slug_index + 1] end @@ -93,6 +43,10 @@ def current_slug_index slugs.index(params[:slug]) || 0 end + def current_template + "reminders/#{current_slug.underscore}" + end + def current_reminder @current_reminder ||= reminder_from_session || @@ -127,51 +81,12 @@ def next_academic_year journey_configuration.current_academic_year + 1 end - def reminder_params - params.require(:reminder).permit(:full_name, :email_address, :one_time_password) - end - - def one_time_password - case current_slug - when "personal-details" - if current_reminder.valid?(:"personal-details") - ReminderMailer.email_verification(current_reminder, otp.code).deliver_now - session[:sent_one_time_password_at] = Time.now - end - when "email-verification" - current_reminder.update(sent_one_time_password_at: session[:sent_one_time_password_at]) - end - end - - def otp - @otp ||= OneTimePassword::Generator.new - end - - def reminder_set_email - return unless current_slug == "set" && current_reminder.email_verified? - - ReminderMailer.reminder_set(current_reminder).deliver_now - end - def clear_sessions - return unless current_template == "set" + return unless current_slug == "set" session.delete(:claim_id) session.delete(:reminder_id) end - - def notify_email_error?(msg) - case msg - when "ValidationError: email_address is a required property" - current_reminder.add_invalid_email_error("Enter an email address in the correct format, like name@example.com") - true - when "BadRequestError: Can’t send to this recipient using a team-only API key" - current_reminder.add_invalid_email_error("Only authorised email addresses can be used when using a team-only API key") - true - else - false - end - end end end end diff --git a/app/controllers/journeys/additional_payments_for_teaching/reminders_form_callbacks.rb b/app/controllers/journeys/additional_payments_for_teaching/reminders_form_callbacks.rb new file mode 100644 index 0000000000..415a32f1dd --- /dev/null +++ b/app/controllers/journeys/additional_payments_for_teaching/reminders_form_callbacks.rb @@ -0,0 +1,83 @@ +module Journeys + module AdditionalPaymentsForTeaching + module RemindersFormCallbacks + def personal_details_before_show + try_mailer { set_a_reminder_immediately_if_possible } + end + + def email_verification_before_show + try_mailer { set_a_reminder_immediately_if_possible } + end + + def email_verification_before_update + inject_sent_one_time_password_at_into_the_form + end + + def personal_details_after_form_save_success + update_reminder_id + try_mailer { send_verification_email } || return + redirect_to_next_slug + end + + def email_verification_after_form_save_success + try_mailer { send_reminder_set_email } || return + redirect_to_next_slug + end + + private + + def set_a_reminder_immediately_if_possible + return if current_reminder.persisted? + + if current_reminder.email_verified? && current_reminder.save + ReminderMailer.reminder_set(current_reminder).deliver_now + + redirect_to reminder_path(current_journey_routing_name, "set") + end + end + + def inject_sent_one_time_password_at_into_the_form + params[:form]&.[]=(:sent_one_time_password_at, session[:sent_one_time_password_at]) + end + + def update_reminder_id + session[:reminder_id] = current_reminder.to_param + end + + def send_verification_email + otp = OneTimePassword::Generator.new + ReminderMailer.email_verification(current_reminder, otp.code).deliver_now + session[:sent_one_time_password_at] = Time.now + end + + def send_reminder_set_email + ReminderMailer.reminder_set(current_reminder).deliver_now + end + + def try_mailer(&block) + yield if block + true + rescue Notifications::Client::BadRequestError => e + if notify_email_error?(e.message) + render_template_for_current_slug + false + else + raise + end + end + + def notify_email_error?(msg) + case msg + when "ValidationError: email_address is a required property" + @form.errors.add(:email_address, :invalid, message: @form.i18n_errors_path(:"email_address.invalid")) + true + when "BadRequestError: Can’t send to this recipient using a team-only API key" + @form.errors.add(:email_address, :invalid, message: @form.i18n_errors_path(:"email_address.unauthorised")) + true + else + false + end + end + end + end +end diff --git a/app/views/additional_payments/reminders/_one_time_password.html.erb b/app/views/additional_payments/reminders/_one_time_password.html.erb index 969236c4c7..4dfe9fbd26 100644 --- a/app/views/additional_payments/reminders/_one_time_password.html.erb +++ b/app/views/additional_payments/reminders/_one_time_password.html.erb @@ -1,25 +1,25 @@
- <%= render("shared/error_summary", instance: current_reminder) if current_reminder.errors.any? %> + <%= render("shared/error_summary", instance: @form) if @form.errors.any? %> Email verification - <%= form_for current_reminder, url: reminder_path(current_journey_routing_name) do |form| %> - <%= form_group_tag current_reminder do %> + <%= form_for @form, url: reminder_path(current_journey_routing_name) do |f| %> + <%= form_group_tag @form do %>

- +

- <%= t("one_time_password.hint1_html", email_or_mobile_message: "an email", email_or_mobile_value: current_reminder.email_address) %> + <%= t("one_time_password.hint1_html", email_or_mobile_message: "an email", email_or_mobile_value: @form.email_address) %>

<%= t("one_time_password.validity_duration", duration_valid: one_time_password_validity_duration) %>
- <%= errors_tag current_reminder, :one_time_password %> - <%= form.text_field :one_time_password, + <%= errors_tag @form, :one_time_password %> + <%= f.text_field :one_time_password, autocomplete: "off", - class: css_classes_for_input(current_reminder, :one_time_password, 'govuk-input--width-5'), + class: css_classes_for_input(@form, :one_time_password, 'govuk-input--width-5'), "aria-describedby" => "one-time-password-hint" %> <% end %> @@ -28,7 +28,7 @@
- <%= form.submit "Confirm", class: "govuk-button" %> + <%= f.submit "Confirm", class: "govuk-button" %> <%= link_to "Change email address", new_reminder_path, class: "govuk-button govuk-button--secondary", role: "button", data: {module: "govuk-button"} %>
<% end %> diff --git a/app/views/additional_payments/reminders/email_verification.html.erb b/app/views/additional_payments/reminders/email_verification.html.erb index 11d4e4d3d8..8e8a5e52e3 100644 --- a/app/views/additional_payments/reminders/email_verification.html.erb +++ b/app/views/additional_payments/reminders/email_verification.html.erb @@ -1,3 +1,3 @@ -<% content_for(:page_title, page_title(t("one_time_password.title"), journey: current_journey_routing_name, show_error: current_reminder.errors.any?)) %> +<% content_for(:page_title, page_title(t("one_time_password.title"), journey: current_journey_routing_name, show_error: @form.errors.any?)) %> -<%= render partial: "reminders/one_time_password", locals: {current_reminder: current_reminder, current_journey_routing_name: current_journey_routing_name} %> +<%= render partial: "reminders/one_time_password", locals: {current_journey_routing_name: current_journey_routing_name} %> diff --git a/app/views/additional_payments/reminders/personal_details.html.erb b/app/views/additional_payments/reminders/personal_details.html.erb index 0adeaabd39..e024de195f 100644 --- a/app/views/additional_payments/reminders/personal_details.html.erb +++ b/app/views/additional_payments/reminders/personal_details.html.erb @@ -1,25 +1,25 @@ -<% content_for(:page_title, page_title(t("questions.personal_details"), journey: current_journey_routing_name, show_error: current_reminder.errors.any?)) %> +<% content_for(:page_title, page_title(t("questions.personal_details"), journey: current_journey_routing_name, show_error: @form.errors.any?)) %>
- <%= render("shared/error_summary", instance: current_reminder) if current_reminder.errors.any? %> + <%= render("shared/error_summary", instance: @form) if @form.errors.any? %> - <%= form_for current_reminder, url: reminders_path(current_journey_routing_name) do |form| %> + <%= form_for @form, url: reminder_path(current_journey_routing_name) do |f| %>

<%= t("questions.personal_details") %>

- <%= form_group_tag current_reminder, :full_name do %> + <%= form_group_tag @form, :full_name do %>

- <%= form.label :full_name, t("additional_payments.reminders.full_name"), {class: "govuk-label govuk-label--l"} %> + <%= f.label :full_name, t("additional_payments.reminders.full_name"), {class: "govuk-label govuk-label--l"} %>

- <%= errors_tag current_reminder, :full_name %> - <%= form.text_field :full_name, class: css_classes_for_input(current_reminder, :full_name), type: "text", spellcheck: "false", autocomplete: "name" %> + <%= errors_tag @form, :full_name %> + <%= f.text_field :full_name, class: css_classes_for_input(@form, :full_name), type: "text", spellcheck: "false", autocomplete: "name" %> <% end %> - <%= form_group_tag current_reminder, :email_address do %> + <%= form_group_tag @form, :email_address do %>

- <%= form.label :email_address, t("questions.email_address"), {class: "govuk-label govuk-label--l"} %> + <%= f.label :email_address, t("questions.email_address"), {class: "govuk-label govuk-label--l"} %>

@@ -31,8 +31,8 @@ You can enter the passcode on the next screen.

- <%= errors_tag current_reminder, :email_address %> - <%= form.text_field :email_address, class: css_classes_for_input(current_reminder, :email_address), type: "text", spellcheck: "false", autocomplete: "email" %> + <%= errors_tag @form, :email_address %> + <%= f.text_field :email_address, class: css_classes_for_input(@form, :email_address), type: "text", spellcheck: "false", autocomplete: "email" %> <% end %>
@@ -47,7 +47,7 @@
- <%= form.submit "Continue", class: "govuk-button", data: {module: "govuk-button"} %> + <%= f.submit "Continue", class: "govuk-button", data: {module: "govuk-button"} %> <% end %>
diff --git a/spec/features/reminders_spec.rb b/spec/features/reminders_spec.rb index f6e9cdfbd9..16be3353d9 100644 --- a/spec/features/reminders_spec.rb +++ b/spec/features/reminders_spec.rb @@ -62,7 +62,7 @@ fill_in "Full name", with: "David Tau" fill_in "Email address", with: "david.tau1988@hotmail.co.uk" click_on "Continue" - fill_in "reminder_one_time_password", with: get_otp_from_email + fill_in "form_one_time_password", with: get_otp_from_email click_on "Confirm" reminder = Reminder.order(:created_at).last @@ -140,7 +140,7 @@ expect(page).to have_text("Personal details") click_on "Continue" - fill_in "reminder_one_time_password", with: get_otp_from_email + fill_in "form_one_time_password", with: get_otp_from_email click_on "Confirm" reminder = Reminder.order(:created_at).last diff --git a/spec/features/trainee_teacher_subjourney_for_lup_schools_spec.rb b/spec/features/trainee_teacher_subjourney_for_lup_schools_spec.rb index dae15820e1..2d603472ed 100644 --- a/spec/features/trainee_teacher_subjourney_for_lup_schools_spec.rb +++ b/spec/features/trainee_teacher_subjourney_for_lup_schools_spec.rb @@ -41,7 +41,7 @@ fill_in "Full name", with: "David Tau" fill_in "Email address", with: "david.tau1988@hotmail.co.uk" click_on "Continue" - fill_in "reminder_one_time_password", with: get_otp_from_email + fill_in "form_one_time_password", with: get_otp_from_email click_on "Confirm" reminder = Reminder.order(:created_at).last @@ -76,7 +76,7 @@ fill_in "Full name", with: "David Tau" fill_in "Email address", with: "david.tau1988@hotmail.co.uk" click_on "Continue" - fill_in "reminder_one_time_password", with: get_otp_from_email + fill_in "form_one_time_password", with: get_otp_from_email click_on "Confirm" reminder = Reminder.order(:created_at).last diff --git a/spec/forms/journeys/additional_payments_for_teaching/reminders/email_verification_form_spec.rb b/spec/forms/journeys/additional_payments_for_teaching/reminders/email_verification_form_spec.rb index 06f591106e..78bc8ce8d3 100644 --- a/spec/forms/journeys/additional_payments_for_teaching/reminders/email_verification_form_spec.rb +++ b/spec/forms/journeys/additional_payments_for_teaching/reminders/email_verification_form_spec.rb @@ -1,9 +1,10 @@ require "rails_helper" RSpec.describe Journeys::AdditionalPaymentsForTeaching::Reminders::EmailVerificationForm do - subject(:form) { described_class.new(claim: form_data_object, journey:, params:) } + subject(:form) { described_class.new(claim: form_data_object, journey:, journey_session:, params:) } let(:journey) { Journeys::AdditionalPaymentsForTeaching } + let(:journey_session) { build(:journeys_session, journey: journey::ROUTING_NAME) } let(:form_data_object) { Reminder.new } let(:slug) { "email-verification" } let(:params) { ActionController::Parameters.new({slug:, form: form_params}) } diff --git a/spec/forms/journeys/additional_payments_for_teaching/reminders/personal_details_form_spec.rb b/spec/forms/journeys/additional_payments_for_teaching/reminders/personal_details_form_spec.rb index 734f17c1e0..57a14906cd 100644 --- a/spec/forms/journeys/additional_payments_for_teaching/reminders/personal_details_form_spec.rb +++ b/spec/forms/journeys/additional_payments_for_teaching/reminders/personal_details_form_spec.rb @@ -1,9 +1,10 @@ require "rails_helper" RSpec.describe Journeys::AdditionalPaymentsForTeaching::Reminders::PersonalDetailsForm, type: :model do - subject(:form) { described_class.new(claim: form_data_object, journey:, params:) } + subject(:form) { described_class.new(claim: form_data_object, journey:, journey_session:, params:) } let(:journey) { Journeys::AdditionalPaymentsForTeaching } + let(:journey_session) { build(:journeys_session, journey: journey::ROUTING_NAME) } let(:form_data_object) { Reminder.new } let(:slug) { "personal-details" } let(:params) { ActionController::Parameters.new({slug:, form: form_params}) } diff --git a/spec/requests/reminders_spec.rb b/spec/requests/reminders_spec.rb index 11f2114ed7..60977342a4 100644 --- a/spec/requests/reminders_spec.rb +++ b/spec/requests/reminders_spec.rb @@ -4,10 +4,14 @@ before { create(:journey_configuration, :additional_payments) } describe "#create" do + before do + allow_any_instance_of(BasePublicController).to receive(:current_claim).and_return(current_claim) + end + let(:current_claim) { create(:claim, policy: Policies::LevellingUpPremiumPayments) } let(:submit_form) { post reminders_path("additional-payments", params: form_params) } context "with full name and valid email address" do - let(:form_params) { {reminder: {full_name: "Joe Bloggs", email_address: "joe.bloggs@example.com"}} } + let(:form_params) { {form: {full_name: "Joe Bloggs", email_address: "joe.bloggs@example.com"}} } it "redirects to /email-verfication slug" do submit_form @@ -16,7 +20,7 @@ end context "with empty form" do - let(:form_params) { {reminder: {full_name: "", email_address: ""}} } + let(:form_params) { {form: {full_name: "", email_address: ""}} } before { submit_form } @@ -30,7 +34,7 @@ end context "invalid email address" do - let(:form_params) { {reminder: {full_name: "Joe Bloggs", email_address: "joe.bloggs.example.com"}} } + let(:form_params) { {form: {full_name: "Joe Bloggs", email_address: "joe.bloggs.example.com"}} } it "renders errors containing invalid email address" do submit_form @@ -39,7 +43,7 @@ end context "Notify returns an error about email address is required" do - let(:form_params) { {reminder: {full_name: "Joe Bloggs", email_address: "joe.bloggs@example.com"}} } + let(:form_params) { {form: {full_name: "Joe Bloggs", email_address: "joe.bloggs@example.com"}} } let(:mailer) { double("notify") } let(:notifications_error_response) { double("response", code: 400, body: "ValidationError: email_address is a required property") } @@ -56,7 +60,7 @@ end context "Notify returns an error about team only API key" do - let(:form_params) { {reminder: {full_name: "Joe Bloggs", email_address: "joe.bloggs@example.com"}} } + let(:form_params) { {form: {full_name: "Joe Bloggs", email_address: "joe.bloggs@example.com"}} } let(:mailer) { double("notify") } let(:notifications_error_response) { double("response", code: 400, body: "BadRequestError: Can’t send to this recipient using a team-only API key") } @@ -73,7 +77,7 @@ end context "Notify returns an unknown error" do - let(:form_params) { {reminder: {full_name: "Joe Bloggs", email_address: "joe.bloggs@example.com"}} } + let(:form_params) { {form: {full_name: "Joe Bloggs", email_address: "joe.bloggs@example.com"}} } let(:mailer) { double("notify") } let(:notifications_error_response) { double("response", code: 400, body: "Something unexpected") } From 5a2e99bf024925bd5df42b0d16e3b867f25b1575 Mon Sep 17 00:00:00 2001 From: Giuseppe Lobraico Date: Thu, 9 May 2024 23:58:42 +0100 Subject: [PATCH 7/8] Remove validations from the `Reminder` model They are now moved to the form objects. --- app/models/reminder.rb | 29 ----------------------------- spec/models/reminder_spec.rb | 30 ------------------------------ 2 files changed, 59 deletions(-) diff --git a/app/models/reminder.rb b/app/models/reminder.rb index b819dd8591..08a69a5359 100644 --- a/app/models/reminder.rb +++ b/app/models/reminder.rb @@ -2,17 +2,6 @@ class Reminder < ApplicationRecord attribute :sent_one_time_password_at, :datetime attribute :one_time_password, :string, limit: 6 - validates :full_name, on: [:"personal-details"], presence: {message: "Enter full name"} - validates :full_name, length: {maximum: 100, message: "Full name must be 100 characters or less"} - - validates :email_address, on: [:"personal-details"], presence: {message: "Enter an email address"} - validates :email_address, format: {with: Rails.application.config.email_regexp, message: "Enter an email address in the correct format, like name@example.com"}, - length: {maximum: 256, message: "Email address must be 256 characters or less"}, if: -> { email_address.present? } - - validate :otp_validate, on: [:"email-verification"] - - before_save :normalise_one_time_password, if: :one_time_password_changed? - scope :email_verified, -> { where(email_verified: true) } scope :not_yet_sent, -> { where(email_sent_at: nil) } scope :inside_academic_year, -> { where(itt_academic_year: AcademicYear.current.to_s) } @@ -35,22 +24,4 @@ def itt_academic_year read_attribute(:itt_academic_year) ) end - - def add_invalid_email_error(msg) - errors.add(:email_address, :invalid, message: msg) - end - - def normalise_one_time_password - self.one_time_password = one_time_password.gsub(/\D/, "") - end - - def otp_validate - return write_attribute(:email_verified, true) if otp.valid? - - errors.add(:one_time_password, otp.warning) - end - - def otp - @otp ||= OneTimePassword::Validator.new(one_time_password, sent_one_time_password_at) - end end diff --git a/spec/models/reminder_spec.rb b/spec/models/reminder_spec.rb index 5ac5563744..b89e1314c0 100644 --- a/spec/models/reminder_spec.rb +++ b/spec/models/reminder_spec.rb @@ -1,36 +1,6 @@ require "rails_helper" RSpec.describe Reminder, type: :model do - context "that has a email address" do - it "validates that the value is in the correct format" do - expect(build(:reminder, email_address: "notan email@address.com")).not_to be_valid - expect(build(:reminder, email_address: "david.tau.2020.gb@example.com")).to be_valid - expect(build(:reminder, email_address: "name@example")).not_to be_valid - end - - it "checks that the email address in not longer than 256 characters" do - expect(build(:reminder, email_address: "#{"e" * 256}@example.com")).not_to be_valid - end - end - - context "that has a full name" do - it "validates the length of name is 100 characters or less" do - expect(build(:reminder, full_name: "Name " * 50)).not_to be_valid - expect(build(:reminder, full_name: "John")).to be_valid - end - end - - context "when saving in the 'personal-details' validation context" do - it "validates the presence of full_name" do - expect(build(:reminder, full_name: nil)).not_to be_valid(:"personal-details") - expect(build(:reminder, full_name: "Miss Sveta Bond-Areemev")).to be_valid(:"personal-details") - end - - it "validates the presence of email_address" do - expect(build(:reminder, email_address: nil)).not_to be_valid(:"personal-details") - end - end - describe ".to_be_sent" do let(:count) { [*1..5].sample } let(:email_sent_at) { nil } From 5ba8e23d4146c84aac1d5b3e0d5b1f0f99fd7371 Mon Sep 17 00:00:00 2001 From: Giuseppe Lobraico Date: Mon, 13 May 2024 02:35:24 +0100 Subject: [PATCH 8/8] Validate OTP code generation timestamp --- app/forms/email_verification_form.rb | 17 +++++++++++++++-- config/locales/en.yml | 4 ++++ spec/forms/email_verification_form_spec.rb | 6 ++++++ 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/app/forms/email_verification_form.rb b/app/forms/email_verification_form.rb index e00ce99617..c19cee1bc1 100644 --- a/app/forms/email_verification_form.rb +++ b/app/forms/email_verification_form.rb @@ -4,7 +4,8 @@ class EmailVerificationForm < Form # Required for shared partial in the view delegate :email_address, to: :claim - validate :otp_validate + validate :sent_one_time_password_must_be_valid + validate :otp_must_be_valid, if: :sent_one_time_password_at? before_validation do self.one_time_password = one_time_password.gsub(/\D/, "") @@ -22,7 +23,13 @@ def sent_one_time_password_at claim.sent_one_time_password_at end - def otp_validate + def sent_one_time_password_must_be_valid + return if sent_one_time_password_at? + + errors.add(:one_time_password, i18n_errors_path(:"one_time_password.invalid")) + end + + def otp_must_be_valid otp = OneTimePassword::Validator.new( one_time_password, sent_one_time_password_at @@ -30,4 +37,10 @@ def otp_validate errors.add(:one_time_password, otp.warning) unless otp.valid? end + + def sent_one_time_password_at? + sent_one_time_password_at&.to_datetime || false + rescue Date::Error + false + end end diff --git a/config/locales/en.yml b/config/locales/en.yml index 6ed37aadaf..e8f78a15d7 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -417,6 +417,10 @@ en: presence: "Enter an email address" format: "Enter an email address in the correct format, like name@example.com" length: "Email address must be 256 characters or less" + email_verification: + errors: + one_time_password: + invalid: An error occured while validating the passcode, please try generating a new one mobile_number: errors: invalid: "Enter a mobile number, like 07700 900 982 or +44 7700 900 982" diff --git a/spec/forms/email_verification_form_spec.rb b/spec/forms/email_verification_form_spec.rb index 57de0f4851..3d5c58f25a 100644 --- a/spec/forms/email_verification_form_spec.rb +++ b/spec/forms/email_verification_form_spec.rb @@ -74,6 +74,12 @@ it { is_expected.not_to be_valid } end + context "when the code generation timestamp is missing" do + let(:one_time_password) { OneTimePassword::Generator.new.code } + let(:sent_one_time_password_at) { nil } + it { is_expected.not_to be_valid } + end + context "when correct code" do let(:one_time_password) { OneTimePassword::Generator.new.code } let(:sent_one_time_password_at) { Time.now }