Commit 4561535a authored by David Kim's avatar David Kim Committed by Jan Provaznik

Notify new merge Request conflicts only once

parent d9dd0e5b
......@@ -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'
......
......@@ -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
......
# 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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......@@ -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!
......
# 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
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment