From 5809fe5cd66d9fb940bf204d931ddcdd3615598d Mon Sep 17 00:00:00 2001
From: Manny Dinssa <44172848+Dinssa@users.noreply.github.com>
Date: Wed, 27 Nov 2024 16:43:22 +0000
Subject: [PATCH] Replace you didnt answer with link
---
...eck_answers_summary_list_card_component.rb | 2 +-
app/helpers/check_answers_helper.rb | 7 +++-
app/helpers/locations_helper.rb | 15 +++++++--
app/helpers/merge_requests_helper.rb | 33 ++++++++++++++-----
app/helpers/organisations_helper.rb | 12 +++++++
app/helpers/schemes_helper.rb | 30 +++++++++++++++++
app/helpers/user_helper.rb | 20 +++++++++++
app/models/form/question.rb | 24 ++++++++++++--
app/views/locations/check_answers.html.erb | 6 ++--
app/views/locations/show.html.erb | 14 ++++----
app/views/organisations/show.html.erb | 5 +--
.../schemes/_scheme_summary_list_row.html.erb | 6 ++--
app/views/users/show.html.erb | 12 +++++--
13 files changed, 153 insertions(+), 33 deletions(-)
diff --git a/app/components/check_answers_summary_list_card_component.rb b/app/components/check_answers_summary_list_card_component.rb
index 5242c7f41..7fb7f10e4 100644
--- a/app/components/check_answers_summary_list_card_component.rb
+++ b/app/components/check_answers_summary_list_card_component.rb
@@ -50,7 +50,7 @@ private
if log.creation_method_bulk_upload? && log.bulk_upload.present? && !log.optional_fields.include?(question.id)
"You still need to answer this question".html_safe
else
- "You didn’t answer this question".html_safe
+ govuk_link_to question.summary_error_message, correct_validation_action_href(question, log, nil, @correcting_hard_validation), class: "govuk-link govuk-link--no-visited-state"
end
end
diff --git a/app/helpers/check_answers_helper.rb b/app/helpers/check_answers_helper.rb
index d69414670..b1c3c0b02 100644
--- a/app/helpers/check_answers_helper.rb
+++ b/app/helpers/check_answers_helper.rb
@@ -55,11 +55,16 @@ private
[question.question_number_string, question.check_answer_label.to_s.presence || question.header.to_s].compact.join(" - ")
end
+ def unanswered_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 unanswered_value(log:, question:)
if log.creation_method_bulk_upload? && log.bulk_upload.present? && !log.optional_fields.include?(question.id)
"You still need to answer this question".html_safe
else
- "You didn’t answer this question".html_safe
+ govuk_link_to question.summary_error_message, unanswered_action_href(question, log), class: "govuk-link govuk-link--no-visited-state"
end
end
end
diff --git a/app/helpers/locations_helper.rb b/app/helpers/locations_helper.rb
index fc1008926..b27179aea 100644
--- a/app/helpers/locations_helper.rb
+++ b/app/helpers/locations_helper.rb
@@ -64,8 +64,8 @@ module LocationsHelper
send("scheme_location_#{attribute}_path", location.scheme, location, referrer: "check_answers", route: params[:route])
end
- def action_text_helper(attr, location)
- attr[:value].blank? || (attr[:attribute] == "availability" && location.startdate.blank?) ? "Answer" : "Change"
+ def location_action_text_helper(attr, location)
+ attr[:value].blank? || (attr[:attribute] == "availability" && location.startdate.blank?) ? "" : "Change"
end
def toggle_location_link(location)
@@ -95,6 +95,17 @@ module LocationsHelper
end
end
+ 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}"
+ end
+
private
ActivePeriod = Struct.new(:from, :to)
diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb
index 6283ef42e..c688aed3e 100644
--- a/app/helpers/merge_requests_helper.rb
+++ b/app/helpers/merge_requests_helper.rb
@@ -2,24 +2,40 @@ module MergeRequestsHelper
include GovukLinkHelper
include GovukVisuallyHiddenHelper
- def display_value_or_placeholder(value, placeholder = "You didn't answer this question")
+ def display_value_or_placeholder(value, placeholder = "No answer provided")
value.presence || content_tag(:span, placeholder, class: "app-!-colour-muted")
end
+ def enter_value_link(page, merge_request)
+ govuk_link_to(merge_request_details_link_message(page), send("#{page}_merge_request_path", merge_request, referrer: "check_answers"), class: "govuk-link govuk-link--no-visited-state")
+ end
+
+ def merge_request_details_link_message(page)
+ messages = {
+ "existing_absorbing_organisation" => "Answer absorbing organisation is already active",
+ "helpdesk_ticket" => "Enter helpdesk ticket number",
+ }
+ messages[page] || "Enter #{lowercase_first_letter(page.humanize)}"
+ end
+
+ def merge_request_action_text_helper(attribute, merge_request)
+ merge_request.send(attribute).blank? ? "" : "Change"
+ end
+
def request_details(merge_request)
[
{ label: "Requester", value: display_value_or_placeholder(merge_request.requester&.name) },
- { label: "Helpdesk ticket", value: merge_request.helpdesk_ticket.present? ? link_to("#{merge_request.helpdesk_ticket} (opens in a new tab)", "https://mhclgdigital.atlassian.net/browse/#{merge_request.helpdesk_ticket}", target: "_blank", rel: "noopener noreferrer") : display_value_or_placeholder(nil), action: merge_request_action(merge_request, "helpdesk_ticket") },
+ { label: "Helpdesk ticket", value: merge_request.helpdesk_ticket.present? ? link_to("#{merge_request.helpdesk_ticket} (opens in a new tab)", "https://mhclgdigital.atlassian.net/browse/#{merge_request.helpdesk_ticket}", target: "_blank", rel: "noopener noreferrer") : display_value_or_placeholder(nil, enter_value_link("helpdesk_ticket", merge_request)), action: merge_request_action(merge_request, "helpdesk_ticket") },
{ label: "Status", value: status_tag(merge_request.status) },
]
end
def merge_details(merge_request)
[
- { label: "Absorbing organisation", value: display_value_or_placeholder(merge_request.absorbing_organisation_name), action: merge_request_action(merge_request, "absorbing_organisation") },
- { label: "Merging organisations", value: merge_request.merge_request_organisations.any? ? merge_request.merge_request_organisations.map(&:merging_organisation_name).join("
").html_safe : display_value_or_placeholder(nil), action: merge_request_action(merge_request, "merging_organisations") },
- { label: "Merge date", value: display_value_or_placeholder(merge_request.merge_date), action: merge_request_action(merge_request, "merge_date") },
- { label: "Absorbing organisation already active?", value: display_value_or_placeholder(merge_request.existing_absorbing_organisation_label), action: merge_request_action(merge_request, "existing_absorbing_organisation") },
+ { label: "Absorbing organisation", value: display_value_or_placeholder(merge_request.absorbing_organisation_name, enter_value_link("absorbing_organisation", merge_request)), action: merge_request_action(merge_request, "absorbing_organisation") },
+ { label: "Merging organisations", value: merge_request.merge_request_organisations.any? ? merge_request.merge_request_organisations.map(&:merging_organisation_name).join("
").html_safe : display_value_or_placeholder(nil, enter_value_link("merging_organisations", merge_request)), action: merge_request_action(merge_request, "merging_organisations") },
+ { label: "Merge date", value: display_value_or_placeholder(merge_request.merge_date, enter_value_link("merge_date", merge_request)), action: merge_request_action(merge_request, "merge_date") },
+ { label: "Absorbing organisation already active?", value: display_value_or_placeholder(merge_request.existing_absorbing_organisation_label, enter_value_link("existing_absorbing_organisation", merge_request)), action: merge_request_action(merge_request, "existing_absorbing_organisation") },
]
end
@@ -75,9 +91,10 @@ module MergeRequestsHelper
end
end
- def merge_request_action(merge_request, page)
+ def merge_request_action(merge_request, page, attribute = nil)
+ attribute = page if attribute.nil?
unless merge_request.status == "request_merged" || merge_request.status == "processing"
- { text: "Change", href: send("#{page}_merge_request_path", merge_request, referrer: "check_answers"), visually_hidden_text: page.humanize }
+ { text: merge_request_action_text_helper(attribute, merge_request), href: send("#{page}_merge_request_path", merge_request, referrer: "check_answers"), visually_hidden_text: page.humanize }
end
end
diff --git a/app/helpers/organisations_helper.rb b/app/helpers/organisations_helper.rb
index 75d5869fa..0b1c84f51 100644
--- a/app/helpers/organisations_helper.rb
+++ b/app/helpers/organisations_helper.rb
@@ -54,4 +54,16 @@ module OrganisationsHelper
def delete_organisation_link(organisation)
govuk_button_link_to "Delete this organisation", delete_confirmation_organisation_path(organisation), warning: true
end
+
+ def organisation_action_text_helper(attr, organisation)
+ 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}"
+ end
end
diff --git a/app/helpers/schemes_helper.rb b/app/helpers/schemes_helper.rb
index bcd40b082..23bc73816 100644
--- a/app/helpers/schemes_helper.rb
+++ b/app/helpers/schemes_helper.rb
@@ -76,6 +76,11 @@ module SchemesHelper
end
end
+ def change_link_text(question_id, scheme)
+ attribute_value = scheme.public_send(question_id)
+ attribute_value.nil? ? "" : "Change"
+ end
+
def scheme_status_hint(scheme)
case scheme.status
when :deactivating_soon
@@ -93,6 +98,31 @@ module SchemesHelper
organisation.owned_schemes.duplicate_sets.any? || organisation.owned_schemes.any? { |scheme| scheme.locations.duplicate_sets.any? }
end
+ def scheme_edit_path(scheme, attribute)
+ case attribute[:id]
+ when "primary_client_group"
+ scheme_primary_client_group_path(scheme, referrer: "check-answers")
+ when "has_other_client_group"
+ scheme_confirm_secondary_client_group_path(scheme, referrer: "check-answers")
+ when "secondary_client_group"
+ scheme_secondary_client_group_path(scheme, referrer: "check-answers")
+ when "support_type", "intended_stay"
+ scheme_support_path(scheme, referrer: "check-answers")
+ end
+ end
+
+ 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}"
+ end
+
private
ActivePeriod = Struct.new(:from, :to)
diff --git a/app/helpers/user_helper.rb b/app/helpers/user_helper.rb
index bbcb0acae..8e8e7fce3 100644
--- a/app/helpers/user_helper.rb
+++ b/app/helpers/user_helper.rb
@@ -75,4 +75,24 @@ module UserHelper
text
end
+
+ def user_details_html(user, current_user, attribute)
+ value = user.send(attribute)
+ return value.humanize if value.present?
+
+ case attribute
+ when "role"
+ current_user.data_coordinator? || current_user.support? ? govuk_link_to("Select role", aliased_user_edit(user, current_user), class: "govuk-link govuk-link--no-visited-state") : "No role assigned"
+ when "phone"
+ govuk_link_to("Enter telephone number", aliased_user_edit(user, current_user), class: "govuk-link govuk-link--no-visited-state")
+ else
+ "No answer provided"
+ end
+ end
+
+ def user_action_text_helper(user, attribute)
+ return "Change" if %w[role phone].include?(attribute) && user.send(attribute).present?
+
+ ""
+ end
end
diff --git a/app/models/form/question.rb b/app/models/form/question.rb
index 05eec89dd..e2ab1ec30 100644
--- a/app/models/form/question.rb
+++ b/app/models/form/question.rb
@@ -130,9 +130,11 @@ class Form::Question
end
def action_text(log, correcting_hard_validation: false)
- return "Answer" unless displayed_as_answered?(log)
-
- correcting_hard_validation ? "Clear" : "Change"
+ if displayed_as_answered?(log)
+ correcting_hard_validation ? "Clear" : "Change"
+ else
+ ""
+ end
end
def displayed_as_answered?(log)
@@ -239,6 +241,22 @@ class Form::Question
I18n.t("validations.not_answered", question: question_text.downcase)
end
+ def summary_error_message
+ question_text = lowercase_first_letter(error_label.presence || check_answer_label.presence || header.presence || id.humanize) || "this question."
+ case type
+ when "checkbox"
+ "Answer #{question_text}"
+ when "radio"
+ "Select #{question_text}"
+ when "select"
+ "Select #{question_text}"
+ when "date"
+ "Set #{question_text}"
+ else
+ "Enter #{question_text}"
+ end
+ end
+
def suffix_label(log)
return "" unless suffix
return suffix if suffix.is_a?(String)
diff --git a/app/views/locations/check_answers.html.erb b/app/views/locations/check_answers.html.erb
index 8cd8bde2a..541c8513b 100644
--- a/app/views/locations/check_answers.html.erb
+++ b/app/views/locations/check_answers.html.erb
@@ -23,14 +23,14 @@
<% row.with_key { attr[:name] } %>
<% if attr[:attribute].eql?("postcode") && @location.is_la_inferred %>
<% row.with_value do %>
- <%= details_html(attr) %>
+ <%= details_html(attr, @location)%>
<%= formatted_local_authority_timeline(@location) %>
<% end %>
<% else %>
- <% row.with_value { details_html(attr) } %>
+ <% row.with_value { details_html(attr, @location) if user_can_edit_scheme?(current_user, @scheme) } %>
<% end %>
<% if LocationPolicy.new(current_user, @location).update? %>
- <% row.with_action(text: action_text_helper(attr, @location), href: location_edit_path(@location, attr[:attribute])) %>
+ <% row.with_action(text: location_action_text_helper(attr, @location), href: location_edit_path(@location, attr[:attribute])) %>
<% end %>
<% end %>
<% end %>
diff --git a/app/views/locations/show.html.erb b/app/views/locations/show.html.erb
index 8ac8f6b23..b0bbc084d 100644
--- a/app/views/locations/show.html.erb
+++ b/app/views/locations/show.html.erb
@@ -27,18 +27,18 @@
<% end %>
<% elsif attr[:attribute].eql?("postcode") && @location.is_la_inferred %>
<% row.with_value do %>
- <%= details_html(attr) %>
+ <%= details_html(attr, @location) %>
<%= formatted_local_authority_timeline(@location) %>
<% end %>
<% else %>
- <%= row.with_value { details_html(attr) } %>
+ <%= 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? %>
- <% row.with_action(text: "Change", href: scheme_location_name_path(@scheme, @location, referrer: "details")) if attr[:attribute] == "name" %>
- <% row.with_action(text: "Change", href: scheme_location_units_path(@scheme, @location, referrer: "details")) if attr[:attribute] == "units" && current_user.support? %>
- <% 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? %>
- <% row.with_action(text: "Change", href: scheme_location_mobility_standards_path(@scheme, @location, referrer: "details")) if attr[:attribute] == "mobility_standards" && current_user.support? %>
+ <% 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? %>
<% end %>
<% end %>
<% end %>
diff --git a/app/views/organisations/show.html.erb b/app/views/organisations/show.html.erb
index 214b988f0..daad69dfd 100644
--- a/app/views/organisations/show.html.erb
+++ b/app/views/organisations/show.html.erb
@@ -22,8 +22,9 @@
<% if can_edit_org?(current_user) && attr[:editable] %>
<%= summary_list.with_row do |row| %>
<% row.with_key { attr[:name] } %>
- <% row.with_value { details_html(attr) } %>
+ <% row.with_value { details_html(attr, @organisation) } %>
<% row.with_action(
+ text: organisation_action_text_helper(attr, @organisation),
visually_hidden_text: attr[:name].to_s.humanize.downcase,
href: edit_organisation_path(@organisation),
html_attributes: { "data-qa": "change-#{attr[:name].downcase}" },
@@ -32,7 +33,7 @@
<% else %>
<%= summary_list.with_row do |row| %>
<% row.with_key { attr[:name] } %>
- <% row.with_value { details_html(attr) } %>
+ <% row.with_value { details_html(attr, @organisation) } %>
<% row.with_action %>
<% end %>
<% end %>
diff --git a/app/views/schemes/_scheme_summary_list_row.html.erb b/app/views/schemes/_scheme_summary_list_row.html.erb
index 6655c17be..822c70c08 100644
--- a/app/views/schemes/_scheme_summary_list_row.html.erb
+++ b/app/views/schemes/_scheme_summary_list_row.html.erb
@@ -7,17 +7,17 @@
Error: <%= scheme.errors[attribute[:id].to_sym][0] %>
- <%= details_html(attribute) %>
+ <%= details_html(attribute, @scheme) %>
<% else %>
- <%= details_html(attribute) %>
+ <%= details_html(attribute, @scheme) %>
<% end %>
<% if can_change_scheme_answer?(attribute[:name], scheme) && attribute[:edit] %>
- Change
+ <%= change_link_text(attribute[:id], @scheme) %>
<% end %>
diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb
index a43b03f82..df6907423 100644
--- a/app/views/users/show.html.erb
+++ b/app/views/users/show.html.erb
@@ -52,9 +52,14 @@
<%= summary_list.with_row do |row|
row.with_key { "Telephone number" }
- row.with_value { @user.phone_with_extension }
+ row.with_value { user_details_html(@user, current_user, "phone") }
if UserPolicy.new(current_user, @user).edit_telephone_numbers?
- row.with_action(visually_hidden_text: "telephone number", href: aliased_user_edit(@user, current_user), html_attributes: { "data-qa": "change-telephone-number" })
+ row.with_action(
+ text: user_action_text_helper(@user, "phone"),
+ visually_hidden_text: "telephone number",
+ href: aliased_user_edit(@user, current_user),
+ html_attributes: { "data-qa": "change-telephone-number" },
+ )
else
row.with_action
end
@@ -90,9 +95,10 @@
<%= summary_list.with_row do |row|
row.with_key { "Role" }
- row.with_value { @user.role&.humanize }
+ row.with_value { user_details_html(@user, current_user, "role") }
if UserPolicy.new(current_user, @user).edit_roles?
row.with_action(
+ text: user_action_text_helper(@user, "role"),
visually_hidden_text: "role",
href: aliased_user_edit(@user, current_user),
html_attributes: { "data-qa": "change-role" },