From 7c3fb2707325d194bb781a10f602cb60f7103fc0 Mon Sep 17 00:00:00 2001
From: Manny Dinssa <44172848+Dinssa@users.noreply.github.com>
Date: Mon, 16 Dec 2024 19:02:00 +0000
Subject: [PATCH] Refactor improvements

---
 ...eck_answers_summary_list_card_component.rb | 12 +++---
 app/helpers/check_answers_helper.rb           | 12 +++---
 app/helpers/details_table_helper.rb           | 39 +++++++++++++------
 app/helpers/locations_helper.rb               | 30 ++++++++++----
 app/helpers/organisations_helper.rb           | 11 +++---
 app/helpers/schemes_helper.rb                 | 17 ++++----
 app/models/form/question.rb                   | 24 ++++--------
 app/views/locations/show.html.erb             | 10 ++---
 8 files changed, 89 insertions(+), 66 deletions(-)

diff --git a/app/components/check_answers_summary_list_card_component.rb b/app/components/check_answers_summary_list_card_component.rb
index 493f190c3..c22b2eee0 100644
--- a/app/components/check_answers_summary_list_card_component.rb
+++ b/app/components/check_answers_summary_list_card_component.rb
@@ -47,11 +47,13 @@ class CheckAnswersSummaryListCardComponent < ViewComponent::Base
 private
 
   def unanswered_value(question)
-    if log.creation_method_bulk_upload? && log.bulk_upload.present? && !log.optional_fields.include?(question.id)
-      govuk_link_to question.check_answer_prompt, correct_validation_action_href(question, log, nil, @correcting_hard_validation), class: "app-red-link app-red-link---no-visited-state"
-    else
-      govuk_link_to question.check_answer_prompt, correct_validation_action_href(question, log, nil, @correcting_hard_validation), class: "govuk-link govuk-link--no-visited-state"
-    end
+    link_class = if log.creation_method_bulk_upload? && log.bulk_upload.present? && !log.optional_fields.include?(question.id)
+                   "app-red-link app-red-link---no-visited-state"
+                 else
+                   "govuk-link govuk-link--no-visited-state"
+                 end
+
+    govuk_link_to question.check_answer_prompt, correct_validation_action_href(question, log, nil, @correcting_hard_validation), class: link_class
   end
 
   def number_of_buyers
diff --git a/app/helpers/check_answers_helper.rb b/app/helpers/check_answers_helper.rb
index defc86222..f8325dd1b 100644
--- a/app/helpers/check_answers_helper.rb
+++ b/app/helpers/check_answers_helper.rb
@@ -61,10 +61,12 @@ private
   end
 
   def unanswered_value(log:, question:)
-    if log.creation_method_bulk_upload? && log.bulk_upload.present? && !log.optional_fields.include?(question.id)
-      govuk_link_to question.check_answer_prompt, unanswered_action_href(question, log), class: "app-red-link app-red-link---no-visited-state"
-    else
-      govuk_link_to question.check_answer_prompt, unanswered_action_href(question, log), class: "govuk-link govuk-link--no-visited-state"
-    end
+    link_class = if log.creation_method_bulk_upload? && log.bulk_upload.present? && !log.optional_fields.include?(question.id)
+                   "app-red-link app-red-link---no-visited-state"
+                 else
+                   "govuk-link govuk-link--no-visited-state"
+                 end
+
+    govuk_link_to question.check_answer_prompt, correct_validation_action_href(question, log, nil, @correcting_hard_validation), class: link_class
   end
 end
diff --git a/app/helpers/details_table_helper.rb b/app/helpers/details_table_helper.rb
index 41e569c5d..957417c9b 100644
--- a/app/helpers/details_table_helper.rb
+++ b/app/helpers/details_table_helper.rb
@@ -8,18 +8,35 @@ module DetailsTableHelper
     else
       return simple_format(attribute[:value].first.to_s, { class: "govuk-body" }, wrapper_tag: "p") if attribute[:value].is_a?(Array) && attribute[:value].any?
 
-      value = attribute[:value].presence || case resource_class
-                                            when "Location"
-                                              LocationPolicy.new(current_user, resource).update? ? govuk_link_to(location_details_link_message(attribute), location_edit_path(resource, attribute[:attribute]), class: "govuk-link govuk-link--no-visited-state") : "<span class=\"app-!-colour-muted\">No answer provided</span>".html_safe
-                                            when "Organisation"
-                                              can_edit_org?(current_user) && attribute[:editable] ? govuk_link_to(organisation_details_link_message(attribute), edit_organisation_path(resource), class: "govuk-link govuk-link--no-visited-state") : "<span class=\"app-!-colour-muted\">No answer provided</span>".html_safe
-                                            when "Scheme"
-                                              can_change_scheme_answer?(attribute[:name], resource) ? govuk_link_to(scheme_details_link_message(attribute), scheme_edit_path(resource, attribute), class: "govuk-link govuk-link--no-visited-state") : "<span class=\"app-!-colour-muted\">No answer provided</span>".html_safe
-                                            else
-                                              "<span class=\"app-!-colour-muted\">No answer provided</span>".html_safe
-                                            end
-
+      value = determine_value(attribute, resource, resource_class)
       simple_format(value.to_s, { class: "govuk-body" }, wrapper_tag: "p")
     end
   end
+
+  private
+
+  def determine_value(attribute, resource, resource_class)
+    attribute[:value].presence || case resource_class
+                                  when "Location"
+                                    location_value(attribute, resource)
+                                  when "Organisation"
+                                    organisation_value(attribute, resource)
+                                  when "Scheme"
+                                    scheme_value(attribute, resource)
+                                  else
+                                    "<span class=\"app-!-colour-muted\">No answer provided</span>".html_safe
+                                  end
+  end
+
+  def location_value(attribute, resource)
+    LocationPolicy.new(current_user, resource).update? ? govuk_link_to(location_details_link_message(attribute), location_edit_path(resource, attribute[:attribute]), class: "govuk-link govuk-link--no-visited-state") : "<span class=\"app-!-colour-muted\">No answer provided</span>".html_safe
+  end
+
+  def organisation_value(attribute, resource)
+    can_edit_org?(current_user) && attribute[:editable] ? govuk_link_to(organisation_details_link_message(attribute), edit_organisation_path(resource), class: "govuk-link govuk-link--no-visited-state") : "<span class=\"app-!-colour-muted\">No answer provided</span>".html_safe
+  end
+
+  def scheme_value(attribute, resource)
+    can_change_scheme_answer?(attribute[:name], resource) ? govuk_link_to(scheme_details_link_message(attribute), scheme_edit_path(resource, attribute), class: "govuk-link govuk-link--no-visited-state") : "<span class=\"app-!-colour-muted\">No answer provided</span>".html_safe
+  end
 end
diff --git a/app/helpers/locations_helper.rb b/app/helpers/locations_helper.rb
index b27179aea..404e7af0f 100644
--- a/app/helpers/locations_helper.rb
+++ b/app/helpers/locations_helper.rb
@@ -65,7 +65,24 @@ module LocationsHelper
   end
 
   def location_action_text_helper(attr, location)
-    attr[:value].blank? || (attr[:attribute] == "availability" && location.startdate.blank?) ? "" : "Change"
+    return "" if attr[:value].blank? || (attr[:attribute] == "availability" && location.startdate.blank?)
+
+    "Change"
+  end
+
+  def location_action_link(attr, scheme, location, current_user)
+    return unless LocationPolicy.new(current_user, location).update?
+    return unless current_user.support? && attr[:value].present?
+
+    paths = {
+      "postcode" => scheme_location_postcode_path(scheme, location, referrer: "details"),
+      "name" => scheme_location_name_path(scheme, location, referrer: "details"),
+      "units" => scheme_location_units_path(scheme, location, referrer: "details"),
+      "type_of_unit" => scheme_location_type_of_unit_path(scheme, location, referrer: "details"),
+      "mobility_standards" => scheme_location_mobility_standards_path(scheme, location, referrer: "details"),
+    }
+
+    paths[attr[:attribute]]
   end
 
   def toggle_location_link(location)
@@ -97,13 +114,10 @@ module LocationsHelper
 
   def location_details_link_message(attribute)
     text = lowercase_first_letter(attribute[:name])
-    messages = {
-      "local_authority" => "Select #{text}",
-      "type_of_unit" => "Select #{text}",
-      "mobility_standards" => "Select #{text}",
-      "availability" => "Set #{text}",
-    }
-    messages[attribute[:attribute]] || "Enter #{text}"
+    return "Select #{text}" if %w[local_authority type_of_unit mobility_standards].include?(attribute[:attribute])
+    return "Set #{text}" if attribute[:attribute] == "availability"
+
+    "Enter #{text}"
   end
 
 private
diff --git a/app/helpers/organisations_helper.rb b/app/helpers/organisations_helper.rb
index 0b1c84f51..3c9dabc77 100644
--- a/app/helpers/organisations_helper.rb
+++ b/app/helpers/organisations_helper.rb
@@ -56,14 +56,15 @@ module OrganisationsHelper
   end
 
   def organisation_action_text_helper(attr, organisation)
-    attr[:value].blank? || (attr[:attribute] == "phone" && organisation.phone.blank?) ? "" : "Change"
+    return "" if attr[:value].blank? || (attr[:attribute] == "phone" && organisation.phone.blank?)
+
+    "Change"
   end
 
   def organisation_details_link_message(attribute)
     text = lowercase_first_letter(attribute[:name])
-    messages = {
-      "Rent periods" => "Add #{text}",
-    }
-    messages[attribute[:name]] || "Enter #{text}"
+    return "Add #{text}" if attribute[:name] == "Rent periods"
+
+    "Enter #{text}"
   end
 end
diff --git a/app/helpers/schemes_helper.rb b/app/helpers/schemes_helper.rb
index 8078bd603..6d638bc61 100644
--- a/app/helpers/schemes_helper.rb
+++ b/app/helpers/schemes_helper.rb
@@ -80,8 +80,9 @@ module SchemesHelper
   end
 
   def change_link_text(question_id, scheme)
-    attribute_value = scheme.public_send(question_id)
-    attribute_value.nil? ? "" : "Change"
+    return "" if scheme.public_send(question_id).nil?
+
+    "Change"
   end
 
   def scheme_status_hint(scheme)
@@ -121,14 +122,10 @@ module SchemesHelper
 
   def scheme_details_link_message(attribute)
     text = lowercase_first_letter(attribute[:name])
-    messages = {
-      "primary_client_group" => "Select #{text}",
-      "secondary_client_group" => "Select #{text}",
-      "support_type" => "Select #{text}",
-      "intended_stay" => "Select #{text}",
-      "has_other_client_group" => "Answer if it #{text}",
-    }
-    messages[attribute[:id]] || "Enter #{text}"
+    return "Select #{text}" if %w[primary_client_group secondary_client_group support_type intended_stay].include?(attribute[:id])
+    return "Answer if it #{text}" if attribute[:id] == "has_other_client_group"
+
+    "Enter #{text}"
   end
 
   def scheme_back_button_path(scheme, current_page)
diff --git a/app/models/form/question.rb b/app/models/form/question.rb
index 117db3c47..2fe172964 100644
--- a/app/models/form/question.rb
+++ b/app/models/form/question.rb
@@ -135,11 +135,9 @@ class Form::Question
   end
 
   def action_text(log, correcting_hard_validation: false)
-    if displayed_as_answered?(log)
-      correcting_hard_validation ? "Clear" : "Change"
-    else
-      ""
-    end
+    return "" unless displayed_as_answered?(log)
+
+    correcting_hard_validation ? "Clear" : "Change"
   end
 
   def displayed_as_answered?(log)
@@ -248,18 +246,10 @@ class Form::Question
 
   def generate_check_answer_prompt
     question_text = lowercase_first_letter(error_label.presence || check_answer_label.presence || header.presence || id.humanize) || "this question."
-    case type
-    when "checkbox"
-      "Select #{question_text}"
-    when "radio"
-      "Select #{question_text}"
-    when "select"
-      "Select #{question_text}"
-    when "date"
-      "Set #{question_text}"
-    else
-      "Enter #{question_text}"
-    end
+    return "Select #{question_text}" if %w[checkbox radio select].include?(type)
+    return "Set #{question_text}" if type == "date"
+
+    "Enter #{question_text}"
   end
 
   def suffix_label(log)
diff --git a/app/views/locations/show.html.erb b/app/views/locations/show.html.erb
index beb8f1540..cc33061ea 100644
--- a/app/views/locations/show.html.erb
+++ b/app/views/locations/show.html.erb
@@ -34,11 +34,11 @@
               <%= row.with_value { details_html(attr, @location) } %>
             <% end %>
             <% if LocationPolicy.new(current_user, @location).update? %>
-              <% row.with_action(text: "Change", href: scheme_location_postcode_path(@scheme, @location, referrer: "details")) if attr[:attribute] == "postcode" && current_user.support? && attr[:value].present? %>
-              <% row.with_action(text: "Change", href: scheme_location_name_path(@scheme, @location, referrer: "details")) if attr[:attribute] == "name" && attr[:value].present? %>
-              <% row.with_action(text: "Change", href: scheme_location_units_path(@scheme, @location, referrer: "details")) if attr[:attribute] == "units" && current_user.support? && attr[:value].present? %>
-              <% row.with_action(text: "Change", href: scheme_location_type_of_unit_path(@scheme, @location, referrer: "details")) if attr[:attribute] == "type_of_unit" && current_user.support? && attr[:value].present? %>
-              <% row.with_action(text: "Change", href: scheme_location_mobility_standards_path(@scheme, @location, referrer: "details")) if attr[:attribute] == "mobility_standards" && current_user.support? && attr[:value].present? %>
+              <% %w[postcode name units type_of_unit mobility_standards].each do |attribute| %>
+                <% if attr[:attribute] == attribute %>
+                  <%= row.with_action(text: "Change", href: location_action_link(attr, @scheme, @location, current_user)) %>
+                <% end %>
+              <% end %>
             <% end %>
           <% end %>
         <% end %>