Skip to content
Snippets Groups Projects
Verified Commit 95a5e7dc authored by Leaminn Ma's avatar Leaminn Ma :two: Committed by GitLab
Browse files

Merge branch '436061-improve-messaging-around-assumed-merged-mrs' into 'master'

Add extra information to state change notes

See merge request !148147



Merged-by: default avatarLeaminn Ma <lma@gitlab.com>
Approved-by: default avatarDmitry Gruzd <dgruzd@gitlab.com>
Approved-by: default avatarAmy Qualls <aqualls@gitlab.com>
Approved-by: default avatarKerri Miller <kerrizor@kerrizor.com>
Approved-by: default avatarDylan Griffith <dyl.griffith@gmail.com>
Approved-by: default avatarLeaminn Ma <lma@gitlab.com>
Reviewed-by: default avatarSincheol (David) Kim <dkim@gitlab.com>
Co-authored-by: default avatarDavid Kim <dkim@gitlab.com>
parents aeb55a04 74389c2b
No related branches found
No related tags found
1 merge request!148147Add extra information to state change notes
Pipeline #1250207441 passed
Showing
with 200 additions and 12 deletions
......@@ -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) })
......
......@@ -481,6 +481,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(
......
......@@ -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
......
......@@ -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
......
......@@ -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)
......
......@@ -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
......
......@@ -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
......
......@@ -334,6 +334,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' }
......
......@@ -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
......
......@@ -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
......@@ -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)
......
......@@ -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
......@@ -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
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment