Merge branch 'rs-approval-hooks' into 'master'

Fire merge request webhooks when a merge request is approved

Closes https://gitlab.com/gitlab-org/gitlab-ee/issues/93

See merge request !270

app/controllers/projects/merge_requests_controller.rb

@@ -253,12 +253,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       return render_404
     end
 
-    @approval = @merge_request.approvals.new
-    @approval.user = current_user
-
-    if @approval.save
-      SystemNoteService.approve_mr(@merge_request, current_user)
-    end
+    MergeRequests::ApprovalService.
+      new(project, current_user).
+      execute(@merge_request)
 
     redirect_to merge_request_path(@merge_request)
   end

app/services/merge_requests/approval_service.rb

+module MergeRequests
+  class ApprovalService < MergeRequests::BaseService
+    def execute(merge_request)
+      approval = merge_request.approvals.new(user: current_user)
+
+      if approval.save
+        create_approval_note(merge_request)
+
+        if merge_request.approvals_left.zero?
+          execute_hooks(merge_request, 'approved')
+        end
+      end
+    end
+
+    private
+
+    def create_approval_note(merge_request)
+      SystemNoteService.approve_mr(merge_request, current_user)
+    end
+  end
+end

doc/web_hooks/web_hooks.md

 # Webhooks
 
-_**Note:**
-Starting from GitLab 8.5:_
-
-- _the `repository` key is deprecated in favor of the `project` key_
-- _the `project.ssh_url` key is deprecated in favor of the `project.git_ssh_url` key_
-- _the `project.http_url` key is deprecated in favor of the `project.git_http_url` key_
+> **Note:** As of GitLab 8.5:
+>
+> - the `repository` key is deprecated in favor of the `project` key
+> - the `project.ssh_url` key is deprecated in favor of the `project.git_ssh_url` key
+> - the `project.http_url` key is deprecated in favor of the `project.git_http_url` key
 
 Project webhooks allow you to trigger an URL if new code is pushed or a new issue is created.
 
@@ -595,7 +594,11 @@ X-Gitlab-Event: Note Hook
 
 ## Merge request events
 
-Triggered when a new merge request is created, an existing merge request was updated/merged/closed or a commit is added in the source branch.
+Triggered when a new merge request is created, an existing merge request was
+updated/merged/closed/approved, or a commit is added in the source branch.
+
+> **Note:** A webhook for the `approved` action is only fired once the total
+> number of required approvals is met, _not_ after each individual approval.
 
 **Request header**:
 

spec/services/merge_requests/approval_service_spec.rb

+require 'rails_helper'
+
+describe MergeRequests::ApprovalService, services: true do
+  describe '#execute' do
+    let(:user)          { build_stubbed(:user) }
+    let(:project)       { build_stubbed(:empty_project) }
+    let(:merge_request) { build_stubbed(:merge_request) }
+
+    context 'with invalid approval' do
+      it 'does not create an approval note' do
+        allow(merge_request.approvals).
+          to receive(:new).and_return(double(save: false))
+        service = described_class.new(double, double)
+
+        expect(SystemNoteService).not_to receive(:approve_mr)
+
+        service.execute(merge_request)
+      end
+    end
+
+    context 'with valid approval' do
+      it 'creates an approval note' do
+        service = described_class.new(project, user)
+
+        expect(SystemNoteService).to receive(:approve_mr).with(merge_request, user)
+
+        service.execute(merge_request)
+      end
+
+      context 'with remaining approvals' do
+        it 'does not fire a webhook' do
+          expect(merge_request).to receive(:approvals_left).and_return(5)
+
+          service = described_class.new(project, user)
+          expect(service).not_to receive(:execute_hooks)
+
+          service.execute(merge_request)
+        end
+      end
+
+      context 'with required approvals' do
+        it 'fires a webhook' do
+          expect(merge_request).to receive(:approvals_left).and_return(0)
+
+          service = described_class.new(project, user)
+          expect(service).to receive(:execute_hooks).with(merge_request, 'approved')
+
+          service.execute(merge_request)
+        end
+      end
+    end
+  end
+end