diff --git a/app/controllers/merge_requests_controller.rb b/app/controllers/merge_requests_controller.rb index c0fc40092..77628492b 100644 --- a/app/controllers/merge_requests_controller.rb +++ b/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 diff --git a/app/jobs/process_merge_request_job.rb b/app/jobs/process_merge_request_job.rb index b706d9a81..cdec20814 100644 --- a/app/jobs/process_merge_request_job.rb +++ b/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 diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 078399c2a..91db085c5 100644 --- a/app/models/merge_request.rb +++ b/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, - "incomplete" => 1, - "ready_to_merge" => 2, - "processing" => 3, - "request_merged" => 4, - "deleted" => 5, + merge_issues: "merge_issues", + incomplete: "incomplete", + ready_to_merge: "ready_to_merge", + processing: "processing", + request_merged: "request_merged", + deleted: "deleted", }.freeze enum status: STATUS - scope :not_merged, -> { where.not(status: "request_merged") } - scope :merged, -> { where(status: "request_merged") } + scope :not_merged, -> { where(request_merged: [false, nil]) } + 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! - 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? + def status + return STATUS[:deleted] if discarded_at.present? + return STATUS[:request_merged] if request_merged + return STATUS[:processing] if processing + return STATUS[:incomplete] unless required_questions_answered? + return STATUS[:ready_to_merge] if absorbing_organisation_signed_dsa? - "merge_issues" + STATUS[:merge_issues] end def required_questions_answered? diff --git a/app/models/merge_request_organisation.rb b/app/models/merge_request_organisation.rb index 18a77d93a..6dda8b35e 100644 --- a/app/models/merge_request_organisation.rb +++ b/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 diff --git a/app/views/merge_requests/_merge_request_list.html.erb b/app/views/merge_requests/_merge_request_list.html.erb index e048472a7..c133aabc1 100644 --- a/app/views/merge_requests/_merge_request_list.html.erb +++ b/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 %> - <%= @merge_requests.where.not(status: 4).count %> unresolved merge requests + <%= @merge_requests.not_merged.count %> unresolved merge requests <% end %> <%= table.with_head do |head| %> <%= head.with_row do |row| %> diff --git a/db/migrate/20240819100411_update_merge_request_fields_for_status.rb b/db/migrate/20240819100411_update_merge_request_fields_for_status.rb new file mode 100644 index 000000000..6613d27a4 --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 5f99e7a38..153770ccf 100644 --- a/db/schema.rb +++ b/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| diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 6ada68985..1f52dd640 100644 --- a/spec/models/merge_request_spec.rb +++ b/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_old) { create(:merge_request, status: "request_merged", merge_date: 18.months.ago) } - let!(:not_merged) { create(:merge_request, status: "incomplete") } + let!(:merged_recent) { create(:merge_request, request_merged: true, merge_date: 3.months.ago) } + let!(:merged_old) { create(:merge_request, request_merged: true, merge_date: 18.months.ago) } + 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") - expect(merge_request.calculate_status).to eq "request_merged" + merge_request = build(:merge_request, id: 1, request_merged: true) + 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 diff --git a/spec/requests/merge_requests_controller_spec.rb b/spec/requests/merge_requests_controller_spec.rb index 8b7a577a7..8e406ea5f 100644 --- a/spec/requests/merge_requests_controller_spec.rb +++ b/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 diff --git a/spec/views/merge_requests/show.html.erb_spec.rb b/spec/views/merge_requests/show.html.erb_spec.rb index b6e6a3ce2..75fa4197d 100644 --- a/spec/views/merge_requests/show.html.erb_spec.rb +++ b/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