From 60cb83202f24f69962c704efa8164b900fdc7ef5 Mon Sep 17 00:00:00 2001 From: Kat Date: Mon, 17 Jun 2024 15:41:06 +0100 Subject: [PATCH] Refactor check errors page --- ...swers_summary_list_card_component.html.erb | 2 +- ...eck_answers_summary_list_card_component.rb | 18 +-- app/controllers/form_controller.rb | 10 +- app/helpers/form_page_error_helper.rb | 4 - .../check_errors.html.erb} | 6 +- app/views/form/page.html.erb | 2 +- config/routes.rb | 6 +- spec/requests/check_errors_controller_spec.rb | 108 ++++++++++++++++++ .../check_your_errors_controller_spec.rb | 69 ----------- 9 files changed, 135 insertions(+), 90 deletions(-) rename app/views/{check_your_errors/index.html.erb => form/check_errors.html.erb} (78%) create mode 100644 spec/requests/check_errors_controller_spec.rb delete mode 100644 spec/requests/check_your_errors_controller_spec.rb diff --git a/app/components/check_answers_summary_list_card_component.html.erb b/app/components/check_answers_summary_list_card_component.html.erb index aa2b0e4e5..4f3df47fa 100644 --- a/app/components/check_answers_summary_list_card_component.html.erb +++ b/app/components/check_answers_summary_list_card_component.html.erb @@ -36,7 +36,7 @@ <% if @log.collection_period_open_for_editing? %> <% row.with_action( text: question.action_text(log, correcting_hard_validation: @correcting_hard_validation), - href: action_href(question, log, correcting_hard_validation: @correcting_hard_validation), + href: @correcting_hard_validation ? correct_validation_action_href(question, log, applicable_questions.map(&:id)) : action_href(question, log), visually_hidden_text: question.check_answer_label.to_s.downcase, ) %> <% end %> diff --git a/app/components/check_answers_summary_list_card_component.rb b/app/components/check_answers_summary_list_card_component.rb index 825ab04b1..b37e226b0 100644 --- a/app/components/check_answers_summary_list_card_component.rb +++ b/app/components/check_answers_summary_list_card_component.rb @@ -29,16 +29,16 @@ class CheckAnswersSummaryListCardComponent < ViewComponent::Base "Person #{question.check_answers_card_number}" end - def action_href(question, log, correcting_hard_validation: false) - if correcting_hard_validation - if question.displayed_as_answered?(log) - lettings_log_confirm_clear_answer_path(log, question_id: question.id, related_question_ids: request.query_parameters["related_question_ids"], original_page_id: request.query_parameters["original_page_id"]) - else - send("#{log.model_name.param_key}_#{question.page.id}_path", log, referrer: "check_your_errors", related_question_ids: request.query_parameters["related_question_ids"], original_page_id: request.query_parameters["original_page_id"]) - end + def action_href(question, log) + referrer = question.displayed_as_answered?(log) ? "check_answers" : "check_answers_new_answer" + send("#{log.model_name.param_key}_#{question.page.id}_path", log, referrer:) + end + + def correct_validation_action_href(question, log, related_question_ids) + if question.displayed_as_answered?(log) + lettings_log_confirm_clear_answer_path(log, question_id: question.id) else - referrer = question.displayed_as_answered?(log) ? "check_answers" : "check_answers_new_answer" - send("#{log.model_name.param_key}_#{question.page.id}_path", log, referrer:) + send("#{log.model_name.param_key}_#{question.page.id}_path", log, referrer: "check_errors", related_question_ids: request.query_parameters["related_question_ids"], original_page_id: request.query_parameters["original_page_id"]) end end diff --git a/app/controllers/form_controller.rb b/app/controllers/form_controller.rb index a8d8fca48..c2890fb89 100644 --- a/app/controllers/form_controller.rb +++ b/app/controllers/form_controller.rb @@ -30,10 +30,16 @@ class FormController < ApplicationController mandatory_questions_with_no_response.map do |question| @log.errors.add question.id.to_sym, question.unanswered_error_message, category: :not_answered end - Rails.logger.info "User triggered validation(s) on: #{@log.errors.map(&:attribute).join(', ')}" + error_attributes = @log.errors.map(&:attribute) + Rails.logger.info "User triggered validation(s) on: #{error_attributes.join(', ')}" @subsection = form.subsection_for_page(@page) restore_error_field_values(@page&.questions) - render "form/page" + if params["check_errors"] + @questions = @log.form.questions.select { |q| error_attributes.include?(q.id.to_sym) } + render "form/check_errors" + else + render "form/page" + end end else render_not_found diff --git a/app/helpers/form_page_error_helper.rb b/app/helpers/form_page_error_helper.rb index 035c34f0a..8a46accca 100644 --- a/app/helpers/form_page_error_helper.rb +++ b/app/helpers/form_page_error_helper.rb @@ -7,8 +7,4 @@ module FormPageErrorHelper def all_questions_affected_by_errors(log) log.errors.map(&:attribute) - [:base] end - - def check_your_errors_link(log, related_question_ids, original_page_id) - govuk_link_to "See all related answers", lettings_log_check_your_errors_path(log, related_question_ids:, original_page_id:), method: :post - end end diff --git a/app/views/check_your_errors/index.html.erb b/app/views/form/check_errors.html.erb similarity index 78% rename from app/views/check_your_errors/index.html.erb rename to app/views/form/check_errors.html.erb index 55ddac172..e498a0d40 100644 --- a/app/views/check_your_errors/index.html.erb +++ b/app/views/form/check_errors.html.erb @@ -1,11 +1,15 @@
+ <%= form_with model: @log, url: request.original_url, method: "post", local: true do |f| %> + <%= f.govuk_error_summary %> + <% end %> +

Make sure these answers are correct: - <%= govuk_link_to "Clear all", lettings_log_confirm_clear_all_answers_path(@log, related_question_ids: request.query_parameters["related_question_ids"], original_page_id: request.query_parameters["original_page_id"]) %> + <%= govuk_link_to "Clear all", lettings_log_confirm_clear_all_answers_path(@log) %>

diff --git a/app/views/form/page.html.erb b/app/views/form/page.html.erb index cfc7e7a71..48bfc4edc 100644 --- a/app/views/form/page.html.erb +++ b/app/views/form/page.html.erb @@ -73,7 +73,7 @@ <% if all_questions_with_errors.count > 1 %>
- <%= check_your_errors_link(@log, all_questions_with_errors, @page.id) %> + <%= f.govuk_submit "See all related answers", :name => "check_errors", secondary: true %>
<% end %> diff --git a/config/routes.rb b/config/routes.rb index 9cc8f4e15..5b1404129 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -212,9 +212,9 @@ Rails.application.routes.draw do get "delete-confirmation", to: "lettings_logs#delete_confirmation" get "duplicate-logs", to: "duplicate_logs#show" get "delete-duplicates", to: "duplicate_logs#delete_duplicates" - get "check-your-errors", to: "check_your_errors#index" - get "confirm-clear-answer", to: "check_your_errors#confirm_clear_answer" - get "confirm-clear-all-answers", to: "check_your_errors#confirm_clear_all_answers" + post "confirm-clear-answer", to: "check_errors#confirm_clear_answer" + post "confirm-clear-all-answers", to: "check_errors#confirm_clear_all_answers" + post "clear-answer", to: "check_errors#clear_answer" collection do get "csv-download", to: "lettings_logs#download_csv" diff --git a/spec/requests/check_errors_controller_spec.rb b/spec/requests/check_errors_controller_spec.rb new file mode 100644 index 000000000..28e4131b8 --- /dev/null +++ b/spec/requests/check_errors_controller_spec.rb @@ -0,0 +1,108 @@ +require "rails_helper" + +RSpec.describe CheckErrorsController, type: :request do + let(:page) { Capybara::Node::Simple.new(response.body) } + let(:user) { create(:user, :data_coordinator) } + let(:lettings_log) { create(:lettings_log, :duplicate, assigned_to: user) } + + describe "check errors page" do + context "when user is not signed in" do + it "redirects to sign in page" do + post "/lettings-logs/#{lettings_log.id}/net-income", params: {} + expect(response).to redirect_to("/account/sign-in") + end + end + + context "when the user is from different organisation" do + let(:other_user) { create(:user) } + + before do + sign_in other_user + end + + it "renders page not found" do + post "/lettings-logs/#{lettings_log.id}/net-income", params: {} + expect(response).to have_http_status(:not_found) + end + end + + context "when user is signed in" do + let(:params) do + { + id: lettings_log.id, + lettings_log: { + page: "income_amount", + earnings: "100000", + incfreq: "1", + }, + check_errors: "", + } + end + + before do + lettings_log.update(needstype: 1, declaration: 1, ecstat1: 10, hhmemb: 2, net_income_known: 0, incfreq: 1, earnings: 1000) + end + + context "with multiple error fields and answered questions" do + before do + sign_in user + post "/lettings-logs/#{lettings_log.id}/income-amount", params: params + end + + it "displays correct clear links" do + expect(page).to have_link("Clear", href: "/lettings-logs/#{lettings_log.id}/confirm-clear-answer?question_id=hhmemb") + expect(page).to have_link("Clear", href: "/lettings-logs/#{lettings_log.id}/confirm-clear-answer?question_id=ecstat1") + expect(page).to have_link("Clear", href: "/lettings-logs/#{lettings_log.id}/confirm-clear-answer?question_id=earnings") + expect(page).to have_link("Clear all", href: "/lettings-logs/#{lettings_log.id}/confirm-clear-all-answers") + end + end + end + end + + describe "confirm clear answer page" do + context "when user is not signed in" do + it "redirects to sign in page" do + get "/lettings-logs/#{lettings_log.id}/confirm-clear-answer?original_page_id=income_amount&question_id=hhmemb&related_question_ids%5B%5D=hhmemb&related_question_ids%5B%5D=ecstat1&related_question_ids%5B%5D=earnings", params: {} + expect(response).to redirect_to("/account/sign-in") + end + end + + context "when the user is from different organisation" do + let(:other_user) { create(:user) } + + before do + sign_in other_user + end + + it "renders page not found" do + get "/lettings-logs/#{lettings_log.id}/confirm-clear-answer?original_page_id=income_amount&question_id=hhmemb&related_question_ids%5B%5D=hhmemb&related_question_ids%5B%5D=ecstat1&related_question_ids%5B%5D=earnings", params: {} + expect(response).to have_http_status(:not_found) + end + end + + context "when user is signed in" do + context "with multiple error fields and answered questions" do + before do + sign_in user + get "/lettings-logs/#{lettings_log.id}/confirm-clear-answer?original_page_id=income_amount&question_id=hhmemb&related_question_ids%5B%5D=hhmemb&related_question_ids%5B%5D=ecstat1&related_question_ids%5B%5D=earnings", params: {} + end + + it "displays correct clear links" do + expect(page).to have_content("Are you sure you want to clear Number of household members answer?") + expect(page).to have_content("This action is permanent") + expect(page).to have_button("Cancel", href: "/lettings-logs/#{lettings_log.id}/income-amount") + expect(page).to have_button("Confirm and continue", href: "/lettings-logs/#{lettings_log.id}/clear-answer?original_page_id=income_amount&question_id=hhmemb&related_question_ids%5B%5D=hhmemb&related_question_ids%5B%5D=ecstat1&related_question_ids%5B%5D=earnings") + end + end + end + end + + describe "confirm clear all answers page" do + end + + describe "clear answer" do + end + + describe "clear all answers" do + end +end diff --git a/spec/requests/check_your_errors_controller_spec.rb b/spec/requests/check_your_errors_controller_spec.rb deleted file mode 100644 index 6914192ed..000000000 --- a/spec/requests/check_your_errors_controller_spec.rb +++ /dev/null @@ -1,69 +0,0 @@ -require "rails_helper" - -RSpec.describe CheckYourErrorsController, type: :request do - let(:page) { Capybara::Node::Simple.new(response.body) } - let(:user) { create(:user, :data_coordinator) } - let(:lettings_log) { create(:lettings_log, :duplicate, assigned_to: user) } - - describe "check your errors page" do - context "when user is not signed in" do - it "redirects to sign in page" do - get "/lettings-logs/#{lettings_log.id}/check-your-errors" - expect(response).to redirect_to("/account/sign-in") - end - end - - context "when the user is from different organisation" do - let(:other_user) { create(:user) } - - before do - sign_in other_user - end - - it "renders page not found" do - get "/lettings-logs/#{lettings_log.id}/check-your-errors" - expect(response).to have_http_status(:not_found) - end - end - - context "when user is signed in" do - context "with multiple error fields and answered questions" do - before do - sign_in user - get "/lettings-logs/#{lettings_log.id}/check-your-errors?related_question_ids[]=startdate&related_question_ids[]=needstype&original_page_id=tenancy_start_date" - end - - it "displays correct clear links" do - expect(page).to have_link("Clear", href: "/lettings-logs/#{lettings_log.id}/confirm-clear-answer?original_page_id=tenancy_start_date&question_id=startdate&related_question_ids%5B%5D=startdate&related_question_ids%5B%5D=needstype") - expect(page).to have_link("Clear", href: "/lettings-logs/#{lettings_log.id}/confirm-clear-answer?original_page_id=tenancy_start_date&question_id=needstype&related_question_ids%5B%5D=startdate&related_question_ids%5B%5D=needstype") - expect(page).to have_link("Clear all", href: "/lettings-logs/#{lettings_log.id}/confirm-clear-all-answers?original_page_id=tenancy_start_date&related_question_ids%5B%5D=startdate&related_question_ids%5B%5D=needstype") - end - end - - context "with multiple error fields and unanswered questions" do - before do - lettings_log.update!(needstype: nil, startdate: nil) - sign_in user - get "/lettings-logs/#{lettings_log.id}/check-your-errors?related_question_ids[]=startdate&related_question_ids[]=needstype&original_page_id=tenancy_start_date" - end - - it "displays correct clear links" do - expect(page).to have_link("Answer", href: "/lettings-logs/#{lettings_log.id}/needs-type?original_page_id=tenancy_start_date&referrer=check_your_errors&related_question_ids%5B%5D=startdate&related_question_ids%5B%5D=needstype") - expect(page).to have_link("Answer", href: "/lettings-logs/#{lettings_log.id}/tenancy-start-date?original_page_id=tenancy_start_date&referrer=check_your_errors&related_question_ids%5B%5D=startdate&related_question_ids%5B%5D=needstype") - end - end - end - end - - describe "confirm clear answer page" do - end - - describe "confirm clear all answers page" do - end - - describe "clear answer" do - end - - describe "clear all answers" do - end -end