Merge branch '212886-excessive-merge-request-conflict-notifications' into 'master'

Notify new merge Request conflicts only once

See merge request gitlab-org/gitlab!28498

app/graphql/types/merge_request_type.rb

@@ -60,7 +60,7 @@ module Types
           description: 'Indicates if the source branch of the merge request will be deleted after merge'
     field :force_remove_source_branch, GraphQL::BOOLEAN_TYPE, method: :force_remove_source_branch?, null: true,
           description: 'Indicates if the project settings will lead to source branch deletion after merge'
-    field :merge_status, GraphQL::STRING_TYPE, null: true,
+    field :merge_status, GraphQL::STRING_TYPE, method: :public_merge_status, null: true,
           description: 'Status of the merge request'
     field :in_progress_merge_commit_sha, GraphQL::STRING_TYPE, null: true,
           description: 'Commit SHA of the merge request if merge is in progress'

app/models/merge_request.rb

@@ -167,20 +167,22 @@ class MergeRequest < ApplicationRecord
     end
 
     event :mark_as_checking do
-      transition [:unchecked, :cannot_be_merged_recheck] => :checking
+      transition unchecked: :checking
+      transition cannot_be_merged_recheck: :cannot_be_merged_rechecking
     end
 
     event :mark_as_mergeable do
-      transition [:unchecked, :cannot_be_merged_recheck, :checking] => :can_be_merged
+      transition [:unchecked, :cannot_be_merged_recheck, :checking, :cannot_be_merged_rechecking] => :can_be_merged
     end
 
     event :mark_as_unmergeable do
-      transition [:unchecked, :cannot_be_merged_recheck, :checking] => :cannot_be_merged
+      transition [:unchecked, :cannot_be_merged_recheck, :checking, :cannot_be_merged_rechecking] => :cannot_be_merged
     end
 
     state :unchecked
     state :cannot_be_merged_recheck
     state :checking
+    state :cannot_be_merged_rechecking
     state :can_be_merged
     state :cannot_be_merged
 
@@ -189,7 +191,7 @@ class MergeRequest < ApplicationRecord
     end
 
     # rubocop: disable CodeReuse/ServiceClass
-    after_transition unchecked: :cannot_be_merged do |merge_request, transition|
+    after_transition [:unchecked, :checking] => :cannot_be_merged do |merge_request, transition|
       if merge_request.notify_conflict?
         NotificationService.new.merge_request_unmergeable(merge_request)
         TodoService.new.merge_request_became_unmergeable(merge_request)
@@ -202,6 +204,12 @@ class MergeRequest < ApplicationRecord
     end
   end
 
+  # Returns current merge_status except it returns `cannot_be_merged_rechecking` as `checking`
+  # to avoid exposing unnecessary internal state
+  def public_merge_status
+    cannot_be_merged_rechecking? ? 'checking' : merge_status
+  end
+
   validates :source_project, presence: true, unless: [:allow_broken, :importing?, :closed_without_fork?]
   validates :source_branch, presence: true
   validates :target_project, presence: true

app/serializers/merge_request_basic_entity.rb

 # frozen_string_literal: true
 
 class MergeRequestBasicEntity < Grape::Entity
-  expose :merge_status
+  expose :public_merge_status, as: :merge_status
   expose :merge_error
   expose :state
   expose :source_branch_exists?, as: :source_branch_exists

app/serializers/merge_request_poll_cached_widget_entity.rb

@@ -6,7 +6,7 @@ class MergeRequestPollCachedWidgetEntity < IssuableEntity
   expose :merge_commit_sha
   expose :short_merge_commit_sha
   expose :merge_error
-  expose :merge_status
+  expose :public_merge_status, as: :merge_status
   expose :merge_user_id
   expose :source_branch
   expose :source_project_id

ee/lib/ee/api/entities/approval_state.rb

@@ -5,7 +5,10 @@ module EE
     module Entities
       class ApprovalState < Grape::Entity
         expose :merge_request, merge: true, using: ::API::Entities::IssuableEntity
-        expose(:merge_status) { |approval_state| approval_state.merge_request.merge_status }
+
+        expose(:merge_status) do |approval_state|
+          approval_state.merge_request.public_merge_status
+        end
 
         expose :approved?, as: :approved
 

ee/spec/requests/api/merge_request_approvals_spec.rb

@@ -137,6 +137,17 @@ describe API::MergeRequestApprovals do
         expect(json_response['message']).to eq(nil)
       end
     end
+
+    context 'when merge_status is cannot_be_merged_rechecking' do
+      before do
+        merge_request.update!(merge_status: 'cannot_be_merged_rechecking')
+        get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", user)
+      end
+
+      it 'returns `checking`' do
+        expect(json_response['merge_status']).to eq 'checking'
+      end
+    end
   end
 
   describe 'GET :id/merge_requests/:merge_request_iid/approval_settings' do

lib/api/entities/merge_request_basic.rb

@@ -52,7 +52,7 @@ module API
       # information.
       expose :merge_status do |merge_request|
         merge_request.check_mergeability(async: true)
-        merge_request.merge_status
+        merge_request.public_merge_status
       end
       expose :diff_head_sha, as: :sha
       expose :merge_commit_sha

lib/gitlab/data_builder/pipeline.rb

@@ -45,7 +45,7 @@ module Gitlab
           target_branch: merge_request.target_branch,
           target_project_id: merge_request.target_project_id,
           state: merge_request.state,
-          merge_status: merge_request.merge_status,
+          merge_status: merge_request.public_merge_status,
           url: Gitlab::UrlBuilder.build(merge_request)
         }
       end

spec/lib/gitlab/data_builder/pipeline_spec.rb

@@ -83,7 +83,7 @@ describe Gitlab::DataBuilder::Pipeline do
         expect(merge_request_attrs[:target_branch]).to eq(merge_request.target_branch)
         expect(merge_request_attrs[:target_project_id]).to eq(merge_request.target_project_id)
         expect(merge_request_attrs[:state]).to eq(merge_request.state)
-        expect(merge_request_attrs[:merge_status]).to eq(merge_request.merge_status)
+        expect(merge_request_attrs[:merge_status]).to eq(merge_request.public_merge_status)
         expect(merge_request_attrs[:url]).to eq("http://localhost/#{merge_request.target_project.full_path}/-/merge_requests/#{merge_request.iid}")
       end
     end

spec/models/merge_request_spec.rb

@@ -2335,6 +2335,21 @@ describe MergeRequest do
     end
   end
 
+  describe "#public_merge_status" do
+    using RSpec::Parameterized::TableSyntax
+    subject { build(:merge_request, merge_status: status) }
+
+    where(:status, :public_status) do
+      'cannot_be_merged_rechecking' | 'checking'
+      'checking'                    | 'checking'
+      'cannot_be_merged'            | 'cannot_be_merged'
+    end
+
+    with_them do
+      it { expect(subject.public_merge_status).to eq(public_status) }
+    end
+  end
+
   describe "#head_pipeline_active? " do
     it do
       is_expected
@@ -3226,20 +3241,51 @@ describe MergeRequest do
             expect(notification_service).to receive(:merge_request_unmergeable).with(subject).once
             expect(todo_service).to receive(:merge_request_became_unmergeable).with(subject).once
 
-            subject.mark_as_unmergeable
-            subject.mark_as_unchecked
-            subject.mark_as_unmergeable
+            subject.mark_as_unmergeable!
+
+            subject.mark_as_unchecked!
+            subject.mark_as_unmergeable!
+          end
+
+          it 'notifies conflict, but does not notify again if rechecking still results in cannot_be_merged with async mergeability check' do
+            expect(notification_service).to receive(:merge_request_unmergeable).with(subject).once
+            expect(todo_service).to receive(:merge_request_became_unmergeable).with(subject).once
+
+            subject.mark_as_checking!
+            subject.mark_as_unmergeable!
+
+            subject.mark_as_unchecked!
+            subject.mark_as_checking!
+            subject.mark_as_unmergeable!
           end
 
           it 'notifies conflict, whenever newly unmergeable' do
             expect(notification_service).to receive(:merge_request_unmergeable).with(subject).twice
             expect(todo_service).to receive(:merge_request_became_unmergeable).with(subject).twice
 
-            subject.mark_as_unmergeable
-            subject.mark_as_unchecked
-            subject.mark_as_mergeable
-            subject.mark_as_unchecked
-            subject.mark_as_unmergeable
+            subject.mark_as_unmergeable!
+
+            subject.mark_as_unchecked!
+            subject.mark_as_mergeable!
+
+            subject.mark_as_unchecked!
+            subject.mark_as_unmergeable!
+          end
+
+          it 'notifies conflict, whenever newly unmergeable with async mergeability check' do
+            expect(notification_service).to receive(:merge_request_unmergeable).with(subject).twice
+            expect(todo_service).to receive(:merge_request_became_unmergeable).with(subject).twice
+
+            subject.mark_as_checking!
+            subject.mark_as_unmergeable!
+
+            subject.mark_as_unchecked!
+            subject.mark_as_checking!
+            subject.mark_as_mergeable!
+
+            subject.mark_as_unchecked!
+            subject.mark_as_checking!
+            subject.mark_as_unmergeable!
           end
 
           it 'does not notify whenever merge request is newly unmergeable due to other reasons' do
@@ -3248,7 +3294,7 @@ describe MergeRequest do
             expect(notification_service).not_to receive(:merge_request_unmergeable)
             expect(todo_service).not_to receive(:merge_request_became_unmergeable)
 
-            subject.mark_as_unmergeable
+            subject.mark_as_unmergeable!
           end
         end
       end
@@ -3261,7 +3307,7 @@ describe MergeRequest do
             expect(notification_service).not_to receive(:merge_request_unmergeable)
             expect(todo_service).not_to receive(:merge_request_became_unmergeable)
 
-            subject.mark_as_unmergeable
+            subject.mark_as_unmergeable!
           end
         end
       end

spec/requests/api/graphql/project/merge_request_spec.rb

@@ -130,4 +130,15 @@ describe 'getting merge request information nested in a project' do
       expect(merge_requests_graphql_data.size).to eq 2
     end
   end
+
+  context 'when merge request is cannot_be_merged_rechecking' do
+    before do
+      merge_request.update!(merge_status: 'cannot_be_merged_rechecking')
+    end
+
+    it 'returns checking' do
+      post_graphql(query, current_user: current_user)
+      expect(merge_request_graphql_data['mergeStatus']).to eq('checking')
+    end
+  end
 end

spec/requests/api/merge_requests_spec.rb

@@ -1060,6 +1060,14 @@ describe API::MergeRequests do
       expect(json_response['user']['can_merge']).to be_falsy
     end
 
+    it 'returns `checking` as its merge_status instead of `cannot_be_merged_rechecking`' do
+      merge_request.update!(merge_status: 'cannot_be_merged_rechecking')
+
+      get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user)
+
+      expect(json_response['merge_status']).to eq 'checking'
+    end
+
     context 'when merge request is unchecked' do
       before do
         merge_request.mark_as_unchecked!

spec/serializers/merge_request_basic_entity_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequestBasicEntity do
+  let(:resource) { build(:merge_request) }
+
+  subject do
+    described_class.new(resource).as_json
+  end
+
+  it 'has public_merge_status as merge_status' do
+    expect(resource).to receive(:public_merge_status).and_return('checking')
+
+    expect(subject[:merge_status]).to eq 'checking'
+  end
+end

spec/serializers/merge_request_poll_cached_widget_entity_spec.rb

@@ -19,6 +19,12 @@ describe MergeRequestPollCachedWidgetEntity do
     is_expected.to include(:target_branch_sha)
   end
 
+  it 'has public_merge_status as merge_status' do
+    expect(resource).to receive(:public_merge_status).and_return('checking')
+
+    expect(subject[:merge_status]).to eq 'checking'
+  end
+
   describe 'diverged_commits_count' do
     context 'when MR open and its diverging' do
       it 'returns diverged commits count' do