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 946092a46..4a26b8c8d 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), - href: action_href(log, question.page.id), + href: 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 8f18b1ffc..19c2181d1 100644 --- a/app/components/check_answers_summary_list_card_component.rb +++ b/app/components/check_answers_summary_list_card_component.rb @@ -28,8 +28,9 @@ class CheckAnswersSummaryListCardComponent < ViewComponent::Base "Person #{question.check_answers_card_number}" end - def action_href(log, page_id, referrer = "check_answers") - send("#{log.model_name.param_key}_#{page_id}_path", log, referrer:) + def action_href(question, log) + referrer = question.displayed_as_answered?(log) ? "check_answers" : nil + send("#{log.model_name.param_key}_#{question.page.id}_path", log, referrer:) end private diff --git a/app/controllers/form_controller.rb b/app/controllers/form_controller.rb index 635dc12e4..b0be0880a 100644 --- a/app/controllers/form_controller.rb +++ b/app/controllers/form_controller.rb @@ -9,6 +9,11 @@ class FormController < ApplicationController def submit_form if @log @page = form.get_page(params[@log.model_name.param_key][:page]) + shown_page_ids_with_unanswered_questions_before_update = @page.subsection.pages + .select { |page| page.routed_to?(@log, current_user) } + .select { |page| page.has_unanswered_questions?(@log) } + .map(&:id) + responses_for_page = responses_for_page(@page) mandatory_questions_with_no_response = mandatory_questions_with_no_response(responses_for_page) @@ -18,7 +23,9 @@ class FormController < ApplicationController updated_question_string = [updated_question&.question_number_string, updated_question&.check_answer_label.to_s.downcase].compact.join(": ") flash[:notice] = "You have successfully updated #{updated_question_string}" end - redirect_to(successful_redirect_path) + + pages_requiring_update = pages_requiring_update(shown_page_ids_with_unanswered_questions_before_update) + redirect_to(successful_redirect_path(pages_requiring_update)) else mandatory_questions_with_no_response.map do |question| @log.errors.add question.id.to_sym, question.unanswered_error_message, category: :not_answered @@ -171,7 +178,7 @@ private params[@log.model_name.param_key]["interruption_page_referrer_type"].presence end - def successful_redirect_path + def successful_redirect_path(pages_to_check) if FeatureToggle.deduplication_flow_enabled? if is_referrer_type?("duplicate_logs") || is_referrer_type?("duplicate_logs_banner") return correcting_duplicate_logs_redirect_path @@ -195,7 +202,9 @@ private previous_page = form.previous_page_id(@page, @log, current_user) if next_page&.interruption_screen? || next_page_id == previous_page || CONFIRMATION_PAGE_IDS.include?(next_page_id) - return send("#{@log.class.name.underscore}_#{next_page_id}_path", @log, { referrer: "check_answers" }) + return redirect_path_to_question(next_page, pages_to_check) + elsif pages_to_check.any? + return redirect_path_to_question(pages_to_check[0], pages_to_check) else return send("#{@log.model_name.param_key}_#{form.subsection_for_page(@page).id}_check_answers_path", @log) end @@ -208,6 +217,24 @@ private send(redirect_path, @log) end + def redirect_path_to_question(page_to_show, unanswered_pages) + remaining_pages = unanswered_pages.excluding(page_to_show) + remaining_page_ids = remaining_pages.any? ? remaining_pages.map(&:id).join(",") : nil + send("#{@log.class.name.underscore}_#{page_to_show.id}_path", @log, { referrer: "check_answers", unanswered_pages: remaining_page_ids }) + end + + def pages_requiring_update(previously_visible_empty_page_ids) + return [] unless is_referrer_type?("check_answers") + + currently_shown_pages = @page.subsection.pages + .select { |page| page.routed_to?(@log, current_user) } + + existing_unanswered_pages = request.params["unanswered_pages"].nil? ? [] : request.params["unanswered_pages"].split(",") + currently_shown_pages + .reject { |page| previously_visible_empty_page_ids.include?(page.id) && !existing_unanswered_pages.include?(page.id) } + .select { |page| page.has_unanswered_questions?(@log) } + end + def form @log&.form end diff --git a/app/helpers/form_page_helper.rb b/app/helpers/form_page_helper.rb index dc471da1c..d503c4f46 100644 --- a/app/helpers/form_page_helper.rb +++ b/app/helpers/form_page_helper.rb @@ -1,5 +1,10 @@ module FormPageHelper - def action_href(log, page_id, referrer = "check_answers") + def check_answers_href(question, log) + referrer = question.displayed_as_answered?(log) ? "check_answers" : nil + action_href(log, question.page.id, referrer) + end + + def action_href(log, page_id, referrer) send("#{log.model_name.param_key}_#{page_id}_path", log, referrer:) end diff --git a/app/models/form/page.rb b/app/models/form/page.rb index 6485ceb07..c1b03fd56 100644 --- a/app/models/form/page.rb +++ b/app/models/form/page.rb @@ -36,6 +36,10 @@ class Form::Page end end + def has_unanswered_questions?(log) + questions.any? { |question| log[question.id].nil? } + end + def interruption_screen? questions.all? { |question| question.type == "interruption_screen" } end diff --git a/app/models/form/question.rb b/app/models/form/question.rb index 0c1611e60..20b73f5d8 100644 --- a/app/models/form/question.rb +++ b/app/models/form/question.rb @@ -110,12 +110,16 @@ class Form::Question end def action_text(log) + displayed_as_answered?(log) ? "Change" : "Answer" + end + + def displayed_as_answered?(log) if is_derived_or_has_inferred_check_answers_value?(log) - "Change" + true elsif type == "checkbox" - answer_options.keys.any? { |key| value_is_yes?(log[key]) } ? "Change" : "Answer" + answer_options.keys.any? { |key| value_is_yes?(log[key]) } else - log[id].blank? ? "Answer" : "Change" + log[id].present? end end diff --git a/app/views/form/_check_answers_summary_list.html.erb b/app/views/form/_check_answers_summary_list.html.erb index 59d6c9bd1..747b6152f 100644 --- a/app/views/form/_check_answers_summary_list.html.erb +++ b/app/views/form/_check_answers_summary_list.html.erb @@ -28,7 +28,7 @@ <% if @log.collection_period_open_for_editing? %> <% row.with_action( text: question.action_text(@log), - href: action_href(@log, question.page.id, referrer), + href: check_answers_href(question, @log), visually_hidden_text: question.check_answer_label.to_s.downcase, ) %> <% end %> diff --git a/spec/helpers/form_page_helper_spec.rb b/spec/helpers/form_page_helper_spec.rb index aeded6f8d..8a63362c5 100644 --- a/spec/helpers/form_page_helper_spec.rb +++ b/spec/helpers/form_page_helper_spec.rb @@ -6,22 +6,36 @@ RSpec.describe FormPageHelper do let(:sales_log) { FactoryBot.create(:sales_log) } context "with a lettings log" do - it "has an update answer link href helper" do - expect(action_href(lettings_log, "net_income")).to eq("/lettings-logs/#{lettings_log.id}/net-income?referrer=check_answers") + let(:question) { lettings_log.form.questions.detect { |q| q.id == "needstype" } } + + it "answer link href helper does not attach referrer when question not answered" do + expect(check_answers_href(question, lettings_log)).to eq("/lettings-logs/#{lettings_log.id}/needs-type") + end + + it "answer link href helper attaches referrer when question already answered" do + lettings_log[question.id] = 1 + expect(check_answers_href(question, lettings_log)).to eq("/lettings-logs/#{lettings_log.id}/needs-type?referrer=check_answers") end - it "returns a correct referrer in the url" do - expect(action_href(lettings_log, "retirement_value_check", "interruption_screen")).to eq("/lettings-logs/#{lettings_log.id}/retirement-value-check?referrer=interruption_screen") + it "has an action href helper" do + expect(action_href(lettings_log, "net_income", "interruption_screen")).to eq("/lettings-logs/#{lettings_log.id}/net-income?referrer=interruption_screen") end end context "with a sales log" do - it "has an update answer link href helper" do - expect(action_href(sales_log, "buyer_1_age")).to eq("/sales-logs/#{sales_log.id}/buyer-1-age?referrer=check_answers") + let(:question) { sales_log.form.questions.detect { |q| q.id == "ownershipsch" } } + + it "answer link href helper does not attach referrer when question not answered" do + expect(check_answers_href(question, sales_log)).to eq("/sales-logs/#{sales_log.id}/ownership-scheme") + end + + it "answer link href helper attaches referrer when question already answered" do + sales_log[question.id] = 1 + expect(check_answers_href(question, sales_log)).to eq("/sales-logs/#{sales_log.id}/ownership-scheme?referrer=check_answers") end - it "returns a correct referrer in the url" do - expect(action_href(sales_log, "grant_value_check", "interruption_screen")).to eq("/sales-logs/#{sales_log.id}/grant-value-check?referrer=interruption_screen") + it "has an action href helper" do + expect(action_href(sales_log, "buyer_1_age", "interruption_screen")).to eq("/sales-logs/#{sales_log.id}/buyer-1-age?referrer=interruption_screen") end end end diff --git a/spec/requests/form_controller_spec.rb b/spec/requests/form_controller_spec.rb index 76f8e52a5..46d2430b0 100644 --- a/spec/requests/form_controller_spec.rb +++ b/spec/requests/form_controller_spec.rb @@ -1036,6 +1036,13 @@ RSpec.describe FormController, type: :request do sales_log.saledate = Time.zone.local(2024, 12, 1) sales_log.save!(validate: false) sales_log.reload + Timecop.freeze(Time.zone.local(2024, 12, 1)) + Singleton.__init__(FormHandler) + end + + after do + Timecop.unfreeze + Singleton.__init__(FormHandler) end it "sets managing organisation to created by organisation" do @@ -1326,6 +1333,57 @@ RSpec.describe FormController, type: :request do end context "when coming from check answers page" do + let(:sales_log) { create(:sales_log, ownershipsch: 3, created_by: user) } + let(:lettings_log_referrer) { "/lettings-logs/#{lettings_log.id}/needs-type?referrer=check_answers" } + let(:sales_log_referrer) { "/sales-logs/#{sales_log.id}/ownership-scheme?referrer=check_answers" } + + let(:sales_log_form_ownership_params) do + { + id: sales_log.id, + sales_log: { + page: "ownership_scheme", + ownershipsch: 1, + }, + } + end + let(:sales_log_form_shared_ownership_type_params) do + { + id: sales_log.id, + unanswered_pages: "joint_purchase", + sales_log: { + page: "shared_ownership_type", + type: 2, + }, + } + end + let(:lettings_log_form_needs_type_params) do + { + id: lettings_log.id, + lettings_log: { + page: "needs_type", + needstype: 2, + }, + } + end + + context "and changing an answer" do + it "navigates to follow-up questions when required" do + post "/lettings-logs/#{lettings_log.id}/needs-type", params: lettings_log_form_needs_type_params, headers: headers.merge({ "HTTP_REFERER" => lettings_log_referrer }) + expect(response).to redirect_to("/lettings-logs/#{lettings_log.id}/scheme?referrer=check_answers") + end + + it "queues up additional follow-up questions if needed" do + post "/sales-logs/#{sales_log.id}/shared-ownership-type", params: sales_log_form_ownership_params, headers: headers.merge({ "HTTP_REFERER" => sales_log_referrer }) + expect(response).to redirect_to("/sales-logs/#{sales_log.id}/shared-ownership-type?referrer=check_answers&unanswered_pages=joint_purchase") + end + + it "moves to a queued up follow-up questions if one was provided" do + post "/sales-logs/#{sales_log.id}/shared-ownership-type", params: sales_log_form_ownership_params, headers: headers.merge({ "HTTP_REFERER" => sales_log_referrer }) + post "/sales-logs/#{sales_log.id}/ownership-scheme", params: sales_log_form_shared_ownership_type_params, headers: headers.merge({ "HTTP_REFERER" => sales_log_referrer }) + expect(response).to redirect_to("/sales-logs/#{sales_log.id}/joint-purchase?referrer=check_answers") + end + end + context "and navigating to an interruption screen" do let(:interrupt_params) do {