From 3d7d7590299254e96470d83312c9ca194238c76b Mon Sep 17 00:00:00 2001 From: Manny Dinssa <44172848+Dinssa@users.noreply.github.com> Date: Mon, 12 Aug 2024 13:23:05 +0100 Subject: [PATCH] Refactor details view and add tests --- .../merge_requests/_details_list.html.erb | 27 +++ .../merge_requests/_summary_card.html.erb | 8 + app/views/merge_requests/details.html.erb | 174 ++++-------------- spec/factories/merge_requests.rb | 3 +- .../merge_requests/details.html.erb_spec.rb | 105 +++++++++++ 5 files changed, 182 insertions(+), 135 deletions(-) create mode 100644 app/views/merge_requests/_details_list.html.erb create mode 100644 app/views/merge_requests/_summary_card.html.erb create mode 100644 spec/views/merge_requests/details.html.erb_spec.rb diff --git a/app/views/merge_requests/_details_list.html.erb b/app/views/merge_requests/_details_list.html.erb new file mode 100644 index 000000000..d67bc8336 --- /dev/null +++ b/app/views/merge_requests/_details_list.html.erb @@ -0,0 +1,27 @@ +<%= govuk_summary_list do |summary_list| %> + <% details.each do |detail| %> + <% summary_list.with_row do |row| %> + <% row.with_key { detail[:label]} %> + + <% row.with_value do %> +
+ <% if detail[:value].html_safe? %> + <%= raw(detail[:value]) %> + <% elsif detail[:value].is_a?(Date) || detail[:value].is_a?(Time) %> + <%= detail[:value].strftime("%d %B %Y") %> + <% else %> + <%= simple_format(detail[:value]) %> + <% end %> +
+ <% end %> + + <% if detail[:action].present? %> + <% row.with_action( + text: detail[:action][:text], + href: detail[:action][:href], + visually_hidden_text: detail[:action][:visually_hidden_text], + ) %> + <% end %> + <% end %> + <% end %> +<% end %> diff --git a/app/views/merge_requests/_summary_card.html.erb b/app/views/merge_requests/_summary_card.html.erb new file mode 100644 index 000000000..b6d5ae0bc --- /dev/null +++ b/app/views/merge_requests/_summary_card.html.erb @@ -0,0 +1,8 @@ +
+
+

<%= title %>

+
+
+ <%= render partial: "merge_requests/details_list", locals: { details: details } %> +
+
diff --git a/app/views/merge_requests/details.html.erb b/app/views/merge_requests/details.html.erb index 45d1cc6b9..6ee98f919 100644 --- a/app/views/merge_requests/details.html.erb +++ b/app/views/merge_requests/details.html.erb @@ -1,158 +1,64 @@ <% content_for :before_content do %> <% title = "Merge details: #{@merge_request.absorbing_organisation_name}" %> <% content_for :title, title %> - <%= govuk_back_link href: "#{organisations_path}#merge-requests" %> <% end %> -<% unless @merge_request.signed_dsa %> -
-
-

- Important -

-
-
- The absorbing organisation must accept the Data Sharing Agreement before merging. -

Contact the Data Protection Officer: +<% unless @merge_request.signed_dsa || @merge_request.absorbing_organisation_id.blank? %> +

+
+

+ Important +

+
+
+ The absorbing organisation must accept the Data Sharing Agreement before merging. +
<% if @merge_request.dpo_user %> - <%= link_to @merge_request.dpo_user.name, user_path(@merge_request.dpo_user.id) %> + Contact the Data Protection Officer: <%= link_to @merge_request.dpo_user.name, user_path(@merge_request.dpo_user.id) %> <% else %> - None belong to the organisation + <%= @merge_request.absorbing_organisation_name %> does not have a Data Protection Officer. You can assign one on the <%= link_to 'users page', "#{organisation_path(@merge_request.absorbing_organisation_id)}/users" %>. <% end %> -

+
-
<% end %>

Merge details <%= @merge_request.absorbing_organisation_name %>

+<% unless @merge_request.status == "request_merged"%>
-
+<% end %> - - - - - - - - - - - - - - - <% ticket_base_url = "https://dluhcdigital.atlassian.net/browse/" %> - - - - - - - - - -
Request details
Requester<%= display_value_or_placeholder(@merge_request.requesting_user&.name) %>
Helpdesk ticket - <% if @merge_request.helpdesk_ticket.blank? %> - <%= display_value_or_placeholder(nil) %> - <% else %> - - <%= @merge_request.helpdesk_ticket %> (opens in a new tab) - - <% end %> - Change
Status<%= status_tag(@merge_request.status) %>
+<% request_details = [ + { label: "Requester", value: display_value_or_placeholder(@merge_request.requesting_user&.name) }, + { label: "Helpdesk ticket", value: @merge_request.helpdesk_ticket.present? ? link_to(@merge_request.helpdesk_ticket, "https://dluhcdigital.atlassian.net/browse/#{@merge_request.helpdesk_ticket}", target: "_blank", rel: "noopener noreferrer") : display_value_or_placeholder(nil), action: @merge_request.status == "request_merged" ? nil : { text: "Change", href: "#", visually_hidden_text: "helpdesk ticket" } }, + { label: "Status", value: status_tag(@merge_request.status) }, +] %> - - - - - - - - - - - - - - - - - - - - - - - -
Merge details
Absorbing organisation<%= display_value_or_placeholder(@merge_request.absorbing_organisation_name) %>Change
Merging organisations - <% if @merge_request.other_merging_organisations.blank? %> - <%= display_value_or_placeholder(nil) %> - <% else %> - <% @merge_request.other_merging_organisations.split(',').each do |organisation| %> -

<%= organisation.strip %>

- <% end %> - <% end %> - 		Change
Merge date<%= display_value_or_placeholder(@merge_request.merge_date) %>Change
+<%= render partial: "merge_requests/summary_card", locals: { title: "Request details", details: request_details } %> - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Merge outcomes
Total users after merge<%= display_value_or_placeholder(@merge_request.total_users) %>View
Total schemes after merge<%= display_value_or_placeholder(@merge_request.total_schemes) %>View
Total logs after merge - <% if @merge_request.total_lettings_logs.blank? && @merge_request.total_sales_logs.blank? %> - <%= display_value_or_placeholder(nil) %> - <% else %> - <% unless @merge_request.total_lettings_logs.blank? %> - <%= @merge_request.total_lettings_logs %> lettings logs - <% end %> -
- <% unless @merge_request.total_sales_logs.blank? %> - <%= @merge_request.total_sales_logs %> sales logs - <% end %> - <% end %> - 		View
Total stock owners & managing agents after merge - <% if @merge_request.total_stock_owners.blank? && @merge_request.total_managing_agents.blank? %> - <%= display_value_or_placeholder(nil) %> - <% else %> - <% unless @merge_request.total_stock_owners.blank? %> - <%= @merge_request.total_stock_owners %> stock owners - <% end %> -
- <% unless @merge_request.total_managing_agents.blank? %> - <%= @merge_request.total_managing_agents %> managing agents - <% end %> - <% end %> - 		View
+<% merge_details = [ + { label: "Absorbing organisation", value: display_value_or_placeholder(@merge_request.absorbing_organisation_name), action: @merge_request.status == "request_merged" ? nil : { text: "Change", href: "#", visually_hidden_text: "absorbing organisation" } }, + { label: "Merging organisations", value: @merge_request.other_merging_organisations.present? ? @merge_request.other_merging_organisations.split(',').map(&:strip).join('
').html_safe : display_value_or_placeholder(nil), action: @merge_request.status == "request_merged" ? nil : { text: "Change", href: "#", visually_hidden_text: "merging organisations" } }, + { label: "Merge date", value: display_value_or_placeholder(@merge_request.merge_date), action: @merge_request.status == "request_merged" ? nil : { text: "Change", href: "#", visually_hidden_text: "merge date" } }, +] %> + +<%= render partial: "merge_requests/summary_card", locals: { title: "Merge details", details: merge_details } %> + +<% unless @merge_request.status == "incomplete" %> + <% merge_outcomes = [ + { label: "Total users after merge", value: display_value_or_placeholder(@merge_request.total_users), action: { text: "View", href: "#", visually_hidden_text: "total users after merge" } }, + { label: "Total schemes after merge", value: display_value_or_placeholder(@merge_request.total_schemes), action: { text: "View", href: "#", visually_hidden_text: "total schemes after merge" } }, + { label: "Total logs after merge", value: @merge_request.total_lettings_logs.present? || @merge_request.total_sales_logs.present? ? "#{@merge_request.total_lettings_logs} lettings logs
#{@merge_request.total_sales_logs} sales logs".html_safe : display_value_or_placeholder(nil), action: { text: "View", href: "#", visually_hidden_text: "total logs after merge" } }, + { label: "Total stock owners & managing agents after merge", value: @merge_request.total_stock_owners.present? || @merge_request.total_managing_agents.present? ? "#{@merge_request.total_stock_owners} stock owners
#{@merge_request.total_managing_agents} managing agents".html_safe : display_value_or_placeholder(nil), action: { text: "View", href: "#", visually_hidden_text: "total stock owners & managing agents after merge" } } + ] %> + + <%= render partial: "merge_requests/summary_card", locals: { title: "Merge outcomes", details: merge_outcomes } %> +<% end %> diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb index 888639b82..d46370e2a 100644 --- a/spec/factories/merge_requests.rb +++ b/spec/factories/merge_requests.rb @@ -1,7 +1,8 @@ FactoryBot.define do factory :merge_request do - status { "incomplete" } + status { "Incomplete" } merge_date { nil } + helpdesk_ticket { "MSD-99999" } association :requesting_organisation, factory: :organisation end end diff --git a/spec/views/merge_requests/details.html.erb_spec.rb b/spec/views/merge_requests/details.html.erb_spec.rb new file mode 100644 index 000000000..dea19a39b --- /dev/null +++ b/spec/views/merge_requests/details.html.erb_spec.rb @@ -0,0 +1,105 @@ +require "rails_helper" + +RSpec.describe "merge_requests/details.html.erb", type: :view do + let(:absorbing_organisation) { create(:organisation, name: "Absorbing Org") } + let(:dpo_user) { create(:user, name: "DPO User", is_dpo: true, organisation: absorbing_organisation) } + let(:merge_request) { create(:merge_request, absorbing_organisation_id: absorbing_organisation.id, signed_dsa: false, status: 1) } + + before do + assign(:merge_request, merge_request) + render + end + + it "displays the correct title" do + expect(rendered).to have_selector("h1.govuk-heading-l") do |h1| + expect(h1).to have_selector("span.govuk-caption-l", text: "Merge details") + expect(h1).to have_content("Absorbing Org") + end + end + + it "displays the notification banner when DSA is not signed" do + expect(rendered).to have_selector(".govuk-notification-banner") + expect(rendered).to have_content("The absorbing organisation must accept the Data Sharing Agreement before merging.") + end + + it "displays the requester details" do + expect(rendered).to have_selector("dt", text: "Requester") + expect(rendered).to have_selector("dd", text: merge_request.requesting_user&.name || "You didn't answer this question") + end + + it "displays the helpdesk ticket details" do + expect(rendered).to have_selector("dt", text: "Helpdesk ticket") + if merge_request.helpdesk_ticket.present? + expect(rendered).to have_link(merge_request.helpdesk_ticket, href: "https://dluhcdigital.atlassian.net/browse/#{merge_request.helpdesk_ticket}") + else + expect(rendered).to have_selector("dd", text: "You didn't answer this question") + end + end + + it "displays the status details" do + expect(rendered).to have_selector("dt", text: "Status") + expect(rendered).to have_selector("dd", text: "Incomplete") + end + + it "displays the absorbing organisation details" do + expect(rendered).to have_selector("dt", text: "Absorbing organisation") + expect(rendered).to have_selector("dd", text: merge_request.absorbing_organisation_name) + end + + it "displays the merging organisations details" do + expect(rendered).to have_selector("dt", text: "Merging organisations") + if merge_request.other_merging_organisations.present? + merge_request.other_merging_organisations.split(",").each do |organisation| + expect(rendered).to have_selector("dd", text: organisation.strip) + end + end + end + + it "displays the merge date details" do + expect(rendered).to have_selector("dt", text: "Merge date") + expect(rendered).to have_selector("dd", text: merge_request.merge_date || "You didn't answer this question") + end + + context "when the merge request is complete" do + + before do + merge_request.update!(status: 4, signed_dsa: true, total_users: 10, total_schemes: 5, total_lettings_logs: 20, total_sales_logs: 30, total_stock_owners: 40, total_managing_agents: 50) + assign(:merge_request, merge_request) + render + end + + it "has status of 'Merged'" do + expect(rendered).to have_selector("dd", text: "Merged") + end + + it "displays the total users after merge details" do + expect(rendered).to have_selector("dt", text: "Total users after merge") + expect(rendered).to have_selector("dd", text: merge_request.total_users) + end + + it "displays the total schemes after merge details" do + expect(rendered).to have_selector("dt", text: "Total schemes after merge") + expect(rendered).to have_selector("dd", text: merge_request.total_schemes) + end + + it "displays the total logs after merge details" do + expect(rendered).to have_selector("dt", text: "Total logs after merge") + if merge_request.total_lettings_logs.present? || merge_request.total_sales_logs.present? + combined_text = [] + combined_text << "#{merge_request.total_lettings_logs} lettings logs" if merge_request.total_lettings_logs.present? + combined_text << "#{merge_request.total_sales_logs} sales logs" if merge_request.total_sales_logs.present? + expect(rendered).to have_selector("dd", text: combined_text.join("")) + end + end + + it "displays the total stock owners & managing agents after merge details" do + expect(rendered).to have_selector("dt", text: "Total stock owners & managing agents after merge") + if merge_request.total_stock_owners.present? || merge_request.total_managing_agents.present? + combined_text = [] + combined_text << "#{merge_request.total_stock_owners} stock owners" if merge_request.total_stock_owners.present? + combined_text << "#{merge_request.total_managing_agents} managing agents" if merge_request.total_managing_agents.present? + expect(rendered).to have_selector("dd", text: combined_text.join("")) + end + end + end +end