Commit 0e2490c0 authored by Douwe Maan's avatar Douwe Maan

Merge branch '14139-sha-parameter-in-accept-merge-request-api' into 'master'

Resolve "SHA parameter in accept merge request API"

Add a `sha` parameter to the MR merge API, which must match the source SHA for the branch to be merged.

Also add the same parameter to the UI:

![MR_SHA](/uploads/616da728695dc19fa7ef7ef6a016ff81/MR_SHA.gif)

@DouweM and I discussed adding some smart feature to that, like updating the source SHA on navigating to the diff tab, but for now it will just require a refresh 😃

Closes #14139.

See merge request !4414
parents 4c8d4c62 4f726683
...@@ -14,6 +14,8 @@ v 8.9.0 (unreleased) ...@@ -14,6 +14,8 @@ v 8.9.0 (unreleased)
- Fix groups API to list only user's accessible projects - Fix groups API to list only user's accessible projects
- Redesign account and email confirmation emails - Redesign account and email confirmation emails
- Use gitlab-shell v3.0.0 - Use gitlab-shell v3.0.0
- Add `sha` parameter to MR merge API, to ensure only reviewed changes are merged
- Don't allow MRs to be merged when commits were added since the last review / page load
- Add DB index on users.state - Add DB index on users.state
- Add rake task 'gitlab:db:configure' for conditionally seeding or migrating the database - Add rake task 'gitlab:db:configure' for conditionally seeding or migrating the database
- Changed the Slack build message to use the singular duration if necessary (Aran Koning) - Changed the Slack build message to use the singular duration if necessary (Aran Koning)
......
...@@ -190,6 +190,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -190,6 +190,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController
return return
end end
if params[:sha] != @merge_request.source_sha
@status = :sha_mismatch
return
end
TodoService.new.merge_merge_request(merge_request, current_user) TodoService.new.merge_merge_request(merge_request, current_user)
@merge_request.update(merge_error: nil) @merge_request.update(merge_error: nil)
......
...@@ -5,6 +5,9 @@ ...@@ -5,6 +5,9 @@
- when :merge_when_build_succeeds - when :merge_when_build_succeeds
:plain :plain
$('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/merge_when_build_succeeds'))}"); $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/merge_when_build_succeeds'))}");
- when :sha_mismatch
:plain
$('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/sha_mismatch'))}");
- else - else
:plain :plain
$('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/reload'))}"); $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/reload'))}");
...@@ -2,6 +2,7 @@ ...@@ -2,6 +2,7 @@
= form_for [:merge, @project.namespace.becomes(Namespace), @project, @merge_request], remote: true, method: :post, html: { class: 'accept-mr-form js-quick-submit js-requires-input' } do |f| = form_for [:merge, @project.namespace.becomes(Namespace), @project, @merge_request], remote: true, method: :post, html: { class: 'accept-mr-form js-quick-submit js-requires-input' } do |f|
= hidden_field_tag :authenticity_token, form_authenticity_token = hidden_field_tag :authenticity_token, form_authenticity_token
= hidden_field_tag :sha, @merge_request.source_sha
.accept-merge-holder.clearfix.js-toggle-container .accept-merge-holder.clearfix.js-toggle-container
.clearfix .clearfix
.accept-action .accept-action
......
...@@ -16,7 +16,7 @@ ...@@ -16,7 +16,7 @@
- if remove_source_branch_button || user_can_cancel_automatic_merge - if remove_source_branch_button || user_can_cancel_automatic_merge
.clearfix.prepend-top-10 .clearfix.prepend-top-10
- if remove_source_branch_button - if remove_source_branch_button
= link_to merge_namespace_project_merge_request_path(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request, merge_when_build_succeeds: true, should_remove_source_branch: true), remote: true, method: :post, class: "btn btn-grouped btn-primary btn-sm remove_source_branch" do = link_to merge_namespace_project_merge_request_path(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request, merge_when_build_succeeds: true, should_remove_source_branch: true, sha: @merge_request.source_sha), remote: true, method: :post, class: "btn btn-grouped btn-primary btn-sm remove_source_branch" do
= icon('times') = icon('times')
Remove Source Branch When Merged Remove Source Branch When Merged
......
%h4
= icon("exclamation-triangle")
This merge request has received new commits since the page was loaded.
%p
Please reload the page to review the new commits before merging.
...@@ -413,11 +413,13 @@ curl -X DELETE -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.c ...@@ -413,11 +413,13 @@ curl -X DELETE -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.c
Merge changes submitted with MR using this API. Merge changes submitted with MR using this API.
If merge success you get `200 OK`. If the merge succeeds you'll get a `200 OK`.
If it has some conflicts and can not be merged - you get 405 and error message 'Branch cannot be merged' If it has some conflicts and can not be merged - you'll get a 405 and the error message 'Branch cannot be merged'
If merge request is already merged or closed - you get 405 and error message 'Method Not Allowed' If merge request is already merged or closed - you'll get a 406 and the error message 'Method Not Allowed'
If the `sha` parameter is passed and does not match the HEAD of the source - you'll get a 409 and the error message 'SHA does not match HEAD of source branch'
If you don't have permissions to accept this merge request - you'll get a 401 If you don't have permissions to accept this merge request - you'll get a 401
...@@ -431,7 +433,8 @@ Parameters: ...@@ -431,7 +433,8 @@ Parameters:
- `merge_request_id` (required) - ID of MR - `merge_request_id` (required) - ID of MR
- `merge_commit_message` (optional) - Custom merge commit message - `merge_commit_message` (optional) - Custom merge commit message
- `should_remove_source_branch` (optional) - if `true` removes the source branch - `should_remove_source_branch` (optional) - if `true` removes the source branch
- `merged_when_build_succeeds` (optional) - if `true` the MR is merge when the build succeeds - `merged_when_build_succeeds` (optional) - if `true` the MR is merged when the build succeeds
- `sha` (optional) - if present, then this SHA must match the HEAD of the source branch, otherwise the merge will fail
```json ```json
{ {
......
...@@ -218,6 +218,7 @@ module API ...@@ -218,6 +218,7 @@ module API
# merge_commit_message (optional) - Custom merge commit message # merge_commit_message (optional) - Custom merge commit message
# should_remove_source_branch (optional) - When true, the source branch will be deleted if possible # should_remove_source_branch (optional) - When true, the source branch will be deleted if possible
# merge_when_build_succeeds (optional) - When true, this MR will be merged when the build succeeds # merge_when_build_succeeds (optional) - When true, this MR will be merged when the build succeeds
# sha (optional) - When present, must have the HEAD SHA of the source branch
# Example: # Example:
# PUT /projects/:id/merge_requests/:merge_request_id/merge # PUT /projects/:id/merge_requests/:merge_request_id/merge
# #
...@@ -233,6 +234,10 @@ module API ...@@ -233,6 +234,10 @@ module API
render_api_error!('Branch cannot be merged', 406) unless merge_request.can_be_merged? render_api_error!('Branch cannot be merged', 406) unless merge_request.can_be_merged?
if params[:sha] && merge_request.source_sha != params[:sha]
render_api_error!("SHA does not match HEAD of source branch: #{merge_request.source_sha}", 409)
end
merge_params = { merge_params = {
commit_message: params[:merge_commit_message], commit_message: params[:merge_commit_message],
should_remove_source_branch: params[:should_remove_source_branch] should_remove_source_branch: params[:should_remove_source_branch]
......
...@@ -185,6 +185,92 @@ describe Projects::MergeRequestsController do ...@@ -185,6 +185,92 @@ describe Projects::MergeRequestsController do
end end
end end
describe 'POST #merge' do
let(:base_params) do
{
namespace_id: project.namespace.path,
project_id: project.path,
id: merge_request.iid,
format: 'raw'
}
end
context 'when the user does not have access' do
before do
project.team.truncate
project.team << [user, :reporter]
post :merge, base_params
end
it 'returns not found' do
expect(response).to be_not_found
end
end
context 'when the merge request is not mergeable' do
before do
merge_request.update_attributes(title: "WIP: #{merge_request.title}")
post :merge, base_params
end
it 'returns :failed' do
expect(assigns(:status)).to eq(:failed)
end
end
context 'when the sha parameter does not match the source SHA' do
before { post :merge, base_params.merge(sha: 'foo') }
it 'returns :sha_mismatch' do
expect(assigns(:status)).to eq(:sha_mismatch)
end
end
context 'when the sha parameter matches the source SHA' do
def merge_with_sha
post :merge, base_params.merge(sha: merge_request.source_sha)
end
it 'returns :success' do
merge_with_sha
expect(assigns(:status)).to eq(:success)
end
it 'starts the merge immediately' do
expect(MergeWorker).to receive(:perform_async).with(merge_request.id, anything, anything)
merge_with_sha
end
context 'when merge_when_build_succeeds is passed' do
def merge_when_build_succeeds
post :merge, base_params.merge(sha: merge_request.source_sha, merge_when_build_succeeds: '1')
end
before do
create(:ci_empty_commit, project: project, sha: merge_request.source_sha, ref: merge_request.source_branch)
end
it 'returns :merge_when_build_succeeds' do
merge_when_build_succeeds
expect(assigns(:status)).to eq(:merge_when_build_succeeds)
end
it 'sets the MR to merge when the build succeeds' do
service = double(:merge_when_build_succeeds_service)
expect(MergeRequests::MergeWhenBuildSucceedsService).to receive(:new).with(project, anything, anything).and_return(service)
expect(service).to receive(:execute).with(merge_request)
merge_when_build_succeeds
end
end
end
end
describe "DELETE #destroy" do describe "DELETE #destroy" do
it "denies access to users unless they're admin or project owner" do it "denies access to users unless they're admin or project owner" do
delete :destroy, namespace_id: project.namespace.path, project_id: project.path, id: merge_request.iid delete :destroy, namespace_id: project.namespace.path, project_id: project.path, id: merge_request.iid
......
...@@ -428,6 +428,19 @@ describe API::API, api: true do ...@@ -428,6 +428,19 @@ describe API::API, api: true do
expect(json_response['message']).to eq('401 Unauthorized') expect(json_response['message']).to eq('401 Unauthorized')
end end
it "returns 409 if the SHA parameter doesn't match" do
put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user), sha: merge_request.source_sha.succ
expect(response.status).to eq(409)
expect(json_response['message']).to start_with('SHA does not match HEAD of source branch')
end
it "succeeds if the SHA parameter matches" do
put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user), sha: merge_request.source_sha
expect(response.status).to eq(200)
end
it "enables merge when build succeeds if the ci is active" do it "enables merge when build succeeds if the ci is active" do
allow_any_instance_of(MergeRequest).to receive(:ci_commit).and_return(ci_commit) allow_any_instance_of(MergeRequest).to receive(:ci_commit).and_return(ci_commit)
allow(ci_commit).to receive(:active?).and_return(true) allow(ci_commit).to receive(:active?).and_return(true)
......
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