first pass at approval removal

app/models/concerns/approvable.rb

@@ -123,6 +123,12 @@ module Approvable
       any_approver_allowed? && approvals.where(user: user).empty?
     end
 
+    def has_approved?(user)
+      return false unless user
+
+      approved_by_users.include?(user)
+    end
+
     # Once there are fewer approvers left in the list than approvals required, allow other
     # project members to approve the MR.
     #

app/services/merge_requests/remove_approval_service.rb

+module MergeRequests
+  class RemoveApprovalService < MergeRequests::BaseService
+    def execute(merge_request)
+      # paranoid protection against running wrong deletes
+      return unless merge_request.id && current_user.id
+
+      approval = merge_request.approvals.where(user: current_user)
+
+      currently_approved = merge_request.approved?
+
+      if approval.destroy_all
+        # bust the cache here, otherwise will show results from
+        # before the deletion
+        merge_request.approvals(true)
+
+        create_note(merge_request)
+
+        if currently_approved
+          notification_service.unapprove_mr(merge_request, current_user)
+          execute_hooks(merge_request, 'unapproved')
+        end
+      end
+    end
+
+    private
+
+    def create_note(merge_request)
+      SystemNoteService.unapprove_mr(merge_request, current_user)
+    end
+  end
+end

app/services/notification_service.rb

@@ -162,6 +162,10 @@ class NotificationService
     approve_mr_email(merge_request, merge_request.target_project, current_user)
   end
 
+  def unapprove_mr(merge_request, current_user)
+    warn 'TODO: send unapproval email'
+  end
+
   def resolve_all_discussions(merge_request, current_user)
     recipients = build_recipients(merge_request, merge_request.target_project, current_user, action: "resolve_all_discussions")
 

app/services/system_note_service.rb

@@ -474,6 +474,11 @@ module SystemNoteService
     create_note(noteable: noteable, project: noteable.project, author: user, note: body)
   end
 
+  def unapprove_mr(noteable, user)
+    body = "Unapproved this merge request"
+    create_note(noteable: noteable, project: noteable.project, author: user, note: body)
+  end
+
   private
 
   def notes_for_mentioner(mentioner, noteable, notes)

lib/api/entities.rb

@@ -318,6 +318,14 @@ module API
       expose :approvals_required
       expose :approvals_left
       expose :approvals, as: :approved_by, using: Entities::Approvals
+
+      expose :user_has_approved do |merge_request, options|
+        merge_request.has_approved?(options[:current_user])
+      end
+
+      expose :user_can_approve do |merge_request, options|
+        merge_request.can_approve?(options[:current_user])
+      end
     end
 
     class MergeRequestDiff < Grape::Entity

lib/api/merge_requests.rb

@@ -302,7 +302,7 @@ module API
         # Examples:
         #   POST /projects/:id/merge_requests/:merge_request_id/approvals
         #
-        post "#{path}/approve" do
+        post "#{path}/approvals" do
           merge_request = user_project.merge_requests.find(params[:merge_request_id])
 
           unauthorized! unless merge_request.can_approve?(current_user)
@@ -313,6 +313,18 @@ module API
 
           present merge_request, with: Entities::MergeRequestApprovals, current_user: current_user
         end
+
+        delete "#{path}/approvals" do
+          merge_request = user_project.merge_requests.find(params[:merge_request_id])
+
+          not_found! unless merge_request.has_approved?(current_user)
+
+          ::MergeRequests::RemoveApprovalService
+            .new(user_project, current_user)
+            .execute(merge_request)
+
+          present merge_request, with: Entities::MergeRequestApprovals, current_user: current_user
+        end
       end
     end
   end

spec/requests/api/merge_requests_spec.rb

@@ -729,10 +729,12 @@ describe API::MergeRequests, api: true  do
       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)
+      expect(json_response['user_can_approve']).to be false
+      expect(json_response['user_has_approved']).to be false
     end
   end
 
-  describe 'POST :id/merge_requests/:merge_request_id/approve' do
+  describe 'POST :id/merge_requests/:merge_request_id/approvals' do
     before { project.update_attribute(:approvals_before_merge, 2) }
 
     context 'as the author of the merge request' do
@@ -750,17 +752,48 @@ describe API::MergeRequests, api: true  do
         project.team << [approver, :developer]
         project.team << [create(:user), :developer]
 
-        post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/approve", approver)
+        post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/approvals", approver)
       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
       end
     end
   end
 
+  describe 'DELETE :id/merge_requests/:merge_request_id/approvals' do
+    before { project.update_attribute(:approvals_before_merge, 2) }
+
+    context 'as a user who has approved the merge request' do
+      let(:approver) { create(:user) }
+      let(:unapprover) { create(:user) }
+
+      before do
+        project.team << [approver, :developer]
+        project.team << [unapprover, :developer]
+        project.team << [create(:user), :developer]
+        merge_request.approvals.create(user: approver)
+        merge_request.approvals.create(user: unapprover)
+
+        delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}/approvals", unapprover)
+      end
+
+      it 'unapproves the merge request' do
+        expect(response.status).to eq(200)
+        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)
+        expect(usernames.size).to be 1
+        expect(json_response['user_has_approved']).to be false
+        expect(json_response['user_can_approve']).to be true
+        binding.pry
+        true
+      end
+    end
+  end
   def mr_with_later_created_and_updated_at_time
     merge_request
     merge_request.created_at += 1.hour