From 7d7acdb86099efe69dac3d6cd1aac9cc3d9c4d75 Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Wed, 15 Jan 2025 10:53:13 +0000 Subject: [PATCH 01/10] remove no dqt data callback - this is already covered by code in slug sequence - therefore has no affect --- app/controllers/claims_form_callbacks.rb | 8 -------- 1 file changed, 8 deletions(-) diff --git a/app/controllers/claims_form_callbacks.rb b/app/controllers/claims_form_callbacks.rb index 35e4c0d9a..2b4c032ab 100644 --- a/app/controllers/claims_form_callbacks.rb +++ b/app/controllers/claims_form_callbacks.rb @@ -11,10 +11,6 @@ def teaching_subject_now_before_show redirect_to_slug("eligible-itt-subject") if no_eligible_itt_subject? end - def qualification_details_before_show - redirect_to_next_slug if no_dqt_data? - end - def address_before_show set_backlink_override(slug: "postcode-search") if no_postcode? end @@ -77,10 +73,6 @@ def no_eligible_itt_subject? !journey_session.answers.eligible_itt_subject end - def no_dqt_data? - journey_session.answers.has_no_dqt_data_for_claim? - end - def no_postcode? !journey_session.answers.postcode end From e25a08dfbfaaa5d455b60a03721c4365b7f1574c Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Wed, 15 Jan 2025 11:10:07 +0000 Subject: [PATCH 02/10] move callack to view --- app/controllers/claims_form_callbacks.rb | 4 ---- app/views/claims/address.html.erb | 4 ++++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/controllers/claims_form_callbacks.rb b/app/controllers/claims_form_callbacks.rb index 2b4c032ab..bbdd7ff4d 100644 --- a/app/controllers/claims_form_callbacks.rb +++ b/app/controllers/claims_form_callbacks.rb @@ -11,10 +11,6 @@ def teaching_subject_now_before_show redirect_to_slug("eligible-itt-subject") if no_eligible_itt_subject? end - def address_before_show - set_backlink_override(slug: "postcode-search") if no_postcode? - end - def select_home_address_before_show set_backlink_override(slug: "postcode-search") end diff --git a/app/views/claims/address.html.erb b/app/views/claims/address.html.erb index 37d253fbe..a07fa6efe 100644 --- a/app/views/claims/address.html.erb +++ b/app/views/claims/address.html.erb @@ -4,6 +4,10 @@ <%= govuk_notification_banner(title_text: "Notice") { |nb| nb.with_heading(text: "Please enter your address manually") } %> <% end %> +<% if !journey_session.answers.postcode %> + <% @backlink_path = claim_path(current_journey_routing_name, "postcode-search") %> +<% end %> +
<%= form_for @form, url: claim_path(current_journey_routing_name), builder: GOVUKDesignSystemFormBuilder::FormBuilder do |f| %> From 3f0796f747f5d201494de01da81db45366e01a09 Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Wed, 15 Jan 2025 11:34:35 +0000 Subject: [PATCH 03/10] remove callback as already default behaviour --- app/controllers/claims_form_callbacks.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/controllers/claims_form_callbacks.rb b/app/controllers/claims_form_callbacks.rb index bbdd7ff4d..a29713d20 100644 --- a/app/controllers/claims_form_callbacks.rb +++ b/app/controllers/claims_form_callbacks.rb @@ -11,10 +11,6 @@ def teaching_subject_now_before_show redirect_to_slug("eligible-itt-subject") if no_eligible_itt_subject? end - def select_home_address_before_show - set_backlink_override(slug: "postcode-search") - end - def personal_bank_account_before_update inject_hmrc_validation_attempt_count_into_the_form end From 448ba6b87ca6e180e99d7854170512ce8ed77f86 Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Wed, 15 Jan 2025 11:36:10 +0000 Subject: [PATCH 04/10] remove unused method --- app/controllers/claims_form_callbacks.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/controllers/claims_form_callbacks.rb b/app/controllers/claims_form_callbacks.rb index a29713d20..927934b9f 100644 --- a/app/controllers/claims_form_callbacks.rb +++ b/app/controllers/claims_form_callbacks.rb @@ -65,10 +65,6 @@ def no_eligible_itt_subject? !journey_session.answers.eligible_itt_subject end - def no_postcode? - !journey_session.answers.postcode - end - def retrieve_student_loan_details journey::AnswersStudentLoansDetailsUpdater.call(journey_session) end From 20d73cb1a11de0f71fe6d800483e2caa93730c58 Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Wed, 15 Jan 2025 13:36:26 +0000 Subject: [PATCH 05/10] move HMRC validation count to form - this removes all controller callbacks for this feature - these are now done in the form - on the backend without controller params --- app/controllers/claims_form_callbacks.rb | 26 ---------------------- app/forms/bank_details_form.rb | 18 ++++++++++++--- app/models/journeys/session_answers.rb | 6 ++++- spec/features/hmrc_bank_validation_spec.rb | 6 ----- spec/forms/bank_details_form_spec.rb | 7 +++--- 5 files changed, 24 insertions(+), 39 deletions(-) diff --git a/app/controllers/claims_form_callbacks.rb b/app/controllers/claims_form_callbacks.rb index 927934b9f..c4b25675d 100644 --- a/app/controllers/claims_form_callbacks.rb +++ b/app/controllers/claims_form_callbacks.rb @@ -11,10 +11,6 @@ def teaching_subject_now_before_show redirect_to_slug("eligible-itt-subject") if no_eligible_itt_subject? end - def personal_bank_account_before_update - inject_hmrc_validation_attempt_count_into_the_form - end - def information_provided_before_update return unless journey.requires_student_loan_details? @@ -28,11 +24,6 @@ def personal_details_after_form_save_success redirect_to_next_slug end - def personal_bank_account_after_form_save_failure - increment_hmrc_validation_attempt_count if hmrc_api_validation_attempted? - render_template_for_current_slug - end - def check_your_email_after_form_save_success @email_resent = true render("check_your_email") @@ -69,23 +60,6 @@ def retrieve_student_loan_details journey::AnswersStudentLoansDetailsUpdater.call(journey_session) end - def hmrc_api_validation_attempted? - @form&.hmrc_api_validation_attempted? - end - - def inject_hmrc_validation_attempt_count_into_the_form - params[:claim][:hmrc_validation_attempt_count] = current_hmrc_validation_attempt_count - end - - def increment_hmrc_validation_attempt_count - journey_session.answers.hmrc_validation_attempt_count = current_hmrc_validation_attempt_count + 1 - journey_session.save! - end - - def current_hmrc_validation_attempt_count - journey_session.answers.hmrc_validation_attempt_count || 0 - end - def on_tid_route? journey_session.answers.logged_in_with_tid? && journey_session.answers.all_personal_details_same_as_tid? end diff --git a/app/forms/bank_details_form.rb b/app/forms/bank_details_form.rb index ca4e22473..9e45328cf 100644 --- a/app/forms/bank_details_form.rb +++ b/app/forms/bank_details_form.rb @@ -3,7 +3,6 @@ class BankDetailsForm < Form MAX_HMRC_API_VALIDATION_ATTEMPTS = 3 BANKING_NAME_REGEX_FILTER = /\A[0-9A-Za-z .\/&-]*\z/ - attribute :hmrc_validation_attempt_count, :integer attribute :banking_name, :string attribute :bank_sort_code, :string attribute :bank_account_number, :string @@ -48,6 +47,10 @@ def hmrc_api_validation_succeeded? private + def hmrc_validation_attempt_count + journey_session.answers.hmrc_validation_attempt_count + end + def normalised_bank_detail(bank_detail) bank_detail&.gsub(/\s|-/, "") end @@ -61,6 +64,7 @@ def bank_sort_code_must_be_six_digits end def bank_account_is_valid + return if @bank_account_is_valid_processed return unless can_validate_with_hmrc_api? response = nil @@ -71,12 +75,17 @@ def bank_account_is_valid @hmrc_api_validation_attempted = true @hmrc_api_validation_succeeded = true if response.success? + if !response.success? + journey_session.answers.increment_hmrc_validation_attempt_count + end + unless met_maximum_attempts? errors.add(:bank_sort_code, i18n_errors_path(:invalid_sort_code)) unless response.sort_code_correct? errors.add(:bank_account_number, i18n_errors_path(:invalid_account_number)) if response.sort_code_correct? && !response.account_exists? errors.add(:banking_name, i18n_errors_path(:invalid_banking_name)) if response.sort_code_correct? && response.account_exists? && !response.name_match? end rescue Hmrc::ResponseError => e + journey_session.answers.increment_hmrc_validation_attempt_count response = e.response @hmrc_api_response_error = true ensure @@ -84,7 +93,10 @@ def bank_account_is_valid journey_session.answers.assign_attributes( hmrc_bank_validation_responses: new_hmrc_bank_validation_responses_value ) + journey_session.save! + + @bank_account_is_valid_processed = true end end @@ -93,10 +105,10 @@ def can_validate_with_hmrc_api? end def within_maximum_attempts? - hmrc_validation_attempt_count + 1 <= MAX_HMRC_API_VALIDATION_ATTEMPTS + hmrc_validation_attempt_count <= MAX_HMRC_API_VALIDATION_ATTEMPTS end def met_maximum_attempts? - hmrc_validation_attempt_count + 1 >= MAX_HMRC_API_VALIDATION_ATTEMPTS + hmrc_validation_attempt_count >= MAX_HMRC_API_VALIDATION_ATTEMPTS end end diff --git a/app/models/journeys/session_answers.rb b/app/models/journeys/session_answers.rb index 3ccc4ab11..cef3c2c79 100644 --- a/app/models/journeys/session_answers.rb +++ b/app/models/journeys/session_answers.rb @@ -60,7 +60,7 @@ class SessionAnswers attribute :student_loan_plan, :string, pii: false attribute :submitted_using_slc_data, :boolean, pii: false attribute :sent_one_time_password_at, :datetime, pii: false - attribute :hmrc_validation_attempt_count, :integer, pii: false + attribute :hmrc_validation_attempt_count, :integer, default: 0, pii: false attribute :reminder_id, :string, pii: false attribute :reminder_full_name, :string, pii: true @@ -68,6 +68,10 @@ class SessionAnswers attribute :reminder_otp_secret, :string, pii: true attribute :reminder_otp_confirmed, :boolean, default: false, pii: false # whether or not they have confirmed email via otp + def increment_hmrc_validation_attempt_count + self.hmrc_validation_attempt_count = attributes["hmrc_validation_attempt_count"] + 1 + end + def has_attribute?(name) attribute_names.include?(name.to_s) end diff --git a/spec/features/hmrc_bank_validation_spec.rb b/spec/features/hmrc_bank_validation_spec.rb index d391ec3e0..446323917 100644 --- a/spec/features/hmrc_bank_validation_spec.rb +++ b/spec/features/hmrc_bank_validation_spec.rb @@ -118,13 +118,10 @@ def get_to_bank_details_page fill_in "Name on your account", with: bank_name fill_in "Sort code", with: sort_code fill_in "Account number", with: "1111111" # Invalid number - click_on "Continue" expect(page).to have_text "Account number must be 8 digits" - fill_in "Account number", with: account_number - click_on "Continue" expect(page).to have_text(I18n.t("forms.gender.questions.payroll_gender")) @@ -146,15 +143,12 @@ def get_to_bank_details_page fill_in "Name on your account", with: bank_name fill_in "Sort code", with: sort_code fill_in "Account number", with: account_number - click_on "Continue" expect(page).to have_text "Enter the account number associated with the name on the account and/or sort code" - click_on "Continue" expect(page).to have_text "Enter the account number associated with the name on the account and/or sort code" - click_on "Continue" # Third attempt succeeds. diff --git a/spec/forms/bank_details_form_spec.rb b/spec/forms/bank_details_form_spec.rb index 9dde933ad..6b1fccd82 100644 --- a/spec/forms/bank_details_form_spec.rb +++ b/spec/forms/bank_details_form_spec.rb @@ -7,11 +7,14 @@ create(:journey_configuration, :additional_payments) } + let(:hmrc_validation_attempt_count) { 0 } + let(:journey_session) do create( :"#{journey::I18N_NAMESPACE}_session", answers: attributes_for( - :"#{journey::I18N_NAMESPACE}_answers" + :"#{journey::I18N_NAMESPACE}_answers", + hmrc_validation_attempt_count: ) ) end @@ -22,7 +25,6 @@ banking_name:, bank_sort_code:, bank_account_number:, - hmrc_validation_attempt_count: } end @@ -37,7 +39,6 @@ let(:banking_name) { "Jo Bloggs" } let(:bank_sort_code) { rand(100000..999999) } let(:bank_account_number) { rand(10000000..99999999) } - let(:hmrc_validation_attempt_count) { 0 } describe "#valid?" do context "banking name with invalid characters" do From 1d9cd35fe059cfe5de1c42d6d87af78dc9f46c78 Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Wed, 15 Jan 2025 14:04:37 +0000 Subject: [PATCH 06/10] remove unused EY callback --- app/controllers/claims_form_callbacks.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/app/controllers/claims_form_callbacks.rb b/app/controllers/claims_form_callbacks.rb index c4b25675d..f38120014 100644 --- a/app/controllers/claims_form_callbacks.rb +++ b/app/controllers/claims_form_callbacks.rb @@ -33,11 +33,6 @@ def check_your_answers_after_form_save_success create_and_save_claim_form end - def employee_email_after_form_save_failure - session[:slugs].delete("employee-email") - render_template_for_current_slug - end - private def set_backlink_override_to_current_slug From 11603b38f150ae7ab9ba32fdf90cc141c6e37a9c Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Wed, 15 Jan 2025 16:13:23 +0000 Subject: [PATCH 07/10] move callback to form --- app/controllers/claims_form_callbacks.rb | 7 ------- app/forms/personal_details_form.rb | 7 +++++-- .../journeys/answers_student_loans_details_updater.rb | 3 ++- spec/forms/personal_details_form_spec.rb | 6 ++---- 4 files changed, 9 insertions(+), 14 deletions(-) diff --git a/app/controllers/claims_form_callbacks.rb b/app/controllers/claims_form_callbacks.rb index f38120014..003064e73 100644 --- a/app/controllers/claims_form_callbacks.rb +++ b/app/controllers/claims_form_callbacks.rb @@ -17,13 +17,6 @@ def information_provided_before_update retrieve_student_loan_details if on_tid_route? end - def personal_details_after_form_save_success - return redirect_to_next_slug unless journey.requires_student_loan_details? - - retrieve_student_loan_details - redirect_to_next_slug - end - def check_your_email_after_form_save_success @email_resent = true render("check_your_email") diff --git a/app/forms/personal_details_form.rb b/app/forms/personal_details_form.rb index e1037d773..555949bfc 100644 --- a/app/forms/personal_details_form.rb +++ b/app/forms/personal_details_form.rb @@ -49,7 +49,7 @@ def date_of_birth end def save - return false unless valid? + return false if invalid? journey_session.answers.assign_attributes( first_name:, @@ -60,8 +60,11 @@ def save ) reset_dependent_answers_attributes - journey_session.save! + + if journey.requires_student_loan_details? + journey::AnswersStudentLoansDetailsUpdater.call(journey_session) + end end def show_name_section? diff --git a/app/models/journeys/answers_student_loans_details_updater.rb b/app/models/journeys/answers_student_loans_details_updater.rb index a27d83013..654404bf0 100644 --- a/app/models/journeys/answers_student_loans_details_updater.rb +++ b/app/models/journeys/answers_student_loans_details_updater.rb @@ -1,7 +1,8 @@ module Journeys class AnswersStudentLoansDetailsUpdater def self.call(journey_session) - new(journey_session).save! + instance = new(journey_session) + instance.save! end def initialize(journey_session) diff --git a/spec/forms/personal_details_form_spec.rb b/spec/forms/personal_details_form_spec.rb index 0c9c5b255..daf2eabea 100644 --- a/spec/forms/personal_details_form_spec.rb +++ b/spec/forms/personal_details_form_spec.rb @@ -420,12 +420,10 @@ journey_session.save! end - it "resets tslr specific depenent answers" do + it "resets tslr specific dependent answers" do form.save - answers = journey_session.answers - - expect(answers.student_loan_repayment_amount).to be nil + expect(journey_session.reload.answers.student_loan_repayment_amount).to be_zero end end end From bd2771096073f91cd498b2888e5e0cee4e1f831a Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Wed, 15 Jan 2025 17:15:09 +0000 Subject: [PATCH 08/10] move callback into form --- app/controllers/claims_form_callbacks.rb | 14 -------------- app/forms/information_provided_form.rb | 19 +++++++++++++++++++ app/models/journeys/base.rb | 1 + 3 files changed, 20 insertions(+), 14 deletions(-) create mode 100644 app/forms/information_provided_form.rb diff --git a/app/controllers/claims_form_callbacks.rb b/app/controllers/claims_form_callbacks.rb index 003064e73..b802217f2 100644 --- a/app/controllers/claims_form_callbacks.rb +++ b/app/controllers/claims_form_callbacks.rb @@ -11,12 +11,6 @@ def teaching_subject_now_before_show redirect_to_slug("eligible-itt-subject") if no_eligible_itt_subject? end - def information_provided_before_update - return unless journey.requires_student_loan_details? - - retrieve_student_loan_details if on_tid_route? - end - def check_your_email_after_form_save_success @email_resent = true render("check_your_email") @@ -43,12 +37,4 @@ def on_school_search_results? def no_eligible_itt_subject? !journey_session.answers.eligible_itt_subject end - - def retrieve_student_loan_details - journey::AnswersStudentLoansDetailsUpdater.call(journey_session) - end - - def on_tid_route? - journey_session.answers.logged_in_with_tid? && journey_session.answers.all_personal_details_same_as_tid? - end end diff --git a/app/forms/information_provided_form.rb b/app/forms/information_provided_form.rb new file mode 100644 index 000000000..499db5979 --- /dev/null +++ b/app/forms/information_provided_form.rb @@ -0,0 +1,19 @@ +class InformationProvidedForm < Form + def save + if journey.requires_student_loan_details? && on_tid_route? + retrieve_student_loan_details + end + + true + end + + private + + def on_tid_route? + journey_session.answers.logged_in_with_tid? && journey_session.answers.all_personal_details_same_as_tid? + end + + def retrieve_student_loan_details + journey::AnswersStudentLoansDetailsUpdater.call(journey_session) + end +end diff --git a/app/models/journeys/base.rb b/app/models/journeys/base.rb index 11176ff85..64c1c08a3 100644 --- a/app/models/journeys/base.rb +++ b/app/models/journeys/base.rb @@ -5,6 +5,7 @@ module Base "sign-in" => SignInForm, "sign-in-or-continue" => SignInOrContinueForm, "current-school" => CurrentSchoolForm, + "information-provided" => InformationProvidedForm, "gender" => GenderForm, "personal-details" => PersonalDetailsForm, "select-email" => SelectEmailForm, From a277d909f90b1385adf0111d6217ffeb882f85ff Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Thu, 16 Jan 2025 09:58:21 +0000 Subject: [PATCH 09/10] remove school controller callbacks --- app/controllers/claims_form_callbacks.rb | 20 ------------------- app/views/claims/current_school.html.erb | 4 ++++ .../claims/claim_school.html.erb | 4 ++++ 3 files changed, 8 insertions(+), 20 deletions(-) diff --git a/app/controllers/claims_form_callbacks.rb b/app/controllers/claims_form_callbacks.rb index b802217f2..70587538f 100644 --- a/app/controllers/claims_form_callbacks.rb +++ b/app/controllers/claims_form_callbacks.rb @@ -1,12 +1,4 @@ module ClaimsFormCallbacks - def current_school_before_show - set_backlink_override_to_current_slug if on_school_search_results? - end - - def claim_school_before_show - set_backlink_override_to_current_slug if on_school_search_results? - end - def teaching_subject_now_before_show redirect_to_slug("eligible-itt-subject") if no_eligible_itt_subject? end @@ -22,18 +14,6 @@ def check_your_answers_after_form_save_success private - def set_backlink_override_to_current_slug - set_backlink_override(slug: current_slug) - end - - def set_backlink_override(slug:) - @backlink_path = claim_path(current_journey_routing_name, slug) if page_sequence.in_sequence?(slug) - end - - def on_school_search_results? - params[:school_search]&.present? - end - def no_eligible_itt_subject? !journey_session.answers.eligible_itt_subject end diff --git a/app/views/claims/current_school.html.erb b/app/views/claims/current_school.html.erb index 3e8b05d59..8c70030e3 100644 --- a/app/views/claims/current_school.html.erb +++ b/app/views/claims/current_school.html.erb @@ -1,3 +1,7 @@ +<% if params[:school_search]&.present? %> + <% @backlink_path = claim_path(current_journey_routing_name, @page_sequence.current_slug) %> +<% end %> +
<% if @form.schools.blank? %> diff --git a/app/views/student_loans/claims/claim_school.html.erb b/app/views/student_loans/claims/claim_school.html.erb index 7754a2f15..73fd8b05c 100644 --- a/app/views/student_loans/claims/claim_school.html.erb +++ b/app/views/student_loans/claims/claim_school.html.erb @@ -1,3 +1,7 @@ +<% if params[:school_search]&.present? %> + <% @backlink_path = claim_path(current_journey_routing_name, @page_sequence.current_slug) %> +<% end %> +
<% if @form.schools.blank? %> From b84ec36300aae1e8cdfdb98a3a3a8ab6bb9fd233 Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Thu, 16 Jan 2025 10:07:19 +0000 Subject: [PATCH 10/10] fix linting --- spec/forms/bank_details_form_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/forms/bank_details_form_spec.rb b/spec/forms/bank_details_form_spec.rb index 6b1fccd82..7cafc40b0 100644 --- a/spec/forms/bank_details_form_spec.rb +++ b/spec/forms/bank_details_form_spec.rb @@ -24,7 +24,7 @@ { banking_name:, bank_sort_code:, - bank_account_number:, + bank_account_number: } end