Add optional sha parameter when approving an MR through the API

This works exactly like the same param when merging an MR.

We don't need this in the UI, because you already get a warning in the widget
when the source branch HEAD changes while you're on the page.

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)