diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 135117926be..a3ceb76845e 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -33,7 +33,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo def show close_merge_request_if_no_source_project - @merge_request.check_mergeability + mark_merge_request_mergeable respond_to do |format| format.html do @@ -251,6 +251,10 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo @merge_request.has_no_commits? && !@merge_request.target_branch_exists? end + def mark_merge_request_mergeable + @merge_request.check_if_can_be_merged + end + def merge! # Disable the CI check if auto_merge_strategy is specified since we have # to wait until CI completes to know diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 4fcaac75655..7481f1939ad 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -725,16 +725,19 @@ class MergeRequest < ApplicationRecord MergeRequests::ReloadDiffsService.new(self, current_user).execute end - - def check_mergeability - MergeRequests::MergeabilityCheckService.new(self).execute - end # rubocop: enable CodeReuse/ServiceClass - # Returns boolean indicating the merge_status should be rechecked in order to - # switch to either can_be_merged or cannot_be_merged. - def recheck_merge_status? - self.class.state_machines[:merge_status].check_state?(merge_status) + def check_if_can_be_merged + return unless self.class.state_machines[:merge_status].check_state?(merge_status) && Gitlab::Database.read_write? + + can_be_merged = + !broken? && project.repository.can_be_merged?(diff_head_sha, target_branch) + + if can_be_merged + mark_as_mergeable + else + mark_as_unmergeable + end end def merge_event @@ -760,7 +763,7 @@ class MergeRequest < ApplicationRecord def mergeable?(skip_ci_check: false) return false unless mergeable_state?(skip_ci_check: skip_ci_check) - check_mergeability + check_if_can_be_merged can_be_merged? && !should_be_rebased? end @@ -775,6 +778,15 @@ class MergeRequest < ApplicationRecord true end + def mergeable_to_ref? + return false unless mergeable_state?(skip_ci_check: true, skip_discussions_check: true) + + # Given the `merge_ref_path` will have the same + # state the `target_branch` would have. Ideally + # we need to check if it can be merged to it. + project.repository.can_be_merged?(diff_head_sha, target_branch) + end + def ff_merge_possible? project.repository.ancestor?(target_branch_sha, diff_head_sha) end @@ -1087,12 +1099,6 @@ class MergeRequest < ApplicationRecord target_project.repository.fetch_source_branch!(source_project.repository, source_branch, ref_path) end - # Returns the current merge-ref HEAD commit. - # - def merge_ref_head - project.repository.commit(merge_ref_path) - end - def ref_path "refs/#{Repository::REF_MERGE_REQUEST}/#{iid}/head" end diff --git a/app/services/merge_requests/merge_to_ref_service.rb b/app/services/merge_requests/merge_to_ref_service.rb index 8670b9ccf3d..87147d90c32 100644 --- a/app/services/merge_requests/merge_to_ref_service.rb +++ b/app/services/merge_requests/merge_to_ref_service.rb @@ -20,14 +20,20 @@ module MergeRequests raise_error('Conflicts detected during merge') unless commit_id - success(commit_id: commit_id) - rescue MergeError, ArgumentError => error + commit = project.commit(commit_id) + target_id, source_id = commit.parent_ids + + success(commit_id: commit.id, + target_id: target_id, + source_id: source_id) + rescue MergeError => error error(error.message) end private def validate! + authorization_check! error_check! end @@ -37,13 +43,21 @@ module MergeRequests error = if !hooks_validation_pass?(merge_request) hooks_validation_error(merge_request) - elsif source.blank? + elsif !@merge_request.mergeable_to_ref? + "Merge request is not mergeable to #{target_ref}" + elsif !source 'No source for merge' end raise_error(error) if error end + def authorization_check! + unless Ability.allowed?(current_user, :admin_merge_request, project) + raise_error("You are not allowed to merge to this ref") + end + end + def target_ref merge_request.merge_ref_path end diff --git a/app/services/merge_requests/mergeability_check_service.rb b/app/services/merge_requests/mergeability_check_service.rb deleted file mode 100644 index ef833774e65..00000000000 --- a/app/services/merge_requests/mergeability_check_service.rb +++ /dev/null @@ -1,82 +0,0 @@ -# frozen_string_literal: true - -module MergeRequests - class MergeabilityCheckService < ::BaseService - include Gitlab::Utils::StrongMemoize - - delegate :project, to: :@merge_request - delegate :repository, to: :project - - def initialize(merge_request) - @merge_request = merge_request - end - - # Updates the MR merge_status. Whenever it switches to a can_be_merged state, - # the merge-ref is refreshed. - # - # Returns a ServiceResponse indicating merge_status is/became can_be_merged - # and the merge-ref is synced. Success in case of being/becoming mergeable, - # error otherwise. - def execute - return ServiceResponse.error(message: 'Invalid argument') unless merge_request - return ServiceResponse.error(message: 'Unsupported operation') if Gitlab::Database.read_only? - - update_merge_status - - unless merge_request.can_be_merged? - return ServiceResponse.error(message: 'Merge request is not mergeable') - end - - unless payload.fetch(:merge_ref_head) - return ServiceResponse.error(message: 'Merge ref was not found') - end - - ServiceResponse.success(payload: payload) - end - - private - - attr_reader :merge_request - - def payload - strong_memoize(:payload) do - { - merge_ref_head: merge_ref_head_payload - } - end - end - - def merge_ref_head_payload - commit = merge_request.merge_ref_head - - return unless commit - - target_id, source_id = commit.parent_ids - - { - commit_id: commit.id, - source_id: source_id, - target_id: target_id - } - end - - def update_merge_status - return unless merge_request.recheck_merge_status? - - if can_git_merge? - merge_to_ref && merge_request.mark_as_mergeable - else - merge_request.mark_as_unmergeable - end - end - - def can_git_merge? - !merge_request.broken? && repository.can_be_merged?(merge_request.diff_head_sha, merge_request.target_branch) - end - - def merge_to_ref - result = MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request) - result[:status] == :success - end - end -end diff --git a/app/services/service_response.rb b/app/services/service_response.rb index f3437ba16de..1de30e68d87 100644 --- a/app/services/service_response.rb +++ b/app/services/service_response.rb @@ -1,20 +1,19 @@ # frozen_string_literal: true class ServiceResponse - def self.success(message: nil, payload: {}) - new(status: :success, message: message, payload: payload) + def self.success(message: nil) + new(status: :success, message: message) end - def self.error(message:, payload: {}, http_status: nil) - new(status: :error, message: message, payload: payload, http_status: http_status) + def self.error(message:, http_status: nil) + new(status: :error, message: message, http_status: http_status) end - attr_reader :status, :message, :http_status, :payload + attr_reader :status, :message, :http_status - def initialize(status:, message: nil, payload: {}, http_status: nil) + def initialize(status:, message: nil, http_status: nil) self.status = status self.message = message - self.payload = payload self.http_status = http_status end @@ -28,5 +27,5 @@ class ServiceResponse private - attr_writer :status, :message, :http_status, :payload + attr_writer :status, :message, :http_status end diff --git a/changelogs/unreleased/osw-sync-merge-ref-upon-mergeability-check.yml b/changelogs/unreleased/osw-sync-merge-ref-upon-mergeability-check.yml deleted file mode 100644 index 1f40089adb8..00000000000 --- a/changelogs/unreleased/osw-sync-merge-ref-upon-mergeability-check.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Sync merge ref upon mergeability check -merge_request: 28513 -author: -type: added diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 96a956ad03a..dd7810c3403 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -1191,29 +1191,33 @@ Parameters: } ``` -## Returns the up to date merge-ref HEAD commit +## Merge to default merge ref path -Merge the changes between the merge request source and target branches into `refs/merge-requests/:iid/merge` -ref, of the target project repository, if possible. This ref will have the state the target branch would have if +Merge the changes between the merge request source and target branches into `refs/merge-requests/:iid/merge` +ref, of the target project repository. This ref will have the state the target branch would have if a regular merge action was taken. -This is not a regular merge action given it doesn't change the merge request target branch state in any manner. +This is not a regular merge action given it doesn't change the merge request state in any manner. -This ref (`refs/merge-requests/:iid/merge`) isn't necessarily overwritten when submitting -requests to this API, though it'll make sure the ref has the latest possible state. +This ref (`refs/merge-requests/:iid/merge`) is **always** overwritten when submitting +requests to this API, so none of its state is kept or used in the process. -If the merge request has conflicts, is empty or already merged, you'll get a `400` and a descriptive error message. +If the merge request has conflicts, is empty or already merged, +you'll get a `400` and a descriptive error message. If you don't have permissions to do so, +you'll get a `403`. -It returns the HEAD commit of `refs/merge-requests/:iid/merge` in the response body in case of `200`. +It returns the HEAD commit of `refs/merge-requests/:iid/merge` in the response body in +case of `200`. ``` -GET /projects/:id/merge_requests/:merge_request_iid/merge_ref +PUT /projects/:id/merge_requests/:merge_request_iid/merge_to_ref ``` Parameters: - `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user - `merge_request_iid` (required) - Internal ID of MR +- `merge_commit_message` (optional) - Custom merge commit message ```json { diff --git a/lib/api/entities.rb b/lib/api/entities.rb index f8b950cb55d..880b07d0a84 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -701,7 +701,7 @@ module API # See https://gitlab.com/gitlab-org/gitlab-ce/issues/42344 for more # information. expose :merge_status do |merge_request| - merge_request.check_mergeability + merge_request.check_if_can_be_merged merge_request.merge_status end expose :diff_head_sha, as: :sha diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 5bbf6df78b0..955624404f1 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -397,16 +397,28 @@ module API present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project end - desc 'Returns the up to date merge-ref HEAD commit' - get ':id/merge_requests/:merge_request_iid/merge_ref' do + desc 'Merge a merge request to its default temporary merge ref path' + params do + optional :merge_commit_message, type: String, desc: 'Custom merge commit message' + end + put ':id/merge_requests/:merge_request_iid/merge_to_ref' do merge_request = find_project_merge_request(params[:merge_request_iid]) - result = ::MergeRequests::MergeabilityCheckService.new(merge_request).execute + authorize! :admin_merge_request, user_project - if result.success? - present :commit_id, result.payload.dig(:merge_ref_head, :commit_id) + merge_params = { + commit_message: params[:merge_commit_message] + } + + result = ::MergeRequests::MergeToRefService + .new(merge_request.target_project, current_user, merge_params) + .execute(merge_request) + + if result[:status] == :success + present result.slice(:commit_id), 200 else - render_api_error!(result.message, 400) + http_status = result[:http_status] || 400 + render_api_error!(result[:message], http_status) end end 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 403 if user has no permissions to merge to the ref' do + user2 = create(:user) + project.add_reporter(user2) - it 'returns 404' do - project.add_guest(user) + put api(url, user2) - get api(url, user) - - 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 for an invalid merge request IID' do + put api("/projects/#{project.id}/merge_requests/12345/merge_to_ref", user) - it 'returns 404' do - get api(url, 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 if the merge request id is used instead of iid" do + put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user) - it 'returns 404' do - get api(url, 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