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

@@ -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

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: "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("<br>").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: "Merging organisations", value: merge_request.merge_request_organisations.any? ? merge_request.merge_request_organisations.map(&:merging_organisation_name).join("<br>").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.status == "request_merged" ? nil : { text: "Change", href: merge_date_merge_request_path(merge_request, referrer: "check_answers"), visually_hidden_text: "merge date" } },
+      { 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

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

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

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

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 %>
+    <p class="govuk-notification-banner__heading govuk-!-width-full" style="max-width: fit-content">
+      The absorbing organisation must accept the Data Sharing Agreement before merging.
+    <p>
+    <% 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 %>
+    <p class="govuk-notification-banner__heading govuk-!-width-full" style="max-width: fit-content">
+      An error occurred while processing the merge.
+    <p>
+    No changes have been made. Try beginning the merge again.
+  <% end %>
+<% end %>

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? %>
+
-  <div class="govuk-notification-banner" role="region" aria-labelledby="govuk-notification-banner-title" data-module="govuk-notification-banner">
+<%= render partial: "notification_banners" %>
-    <div class="govuk-notification-banner__header">
+
-      <h2 class="govuk-notification-banner__title" id="govuk-notification-banner-title">
-        Important
-      </h2>
-    </div>
-    <div class="govuk-notification-banner__content">
-      <strong>The absorbing organisation must accept the Data Sharing Agreement before merging.</strong>
-      <br>
-      <% 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 %>
-    </div>
-  </div>
-<% end %>
 <h1 class="govuk-heading-l">
   <span class="govuk-caption-l">Merge details</span>
   <%= display_value_or_placeholder(@merge_request.absorbing_organisation_name) %>
 </h1>
 <% unless @merge_request.status == "request_merged" %>
 <div class="govuk-button-group">
-  <%= 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 %>
 </div>
 <% end %>

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
 

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

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|

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

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

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

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

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) }