From 914092e9feeef4410d0c1d4bb1072b77d9369bb4 Mon Sep 17 00:00:00 2001
From: David Kim <dkim@gitlab.com>
Date: Thu, 28 Mar 2024 19:21:57 +1100
Subject: [PATCH 1/4] Improve system note messaging for assumed merges

- Mark MRs as merged by other MR if other MR contains the diff_head_sha
- Mark MRs as manually merged when there is no other MR that contains the
  diff_head_sha

Changelog: changed
---
 app/models/state_note.rb                           | 10 +++++++++-
 app/services/merge_requests/post_merge_service.rb  | 14 ++++++++++++--
 app/services/merge_requests/refresh_service.rb     | 13 ++++++++++++-
 .../ee/merge_requests/post_merge_service.rb        |  2 +-
 4 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/app/models/state_note.rb b/app/models/state_note.rb
index 8b1474b3c7a0a5..4d97187c2aa5e3 100644
--- a/app/models/state_note.rb
+++ b/app/models/state_note.rb
@@ -29,7 +29,15 @@ def note_text(html: false)
     end
 
     body = event.state.dup
-    body << " via #{event_source.gfm_reference(project)}" if event_source
+
+    if event_source
+      body << if event.state == 'merged' && event_source.is_a?(Commit)
+                " manually"
+              else
+                " via #{event_source.gfm_reference(project)}"
+              end
+    end
+
     body
   end
 
diff --git a/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb
index d2bfadc220557c..719ce57942a204 100644
--- a/app/services/merge_requests/post_merge_service.rb
+++ b/app/services/merge_requests/post_merge_service.rb
@@ -11,7 +11,7 @@ class PostMergeService < MergeRequests::BaseService
 
     MAX_RETARGET_MERGE_REQUESTS = 4
 
-    def execute(merge_request)
+    def execute(merge_request, source = nil)
       return if merge_request.merged?
 
       # Mark the merge request as merged, everything that happens afterwards is
@@ -23,7 +23,7 @@ def execute(merge_request)
 
       merge_request_activity_counter.track_merge_mr_action(user: current_user)
 
-      create_note(merge_request)
+      create_note(merge_request, source)
       close_issues(merge_request)
       notification_service.merge_mr(merge_request, current_user)
       invalidate_cache_counts(merge_request, users: merge_request.assignees | merge_request.reviewers)
@@ -37,6 +37,16 @@ def execute(merge_request)
       execute_hooks(merge_request, 'merge')
     end
 
+    def create_note(merge_request, source)
+      SystemNoteService.change_status(
+        merge_request,
+        merge_request.target_project,
+        current_user,
+        merge_request.state,
+        source
+      )
+    end
+
     private
 
     def close_issues(merge_request)
diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb
index 7a7d0dbfef2cbc..0a53f45202279f 100644
--- a/app/services/merge_requests/refresh_service.rb
+++ b/app/services/merge_requests/refresh_service.rb
@@ -98,9 +98,20 @@ def post_merge_manually_merged
         merge_request.merge_commit_sha = sha
         merge_request.merged_commit_sha = sha
 
+        # Look for an MR that would've merged the sha
+        source_merge_request = MergeRequestsFinder.new(
+          @current_user,
+          project_id: @project.id,
+          state: 'merged',
+          sort: 'merged_at',
+          commit_sha: sha
+        ).execute.first
+
+        source = source_merge_request || @project.commit(sha)
+
         MergeRequests::PostMergeService
           .new(project: merge_request.target_project, current_user: @current_user)
-          .execute(merge_request)
+          .execute(merge_request, source)
       end
     end
     # rubocop: enable CodeReuse/ActiveRecord
diff --git a/ee/app/services/ee/merge_requests/post_merge_service.rb b/ee/app/services/ee/merge_requests/post_merge_service.rb
index ed9324cbd5c4a7..1db1644f09aed3 100644
--- a/ee/app/services/ee/merge_requests/post_merge_service.rb
+++ b/ee/app/services/ee/merge_requests/post_merge_service.rb
@@ -6,7 +6,7 @@ module PostMergeService
       extend ::Gitlab::Utils::Override
 
       override :execute
-      def execute(merge_request)
+      def execute(merge_request, source = nil)
         super
         ApprovalRules::FinalizeService.new(merge_request).execute
 
-- 
GitLab


From 4b1ed369b5f8b41f824392e99e65246d31c4877b Mon Sep 17 00:00:00 2001
From: David Kim <dkim@gitlab.com>
Date: Fri, 5 Apr 2024 17:52:48 +1100
Subject: [PATCH 2/4] Add spec to cover merge_requests notes

---
 app/models/state_note.rb       | 12 +++-------
 spec/models/state_note_spec.rb | 42 +++++++++++++++++++++++++++++-----
 2 files changed, 39 insertions(+), 15 deletions(-)

diff --git a/app/models/state_note.rb b/app/models/state_note.rb
index 4d97187c2aa5e3..b203f446b82304 100644
--- a/app/models/state_note.rb
+++ b/app/models/state_note.rb
@@ -28,16 +28,10 @@ def note_text(html: false)
       end
     end
 
-    body = event.state.dup
-
-    if event_source
-      body << if event.state == 'merged' && event_source.is_a?(Commit)
-                " manually"
-              else
-                " via #{event_source.gfm_reference(project)}"
-              end
-    end
+    return "merged manually" if event.state == 'merged' && event_source.is_a?(Commit)
 
+    body = event.state.dup
+    body << " via #{event_source.gfm_reference(project)}" if event_source
     body
   end
 
diff --git a/spec/models/state_note_spec.rb b/spec/models/state_note_spec.rb
index 0afdf6bbcb912f..de07403bdc887f 100644
--- a/spec/models/state_note_spec.rb
+++ b/spec/models/state_note_spec.rb
@@ -2,15 +2,17 @@
 
 require 'spec_helper'
 
-RSpec.describe StateNote do
+RSpec.describe StateNote, feature_category: :team_planning do
   describe '.from_event' do
     let_it_be(:author) { create(:user) }
     let_it_be(:project) { create(:project, :repository) }
     let_it_be(:noteable) { create(:issue, author: author, project: project) }
 
+    let(:event) { create(:resource_state_event, **event_args) }
+
     ResourceStateEvent.states.each do |state, _value|
       context "with event state #{state}" do
-        let(:event) { create(:resource_state_event, issue: noteable, state: state, created_at: '2020-02-05') }
+        let(:event_args) { { issue: noteable, state: state, created_at: '2020-02-05' } }
 
         subject { described_class.from_event(event, resource: noteable, resource_parent: project) }
 
@@ -30,7 +32,7 @@
 
       context 'with a commit' do
         let(:commit) { create(:commit, project: project) }
-        let(:event) { create(:resource_state_event, issue: noteable, state: :closed, created_at: '2020-02-05', source_commit: commit.id) }
+        let(:event_args) { { issue: noteable, state: :closed, created_at: '2020-02-05', source_commit: commit.id } }
 
         it 'contains the expected values' do
           expect(subject.author).to eq(author)
@@ -42,7 +44,7 @@
 
       context 'with a merge request' do
         let(:merge_request) { create(:merge_request, source_project: project) }
-        let(:event) { create(:resource_state_event, issue: noteable, state: :closed, created_at: '2020-02-05', source_merge_request: merge_request) }
+        let(:event_args) { { issue: noteable, state: :closed, created_at: '2020-02-05', source_merge_request: merge_request } }
 
         it 'contains the expected values' do
           expect(subject.author).to eq(author)
@@ -52,8 +54,36 @@
         end
       end
 
+      context 'when noteable is a merge request', feature_category: :code_review_workflow do
+        let(:noteable) { create(:merge_request, source_project: project, author: author) }
+
+        context 'when merged by a commit' do
+          let(:source_commit) { create(:commit, project: project) }
+          let(:event_args) { { merge_request: noteable, state: :merged, created_at: '2020-02-05', source_commit: source_commit.id, user: author } }
+
+          it 'contains the expected values' do
+            expect(subject.author).to eq(author)
+            expect(subject.created_at).to eq(event.created_at)
+            expect(subject.updated_at).to eq(event.created_at)
+            expect(subject.note).to eq("merged manually")
+          end
+        end
+
+        context 'when merged by another merge_request' do
+          let(:merge_request) { create(:merge_request, :simple, source_project: project) }
+          let(:event_args) { { merge_request: noteable, state: :merged, created_at: '2020-02-05', source_merge_request: merge_request, user: author } }
+
+          it 'contains the expected values' do
+            expect(subject.author).to eq(author)
+            expect(subject.created_at).to eq(event.created_at)
+            expect(subject.updated_at).to eq(event.created_at)
+            expect(subject.note).to eq("merged via merge request !#{merge_request.iid}")
+          end
+        end
+      end
+
       context 'when closed by error tracking' do
-        let(:event) { create(:resource_state_event, issue: noteable, state: :closed, created_at: '2020-02-05', close_after_error_tracking_resolve: true) }
+        let(:event_args) { { issue: noteable, state: :closed, created_at: '2020-02-05', close_after_error_tracking_resolve: true } }
 
         it 'contains the expected values' do
           expect(subject.author).to eq(author)
@@ -64,7 +94,7 @@
       end
 
       context 'when closed by promotheus alert' do
-        let(:event) { create(:resource_state_event, issue: noteable, state: :closed, created_at: '2020-02-05', close_auto_resolve_prometheus_alert: true) }
+        let(:event_args) { { issue: noteable, state: :closed, created_at: '2020-02-05', close_auto_resolve_prometheus_alert: true } }
 
         it 'contains the expected values' do
           expect(subject.author).to eq(author)
-- 
GitLab


From 51b9a4d0ea38e3c701a0c589da48090b205d80c1 Mon Sep 17 00:00:00 2001
From: David Kim <dkim@gitlab.com>
Date: Fri, 5 Apr 2024 18:20:17 +1100
Subject: [PATCH 3/4] Add an extra spec to cover providing an event source

---
 .../merge_requests/post_merge_service_spec.rb     | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/spec/services/merge_requests/post_merge_service_spec.rb b/spec/services/merge_requests/post_merge_service_spec.rb
index 61c754e30a9b09..864ffd0ebfc6c4 100644
--- a/spec/services/merge_requests/post_merge_service_spec.rb
+++ b/spec/services/merge_requests/post_merge_service_spec.rb
@@ -166,4 +166,19 @@
       end
     end
   end
+
+  context 'when event source is given' do
+    let(:source) { create(:merge_request, :simple, source_project: project) }
+
+    subject { described_class.new(project: project, current_user: user).execute(merge_request, source) }
+
+    it 'creates a resource_state_event as expected' do
+      expect { subject }.to change { ResourceStateEvent.count }.by 1
+
+      event = merge_request.resource_state_events.last
+
+      expect(event.state).to eq 'merged'
+      expect(event.source_merge_request).to eq source
+    end
+  end
 end
-- 
GitLab


From f4e2f86f2aee02492d267b0e22896f42fddf5029 Mon Sep 17 00:00:00 2001
From: David Kim <dkim@gitlab.com>
Date: Wed, 10 Apr 2024 19:05:40 +1000
Subject: [PATCH 4/4] Handle merged without event source

---
 app/finders/merge_requests_finder.rb          |  8 ++++
 app/models/merge_request.rb                   |  5 +++
 app/models/resource_state_event.rb            |  4 ++
 .../merge_requests/refresh_service.rb         |  6 ++-
 spec/finders/merge_requests_finder_spec.rb    | 14 +++++++
 spec/models/merge_request_spec.rb             | 33 +++++++++++++++
 spec/models/resource_state_event_spec.rb      | 16 ++++++++
 .../merge_requests/refresh_service_spec.rb    | 40 ++++++++++++++++++-
 8 files changed, 123 insertions(+), 3 deletions(-)

diff --git a/app/finders/merge_requests_finder.rb b/app/finders/merge_requests_finder.rb
index c19d8b68f605a5..a33030e8b9a1bf 100644
--- a/app/finders/merge_requests_finder.rb
+++ b/app/finders/merge_requests_finder.rb
@@ -21,6 +21,7 @@
 #     label_name: string
 #     sort: string
 #     non_archived: boolean
+#     merged_without_event_source: boolean
 #     my_reaction_emoji: string
 #     source_branch: string
 #     target_branch: string
@@ -78,6 +79,7 @@ def filter_items(_items)
     items = by_deployments(items)
     items = by_reviewer(items)
     items = by_source_project_id(items)
+    items = by_resource_event_state(items)
 
     by_approved(items)
   end
@@ -164,6 +166,12 @@ def by_source_project_id(items)
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
+  def by_resource_event_state(items)
+    return items unless params[:merged_without_event_source].present?
+
+    items.merged_without_state_event_source
+  end
+
   # rubocop: disable CodeReuse/ActiveRecord
   def by_draft(items)
     draft_param = Gitlab::Utils.to_boolean(params.fetch(:draft) { params.fetch(:wip, nil) })
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 578d56e9049459..9d5df4ecd67a35 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -479,6 +479,11 @@ def public_merge_status
     end
   }
 
+  scope :merged_without_state_event_source, -> {
+    joins(:resource_state_events)
+    .merge(ResourceStateEvent.merged_with_no_event_source)
+  }
+
   def self.total_time_to_merge
     join_metrics
       .where(
diff --git a/app/models/resource_state_event.rb b/app/models/resource_state_event.rb
index 88a86258b0aa99..9fe2a2df86e25c 100644
--- a/app/models/resource_state_event.rb
+++ b/app/models/resource_state_event.rb
@@ -13,6 +13,10 @@ class ResourceStateEvent < ResourceEvent
 
   after_create :issue_usage_metrics
 
+  scope :merged_with_no_event_source, -> do
+    where(state: :merged, source_merge_request: nil, source_commit: nil)
+  end
+
   def self.issuable_attrs
     %i[issue merge_request].freeze
   end
diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb
index 0a53f45202279f..81ce2488492bc9 100644
--- a/app/services/merge_requests/refresh_service.rb
+++ b/app/services/merge_requests/refresh_service.rb
@@ -98,10 +98,14 @@ def post_merge_manually_merged
         merge_request.merge_commit_sha = sha
         merge_request.merged_commit_sha = sha
 
-        # Look for an MR that would've merged the sha
+        # Look for a merged MR that includes the SHA to associate it with
+        # the MR we're about to mark as merged.
+        # Only the merged MRs without the event source would be considered
+        # to avoid associating it with other MRs that we may have marked as merged here.
         source_merge_request = MergeRequestsFinder.new(
           @current_user,
           project_id: @project.id,
+          merged_without_event_source: true,
           state: 'merged',
           sort: 'merged_at',
           commit_sha: sha
diff --git a/spec/finders/merge_requests_finder_spec.rb b/spec/finders/merge_requests_finder_spec.rb
index d85ae92f95413e..d5f8fc81f422e7 100644
--- a/spec/finders/merge_requests_finder_spec.rb
+++ b/spec/finders/merge_requests_finder_spec.rb
@@ -336,6 +336,20 @@ def find(label_name)
         expect(merge_requests).to contain_exactly(merge_request1, merge_request2, merge_request3)
       end
 
+      context 'filter by state event source' do
+        let(:params) { { merged_without_event_source: true } }
+
+        before do
+          create(:resource_state_event, merge_request: merge_request1, state: :merged)
+        end
+
+        it 'filters by resource_state_event' do
+          merge_requests = described_class.new(user, params).execute
+
+          expect(merge_requests).to contain_exactly(merge_request1)
+        end
+      end
+
       it 'filters by state' do
         params = { state: 'locked' }
 
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index a0b887655b2693..57914866c286e6 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -238,6 +238,39 @@
         end
       end
     end
+
+    describe '.merged_without_state_event_source' do
+      let(:source_merge_request) { nil }
+      let(:source_commit) { nil }
+
+      subject { described_class.merged_without_state_event_source }
+
+      before do
+        create(:resource_state_event, merge_request: merge_request1, state: :merged, source_merge_request: source_merge_request, source_commit: source_commit)
+      end
+
+      context 'when the state matches and event source is empty' do
+        it 'filters by resource_state_event' do
+          expect(subject).to contain_exactly(merge_request1)
+        end
+      end
+
+      context 'when source_merge_request is not empty' do
+        let(:source_merge_request) { merge_request2 }
+
+        it 'filters by resource_state_event' do
+          expect(subject).to be_empty
+        end
+      end
+
+      context 'when source_commit is not empty' do
+        let(:source_commit) { 'abcd1234' }
+
+        it 'filters by resource_state_event' do
+          expect(subject).to be_empty
+        end
+      end
+    end
   end
 
   describe '#squash?' do
diff --git a/spec/models/resource_state_event_spec.rb b/spec/models/resource_state_event_spec.rb
index 5a3f21631cae04..951676ffe4305d 100644
--- a/spec/models/resource_state_event_spec.rb
+++ b/spec/models/resource_state_event_spec.rb
@@ -100,4 +100,20 @@
       end
     end
   end
+
+  describe '.merged_with_no_event_source', feature_category: :code_review_workflow do
+    let!(:merged_event) { create(:resource_state_event, merge_request: merge_request, state: :merged) }
+
+    before do
+      create(:resource_state_event, merge_request: merge_request, state: :closed)
+      create(:resource_state_event, merge_request: merge_request, state: :merged, source_merge_request: merge_request)
+      create(:resource_state_event, merge_request: merge_request, state: :merged, source_commit: 'abcd1234')
+    end
+
+    subject { described_class.merged_with_no_event_source }
+
+    it 'returns expected events' do
+      expect(subject).to contain_exactly(merged_event)
+    end
+  end
 end
diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb
index 79c3e4c0bb8fcc..a956fa4d66c48d 100644
--- a/spec/services/merge_requests/refresh_service_spec.rb
+++ b/spec/services/merge_requests/refresh_service_spec.rb
@@ -509,8 +509,17 @@
         end
 
         it 'updates the merge state' do
-          expect(@merge_request.resource_state_events.last.state).to eq('merged')
-          expect(@fork_merge_request.resource_state_events.last.state).to eq('merged')
+          commit = @project.repository.commit('feature')
+
+          state_event_1 = @merge_request.resource_state_events.last
+          expect(state_event_1.state).to eq('merged')
+          expect(state_event_1.source_merge_request).to eq(nil)
+          expect(state_event_1.source_commit).to eq(commit.id)
+
+          state_event_2 = @fork_merge_request.resource_state_events.last
+          expect(state_event_2.state).to eq('merged')
+          expect(state_event_2.source_merge_request).to eq(nil)
+          expect(state_event_2.source_commit).to eq(commit.id)
 
           expect(@merge_request).to be_merged
           expect(@merge_request.diffs.size).to be > 0
@@ -519,6 +528,33 @@
           expect(@fork_build_failed_todo).to be_done
         end
       end
+
+      context 'With merged MR that contains the same SHA' do
+        before do
+          # Merged via UI
+          MergeRequests::MergeService
+            .new(project: @merge_request.target_project, current_user: @user, params: { sha: @merge_request.diff_head_sha })
+            .execute(@merge_request)
+
+          commit = @project.repository.commit('feature')
+          service.new(project: @project, current_user: @user).execute(@oldrev, commit.id, 'refs/heads/feature')
+          reload_mrs
+        end
+
+        it 'updates the merge state' do
+          state_event_1 = @merge_request.resource_state_events.last
+          expect(state_event_1.state).to eq('merged')
+          expect(state_event_1.source_merge_request).to eq(nil)
+          expect(state_event_1.source_commit).to eq(nil)
+
+          state_event_2 = @fork_merge_request.resource_state_events.last
+          expect(state_event_2.state).to eq('merged')
+          expect(state_event_2.source_merge_request).to eq(@merge_request)
+          expect(state_event_2.source_commit).to eq(nil)
+
+          expect(@fork_merge_request).to be_merged
+        end
+      end
     end
 
     context 'push to fork repo source branch' do
-- 
GitLab