From 7517105303c264484d8677c81268f9f43ecc5593 Mon Sep 17 00:00:00 2001 From: lulalala Date: Wed, 28 Mar 2018 11:20:12 +0800 Subject: Add cannot_be_merged_recheck merge_status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First, transitions between can_be_merged & cannot_be_merged are removed, as they are currently blocked in `check_if_can_be_merged`. `can_be_merge` always returns to `unchecked` first, before it can transition to `cannot_be_merged` (and vice versa). We want to avoid repeated notification triggered by repeated transition between `cannot_be_merged` & `unchecked`. So we added `cannot_be_merged_recheck` state, similar to `unchecked`, but as a mean to remember it’s from cannot_be_merged. See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/18042/#note_65945407 Since `unchecked` and `cannot_be_merged_recheck` both mean “we are in the middle of checking if it is mergeable”, quite often we need to see if merge_status is in either one of them, so `check_state?` is added to achieve this. --- .../merge_requests/conflicts_controller_spec.rb | 2 +- .../projects/merge_requests_controller_spec.rb | 2 +- .../merge_request/user_resolves_conflicts_spec.rb | 2 +- spec/models/merge_request_spec.rb | 47 ++++++++++++++++++++++ .../merge_requests/conflicts/list_service_spec.rb | 2 +- 5 files changed, 51 insertions(+), 4 deletions(-) (limited to 'spec') diff --git a/spec/controllers/projects/merge_requests/conflicts_controller_spec.rb b/spec/controllers/projects/merge_requests/conflicts_controller_spec.rb index 2d7647a6e12..397cc79bde4 100644 --- a/spec/controllers/projects/merge_requests/conflicts_controller_spec.rb +++ b/spec/controllers/projects/merge_requests/conflicts_controller_spec.rb @@ -5,7 +5,7 @@ describe Projects::MergeRequests::ConflictsController do let(:user) { project.owner } let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) } let(:merge_request_with_conflicts) do - create(:merge_request, source_branch: 'conflict-resolvable', target_branch: 'conflict-start', source_project: project) do |mr| + create(:merge_request, source_branch: 'conflict-resolvable', target_branch: 'conflict-start', source_project: project, merge_status: :unchecked) do |mr| mr.mark_as_unmergeable end end diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index c8cc6b374f6..d3042be9e8b 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -7,7 +7,7 @@ describe Projects::MergeRequestsController do let(:user) { project.owner } let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) } let(:merge_request_with_conflicts) do - create(:merge_request, source_branch: 'conflict-resolvable', target_branch: 'conflict-start', source_project: project) do |mr| + create(:merge_request, source_branch: 'conflict-resolvable', target_branch: 'conflict-start', source_project: project, merge_status: :unchecked) do |mr| mr.mark_as_unmergeable end end diff --git a/spec/features/merge_request/user_resolves_conflicts_spec.rb b/spec/features/merge_request/user_resolves_conflicts_spec.rb index 19995559fae..ac3c734599c 100644 --- a/spec/features/merge_request/user_resolves_conflicts_spec.rb +++ b/spec/features/merge_request/user_resolves_conflicts_spec.rb @@ -10,7 +10,7 @@ describe 'Merge request > User resolves conflicts', :js do end def create_merge_request(source_branch) - create(:merge_request, source_branch: source_branch, target_branch: 'conflict-start', source_project: project) do |mr| + create(:merge_request, source_branch: source_branch, target_branch: 'conflict-start', source_project: project, merge_status: :unchecked) do |mr| mr.mark_as_unmergeable end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 04379e7d2c3..bcc836acefa 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -2064,6 +2064,53 @@ describe MergeRequest do expect(subject.merge_jid).to be_nil end end + + describe 'transition to cannot_be_merged' do + let(:notification_service) { double(:notification_service) } + let(:todo_service) { double(:todo_service) } + + subject { create(:merge_request, merge_status: :unchecked) } + + before do + allow(NotificationService).to receive(:new).and_return(notification_service) + allow(TodoService).to receive(:new).and_return(todo_service) + end + + it 'notifies, but does not notify again if rechecking still results in cannot_be_merged' 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 + end + + it 'notifies whenever merge request is 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 + end + end + + describe 'check_state?' do + it 'indicates whether MR is still checking for mergeability' do + state_machine = described_class.state_machines[:merge_status] + check_states = [:unchecked, :cannot_be_merged_recheck] + + check_states.each do |merge_status| + expect(state_machine.check_state?(merge_status)).to be true + end + + (state_machine.states.map(&:name) - check_states).each do |merge_status| + expect(state_machine.check_state?(merge_status)).to be false + end + end + end end describe '#should_be_rebased?' do diff --git a/spec/services/merge_requests/conflicts/list_service_spec.rb b/spec/services/merge_requests/conflicts/list_service_spec.rb index 837b8a56d12..d57852615d9 100644 --- a/spec/services/merge_requests/conflicts/list_service_spec.rb +++ b/spec/services/merge_requests/conflicts/list_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe MergeRequests::Conflicts::ListService do describe '#can_be_resolved_in_ui?' do def create_merge_request(source_branch) - create(:merge_request, source_branch: source_branch, target_branch: 'conflict-start') do |mr| + create(:merge_request, source_branch: source_branch, target_branch: 'conflict-start', merge_status: :unchecked) do |mr| mr.mark_as_unmergeable end end -- cgit v1.2.1