From b5e2fecb03e94864d3009eabfb2d223dbf2c9b62 Mon Sep 17 00:00:00 2001
From: Manny Dinssa <44172848+Dinssa@users.noreply.github.com>
Date: Tue, 13 Aug 2024 10:09:11 +0100
Subject: [PATCH] CLDC 2094: View a merge request (#2565)
---
app/controllers/organisations_controller.rb | 1 +
app/helpers/merge_requests_helper.rb | 30 +++++
app/models/merge_request.rb | 5 +
app/models/merge_request_organisation.rb | 4 +
.../merge_requests/_details_list.html.erb | 33 ++++++
.../_merge_request_list.html.erb | 24 +---
.../merge_requests/_summary_card.html.erb | 8 ++
.../merge_request.html.erb | 0
.../merge_requests/organisations.html.erb | 1 +
app/views/merge_requests/show.html.erb | 45 ++++++++
app/views/organisations/index.html.erb | 2 +-
config/routes.rb | 1 +
...add_additional_fields_to_merge_requests.rb | 16 +++
db/schema.rb | 12 +-
spec/factories/merge_requests.rb | 1 +
.../merge_requests/show.html.erb_spec.rb | 104 ++++++++++++++++++
16 files changed, 267 insertions(+), 20 deletions(-)
create mode 100644 app/helpers/merge_requests_helper.rb
create mode 100644 app/views/merge_requests/_details_list.html.erb
rename app/views/{organisations => merge_requests}/_merge_request_list.html.erb (59%)
create mode 100644 app/views/merge_requests/_summary_card.html.erb
rename app/views/{organisations => merge_requests}/merge_request.html.erb (100%)
create mode 100644 app/views/merge_requests/show.html.erb
create mode 100644 db/migrate/20240809154241_add_additional_fields_to_merge_requests.rb
create mode 100644 spec/views/merge_requests/show.html.erb_spec.rb
diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb
index 5d2225a6e..04e100ce6 100644
--- a/app/controllers/organisations_controller.rb
+++ b/app/controllers/organisations_controller.rb
@@ -238,6 +238,7 @@ class OrganisationsController < ApplicationController
def merge_request
@merge_request = MergeRequest.new
+ render "merge_requests/merge_request"
end
def data_sharing_agreement
diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb
new file mode 100644
index 000000000..9f6afe1df
--- /dev/null
+++ b/app/helpers/merge_requests_helper.rb
@@ -0,0 +1,30 @@
+module MergeRequestsHelper
+ def display_value_or_placeholder(value, placeholder = "You didn't answer this question")
+ value.presence || content_tag(:span, placeholder, class: "app-!-colour-muted")
+ 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://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) },
+ ]
+ end
+
+ def merge_details(merge_request)
+ [
+ { label: "Absorbing organisation", value: display_value_or_placeholder(merge_request.absorbing_organisation_name), action: merge_request.status == "request_merged" ? nil : { text: "Change", href: absorbing_organisation_merge_request_path(merge_request), visually_hidden_text: "absorbing organisation" } },
+ { label: "Merging organisations", value: merge_request.merge_request_organisations.any? ? merge_request.merge_request_organisations.map(&:merging_organisation_name).join("
+ <%= 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],
+ wrapper_tag: "span",
+ class: "govuk-!-margin-left-8 govuk-!-margin-right-4",
+ ) %>
+ <% 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/organisations/_merge_request_list.html.erb b/app/views/merge_requests/_merge_request_list.html.erb
similarity index 59%
rename from app/views/organisations/_merge_request_list.html.erb
rename to app/views/merge_requests/_merge_request_list.html.erb
index 6e4bab9fa..e048472a7 100644
--- a/app/views/organisations/_merge_request_list.html.erb
+++ b/app/views/merge_requests/_merge_request_list.html.erb
@@ -3,7 +3,7 @@
No merge requests
<% else %>
<%= govuk_table do |table| %>
- <%= table.with_caption(classes: %w[govuk-!-font-size-19 govuk-!-font-weight-regular]) do |caption| %>
+ <%= table.with_caption(classes: %w[govuk-!-font-size-19 govuk-!-font-weight-regular]) do %>
<%= @merge_requests.where.not(status: 4).count %> unresolved merge requests
<% end %>
<%= table.with_head do |head| %>
@@ -25,30 +25,18 @@
<% @merge_requests.each do |merge_request| %>
<%= table.with_body do |body| %>
<%= body.with_row do |row| %>
- <% if merge_request.absorbing_organisation_name.blank? %>
- <% row.with_cell(text: "You didn't answer this question",
- html_attributes: {
- scope: "row",
- class: "app-!-colour-muted",
- }) %>
- <% else %>
- <% row.with_cell(text: merge_request.absorbing_organisation_name) %>
+ <%= row.with_cell(html_attributes: { scope: "row" }) do %>
+ <%= display_value_or_placeholder(merge_request.absorbing_organisation_name) %>
<% end %>
<% merge_date = merge_request.merge_date %>
- <% if merge_date.nil? %>
- <% row.with_cell(text: "You didn't answer this question",
- html_attributes: {
- scope: "row",
- class: "app-!-colour-muted",
- }) %>
- <% else %>
- <% row.with_cell(text: merge_date.strftime("%d %B %Y")) %>
+ <%= row.with_cell(html_attributes: { scope: "row" }) do %>
+ <%= display_value_or_placeholder(merge_date&.strftime("%d %B %Y")) %>
<% end %>
<% row.with_cell(text: status_tag(merge_request.status)) %>
<% row.with_cell(html_attributes: {
scope: "row",
}) do %>
- <%= govuk_link_to("View details", "/merge-request/#{merge_request.id}/organisations") %>
+ <%= govuk_link_to("View details", merge_request_path(merge_request.id)) %>
<% 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..4a372f0a0
--- /dev/null
+++ b/app/views/merge_requests/_summary_card.html.erb
@@ -0,0 +1,8 @@
+
+
+
<%= title %>
+
+
+ <%= render partial: "merge_requests/details_list", locals: { details: } %>
+
+
+
+
+ The absorbing organisation must accept the Data Sharing Agreement before merging.
+
+
+ Merge details
+ <%= display_value_or_placeholder(@merge_request.absorbing_organisation_name) %>
+
+<% unless @merge_request.status == "request_merged" %>
+
+ class="govuk-button" data-module="govuk-button">
+ Begin merge
+
+
+ Delete merge request
+
+
+<% end %>
+
+<%= render partial: "merge_requests/summary_card", locals: { title: "Request details", details: request_details(@merge_request) } %>
+
+<%= render partial: "merge_requests/summary_card", locals: { title: "Merge details", details: merge_details(@merge_request) } %>
+
+<% unless @merge_request.status == "incomplete" %>
+ <%= render partial: "merge_requests/summary_card", locals: { title: "Merge outcomes", details: merge_outcomes(@merge_request) } %>
+<% end %>
diff --git a/app/views/organisations/index.html.erb b/app/views/organisations/index.html.erb
index d95516eba..64c311472 100644
--- a/app/views/organisations/index.html.erb
+++ b/app/views/organisations/index.html.erb
@@ -16,7 +16,7 @@
<% end %>
<% c.with_tab(label: "Merge requests") do %>
<%= govuk_button_to "Create new merge request", merge_requests_path, html: { method: :post } %>
- <%= render partial: "merge_request_list", locals: { merge_requests: @merge_requests, title: "Merge requests", pagy: @pagy, searched: @searched, item_label:, total_count: @total_count } %>
+ <%= render partial: "merge_requests/merge_request_list", locals: { merge_requests: @merge_requests, title: "Merge requests", pagy: @pagy, searched: @searched, item_label:, total_count: @total_count } %>
<% end %>
<% end %>
diff --git a/config/routes.rb b/config/routes.rb
index e0d9631e9..fbb59a7d0 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -205,6 +205,7 @@ Rails.application.routes.draw do
get "new-organisation-telephone-number"
get "new-organisation-type"
get "merge-date"
+ get "details", to: "merge_requests#details"
end
end
diff --git a/db/migrate/20240809154241_add_additional_fields_to_merge_requests.rb b/db/migrate/20240809154241_add_additional_fields_to_merge_requests.rb
new file mode 100644
index 000000000..6f2a37fd0
--- /dev/null
+++ b/db/migrate/20240809154241_add_additional_fields_to_merge_requests.rb
@@ -0,0 +1,16 @@
+class AddAdditionalFieldsToMergeRequests < ActiveRecord::Migration[7.0]
+ def change
+ change_table :merge_requests, bulk: true do |t|
+ t.integer :requester_id
+ t.string :helpdesk_ticket
+ t.integer :total_users
+ t.integer :total_schemes
+ t.integer :total_lettings_logs
+ t.integer :total_sales_logs
+ t.integer :total_stock_owners
+ t.integer :total_managing_agents
+ t.boolean :signed_dsa, default: false
+ t.datetime :discarded_at
+ end
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index f9a17bd49..8296332f5 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.
-ActiveRecord::Schema[7.0].define(version: 2024_08_08_134014) do
+ActiveRecord::Schema[7.0].define(version: 2024_08_09_154241) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
@@ -431,6 +431,16 @@ ActiveRecord::Schema[7.0].define(version: 2024_08_08_134014) do
t.string "new_organisation_postcode"
t.string "new_organisation_telephone_number"
t.datetime "merge_date"
+ t.integer "requester_id"
+ t.string "helpdesk_ticket"
+ t.integer "total_users"
+ t.integer "total_schemes"
+ t.integer "total_lettings_logs"
+ t.integer "total_sales_logs"
+ t.integer "total_stock_owners"
+ t.integer "total_managing_agents"
+ t.boolean "signed_dsa", default: false
+ t.datetime "discarded_at"
end
create_table "notifications", force: :cascade do |t|
diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb
index 888639b82..19020fce1 100644
--- a/spec/factories/merge_requests.rb
+++ b/spec/factories/merge_requests.rb
@@ -2,6 +2,7 @@ FactoryBot.define do
factory :merge_request do
status { "incomplete" }
merge_date { nil }
+ helpdesk_ticket { "MSD-99999" }
association :requesting_organisation, factory: :organisation
end
end
diff --git a/spec/views/merge_requests/show.html.erb_spec.rb b/spec/views/merge_requests/show.html.erb_spec.rb
new file mode 100644
index 000000000..0f0e42b2d
--- /dev/null
+++ b/spec/views/merge_requests/show.html.erb_spec.rb
@@ -0,0 +1,104 @@
+require "rails_helper"
+
+RSpec.describe "merge_requests/show.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.requester&.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