Refactor details view and add tests

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 %>
+        <div class="govuk-!-margin-left-8 govuk-!-margin-right-4">
+          <% 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 %>
+        </div>
+      <% 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 %>

app/views/merge_requests/_summary_card.html.erb

@@ -0,0 +1,8 @@
+<div class="govuk-summary-card govuk-!-margin-bottom-6 govuk-!-width-three-quarters">
+  <div class="govuk-summary-card__title-wrapper">
+    <h3 class="govuk-summary-card__title govuk-!-font-weight-regular"><%= title %></h3>
+  </div>
+  <div class="govuk-summary-card__content">
+    <%= render partial: "merge_requests/details_list", locals: { details: details } %>
+  </div>
+</div>

app/views/merge_requests/details.html.erb

@@ -1,10 +1,9 @@
 <% 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 %>
+<% unless @merge_request.signed_dsa || @merge_request.absorbing_organisation_id.blank? %>
   <div class="govuk-notification-banner" role="region" aria-labelledby="govuk-notification-banner-title" data-module="govuk-notification-banner">
     <div class="govuk-notification-banner__header">
       <h2 class="govuk-notification-banner__title" id="govuk-notification-banner-title">
@@ -13,13 +12,12 @@
     </div>
     <div class="govuk-notification-banner__content">
       <strong>The absorbing organisation must accept the Data Sharing Agreement before merging.</strong>
-    <p>Contact the Data Protection Officer:
+      <br>
       <% 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 %>
-    </p>
     </div>
   </div>
 <% end %>
@@ -27,132 +25,40 @@
   <span class="govuk-caption-l">Merge details</span>
   <%= @merge_request.absorbing_organisation_name %>
 </h1>
+<% unless @merge_request.status == "request_merged"%>
 <div class="govuk-button-group">
   <button type="submit" <%= @merge_request.status == "ready_to_merge" ? "" : "disabled aria-disabled=\"true\"" %> class="govuk-button" data-module="govuk-button">
     Begin merge
   </button>
-  <button type="submit" <%= @merge_request.status != "request_merged" ? "" : "disabled aria-disabled=\"true\"" %> class="govuk-button govuk-button--warning" data-module="govuk-button">
+  <button type="submit" class="govuk-button govuk-button--warning" data-module="govuk-button">
     Delete merge request
   </button>
 </div>
-
-<table class="govuk-table govuk-!-width-three-quarters" style="border: 1px solid #b1b4b6;">
-  <thead class="govuk-table__head">
-  <tr class="govuk-table__row">
-    <th scope="col" colspan="3" class="govuk-table__header govuk-!-font-weight-regular govuk-!-padding-3" style="background-color:#f3f2f1">Request details</th>
-  </tr>
-  </thead>
-  <tbody class="govuk-table__body app-table-group">
-  <tr class="govuk-table__row">
-    <th scope="row" class="govuk-table__header govuk-!-padding-2 govuk-!-padding-right-9 govuk-!-padding-top-4">Requester</th>
-    <td class="govuk-table__cell govuk-!-width-one-half govuk-!-padding-2 govuk-!-padding-right-9 govuk-!-padding-top-4"><%= display_value_or_placeholder(@merge_request.requesting_user&.name) %></td>
-    <td class="govuk-table__cell govuk-!-width-one-quarter-width govuk-!-padding-2 govuk-!-padding-top-4 govuk-!-text-align-right" style="width: 10%;"></td>
-  </tr>
-  <tr class="govuk-table__row">
-    <th scope="row" class="govuk-table__header govuk-!-padding-2 govuk-!-padding-right-9">Helpdesk ticket</th>
-    <% ticket_base_url = "https://dluhcdigital.atlassian.net/browse/" %>
-    <td class="govuk-table__cell govuk-!-padding-2 govuk-!-padding-right-9">
-      <% if @merge_request.helpdesk_ticket.blank? %>
-        <%= display_value_or_placeholder(nil) %>
-    <% else %>
-        <a href="<%= ticket_base_url + @merge_request.helpdesk_ticket %>" target="_blank" rel="noopener noreferrer" title="<%= @merge_request.helpdesk_ticket %> (opens in a new tab)">
-          <%= @merge_request.helpdesk_ticket %> (opens in a new tab)
-        </a>
 <% end %>
-    </td>
-    <td class="govuk-table__cell govuk-!-padding-2 govuk-!-text-align-right" style="width: 10%;"><a href="#" aria-label="Change helpdesk ticket">Change</a></td>
-  </tr>
-  <tr class="govuk-table__row">
-    <th scope="row" class="govuk-table__header govuk-!-padding-2 govuk-!-padding-right-9 govuk-!-padding-bottom-5">Status</th>
-    <td class="govuk-table__cell govuk-!-padding-2 govuk-!-padding-right-9"><%= status_tag(@merge_request.status) %></td>
-    <td class="govuk-table__cell govuk-!-padding-2 govuk-!-text-align-right" style="width: 10%;"></td>
-  </tr>
-  </tbody>
-</table>
 
-<table class="govuk-table govuk-!-width-three-quarters" style="border: 1px solid #b1b4b6;">
+<% request_details = [
-  <thead class="govuk-table__head">
+  { label: "Requester", value: display_value_or_placeholder(@merge_request.requesting_user&.name) },
-  <tr class="govuk-table__row">
+  { 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" } },
-    <th scope="col" colspan="3" class="govuk-table__header govuk-!-font-weight-regular govuk-!-padding-3" style="background-color:#f3f2f1">Merge details</th>
+  { label: "Status", value: status_tag(@merge_request.status) },
-  </tr>
+] %>
-  </thead>
-  <tbody class="govuk-table__body app-table-group">
-  <tr class="govuk-table__row">
-    <th scope="row" class="govuk-table__header govuk-!-padding-2 govuk-!-padding-right-9 govuk-!-padding-top-4">Absorbing organisation</th>
-    <td class="govuk-table__cell govuk-!-width-one-half govuk-!-padding-2 govuk-!-padding-right-9 govuk-!-padding-top-4"><%= display_value_or_placeholder(@merge_request.absorbing_organisation_name) %></td>
-    <td class="govuk-table__cell govuk-!-width-one-quarter-width govuk-!-padding-2 govuk-!-padding-top-4 govuk-!-text-align-right" style="width: 10%;"><a href="#" aria-label="Change absorbing organisation">Change</a></td>
-  </tr>
-  <tr class="govuk-table__row">
-    <th scope="row" class="govuk-table__header govuk-!-padding-2 govuk-!-padding-right-9">Merging organisations</th>
-    <td class="govuk-table__cell govuk-!-padding-2 govuk-!-padding-right-9">
-      <% if @merge_request.other_merging_organisations.blank? %>
-        <%= display_value_or_placeholder(nil) %>
-      <% else %>
-        <% @merge_request.other_merging_organisations.split(',').each do |organisation| %>
-          <p class="govuk-!-padding-bottom-1 govuk-!-margin-0"><%= organisation.strip %></p>
-        <% end %>
-      <% end %>
-    </td>
-    <td class="govuk-table__cell govuk-!-padding-2 govuk-!-text-align-right" style="width: 10%;"><a href="#" aria-label="Change merging organisations">Change</a></td>
-  </tr>
-  <tr class="govuk-table__row">
-    <th scope="row" class="govuk-table__header govuk-!-padding-2 govuk-!-padding-right-9 govuk-!-padding-bottom-5">Merge date</th>
-    <td class="govuk-table__cell govuk-!-padding-2 govuk-!-padding-right-9"><%= display_value_or_placeholder(@merge_request.merge_date) %></td>
-    <td class="govuk-table__cell govuk-!-padding-2 govuk-!-text-align-right" style="width: 10%;"><a href="#" aria-label="Change merge date">Change</a></td>
-  </tr>
-  </tbody>
-</table>
 
-<table class="govuk-table govuk-!-width-three-quarters" style="border: 1px solid #b1b4b6;">
+<%= render partial: "merge_requests/summary_card", locals: { title: "Request details", details: request_details } %>
-  <thead class="govuk-table__head">
+
-  <tr class="govuk-table__row">
+<% merge_details = [
-    <th scope="col" colspan="3" class="govuk-table__header govuk-!-font-weight-regular govuk-!-padding-3" style="background-color:#f3f2f1">Merge outcomes</th>
+  { 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" } },
-  </tr>
+  { label: "Merging organisations", value: @merge_request.other_merging_organisations.present? ? @merge_request.other_merging_organisations.split(',').map(&:strip).join('<br>').html_safe : display_value_or_placeholder(nil), action: @merge_request.status == "request_merged" ? nil : { text: "Change", href: "#", visually_hidden_text: "merging organisations" } },
-  </thead>
+  { 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" } },
-  <tbody class="govuk-table__body app-table-group">
+] %>
-  <tr class="govuk-table__row">
+
-    <th scope="row" class="govuk-table__header govuk-!-padding-2 govuk-!-padding-right-9 govuk-!-padding-top-4">Total users after merge</th>
+<%= render partial: "merge_requests/summary_card", locals: { title: "Merge details", details: merge_details } %>
-    <td class="govuk-table__cell govuk-!-width-one-half govuk-!-padding-2 govuk-!-padding-right-9 govuk-!-padding-top-4"><%= display_value_or_placeholder(@merge_request.total_users) %></td>
+
-    <td class="govuk-table__cell govuk-!-width-one-quarter-width govuk-!-padding-2 govuk-!-padding-top-4 govuk-!-text-align-right" style="width: 10%;"><a href="#" aria-label="View all users after merge">View</a></td>
+<% unless @merge_request.status == "incomplete" %>
-  </tr>
+  <% merge_outcomes = [
-  <tr class="govuk-table__row">
+    { 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" } },
-    <th scope="row" class="govuk-table__header govuk-!-padding-2 govuk-!-padding-right-9">Total schemes after merge</th>
+    { 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" } },
-    <td class="govuk-table__cell govuk-!-padding-2 govuk-!-padding-right-9"><%= display_value_or_placeholder(@merge_request.total_schemes) %></td>
+    { 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<br>#{@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" } },
-    <td class="govuk-table__cell govuk-!-padding-2 govuk-!-text-align-right" style="width: 10%;"><a href="#" aria-label="View all schemes after merge">View</a></td>
+    { 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<br>#{@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" } }
-  </tr>
+  ] %>
-  <tr class="govuk-table__row">
+
-    <th scope="row" class="govuk-table__header govuk-!-padding-2 govuk-!-padding-right-9">Total logs after merge</th>
+  <%= render partial: "merge_requests/summary_card", locals: { title: "Merge outcomes", details: merge_outcomes } %>
-    <td class="govuk-table__cell govuk-!-padding-2 govuk-!-padding-right-9">
-      <% 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 %>
-          <br>
-        <% unless @merge_request.total_sales_logs.blank? %>
-          <%=  @merge_request.total_sales_logs  %> sales logs
-        <% end %>
-      <%  end %>
-    </td>
-    <td class="govuk-table__cell govuk-!-padding-2 govuk-!-text-align-right" style="width: 10%;"><a href="#" aria-label="View all lettings and sales logs after merge">View</a></td>
-  </tr>
-  <tr class="govuk-table__row">
-    <th scope="row" class="govuk-table__header govuk-!-padding-2 govuk-!-padding-right-9 govuk-!-padding-bottom-5">Total stock owners & managing agents after merge</th>
-    <td class="govuk-table__cell govuk-!-padding-2 govuk-!-padding-right-9">
-      <% 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 %>
-        <br>
-        <% unless @merge_request.total_managing_agents.blank? %>
-          <%= @merge_request.total_managing_agents %> managing agents
-        <% end %>
 <% end %>
-    </td>
-    <td class="govuk-table__cell govuk-!-padding-2 govuk-!-text-align-right" style="width: 10%;"><a href="#" aria-label="View all stock owners and managing agents after merge">View</a></td>
-  </tr>
-  </tbody>
-</table>

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

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