From 2d2da174fea70f4f0464b3b9a28ed0860f67163b Mon Sep 17 00:00:00 2001
From: Paul Robert Lloyd <me+git@paulrobertlloyd.com>
Date: Mon, 25 Apr 2022 17:49:04 +0100
Subject: [PATCH 1/2] Refactor check answers row to use components

---
 app/models/form/question.rb                  | 21 +++++++++++--------
 app/views/form/_check_answers_table.html.erb | 22 ++++++++++----------
 app/views/form/check_answers.html.erb        | 10 ++++++---
 app/views/form/review.html.erb               | 12 +++++++----
 spec/models/form/question_spec.rb            | 11 +++++++---
 5 files changed, 46 insertions(+), 30 deletions(-)

diff --git a/app/models/form/question.rb b/app/models/form/question.rb
index a3b8ea21b..ca1eef09a 100644
--- a/app/models/form/question.rb
+++ b/app/models/form/question.rb
@@ -86,15 +86,18 @@ class Form::Question
     end
   end
 
-  def update_answer_link_name(case_log)
-    link_type = if has_inferred_check_answers_value?(case_log)
-                  "Change"
-                elsif type == "checkbox"
-                  answer_options.keys.any? { |key| value_is_yes?(case_log[key]) } ? "Change" : "Answer"
-                else
-                  case_log[id].blank? ? "Answer" : "Change"
-                end
-    "#{link_type}<span class=\"govuk-visually-hidden\"> #{check_answer_label.to_s.downcase}</span>".html_safe
+  def action_text(case_log)
+    if has_inferred_check_answers_value?(case_log)
+      "Change"
+    elsif type == "checkbox"
+      answer_options.keys.any? { |key| value_is_yes?(case_log[key]) } ? "Change" : "Answer"
+    else
+      case_log[id].blank? ? "Answer" : "Change"
+    end
+  end
+
+  def action_href(case_log, page_id)
+    "/logs/#{case_log.id}/#{page_id.to_s.dasherize}?referrer=check_answers"
   end
 
   def completed?(case_log)
diff --git a/app/views/form/_check_answers_table.html.erb b/app/views/form/_check_answers_table.html.erb
index c34c868e0..afae78056 100644
--- a/app/views/form/_check_answers_table.html.erb
+++ b/app/views/form/_check_answers_table.html.erb
@@ -1,14 +1,14 @@
-<div class="govuk-summary-list__row">
-  <dt class="govuk-summary-list__key">
-    <%= question.check_answer_label.to_s.presence || question.header.to_s %>
-  </dt>
-  <dd class="govuk-summary-list__value">
+<% summary_list.row do |row| %>
+  <% row.key { question.check_answer_label.to_s.presence || question.header.to_s } %>
+  <% row.value do %>
     <%= get_answer_label(question, @case_log) %><br>
     <% question.get_inferred_answers(@case_log).each do |inferred_answer| %>
-      <span class="govuk-!-font-weight-regular app-!-colour-muted"><%= inferred_answer %></span><br>
+      <span class="govuk-!-font-weight-regular app-!-colour-muted"><%= inferred_answer %></span>
     <% end %>
-  </dd>
-  <dd class="govuk-summary-list__actions">
-    <%= govuk_link_to(question.update_answer_link_name(@case_log), "/logs/#{@case_log.id}/#{question.page.id.to_s.dasherize}?referrer=check_answers").html_safe %>
-  </dd>
-</div>
+  <% end %>
+  <% row.action(
+    text: question.action_text(@case_log),
+    href: question.action_href(@case_log, question.page.id),
+    visually_hidden_text: question.check_answer_label.to_s.downcase,
+  ) %>
+<% end %>
diff --git a/app/views/form/check_answers.html.erb b/app/views/form/check_answers.html.erb
index d38cb1f0a..b7ac5c24b 100644
--- a/app/views/form/check_answers.html.erb
+++ b/app/views/form/check_answers.html.erb
@@ -19,11 +19,15 @@
     <% end %>
     <%= display_answered_questions_summary(subsection, @case_log) %>
 
-    <dl class="govuk-summary-list">
+    <%= govuk_summary_list do |summary_list| %>
       <% subsection.applicable_questions(@case_log).each do |question| %>
-        <%= render partial: "form/check_answers_table", locals: { question:, case_log: @case_log } %>
+        <%= render partial: "form/check_answers_table", locals: {
+          summary_list:,
+          question:,
+          case_log: @case_log,
+        } %>
       <% end %>
-    </dl>
+    <% end %>
 
     <%= form_with model: @case_log, method: "get" do |f| %>
       <%= f.govuk_submit "Save and return to log" do %>
diff --git a/app/views/form/review.html.erb b/app/views/form/review.html.erb
index 2bfb8f9e3..0254e1022 100644
--- a/app/views/form/review.html.erb
+++ b/app/views/form/review.html.erb
@@ -21,14 +21,18 @@
             <h3 class="x-govuk-summary-card__title"><%= subsection.label %></h3>
           </div>
           <div class="x-govuk-summary-card__body">
-            <dl class="govuk-summary-list">
+            <%= govuk_summary_list do |summary_list| %>
               <% subsection.applicable_questions(@case_log).each do |question| %>
-                <%= render partial: 'form/check_answers_table', locals: { question: question, case_log: @case_log } %>
+                <%= render partial: 'form/check_answers_table', locals: {
+                  summary_list:,
+                  question:,
+                  case_log: @case_log,
+                } %>
               <% end %>
-            </dl>
+            <% end %>
           </div>
         </div>
       <% end %>
     <% end %>
   </div>
-</div>
\ No newline at end of file
+</div>
diff --git a/spec/models/form/question_spec.rb b/spec/models/form/question_spec.rb
index 094351870..5ae5790f9 100644
--- a/spec/models/form/question_spec.rb
+++ b/spec/models/form/question_spec.rb
@@ -226,9 +226,14 @@ RSpec.describe Form::Question, type: :model do
     end
 
     it "has an update answer link text helper" do
-      expect(question.update_answer_link_name(case_log)).to match(/Answer/)
+      expect(question.action_text(case_log)).to match(/Answer/)
       case_log["incfreq"] = 0
-      expect(question.update_answer_link_name(case_log)).to match(/Change/)
+      expect(question.action_text(case_log)).to match(/Change/)
+    end
+
+    it "has an update answer link href helper" do
+      case_log.id = 1
+      expect(question.action_href(case_log, page.id)).to eq("/logs/1/net-income?referrer=check_answers")
     end
 
     context "when the question has an inferred answer" do
@@ -239,7 +244,7 @@ RSpec.describe Form::Question, type: :model do
       let(:question_id) { "postcode_full" }
 
       it "displays 'change' in the check answers link text" do
-        expect(question.update_answer_link_name(case_log)).to match(/Change/)
+        expect(question.action_text(case_log)).to match(/Change/)
       end
     end
 

From 26e7f16d28fefed20dff33057dd12064723a256d Mon Sep 17 00:00:00 2001
From: Paul Robert Lloyd <me+git@paulrobertlloyd.com>
Date: Mon, 25 Apr 2022 17:50:58 +0100
Subject: [PATCH 2/2] Refactor partial used for check answers summary list

---
 .../form/_check_answers_summary_list.html.erb  | 18 ++++++++++++++++++
 app/views/form/_check_answers_table.html.erb   | 14 --------------
 app/views/form/check_answers.html.erb          | 13 ++++---------
 app/views/form/review.html.erb                 | 13 ++++---------
 4 files changed, 26 insertions(+), 32 deletions(-)
 create mode 100644 app/views/form/_check_answers_summary_list.html.erb
 delete mode 100644 app/views/form/_check_answers_table.html.erb

diff --git a/app/views/form/_check_answers_summary_list.html.erb b/app/views/form/_check_answers_summary_list.html.erb
new file mode 100644
index 000000000..a2aa3311f
--- /dev/null
+++ b/app/views/form/_check_answers_summary_list.html.erb
@@ -0,0 +1,18 @@
+<%= govuk_summary_list do |summary_list| %>
+  <% subsection.applicable_questions(@case_log).each do |question| %>
+    <% summary_list.row do |row| %>
+      <% row.key { question.check_answer_label.to_s.presence || question.header.to_s } %>
+      <% row.value do %>
+        <%= get_answer_label(question, @case_log) %><br>
+        <% question.get_inferred_answers(@case_log).each do |inferred_answer| %>
+          <span class="govuk-!-font-weight-regular app-!-colour-muted"><%= inferred_answer %></span>
+        <% end %>
+      <% end %>
+      <% row.action(
+        text: question.action_text(@case_log),
+        href: question.action_href(@case_log, question.page.id),
+        visually_hidden_text: question.check_answer_label.to_s.downcase,
+      ) %>
+    <% end %>
+  <% end %>
+<% end %>
diff --git a/app/views/form/_check_answers_table.html.erb b/app/views/form/_check_answers_table.html.erb
deleted file mode 100644
index afae78056..000000000
--- a/app/views/form/_check_answers_table.html.erb
+++ /dev/null
@@ -1,14 +0,0 @@
-<% summary_list.row do |row| %>
-  <% row.key { question.check_answer_label.to_s.presence || question.header.to_s } %>
-  <% row.value do %>
-    <%= get_answer_label(question, @case_log) %><br>
-    <% question.get_inferred_answers(@case_log).each do |inferred_answer| %>
-      <span class="govuk-!-font-weight-regular app-!-colour-muted"><%= inferred_answer %></span>
-    <% end %>
-  <% end %>
-  <% row.action(
-    text: question.action_text(@case_log),
-    href: question.action_href(@case_log, question.page.id),
-    visually_hidden_text: question.check_answer_label.to_s.downcase,
-  ) %>
-<% end %>
diff --git a/app/views/form/check_answers.html.erb b/app/views/form/check_answers.html.erb
index b7ac5c24b..0a98b35b1 100644
--- a/app/views/form/check_answers.html.erb
+++ b/app/views/form/check_answers.html.erb
@@ -19,15 +19,10 @@
     <% end %>
     <%= display_answered_questions_summary(subsection, @case_log) %>
 
-    <%= govuk_summary_list do |summary_list| %>
-      <% subsection.applicable_questions(@case_log).each do |question| %>
-        <%= render partial: "form/check_answers_table", locals: {
-          summary_list:,
-          question:,
-          case_log: @case_log,
-        } %>
-      <% end %>
-    <% end %>
+    <%= render partial: 'form/check_answers_summary_list', locals: {
+      subsection:,
+      case_log: @case_log,
+    } %>
 
     <%= form_with model: @case_log, method: "get" do |f| %>
       <%= f.govuk_submit "Save and return to log" do %>
diff --git a/app/views/form/review.html.erb b/app/views/form/review.html.erb
index 0254e1022..19e8498d6 100644
--- a/app/views/form/review.html.erb
+++ b/app/views/form/review.html.erb
@@ -21,15 +21,10 @@
             <h3 class="x-govuk-summary-card__title"><%= subsection.label %></h3>
           </div>
           <div class="x-govuk-summary-card__body">
-            <%= govuk_summary_list do |summary_list| %>
-              <% subsection.applicable_questions(@case_log).each do |question| %>
-                <%= render partial: 'form/check_answers_table', locals: {
-                  summary_list:,
-                  question:,
-                  case_log: @case_log,
-                } %>
-              <% end %>
-            <% end %>
+            <%= render partial: 'form/check_answers_summary_list', locals: {
+              subsection:,
+              case_log: @case_log,
+            } %>
           </div>
         </div>
       <% end %>