diff --git a/app/finders/merge_requests_finder.rb b/app/finders/merge_requests_finder.rb index c19d8b68f605a59e909ba788b88f1deac67385d8..a33030e8b9a1bfc4245395f04da51b75994d769c 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 578d56e90494595803f2d71a7681152aa1bfb392..9d5df4ecd67a35ebb0be7def3bf692dd29a99df5 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 88a86258b0aa996ce772152b868d1aed14cd635a..9fe2a2df86e25c9d1a1eac6f82a6c0c535964e12 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/models/state_note.rb b/app/models/state_note.rb index 8b1474b3c7a0a5bf1f3df908f019c7eef4733950..b203f446b82304d28a6f2d4970c8fb7b8c931fba 100644 --- a/app/models/state_note.rb +++ b/app/models/state_note.rb @@ -28,6 +28,8 @@ def note_text(html: false) 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 diff --git a/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb index d2bfadc220557c12e2378e9d6287b559dd1d4644..719ce57942a204f456943d90e9ca8045f765fa1b 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 7a7d0dbfef2cbc7c7e8b87d2d0d8541245d08bbd..81ce2488492bc9e23c013d7aa6aad4a5690ae376 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -98,9 +98,24 @@ def post_merge_manually_merged merge_request.merge_commit_sha = sha merge_request.merged_commit_sha = 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 + ).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 ed9324cbd5c4a7398cc8055287a3a560146a3008..1db1644f09aed3b1aa97a0e2a7003c4cfd541402 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 diff --git a/spec/finders/merge_requests_finder_spec.rb b/spec/finders/merge_requests_finder_spec.rb index d85ae92f95413eb5368209773af6f97d83bf5a19..d5f8fc81f422e7059ae1170e1aec770716f8338b 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 a0b887655b2693b0dc6a303d5e240b414ed4b1a4..57914866c286e64b6e2252f30e0333be57eae97f 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 5a3f21631cae04640dff61874cfb4b224b0f8787..951676ffe4305d3897f5844780a798c5672a6579 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/models/state_note_spec.rb b/spec/models/state_note_spec.rb index 0afdf6bbcb912f27db0d42fb46e8ae0fd209e248..de07403bdc887f6c1023e3bcc4d4d20b2acb0eb1 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) diff --git a/spec/services/merge_requests/post_merge_service_spec.rb b/spec/services/merge_requests/post_merge_service_spec.rb index 61c754e30a9b09239713a5af03580b650993a764..864ffd0ebfc6c4ec7881ccbcc85cba99742f7369 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 diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 79c3e4c0bb8fcc219a336fff4ac82027fb3a2496..a956fa4d66c48da11068bc00194e55a9ec8881ed 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