CLDC-2670: Improve navigation flow when revisiting questions from CYA page (#2362)

* CLDC-2670: Improve navigation flow when revisiting questions from CYA page

* CLDC-2670: Fix failing test

* CLDC-2670: fix issue with wrong referrer behaviour on interrupt screens

* CLDC-2670: Fix build errors

* Update validation feature

* CLDC-2670: Do not show already-answered questions when answering new questions from CYA page

* CLDC-2670: Fix linter error and tests

* Fix incorrect URLs in feature test

* feat: fix typo

* feat: make skip links for new answers route to check_answers

* feat: make skip links for new answers route to check_answers

* feat: update tests

* feat: bug fix

* feat: test fix

* feat: skip unanswered questions for interruption screens

---------

Co-authored-by: natdeanlewissoftwire <nat.dean-lewis@softwire.com>

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 %>

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" : "check_answers_new_answer"
+    send("#{log.model_name.param_key}_#{question.page.id}_path", log, referrer:)
   end
 
 private

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
@@ -204,8 +213,29 @@ private
       return send("#{@log.class.name.underscore}_#{previous_interruption_screen_page_id}_path", @log, { referrer: previous_interruption_screen_referrer, original_log_id: original_duplicate_log_id_from_query }.compact)
     end
 
-    redirect_path = form.next_page_redirect_path(@page, @log, current_user)
-    send(redirect_path, @log)
+    is_new_answer_from_check_answers = is_referrer_type?("check_answers_new_answer")
+    redirect_path = form.next_page_redirect_path(@page, @log, current_user, ignore_answered: is_new_answer_from_check_answers)
+    referrer = is_new_answer_from_check_answers ? "check_answers_new_answer" : nil
+
+    send(redirect_path, @log, { referrer: })
+  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

app/helpers/form_page_helper.rb

@@ -43,7 +43,7 @@ module FormPageHelper
     elsif returning_to_question_page?(page, referrer)
       send(log.form.cancel_path(page, log), log)
     else
-      page.skip_href(log) || send(log.form.next_page_redirect_path(page, log, current_user), log)
+      page.skip_href(log) || send(log.form.next_page_redirect_path(page, log, current_user, ignore_answered: true), log)
     end
   end
 end

app/models/form.rb

@@ -80,7 +80,7 @@ class Form
     subsections.find { |s| s.pages.find { |p| p.id == page.id } }
   end
 
-  def next_page_id(page, log, current_user)
+  def next_page_id(page, log, current_user, ignore_answered: false)
     return page.next_unresolved_page_id || :check_answers if log.unresolved
 
     page_ids = subsection_for_page(page).pages.map(&:id)
@@ -93,13 +93,14 @@ class Form
     next_page = get_page(page_id)
 
     return :check_answers if next_page.nil?
-    return next_page.id if next_page.routed_to?(log, current_user)
+    return next_page.id if next_page.routed_to?(log, current_user) &&
+      (!ignore_answered || next_page.has_unanswered_questions?(log))
 
-    next_page_id(next_page, log, current_user)
+    next_page_id(next_page, log, current_user, ignore_answered:)
   end
 
-  def next_page_redirect_path(page, log, current_user)
-    next_page_id = next_page_id(page, log, current_user)
+  def next_page_redirect_path(page, log, current_user, ignore_answered: false)
+    next_page_id = next_page_id(page, log, current_user, ignore_answered:)
     if next_page_id == :check_answers
       "#{type}_log_#{subsection_for_page(page).id}_check_answers_path"
     else

app/models/form/page.rb

@@ -36,6 +36,10 @@ class Form::Page
     end
   end
 
+  def has_unanswered_questions?(log)
+    questions.any? { |question| question.displayed_to_user?(log) && question.unanswered?(log) }
+  end
+
   def interruption_screen?
     questions.all? { |question| question.type == "interruption_screen" }
   end

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
 

app/views/form/_check_answers_summary_list.html.erb

@@ -28,7 +28,11 @@
       <% if @log.collection_period_open_for_editing? %>
         <% row.with_action(
           text: question.action_text(@log),
-          href: action_href(@log, question.page.id, referrer),
+          href: action_href(
+            @log,
+            question.page.id,
+            question.displayed_as_answered?(@log) || !defined?(referrer_unanswered) ? referrer : referrer_unanswered,
+          ),
           visually_hidden_text: question.check_answer_label.to_s.downcase,
         ) %>
       <% end %>

app/views/form/_interruption_screen_question.html.erb

@@ -19,6 +19,6 @@
   <%= f.govuk_submit "Confirm and continue" %>
   <%= govuk_link_to(
     (@page.skip_text || "Skip for now"),
-    (@page.skip_href(@log) || send(@log.form.next_page_redirect_path(@page, @log, current_user), @log)),
+    (@page.skip_href(@log) || send(@log.form.next_page_redirect_path(@page, @log, current_user, ignore_answered: true), @log)),
   ) %>
 </div>

app/views/form/check_answers.html.erb

@@ -27,6 +27,7 @@
         lettings_log: @log,
         questions: total_applicable_questions(subsection, @log, current_user),
         referrer: "check_answers",
+        referrer_unanswered: "check_answers_new_answer",
       } %>
     <% end %>
 

app/views/form/review.html.erb

@@ -19,7 +19,12 @@
               <h3 class="govuk-summary-card__title"><%= subsection.label %></h3>
             </div>
             <div class="govuk-summary-card__content">
-              <%= render partial: "form/check_answers_summary_list", locals: { subsection:, questions: total_applicable_questions(subsection, @log, current_user), referrer: "check_answers" } %>
+              <%= render partial: "form/check_answers_summary_list", locals: {
+                subsection:,
+                questions: total_applicable_questions(subsection, @log, current_user),
+                referrer: "check_answers",
+                referrer_unanswered: "check_answers_new_answer",
+              } %>
             </div>
           </div>
         <% end %>

spec/features/form/check_answers_page_lettings_logs_spec.rb

@@ -89,11 +89,11 @@ RSpec.describe "Lettings Log Check Answers Page" do
 
     # Regex explanation: match the string "Answer" but not if it's follow by "the missing questions"
     # This way only the links in the table will get picked up
-    it "has an answer link for questions missing an answer" do
+    it "has an answer link with the check_answers_new_answer referrer for questions missing an answer" do
       visit("/lettings-logs/#{empty_lettings_log.id}/#{subsection}/check-answers?referrer=check_answers")
       assert_selector "a", text: /Answer (?!the missing questions)/, count: 4
       assert_selector "a", text: "Change", count: 0
-      expect(page).to have_link("Answer", href: "/lettings-logs/#{empty_lettings_log.id}/person-1-age?referrer=check_answers")
+      expect(page).to have_link("Answer", href: "/lettings-logs/#{empty_lettings_log.id}/person-1-age?referrer=check_answers_new_answer")
     end
 
     it "has a change link for answered questions" do

spec/features/form/form_navigation_spec.rb

@@ -66,12 +66,12 @@ RSpec.describe "Form Navigation" do
       expect(page).to have_field("lettings-log-age1-field")
     end
 
-    it "a question page leads to the next question defined in the form definition" do
+    it "a question page leads to the next unanswered question defined in the form definition" do
       pages = question_answers.map { |_key, val| val[:path] }
       pages[0..-2].each_with_index do |val, index|
-        visit("/lettings-logs/#{id}/#{val}")
+        visit("/lettings-logs/#{empty_lettings_log.id}/#{val}")
         click_link("Skip for now")
-        expect(page).to have_current_path("/lettings-logs/#{id}/#{pages[index + 1]}")
+        expect(page).to have_current_path("/lettings-logs/#{empty_lettings_log.id}/#{pages[index + 1]}")
       end
     end
 

spec/features/form/validations_spec.rb

@@ -171,15 +171,17 @@ RSpec.describe "validations" do
         expect(page).to have_css(".govuk-notification-banner.govuk-notification-banner--success")
       end
 
-      it "returns the user back to the check_your_answers after fixing a validation from check_your_anwers" do
-        lettings_log.update!(earnings: income_over_soft_limit, incfreq: 1)
+      it "returns the user back to the check_your_answers after fixing a validation from check_your_answers" do
+        lettings_log.update!(earnings: income_under_soft_limit, incfreq: 1, net_income_value_check: 1)
         visit("/lettings-logs/#{lettings_log.id}/income-and-benefits/check-answers")
-        click_link("Answer", href: "/lettings-logs/#{lettings_log.id}/net-income-value-check?referrer=check_answers")
+        click_link("Change", href: "/lettings-logs/#{lettings_log.id}/net-income?referrer=check_answers", match: :first)
+        expect(page).to have_current_path("/lettings-logs/#{lettings_log.id}/net-income?referrer=check_answers")
+        fill_in("lettings-log-earnings-field", with: income_over_soft_limit)
+        click_button("Save changes")
         expect(page).to have_current_path("/lettings-logs/#{lettings_log.id}/net-income-value-check?referrer=check_answers")
         click_link("Change", href: "/lettings-logs/#{lettings_log.id}/net-income?referrer=interruption_screen", match: :first)
         expect(page).to have_current_path("/lettings-logs/#{lettings_log.id}/net-income?referrer=interruption_screen")
         fill_in("lettings-log-earnings-field", with: income_under_soft_limit)
-        choose("lettings-log-incfreq-1-field", allow_label_click: true)
         click_button("Save and continue")
         expect(page).to have_current_path("/lettings-logs/#{lettings_log.id}/net-income-value-check?referrer=check_answers")
         click_button("Confirm and continue")

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
@@ -1261,7 +1268,7 @@ RSpec.describe FormController, type: :request do
                     "housingneeds_g" => "No disability requirements",
                     "divider_a" => true,
                     "housingneeds_h" => "Don’t know" },
-                }, nil
+                }, page
               ),
               Form::Question.new("tenancycode", { "type" => "text" }, nil),
             ]
@@ -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
             {