From 00a273d3a90975d22f39b142fdb85c06779d7b63 Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Tue, 11 Jun 2019 13:08:25 -0300 Subject: Revert "Automatically update MR merge-ref along merge status" --- spec/models/merge_request_spec.rb | 87 +++++++++- spec/requests/api/merge_requests_spec.rb | 61 +++---- .../merge_requests/merge_to_ref_service_spec.rb | 41 ++++- .../mergeability_check_service_spec.rb | 187 --------------------- spec/services/service_response_spec.rb | 16 -- 5 files changed, 141 insertions(+), 251 deletions(-) delete mode 100644 spec/services/merge_requests/mergeability_check_service_spec.rb (limited to 'spec') diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index fc28c216b21..c6251326c22 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1996,6 +1996,57 @@ describe MergeRequest do end end + describe '#check_if_can_be_merged' do + let(:project) { create(:project, only_allow_merge_if_pipeline_succeeds: true) } + + shared_examples 'checking if can be merged' do + context 'when it is not broken and has no conflicts' do + before do + allow(subject).to receive(:broken?) { false } + allow(project.repository).to receive(:can_be_merged?).and_return(true) + end + + it 'is marked as mergeable' do + expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('can_be_merged') + end + end + + context 'when broken' do + before do + allow(subject).to receive(:broken?) { true } + allow(project.repository).to receive(:can_be_merged?).and_return(false) + end + + it 'becomes unmergeable' do + expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged') + end + end + + context 'when it has conflicts' do + before do + allow(subject).to receive(:broken?) { false } + allow(project.repository).to receive(:can_be_merged?).and_return(false) + end + + it 'becomes unmergeable' do + expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged') + end + end + end + + context 'when merge_status is unchecked' do + subject { create(:merge_request, source_project: project, merge_status: :unchecked) } + + it_behaves_like 'checking if can be merged' + end + + context 'when merge_status is unchecked' do + subject { create(:merge_request, source_project: project, merge_status: :cannot_be_merged_recheck) } + + it_behaves_like 'checking if can be merged' + end + end + describe '#mergeable?' do let(:project) { create(:project) } @@ -2009,7 +2060,7 @@ describe MergeRequest do it 'return true if #mergeable_state? is true and the MR #can_be_merged? is true' do allow(subject).to receive(:mergeable_state?) { true } - expect(subject).to receive(:check_mergeability) + expect(subject).to receive(:check_if_can_be_merged) expect(subject).to receive(:can_be_merged?) { true } expect(subject.mergeable?).to be_truthy @@ -2023,7 +2074,7 @@ describe MergeRequest do it 'checks if merge request can be merged' do allow(subject).to receive(:mergeable_ci_state?) { true } - expect(subject).to receive(:check_mergeability) + expect(subject).to receive(:check_if_can_be_merged) subject.mergeable? end @@ -3091,6 +3142,38 @@ describe MergeRequest do end end + describe '#mergeable_to_ref?' do + it 'returns true when merge request is mergeable' do + subject = create(:merge_request) + + expect(subject.mergeable_to_ref?).to be(true) + end + + it 'returns false when merge request is already merged' do + subject = create(:merge_request, :merged) + + expect(subject.mergeable_to_ref?).to be(false) + end + + it 'returns false when merge request is closed' do + subject = create(:merge_request, :closed) + + expect(subject.mergeable_to_ref?).to be(false) + end + + it 'returns false when merge request is work in progress' do + subject = create(:merge_request, title: 'WIP: The feature') + + expect(subject.mergeable_to_ref?).to be(false) + end + + it 'returns false when merge request has no commits' do + subject = create(:merge_request, source_branch: 'empty-branch', target_branch: 'master') + + expect(subject.mergeable_to_ref?).to be(false) + end + end + describe '#merge_participants' do it 'contains author' do expect(subject.merge_participants).to eq([subject.author]) diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 4cb4fcc890d..9f9180bc8c9 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -1546,65 +1546,52 @@ describe API::MergeRequests do end end - describe "GET /projects/:id/merge_requests/:merge_request_iid/merge_ref" do - before do - merge_request.mark_as_unchecked! - end - - let(:merge_request_iid) { merge_request.iid } - + describe "PUT /projects/:id/merge_requests/:merge_request_iid/merge_to_ref" do + let(:pipeline) { create(:ci_pipeline_without_jobs) } let(:url) do - "/projects/#{project.id}/merge_requests/#{merge_request_iid}/merge_ref" + "/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge_to_ref" end it 'returns the generated ID from the merge service in case of success' do - get api(url, user) + put api(url, user), params: { merge_commit_message: 'Custom message' } + + commit = project.commit(json_response['commit_id']) expect(response).to have_gitlab_http_status(200) - expect(json_response['commit_id']).to eq(merge_request.merge_ref_head.sha) + expect(json_response['commit_id']).to be_present + expect(commit.message).to eq('Custom message') end it "returns 400 if branch can't be merged" do - merge_request.update!(merge_status: 'cannot_be_merged') + merge_request.update!(state: 'merged') - get api(url, user) + put api(url, user) expect(response).to have_gitlab_http_status(400) - expect(json_response['message']).to eq('Merge request is not mergeable') + expect(json_response['message']) + .to eq("Merge request is not mergeable to #{merge_request.merge_ref_path}") end - context 'when user has no access to the MR' do - let(:project) { create(:project, :private) } - let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } - - it 'returns 404' do - project.add_guest(user) + it 'returns 403 if user has no permissions to merge to the ref' do + user2 = create(:user) + project.add_reporter(user2) - get api(url, user) + put api(url, user2) - expect(response).to have_gitlab_http_status(404) - expect(json_response['message']).to eq('404 Not found') - end + expect(response).to have_gitlab_http_status(403) + expect(json_response['message']).to eq('403 Forbidden') end - context 'when invalid merge request IID' do - let(:merge_request_iid) { '12345' } - - it 'returns 404' do - get api(url, user) + it 'returns 404 for an invalid merge request IID' do + put api("/projects/#{project.id}/merge_requests/12345/merge_to_ref", user) - expect(response).to have_gitlab_http_status(404) - end + expect(response).to have_gitlab_http_status(404) end - context 'when merge request ID is used instead IID' do - let(:merge_request_iid) { merge_request.id } - - it 'returns 404' do - get api(url, user) + it "returns 404 if the merge request id is used instead of iid" do + put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user) - expect(response).to have_gitlab_http_status(404) - end + expect(response).to have_gitlab_http_status(404) end end diff --git a/spec/services/merge_requests/merge_to_ref_service_spec.rb b/spec/services/merge_requests/merge_to_ref_service_spec.rb index 5d492e4b013..0ac23050caf 100644 --- a/spec/services/merge_requests/merge_to_ref_service_spec.rb +++ b/spec/services/merge_requests/merge_to_ref_service_spec.rb @@ -32,8 +32,10 @@ describe MergeRequests::MergeToRefService do expect(result[:status]).to eq(:success) expect(result[:commit_id]).to be_present + expect(result[:source_id]).to eq(merge_request.source_branch_sha) + expect(result[:target_id]).to eq(merge_request.target_branch_sha) expect(repository.ref_exists?(target_ref)).to be(true) - expect(ref_head.sha).to eq(result[:commit_id]) + expect(ref_head.id).to eq(result[:commit_id]) end end @@ -70,6 +72,10 @@ describe MergeRequests::MergeToRefService do let(:merge_request) { create(:merge_request, :simple) } let(:project) { merge_request.project } + before do + project.add_maintainer(user) + end + describe '#execute' do let(:service) do described_class.new(project, user, commit_message: 'Awesome message', @@ -86,12 +92,6 @@ describe MergeRequests::MergeToRefService do it_behaves_like 'successfully evaluates pre-condition checks' context 'commit history comparison with regular MergeService' do - before do - # The merge service needs an authorized user while merge-to-ref - # doesn't. - project.add_maintainer(user) - end - let(:merge_ref_service) do described_class.new(project, user, {}) end @@ -136,9 +136,9 @@ describe MergeRequests::MergeToRefService do let(:merge_method) { :merge } it 'returns error' do - allow(project).to receive_message_chain(:repository, :merge_to_ref) { nil } + allow(merge_request).to receive(:mergeable_to_ref?) { false } - error_message = 'Conflicts detected during merge' + error_message = "Merge request is not mergeable to #{merge_request.merge_ref_path}" result = service.execute(merge_request) @@ -170,5 +170,28 @@ describe MergeRequests::MergeToRefService do it { expect(todo).not_to be_done } end + + context 'when merge request is WIP state' do + it 'fails to merge' do + merge_request = create(:merge_request, title: 'WIP: The feature') + + result = service.execute(merge_request) + + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq("Merge request is not mergeable to #{merge_request.merge_ref_path}") + end + end + + it 'returns error when user has no authorization to admin the merge request' do + unauthorized_user = create(:user) + project.add_reporter(unauthorized_user) + + service = described_class.new(project, unauthorized_user) + + result = service.execute(merge_request) + + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq('You are not allowed to merge to this ref') + end end end diff --git a/spec/services/merge_requests/mergeability_check_service_spec.rb b/spec/services/merge_requests/mergeability_check_service_spec.rb deleted file mode 100644 index aa0485467ed..00000000000 --- a/spec/services/merge_requests/mergeability_check_service_spec.rb +++ /dev/null @@ -1,187 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe MergeRequests::MergeabilityCheckService do - shared_examples_for 'unmergeable merge request' do - it 'updates or keeps merge status as cannot_be_merged' do - subject - - expect(merge_request.merge_status).to eq('cannot_be_merged') - end - - it 'does not change the merge ref HEAD' do - expect { subject }.not_to change(merge_request, :merge_ref_head) - end - - it 'returns ServiceResponse.error' do - result = subject - - expect(result).to be_a(ServiceResponse) - expect(result).to be_error - end - end - - shared_examples_for 'mergeable merge request' do - it 'updates or keeps merge status as can_be_merged' do - subject - - expect(merge_request.merge_status).to eq('can_be_merged') - end - - it 'updates the merge ref' do - expect { subject }.to change(merge_request, :merge_ref_head).from(nil) - end - - it 'returns ServiceResponse.success' do - result = subject - - expect(result).to be_a(ServiceResponse) - expect(result).to be_success - end - - it 'ServiceResponse has merge_ref_head payload' do - result = subject - - expect(result.payload.keys).to contain_exactly(:merge_ref_head) - expect(result.payload[:merge_ref_head].keys) - .to contain_exactly(:commit_id, :target_id, :source_id) - end - end - - describe '#execute' do - let(:project) { create(:project, :repository) } - let(:merge_request) { create(:merge_request, merge_status: :unchecked, source_project: project, target_project: project) } - let(:repo) { project.repository } - - subject { described_class.new(merge_request).execute } - - before do - project.add_developer(merge_request.author) - end - - it_behaves_like 'mergeable merge request' - - context 'when multiple calls to the service' do - it 'returns success' do - subject - result = subject - - expect(result).to be_a(ServiceResponse) - expect(result.success?).to be(true) - end - - it 'second call does not change the merge-ref' do - expect { subject }.to change(merge_request, :merge_ref_head).from(nil) - expect { subject }.not_to change(merge_request, :merge_ref_head) - end - end - - context 'when broken' do - before do - allow(merge_request).to receive(:broken?) { true } - allow(project.repository).to receive(:can_be_merged?) { false } - end - - it_behaves_like 'unmergeable merge request' - - it 'returns ServiceResponse.error' do - result = subject - - expect(result).to be_a(ServiceResponse) - expect(result.error?).to be(true) - expect(result.message).to eq('Merge request is not mergeable') - end - end - - context 'when it has conflicts' do - before do - allow(merge_request).to receive(:broken?) { false } - allow(project.repository).to receive(:can_be_merged?) { false } - end - - it_behaves_like 'unmergeable merge request' - - it 'returns ServiceResponse.error' do - result = subject - - expect(result).to be_a(ServiceResponse) - expect(result.error?).to be(true) - expect(result.message).to eq('Merge request is not mergeable') - end - end - - context 'when MR cannot be merged and has no merge ref' do - before do - merge_request.mark_as_unmergeable! - end - - it_behaves_like 'unmergeable merge request' - - it 'returns ServiceResponse.error' do - result = subject - - expect(result).to be_a(ServiceResponse) - expect(result.error?).to be(true) - expect(result.message).to eq('Merge request is not mergeable') - end - end - - context 'when MR cannot be merged and has outdated merge ref' do - before do - MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request) - merge_request.mark_as_unmergeable! - end - - it_behaves_like 'unmergeable merge request' - - it 'returns ServiceResponse.error' do - result = subject - - expect(result).to be_a(ServiceResponse) - expect(result.error?).to be(true) - expect(result.message).to eq('Merge request is not mergeable') - end - end - - context 'when merge request is not given' do - subject { described_class.new(nil).execute } - - it 'returns ServiceResponse.error' do - result = subject - - expect(result).to be_a(ServiceResponse) - expect(result.message).to eq('Invalid argument') - end - end - - context 'when read only DB' do - it 'returns ServiceResponse.error' do - allow(Gitlab::Database).to receive(:read_only?) { true } - - result = subject - - expect(result).to be_a(ServiceResponse) - expect(result.message).to eq('Unsupported operation') - end - end - - context 'when MR is mergeable but merge-ref does not exists' do - before do - merge_request.mark_as_mergeable! - end - - it 'keeps merge status as can_be_merged' do - expect { subject }.not_to change(merge_request, :merge_status).from('can_be_merged') - end - - it 'returns ServiceResponse.error' do - result = subject - - expect(result).to be_a(ServiceResponse) - expect(result.error?).to be(true) - expect(result.message).to eq('Merge ref was not found') - end - end - end -end diff --git a/spec/services/service_response_spec.rb b/spec/services/service_response_spec.rb index e790d272e61..30bd4d6820b 100644 --- a/spec/services/service_response_spec.rb +++ b/spec/services/service_response_spec.rb @@ -16,13 +16,6 @@ describe ServiceResponse do expect(response).to be_success expect(response.message).to eq('Good orange') end - - it 'creates a successful response with payload' do - response = described_class.success(payload: { good: 'orange' }) - - expect(response).to be_success - expect(response.payload).to eq(good: 'orange') - end end describe '.error' do @@ -40,15 +33,6 @@ describe ServiceResponse do expect(response.message).to eq('Bad apple') expect(response.http_status).to eq(400) end - - it 'creates a failed response with payload' do - response = described_class.error(message: 'Bad apple', - payload: { bad: 'apple' }) - - expect(response).to be_error - expect(response.message).to eq('Bad apple') - expect(response.payload).to eq(bad: 'apple') - end end describe '#success?' do -- cgit v1.2.1