Make merge request status dynamic (#2592)

* Make merge request status dynamic

* Update tests

app/controllers/merge_requests_controller.rb

@@ -64,7 +64,7 @@ class MergeRequestsController < ApplicationController
 
   def start_merge
     if @merge_request.status == "ready_to_merge"
-      @merge_request.update!(status: :processing)
+      @merge_request.update!(processing: true)
       ProcessMergeRequestJob.perform_later(merge_request: @merge_request)
     end
 

app/jobs/process_merge_request_job.rb

@@ -7,7 +7,7 @@ class ProcessMergeRequestJob < ApplicationJob
     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)
+    merge_request.update!(request_merged: true, last_failed_attempt: nil)
   rescue StandardError
     merge_request.update!(last_failed_attempt: Time.zone.now)
   end

app/models/merge_request.rb

@@ -4,27 +4,24 @@ 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,
+    merge_issues: "merge_issues",
-    "incomplete" => 1,
+    incomplete: "incomplete",
-    "ready_to_merge" => 2,
+    ready_to_merge: "ready_to_merge",
-    "processing" => 3,
+    processing: "processing",
-    "request_merged" => 4,
+    request_merged: "request_merged",
-    "deleted" => 5,
+    deleted: "deleted",
   }.freeze
   enum status: STATUS
 
-  scope :not_merged, -> { where.not(status: "request_merged") }
+  scope :not_merged, -> { where(request_merged: [false, nil]) }
-  scope :merged, -> { where(status: "request_merged") }
+  scope :merged, -> { where(request_merged: true) }
   scope :visible, lambda {
     open_collection_period_start_date = FormHandler.instance.start_date_of_earliest_open_collection_period
     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
@@ -37,20 +34,14 @@ class MergeRequest < ApplicationRecord
     update!(discarded_at: Time.zone.now)
   end
 
-  def update_status!
+  def status
-    return if skip_update_status
+    return STATUS[:deleted] if discarded_at.present?
-
+    return STATUS[:request_merged] if request_merged
-    self.status = calculate_status
+    return STATUS[:processing] if processing
-  end
+    return STATUS[:incomplete] unless required_questions_answered?
-
+    return STATUS[:ready_to_merge] if absorbing_organisation_signed_dsa?
-  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"
+    STATUS[:merge_issues]
   end
 
   def required_questions_answered?

app/models/merge_request_organisation.rb

@@ -5,22 +5,15 @@ class MergeRequestOrganisation < ApplicationRecord
   validates :merging_organisation, presence: { message: I18n.t("validations.merge_request.merging_organisation_id.blank") }
   validate :validate_merging_organisations
 
-  scope :merged, -> { joins(:merge_request).where(merge_requests: { status: "request_merged" }) }
+  scope :merged, -> { joins(:merge_request).where(merge_requests: { request_merged: true }) }
   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/_merge_request_list.html.erb

@@ -4,7 +4,7 @@
   <% else %>
   <%= govuk_table do |table| %>
     <%= table.with_caption(classes: %w[govuk-!-font-size-19 govuk-!-font-weight-regular]) do %>
-      <strong><%= @merge_requests.where.not(status: 4).count %></strong> unresolved merge requests
+      <strong><%= @merge_requests.not_merged.count %></strong> unresolved merge requests
     <% end %>
     <%= table.with_head do |head| %>
       <%= head.with_row do |row| %>

db/migrate/20240819100411_update_merge_request_fields_for_status.rb

@@ -0,0 +1,17 @@
+class UpdateMergeRequestFieldsForStatus < ActiveRecord::Migration[7.0]
+  def up
+    change_table :merge_requests, bulk: true do |t|
+      t.column :request_merged, :boolean
+      t.column :processing, :boolean
+      t.remove :status
+    end
+  end
+
+  def down
+    change_table :merge_requests, bulk: true do |t|
+      t.remove :request_merged
+      t.remove :processing
+      t.column :status, :string
+    end
+  end
+end

db/schema.rb

@@ -431,7 +431,6 @@ ActiveRecord::Schema[7.0].define(version: 2024_08_19_143150) do
     t.integer "requesting_organisation_id"
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
-    t.integer "status"
     t.integer "absorbing_organisation_id"
     t.datetime "merge_date"
     t.integer "requester_id"
@@ -445,6 +444,8 @@ ActiveRecord::Schema[7.0].define(version: 2024_08_19_143150) do
     t.boolean "signed_dsa", default: false
     t.datetime "discarded_at"
     t.datetime "last_failed_attempt"
+    t.boolean "request_merged"
+    t.boolean "processing"
   end
 
   create_table "notifications", force: :cascade do |t|

spec/models/merge_request_spec.rb

@@ -3,9 +3,9 @@ require "rails_helper"
 RSpec.describe MergeRequest, type: :model do
   describe ".visible" do
     let(:open_collection_period_start_date) { 1.year.ago }
-    let!(:merged_recent) { create(:merge_request, status: "request_merged", merge_date: 3.months.ago) }
+    let!(:merged_recent) { create(:merge_request, request_merged: true, merge_date: 3.months.ago) }
-    let!(:merged_old) { create(:merge_request, status: "request_merged", merge_date: 18.months.ago) }
+    let!(:merged_old) { create(:merge_request, request_merged: true, merge_date: 18.months.ago) }
-    let!(:not_merged) { create(:merge_request, status: "incomplete") }
+    let!(:not_merged) { create(:merge_request, request_merged: false) }
 
     before do
       allow(FormHandler.instance).to receive(:start_date_of_earliest_open_collection_period).and_return(open_collection_period_start_date)
@@ -43,50 +43,50 @@ RSpec.describe MergeRequest, type: :model do
     end
   end
 
-  describe "#calculate_status" do
+  describe "#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"
+      expect(merge_request.status).to eq MergeRequest::STATUS[:deleted]
     end
 
     it "returns the correct status for a merged request" do
-      merge_request = build(:merge_request, id: 1, status: "request_merged")
+      merge_request = build(:merge_request, id: 1, request_merged: true)
-      expect(merge_request.calculate_status).to eq "request_merged"
+      expect(merge_request.status).to eq MergeRequest::STATUS[: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"
+      expect(merge_request.status).to eq MergeRequest::STATUS[: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"
+      expect(merge_request.status).to eq MergeRequest::STATUS[: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"
+      expect(merge_request.status).to eq MergeRequest::STATUS[: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"
+      expect(merge_request.status).to eq MergeRequest::STATUS[: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"
+      expect(merge_request.status).to eq MergeRequest::STATUS[:incomplete]
     end
 
     it "returns processing if merge is processing" do
-      merge_request = build(:merge_request, id: 1, absorbing_organisation: create(:organisation), status: "processing")
+      merge_request = build(:merge_request, id: 1, absorbing_organisation: create(:organisation), processing: true)
       create(:merge_request_organisation, merge_request:)
-      expect(merge_request.calculate_status).to eq "processing"
+      expect(merge_request.status).to eq MergeRequest::STATUS[:processing]
     end
   end
 end

spec/requests/merge_requests_controller_spec.rb

@@ -17,7 +17,7 @@ RSpec.describe MergeRequestsController, type: :request do
     end
 
     context "when creating a new merge request" do
-      let(:params) { { merge_request: { requesting_organisation_id: support_user.organisation_id, status: "incomplete" } } }
+      let(:params) { { merge_request: { requesting_organisation_id: support_user.organisation_id } } }
 
       before do
         post "/merge-request", headers:, params:
@@ -30,7 +30,7 @@ RSpec.describe MergeRequestsController, type: :request do
       end
 
       context "when passing a different requesting organisation id" do
-        let(:params) { { merge_request: { requesting_organisation_id: other_organisation.id, status: "incomplete" } } }
+        let(:params) { { merge_request: { requesting_organisation_id: other_organisation.id } } }
 
         it "creates merge request with current user organisation" do
           follow_redirect!
@@ -74,7 +74,7 @@ RSpec.describe MergeRequestsController, type: :request do
         let(:params) { { merge_request: { merging_organisation: other_organisation.id, new_merging_org_ids: [] } } }
 
         before do
-          MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "request_merged")
+          MergeRequest.create!(requesting_organisation_id: other_organisation.id, request_merged: true)
           patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params:
         end
 
@@ -89,7 +89,7 @@ RSpec.describe MergeRequestsController, type: :request do
         let(:params) { { merge_request: { merging_organisation: another_organisation.id, new_merging_org_ids: [] } } }
 
         before do
-          existing_merge_request = MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "request_merged")
+          existing_merge_request = MergeRequest.create!(requesting_organisation_id: other_organisation.id, request_merged: true)
           MergeRequestOrganisation.create!(merge_request_id: existing_merge_request.id, merging_organisation_id: another_organisation.id)
           patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params:
         end
@@ -107,7 +107,7 @@ RSpec.describe MergeRequestsController, type: :request do
         let(:params) { { merge_request: { merging_organisation: another_organisation.id, new_merging_org_ids: [] } } }
 
         before do
-          existing_merge_request = MergeRequest.create!(requesting_organisation_id: other_organisation.id, status: "incomplete")
+          existing_merge_request = MergeRequest.create!(requesting_organisation_id: other_organisation.id)
           MergeRequestOrganisation.create!(merge_request_id: existing_merge_request.id, merging_organisation_id: another_organisation.id)
           patch "/merge-request/#{merge_request.id}/merging-organisations", headers:, params:
         end
@@ -428,7 +428,7 @@ RSpec.describe MergeRequestsController, type: :request do
     end
 
     describe "#merging_organisations" do
-      let(:params) { { merge_request: { requesting_organisation_id: other_organisation.id, status: "incomplete" } } }
+      let(:params) { { merge_request: { requesting_organisation_id: other_organisation.id } } }
 
       context "when creating a new merge request" do
         before do

spec/views/merge_requests/show.html.erb_spec.rb

@@ -3,7 +3,7 @@ require "rails_helper"
 RSpec.describe "merge_requests/show.html.erb", type: :view do
   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) }
+  let(:merge_request) { create(:merge_request, absorbing_organisation_id: absorbing_organisation.id, signed_dsa: false) }
 
   before do
     assign(:merge_request, merge_request)
@@ -53,7 +53,7 @@ RSpec.describe "merge_requests/show.html.erb", type: :view do
 
   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)
+      merge_request.update!(request_merged: true, 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