Merge branch 'add-sha-check-to-approvals-api' into 'master'

Add optional sha parameter when approving an MR through the API

Closes #2394

See merge request !2370

changelogs/unreleased-ee/add-sha-check-to-approvals-api.yml

+---
+title: Add optional sha param when approving a merge request through the API
+merge_request:
+author:

doc/api/merge_requests.md

@@ -609,6 +609,12 @@ POST /projects/:id/merge_requests/:merge_request_iid/approve
 |---------------------|---------|----------|---------------------|
 | `id`                | integer | yes      | The ID of a project |
 | `merge_request_iid` | integer | yes      | The IID of MR       |
+| `sha`               | string  | no       | The HEAD of the MR  |
+
+The `sha` parameter works in the same way as
+when [accepting a merge request](#accept-mr): if it is passed, then it must
+match the current HEAD of the merge request for the approval to be added. If it
+does not match, the response code will be `409`.
 
 ```json
 {

lib/api/merge_requests.rb

@@ -27,6 +27,12 @@ module API
           render_api_error!(errors, 400)
         end
 
+        def check_sha_param!(params, merge_request)
+          if params[:sha] && merge_request.diff_head_sha != params[:sha]
+            render_api_error!("SHA does not match HEAD of source branch: #{merge_request.diff_head_sha}", 409)
+          end
+        end
+
         def issue_entity(project)
           if project.has_external_issue_tracker?
             Entities::ExternalIssue
@@ -228,9 +234,7 @@ module API
 
         render_api_error!('Branch cannot be merged', 406) unless merge_request.mergeable?(skip_ci_check: merge_when_pipeline_succeeds)
 
-        if params[:sha] && merge_request.diff_head_sha != params[:sha]
-          render_api_error!("SHA does not match HEAD of source branch: #{merge_request.diff_head_sha}", 409)
-        end
+        check_sha_param!(params, merge_request)
 
         if params[:squash] && merge_request.project.feature_available?(:merge_request_squash)
           merge_request.update(squash: params[:squash])
@@ -275,6 +279,9 @@ module API
       # Examples:
       #   GET /projects/:id/merge_requests/:merge_request_iid/approvals
       #
+      desc "List a merge request's approvals" do
+        success Entities::MergeRequestApprovals
+      end
       get ':id/merge_requests/:merge_request_iid/approvals' do
         merge_request = find_merge_request_with_access(params[:merge_request_iid])
 
@@ -289,11 +296,19 @@ module API
       # Examples:
       #   POST /projects/:id/merge_requests/:merge_request_iid/approve
       #
+      desc 'Approve a merge request' do
+        success Entities::MergeRequestApprovals
+      end
+      params do
+        optional :sha, type: String, desc: 'When present, must have the HEAD SHA of the source branch'
+      end
       post ':id/merge_requests/:merge_request_iid/approve' do
         merge_request = find_project_merge_request(params[:merge_request_iid])
 
         unauthorized! unless merge_request.can_approve?(current_user)
 
+        check_sha_param!(params, merge_request)
+
         ::MergeRequests::ApprovalService
           .new(user_project, current_user)
           .execute(merge_request)
@@ -301,6 +316,9 @@ module API
         present merge_request, with: Entities::MergeRequestApprovals, current_user: current_user
       end
 
+      desc 'Remove an approval from a merge request' do
+        success Entities::MergeRequestApprovals
+      end
       post ':id/merge_requests/:merge_request_iid/unapprove' do
         merge_request = find_project_merge_request(params[:merge_request_iid])
 

spec/requests/api/merge_requests_spec.rb

@@ -978,7 +978,7 @@ describe API::MergeRequests do
 
       get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", user)
 
-      expect(response.status).to eq(200)
+      expect(response).to have_http_status(200)
       expect(json_response['approvals_required']).to eq 2
       expect(json_response['approvals_left']).to eq 1
       expect(json_response['approved_by'][0]['user']['username']).to eq(approver.username)
@@ -1008,15 +1008,50 @@ describe API::MergeRequests do
       before do
         project.team << [approver, :developer]
         project.team << [create(:user), :developer]
+      end
 
-        post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approve", approver)
+      def approve(extra_params = {})
+        post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approve", approver), extra_params
       end
 
-      it 'approves the merge request' do
-        expect(response.status).to eq(201)
-        expect(json_response['approvals_left']).to eq(1)
-        expect(json_response['approved_by'][0]['user']['username']).to eq(approver.username)
-        expect(json_response['user_has_approved']).to be true
+      context 'when the sha param is not set' do
+        before do
+          approve
+        end
+
+        it 'approves the merge request' do
+          expect(response).to have_http_status(201)
+          expect(json_response['approvals_left']).to eq(1)
+          expect(json_response['approved_by'][0]['user']['username']).to eq(approver.username)
+          expect(json_response['user_has_approved']).to be true
+        end
+      end
+
+      context 'when the sha param is correct' do
+        before do
+          approve(sha: merge_request.diff_head_sha)
+        end
+
+        it 'approves the merge request' do
+          expect(response).to have_http_status(201)
+          expect(json_response['approvals_left']).to eq(1)
+          expect(json_response['approved_by'][0]['user']['username']).to eq(approver.username)
+          expect(json_response['user_has_approved']).to be true
+        end
+      end
+
+      context 'when the sha param is incorrect' do
+        before do
+          approve(sha: merge_request.diff_head_sha.reverse)
+        end
+
+        it 'returns a 409' do
+          expect(response).to have_http_status(409)
+        end
+
+        it 'does not approve the merge request' do
+          expect(merge_request.reload.approvals_left).to eq(2)
+        end
       end
     end
   end
@@ -1041,7 +1076,7 @@ describe API::MergeRequests do
       end
 
       it 'unapproves the merge request' do
-        expect(response.status).to eq(201)
+        expect(response).to have_http_status(201)
         expect(json_response['approvals_left']).to eq(1)
         usernames = json_response['approved_by'].map { |u| u['user']['username'] }
         expect(usernames).not_to include(unapprover.username)