From e94a628948fefa439d9592fd30a30d0dd0165830 Mon Sep 17 00:00:00 2001
From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com>
Date: Thu, 15 Aug 2024 15:58:28 +0100
Subject: [PATCH] CLDC-2101 Add begin merge (#2575)
* Calculate merge request status
* Add start merge and merge request job
* Update merge request when it gets processed
* Refactor and display a banner for failed requests
* update test
* Update change links
---
app/controllers/merge_requests_controller.rb | 9 +++
app/helpers/merge_requests_helper.rb | 14 +++--
app/jobs/process_merge_request_job.rb | 14 +++++
app/models/merge_request.rb | 31 ++++++++++
app/models/merge_request_organisation.rb | 7 +++
.../_notification_banners.html.erb | 21 +++++++
app/views/merge_requests/show.html.erb | 25 ++-------
config/routes.rb | 1 +
.../20240814083017_add_last_failed_attempt.rb | 5 ++
db/schema.rb | 3 +-
.../{merge_requests.rb => merge_request.rb} | 0
spec/factories/merge_request_organisation.rb | 6 ++
spec/jobs/process_merge_request_job_spec.rb | 47 ++++++++++++++++
spec/models/merge_request_spec.rb | 47 ++++++++++++++++
.../merge_requests_controller_spec.rb | 56 +++++++++++++++++++
.../merge_requests/show.html.erb_spec.rb | 2 +-
16 files changed, 263 insertions(+), 25 deletions(-)
create mode 100644 app/jobs/process_merge_request_job.rb
create mode 100644 app/views/merge_requests/_notification_banners.html.erb
create mode 100644 db/migrate/20240814083017_add_last_failed_attempt.rb
rename spec/factories/{merge_requests.rb => merge_request.rb} (100%)
create mode 100644 spec/factories/merge_request_organisation.rb
create mode 100644 spec/jobs/process_merge_request_job_spec.rb
diff --git a/app/controllers/merge_requests_controller.rb b/app/controllers/merge_requests_controller.rb
index 95be8d875..f19224a92 100644
--- a/app/controllers/merge_requests_controller.rb
+++ b/app/controllers/merge_requests_controller.rb
@@ -61,6 +61,15 @@ class MergeRequestsController < ApplicationController
@new_merging_org_ids = []
end
+ def start_merge
+ if @merge_request.status == "ready_to_merge"
+ @merge_request.update!(status: :processing)
+ ProcessMergeRequestJob.perform_later(merge_request: @merge_request)
+ end
+
+ redirect_to merge_request_path(@merge_request)
+ end
+
private
def page
diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb
index 8318b533a..4d7408349 100644
--- a/app/helpers/merge_requests_helper.rb
+++ b/app/helpers/merge_requests_helper.rb
@@ -6,16 +6,16 @@ module MergeRequestsHelper
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: helpdesk_ticket_merge_request_path(merge_request, referrer: "check_answers"), visually_hidden_text: "helpdesk ticket" } },
+ { 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_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.status == "request_merged" ? nil : { text: "Change", href: absorbing_organisation_merge_request_path(merge_request, referrer: "check_answers"), 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("
").html_safe : display_value_or_placeholder(nil), action: merge_request.status == "request_merged" ? nil : { text: "Change", href: merging_organisations_merge_request_path(merge_request, referrer: "check_answers"), 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: merge_date_merge_request_path(merge_request, referrer: "check_answers"), visually_hidden_text: "merge date" } },
+ { 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") },
]
end
@@ -68,4 +68,10 @@ module MergeRequestsHelper
merge_date_merge_request_path(merge_request)
end
end
+
+ def merge_request_action(merge_request, page)
+ 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 }
+ end
+ end
end
diff --git a/app/jobs/process_merge_request_job.rb b/app/jobs/process_merge_request_job.rb
new file mode 100644
index 000000000..b706d9a81
--- /dev/null
+++ b/app/jobs/process_merge_request_job.rb
@@ -0,0 +1,14 @@
+class ProcessMergeRequestJob < ApplicationJob
+ queue_as :default
+
+ def perform(merge_request:)
+ absorbing_organisation_id = merge_request.absorbing_organisation_id
+ merging_organisation_ids = merge_request.merging_organisations.pluck(:id)
+ merge_date = merge_request.merge_date
+
+ Merge::MergeOrganisationsService.new(absorbing_organisation_id:, merging_organisation_ids:, merge_date:).call
+ merge_request.update!(status: "request_merged", last_failed_attempt: nil)
+ rescue StandardError
+ merge_request.update!(last_failed_attempt: Time.zone.now)
+ end
+end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 7c95e199c..078399c2a 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -4,6 +4,7 @@ class MergeRequest < ApplicationRecord
belongs_to :absorbing_organisation, class_name: "Organisation", optional: true
has_many :merging_organisations, through: :merge_request_organisations, source: :merging_organisation
belongs_to :requester, class_name: "User", optional: true
+ before_save :update_status!
STATUS = {
"merge_issues" => 0,
@@ -11,6 +12,7 @@ class MergeRequest < ApplicationRecord
"ready_to_merge" => 2,
"processing" => 3,
"request_merged" => 4,
+ "deleted" => 5,
}.freeze
enum status: STATUS
@@ -21,6 +23,8 @@ class MergeRequest < ApplicationRecord
merged.where("merge_requests.merge_date >= ?", open_collection_period_start_date).or(not_merged).where(discarded_at: nil)
}
+ attr_accessor :skip_update_status
+
def absorbing_organisation_name
absorbing_organisation&.name || ""
end
@@ -32,4 +36,31 @@ class MergeRequest < ApplicationRecord
def discard!
update!(discarded_at: Time.zone.now)
end
+
+ def update_status!
+ return if skip_update_status
+
+ self.status = calculate_status
+ end
+
+ def calculate_status
+ return "deleted" if discarded_at.present?
+ return "request_merged" if status == "request_merged"
+ return "processing" if status == "processing"
+ return "incomplete" unless required_questions_answered?
+ return "ready_to_merge" if absorbing_organisation_signed_dsa?
+
+ "merge_issues"
+ end
+
+ def required_questions_answered?
+ absorbing_organisation_id.present? &&
+ merge_date.present? &&
+ merging_organisations.count.positive? &&
+ errors.empty?
+ end
+
+ def absorbing_organisation_signed_dsa?
+ absorbing_organisation&.data_protection_confirmed?
+ end
end
diff --git a/app/models/merge_request_organisation.rb b/app/models/merge_request_organisation.rb
index e181ca304..18a77d93a 100644
--- a/app/models/merge_request_organisation.rb
+++ b/app/models/merge_request_organisation.rb
@@ -8,12 +8,19 @@ class MergeRequestOrganisation < ApplicationRecord
scope :merged, -> { joins(:merge_request).where(merge_requests: { status: "request_merged" }) }
scope :with_merging_organisation, ->(merging_organisation) { where(merging_organisation:) }
+ after_save :update_merge_request_status
+
has_paper_trail
def merging_organisation_name
merging_organisation.name || ""
end
+ def update_merge_request_status
+ merge_request.update_status!
+ merge_request.save!
+ end
+
private
def validate_merging_organisations
diff --git a/app/views/merge_requests/_notification_banners.html.erb b/app/views/merge_requests/_notification_banners.html.erb
new file mode 100644
index 000000000..7674773ab
--- /dev/null
+++ b/app/views/merge_requests/_notification_banners.html.erb
@@ -0,0 +1,21 @@
+<% unless @merge_request.absorbing_organisation_signed_dsa? || @merge_request.absorbing_organisation_id.blank? %>
+ <%= govuk_notification_banner(title_text: "Important") do %>
+
+ The absorbing organisation must accept the Data Sharing Agreement before merging.
+
+ <% if @merge_request.dpo_user %>
+ Contact the Data Protection Officer: <%= link_to @merge_request.dpo_user.name, user_path(@merge_request.dpo_user.id) %>
+ <% else %>
+ <%= @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 %>
+<% end %>
+
+<% if @merge_request.last_failed_attempt.present? %>
+ <%= govuk_notification_banner(title_text: "Important") do %>
+
+ An error occurred while processing the merge.
+
+ No changes have been made. Try beginning the merge again.
+ <% end %>
+<% end %>
diff --git a/app/views/merge_requests/show.html.erb b/app/views/merge_requests/show.html.erb
index ce4fbb38d..c93e99738 100644
--- a/app/views/merge_requests/show.html.erb
+++ b/app/views/merge_requests/show.html.erb
@@ -3,31 +3,18 @@
<% content_for :title, title %>
<%= govuk_back_link href: organisations_path(anchor: "merge-requests") %>
<% end %>
-<% unless @merge_request.signed_dsa || @merge_request.absorbing_organisation_id.blank? %>
-
-
-
- The absorbing organisation must accept the Data Sharing Agreement before merging.
-
- <% if @merge_request.dpo_user %>
- Contact the Data Protection Officer: <%= link_to @merge_request.dpo_user.name, user_path(@merge_request.dpo_user.id) %>
- <% else %>
- <%= @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 %>
-
-
Merge details
<%= display_value_or_placeholder(@merge_request.absorbing_organisation_name) %>
<% unless @merge_request.status == "request_merged" %>
- <%= govuk_button_link_to "Begin merge", "#", disabled: @merge_request.status != "ready_to_merge" %>
+ <%= form_with model: @merge_request, url: start_merge_merge_request_path(@merge_request) do |f| %>
+ <%= f.govuk_submit "Begin merge", disabled: @merge_request.status != "ready_to_merge" %>
+ <% end %>
<%= govuk_button_link_to "Delete merge request", delete_confirmation_merge_request_path(@merge_request), warning: true %>
<% end %>
diff --git a/config/routes.rb b/config/routes.rb
index ecdb8f2c1..ebbb588ca 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -203,6 +203,7 @@ Rails.application.routes.draw do
get "helpdesk-ticket"
get "delete-confirmation", to: "merge_requests#delete_confirmation"
delete "delete", to: "merge_requests#delete"
+ patch "start-merge", to: "merge_requests#start_merge"
end
end
diff --git a/db/migrate/20240814083017_add_last_failed_attempt.rb b/db/migrate/20240814083017_add_last_failed_attempt.rb
new file mode 100644
index 000000000..34e0e4046
--- /dev/null
+++ b/db/migrate/20240814083017_add_last_failed_attempt.rb
@@ -0,0 +1,5 @@
+class AddLastFailedAttempt < ActiveRecord::Migration[7.0]
+ def change
+ add_column :merge_requests, :last_failed_attempt, :datetime
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 2d728a72d..9f46a7ae4 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_13_112119) do
+ActiveRecord::Schema[7.0].define(version: 2024_08_14_083017) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
@@ -432,6 +432,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_08_13_112119) do
t.integer "total_managing_agents"
t.boolean "signed_dsa", default: false
t.datetime "discarded_at"
+ t.datetime "last_failed_attempt"
end
create_table "notifications", force: :cascade do |t|
diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_request.rb
similarity index 100%
rename from spec/factories/merge_requests.rb
rename to spec/factories/merge_request.rb
diff --git a/spec/factories/merge_request_organisation.rb b/spec/factories/merge_request_organisation.rb
new file mode 100644
index 000000000..178401fb0
--- /dev/null
+++ b/spec/factories/merge_request_organisation.rb
@@ -0,0 +1,6 @@
+FactoryBot.define do
+ factory :merge_request_organisation do
+ association :merging_organisation, factory: :organisation
+ association :merge_request, factory: :merge_request
+ end
+end
diff --git a/spec/jobs/process_merge_request_job_spec.rb b/spec/jobs/process_merge_request_job_spec.rb
new file mode 100644
index 000000000..0c4ae30f1
--- /dev/null
+++ b/spec/jobs/process_merge_request_job_spec.rb
@@ -0,0 +1,47 @@
+require "rails_helper"
+
+describe ProcessMergeRequestJob do
+ let(:job) { described_class.new }
+ let(:merge_organisations_service) { instance_double(Merge::MergeOrganisationsService) }
+
+ before do
+ allow(Merge::MergeOrganisationsService).to receive(:new).and_return(merge_organisations_service)
+ allow(merge_organisations_service).to receive(:call).and_return(nil)
+ end
+
+ context "when processing a merge request" do
+ let(:organisation) { create(:organisation) }
+ let(:merging_organisation) { create(:organisation) }
+ let(:other_merging_organisation) { create(:organisation) }
+ let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: organisation, merge_date: Time.zone.local(2022, 3, 3)) }
+
+ before do
+ create(:merge_request_organisation, merge_request:, merging_organisation:)
+ create(:merge_request_organisation, merge_request:, merging_organisation: other_merging_organisation)
+ end
+
+ it "calls the merge organisations service with correct arguments" do
+ expect(Merge::MergeOrganisationsService).to receive(:new).with(absorbing_organisation_id: organisation.id, merging_organisation_ids: [merging_organisation.id, other_merging_organisation.id], merge_date: Time.zone.local(2022, 3, 3))
+
+ job.perform(merge_request:)
+ expect(merge_request.reload.status).to eq("request_merged")
+ end
+
+ it "clears last_failed_attempt value" do
+ merge_request.update!(last_failed_attempt: Time.zone.now)
+ job.perform(merge_request:)
+
+ expect(merge_request.reload.last_failed_attempt).to be_nil
+ end
+
+ it "sets last_failed_attempt value if there's an error" do
+ allow(merge_organisations_service).to receive(:call).and_raise(ActiveRecord::Rollback)
+
+ expect(merge_request.last_failed_attempt).to be_nil
+ job.perform(merge_request:)
+
+ merge_request.reload
+ expect(merge_request.last_failed_attempt).to be_within(10.seconds).of(Time.zone.now)
+ end
+ end
+end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index cf38d5033..6ada68985 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -42,4 +42,51 @@ RSpec.describe MergeRequest, type: :model do
expect(described_class.visible).not_to include(merge_request)
end
end
+
+ describe "#calculate_status" do
+ it "returns the correct status for deleted merge request" do
+ merge_request = build(:merge_request, id: 1, discarded_at: Time.zone.today)
+ expect(merge_request.calculate_status).to eq "deleted"
+ end
+
+ it "returns the correct status for a merged request" do
+ merge_request = build(:merge_request, id: 1, status: "request_merged")
+ expect(merge_request.calculate_status).to eq "request_merged"
+ end
+
+ it "returns the correct status for a ready to merge request" do
+ merge_request = build(:merge_request, id: 1, absorbing_organisation: create(:organisation), merge_date: Time.zone.today)
+ create(:merge_request_organisation, merge_request:)
+ expect(merge_request.calculate_status).to eq "ready_to_merge"
+ end
+
+ it "returns the merge issues if dsa is not signed for absorbing organisation" do
+ merge_request = build(:merge_request, id: 1, absorbing_organisation: create(:organisation, with_dsa: false), merge_date: Time.zone.today)
+ create(:merge_request_organisation, merge_request:)
+ expect(merge_request.calculate_status).to eq "merge_issues"
+ end
+
+ it "returns the incomplete if absorbing organisation is missing" do
+ merge_request = build(:merge_request, id: 1, absorbing_organisation: nil, merge_date: Time.zone.today)
+ create(:merge_request_organisation, merge_request:)
+ expect(merge_request.calculate_status).to eq "incomplete"
+ end
+
+ it "returns the incomplete if merge requests organisation is missing" do
+ merge_request = build(:merge_request, id: 1, absorbing_organisation: create(:organisation), merge_date: Time.zone.today)
+ expect(merge_request.calculate_status).to eq "incomplete"
+ end
+
+ it "returns the incomplete if merge date is missing" do
+ merge_request = build(:merge_request, id: 1, absorbing_organisation: create(:organisation))
+ create(:merge_request_organisation, merge_request:)
+ expect(merge_request.calculate_status).to eq "incomplete"
+ end
+
+ it "returns processing if merge is processing" do
+ merge_request = build(:merge_request, id: 1, absorbing_organisation: create(:organisation), status: "processing")
+ create(:merge_request_organisation, merge_request:)
+ expect(merge_request.calculate_status).to eq "processing"
+ end
+ end
end
diff --git a/spec/requests/merge_requests_controller_spec.rb b/spec/requests/merge_requests_controller_spec.rb
index 865b75ba1..27cace08e 100644
--- a/spec/requests/merge_requests_controller_spec.rb
+++ b/spec/requests/merge_requests_controller_spec.rb
@@ -364,6 +364,62 @@ RSpec.describe MergeRequestsController, type: :request do
end
end
end
+
+ describe "#start_merge" do
+ let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: organisation, merge_date: Time.zone.local(2022, 3, 3)) }
+ let(:merging_organisation) { create(:organisation, name: "Merging Test Org") }
+
+ before do
+ allow(ProcessMergeRequestJob).to receive(:perform_later).and_return(nil)
+ end
+
+ context "when merge request is ready to merge" do
+ before do
+ create(:merge_request_organisation, merge_request:, merging_organisation: other_organisation)
+ create(:merge_request_organisation, merge_request:, merging_organisation:)
+ end
+
+ it "runs the job with correct merge request" do
+ expect(merge_request.reload.status).to eq("ready_to_merge")
+ expect(ProcessMergeRequestJob).to receive(:perform_later).with(merge_request:).once
+ patch "/merge-request/#{merge_request.id}/start-merge"
+ expect(merge_request.reload.status).to eq("processing")
+ end
+ end
+
+ context "when merge request is not ready to merge" do
+ it "does not run the job" do
+ expect(merge_request.status).to eq("incomplete")
+ expect(ProcessMergeRequestJob).not_to receive(:perform_later).with(merge_request:)
+ patch "/merge-request/#{merge_request.id}/start-merge"
+ expect(merge_request.reload.status).to eq("incomplete")
+ end
+ end
+ end
+
+ describe "#show" do
+ before do
+ get "/merge-request/#{merge_request.id}", headers:
+ end
+
+ context "when request has previously failed" do
+ let(:merge_request) { create(:merge_request, last_failed_attempt: Time.zone.yesterday) }
+
+ it "shows a banner" do
+ expect(page).to have_content("An error occurred while processing the merge.")
+ expect(page).to have_content("No changes have been made. Try beginning the merge again.")
+ end
+ end
+
+ context "when request has not previously failed" do
+ let(:merge_request) { create(:merge_request, last_failed_attempt: nil) }
+
+ it "does not show a banner" do
+ expect(page).not_to have_content("An error occurred while processing the merge.")
+ expect(page).not_to have_content("No changes have been made. Try beginning the merge again.")
+ end
+ end
+ end
end
context "when user is signed in with a data coordinator user" do
diff --git a/spec/views/merge_requests/show.html.erb_spec.rb b/spec/views/merge_requests/show.html.erb_spec.rb
index 72974fc9e..0e52e3f58 100644
--- a/spec/views/merge_requests/show.html.erb_spec.rb
+++ b/spec/views/merge_requests/show.html.erb_spec.rb
@@ -1,7 +1,7 @@
require "rails_helper"
RSpec.describe "merge_requests/show.html.erb", type: :view do
- let(:absorbing_organisation) { create(:organisation, name: "Absorbing Org") }
+ let(:absorbing_organisation) { create(:organisation, name: "Absorbing Org", with_dsa: false) }
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) }