Merge branch 'feature/merge-request-system-hook' into 'master'

System hooks for Merge Requests

See merge request gitlab-org/gitlab-ce!14387

8 files changed, 189 insertions, 13 deletions

diff --git a/spec/controllers/admin/hooks_controller_spec.rb b/spec/controllers/admin/hooks_controller_spec.rb
index e6ba596117a..d2c1e634930 100644
--- a/spec/controllers/admin/hooks_controller_spec.rb
+++ b/spec/controllers/admin/hooks_controller_spec.rb
@@ -11,11 +11,13 @@ describe Admin::HooksController do
     it 'sets all parameters' do
       hook_params = {
         enable_ssl_verification: true,
+        token: "TEST TOKEN",
+        url: "http://example.com",
+
         push_events: true,
         tag_push_events: true,
         repository_update_events: true,
-        token: "TEST TOKEN",
-        url: "http://example.com"
+        merge_requests_events: true
       }
 
       post :create, hook: hook_params
diff --git a/spec/controllers/projects/hooks_controller_spec.rb b/spec/controllers/projects/hooks_controller_spec.rb
index aba70c6d4c1..2d473d5bf52 100644
--- a/spec/controllers/projects/hooks_controller_spec.rb
+++ b/spec/controllers/projects/hooks_controller_spec.rb
@@ -18,4 +18,30 @@ describe Projects::HooksController do
       )
     end
   end
+
+  describe '#create' do
+    it 'sets all parameters' do
+      hook_params = {
+        enable_ssl_verification: true,
+        token: "TEST TOKEN",
+        url: "http://example.com",
+
+        push_events: true,
+        tag_push_events: true,
+        merge_requests_events: true,
+        issues_events: true,
+        confidential_issues_events: true,
+        note_events: true,
+        job_events: true,
+        pipeline_events: true,
+        wiki_page_events: true
+      }
+
+      post :create, namespace_id: project.namespace, project_id: project, hook: hook_params
+
+      expect(response).to have_http_status(302)
+      expect(ProjectHook.all.size).to eq(1)
+      expect(ProjectHook.first).to have_attributes(hook_params)
+    end
+  end
 end
diff --git a/spec/features/admin/admin_hooks_spec.rb b/spec/features/admin/admin_hooks_spec.rb
index eec44549a03..f266f2ecc54 100644
--- a/spec/features/admin/admin_hooks_spec.rb
+++ b/spec/features/admin/admin_hooks_spec.rb
@@ -1,11 +1,10 @@
 require 'spec_helper'
 
-describe 'Admin::Hooks', :js do
-  before do
-    @project = create(:project)
-    sign_in(create(:admin))
+describe 'Admin::Hooks' do
+  let(:user) { create(:admin) }
 
-    @system_hook = create(:system_hook)
+  before do
+    sign_in(user)
   end
 
   describe 'GET /admin/hooks' do
@@ -13,15 +12,17 @@ describe 'Admin::Hooks', :js do
       visit admin_root_path
 
       page.within '.nav-sidebar' do
-        click_on 'Hooks'
+        click_on 'System Hooks', match: :first
       end
 
       expect(current_path).to eq(admin_hooks_path)
     end
 
     it 'has hooks list' do
+      system_hook = create(:system_hook)
+
       visit admin_hooks_path
-      expect(page).to have_content(@system_hook.url)
+      expect(page).to have_content(system_hook.url)
     end
   end
 
@@ -43,6 +44,10 @@ describe 'Admin::Hooks', :js do
   describe 'Update existing hook' do
     let(:new_url) { generate(:url) }
 
+    before do
+      create(:system_hook)
+    end
+
     it 'updates existing hook' do
       visit admin_hooks_path
 
@@ -57,7 +62,11 @@ describe 'Admin::Hooks', :js do
     end
   end
 
-  describe 'Remove existing hook' do
+  describe 'Remove existing hook', :js do
+    before do
+      create(:system_hook)
+    end
+
     context 'removes existing hook' do
       it 'from hooks list page' do
         visit admin_hooks_path
@@ -76,7 +85,8 @@ describe 'Admin::Hooks', :js do
 
   describe 'Test', :js do
     before do
-      WebMock.stub_request(:post, @system_hook.url)
+      system_hook = create(:system_hook)
+      WebMock.stub_request(:post, system_hook.url)
       visit admin_hooks_path
 
       find('.hook-test-button.dropdown').click
@@ -85,4 +95,41 @@ describe 'Admin::Hooks', :js do
 
     it { expect(current_path).to eq(admin_hooks_path) }
   end
+
+  context 'Merge request hook' do
+    describe 'New Hook' do
+      let(:url) { generate(:url) }
+
+      it 'adds new hook' do
+        visit admin_hooks_path
+
+        fill_in 'hook_url', with: url
+        uncheck 'Repository update events'
+        check 'Merge request events'
+
+        expect { click_button 'Add system hook' }.to change(SystemHook, :count).by(1)
+        expect(current_path).to eq(admin_hooks_path)
+        expect(page).to have_content(url)
+      end
+    end
+
+    describe 'Test', :js do
+      before do
+        system_hook = create(:system_hook)
+        WebMock.stub_request(:post, system_hook.url)
+      end
+
+      it 'succeeds if the user has a repository with a merge request' do
+        project = create(:project, :repository)
+        create(:project_member, user: user, project: project)
+        create(:merge_request, source_project: project)
+
+        visit admin_hooks_path
+        find('.hook-test-button.dropdown').click
+        click_link 'Merge requests events'
+
+        expect(page).to have_content 'Hook executed successfully'
+      end
+    end
+  end
 end
diff --git a/spec/models/concerns/triggerable_hooks_spec.rb b/spec/models/concerns/triggerable_hooks_spec.rb
new file mode 100644
index 00000000000..621d2d38eae
--- /dev/null
+++ b/spec/models/concerns/triggerable_hooks_spec.rb
@@ -0,0 +1,43 @@
+require 'rails_helper'
+
+RSpec.describe TriggerableHooks do
+  before do
+    class TestableHook < WebHook
+      include TriggerableHooks
+      triggerable_hooks [:push_hooks]
+    end
+  end
+
+  describe 'scopes' do
+    it 'defines a scope for each of the requested triggers' do
+      expect(TestableHook).to respond_to :push_hooks
+      expect(TestableHook).not_to respond_to :tag_push_hooks
+    end
+  end
+
+  describe '.hooks_for' do
+    context 'the model has the required trigger scope' do
+      it 'returns the record' do
+        hook = TestableHook.create!(url: 'http://example.com', push_events: true)
+
+        expect(TestableHook.hooks_for(:push_hooks)).to eq [hook]
+      end
+    end
+
+    context 'the model does not have the required trigger scope' do
+      it 'returns an empty relation' do
+        TestableHook.create!(url: 'http://example.com')
+
+        expect(TestableHook.hooks_for(:tag_push_hooks)).to eq []
+      end
+    end
+
+    context 'the stock scope ".all" is accepted' do
+      it 'returns the record' do
+        hook = TestableHook.create!(url: 'http://example.com')
+
+        expect(TestableHook.hooks_for(:all)).to eq [hook]
+      end
+    end
+  end
+end
diff --git a/spec/models/hooks/system_hook_spec.rb b/spec/models/hooks/system_hook_spec.rb
index 0e965f541d8..8bc45715dcd 100644
--- a/spec/models/hooks/system_hook_spec.rb
+++ b/spec/models/hooks/system_hook_spec.rb
@@ -7,7 +7,8 @@ describe SystemHook do
     it 'sets defined default parameters' do
       attrs = {
         push_events: false,
-        repository_update_events: true
+        repository_update_events: true,
+        merge_requests_events: false
       }
       expect(system_hook).to have_attributes(attrs)
     end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 78223c44999..987be8e8b46 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3206,4 +3206,23 @@ describe Project do
       expect { project.write_repository_config }.not_to raise_error
     end
   end
+
+  describe '#execute_hooks' do
+    it 'executes the projects hooks with the specified scope' do
+      hook1 = create(:project_hook, merge_requests_events: true, tag_push_events: false)
+      hook2 = create(:project_hook, merge_requests_events: false, tag_push_events: true)
+      project = create(:project, hooks: [hook1, hook2])
+
+      expect_any_instance_of(ProjectHook).to receive(:async_execute).once
+
+      project.execute_hooks({}, :tag_push_hooks)
+    end
+
+    it 'executes the system hooks with the specified scope' do
+      expect_any_instance_of(SystemHooksService).to receive(:execute_hooks).with({ data: 'data' }, :merge_request_hooks)
+
+      project = build(:project)
+      project.execute_hooks({ data: 'data' }, :merge_request_hooks)
+    end
+  end
 end
diff --git a/spec/requests/api/system_hooks_spec.rb b/spec/requests/api/system_hooks_spec.rb
index c7a009e999e..6c57d443cbf 100644
--- a/spec/requests/api/system_hooks_spec.rb
+++ b/spec/requests/api/system_hooks_spec.rb
@@ -36,6 +36,7 @@ describe API::SystemHooks do
         expect(json_response.first['url']).to eq(hook.url)
         expect(json_response.first['push_events']).to be false
         expect(json_response.first['tag_push_events']).to be false
+        expect(json_response.first['merge_requests_events']).to be false
         expect(json_response.first['repository_update_events']).to be true
       end
     end
@@ -67,11 +68,28 @@ describe API::SystemHooks do
     end
 
     it 'sets default values for events' do
-      post api('/hooks', admin), url: 'http://mep.mep', enable_ssl_verification: true
+      post api('/hooks', admin), url: 'http://mep.mep'
 
       expect(response).to have_gitlab_http_status(201)
       expect(json_response['enable_ssl_verification']).to be true
+      expect(json_response['push_events']).to be false
       expect(json_response['tag_push_events']).to be false
+      expect(json_response['merge_requests_events']).to be false
+    end
+
+    it 'sets explicit values for events' do
+      post api('/hooks', admin),
+        url: 'http://mep.mep',
+        enable_ssl_verification: false,
+        push_events: true,
+        tag_push_events: true,
+        merge_requests_events: true
+
+      expect(response).to have_http_status(201)
+      expect(json_response['enable_ssl_verification']).to be false
+      expect(json_response['push_events']).to be true
+      expect(json_response['tag_push_events']).to be true
+      expect(json_response['merge_requests_events']).to be true
     end
   end
 
diff --git a/spec/services/test_hooks/system_service_spec.rb b/spec/services/test_hooks/system_service_spec.rb
index ff8b9595538..74d7715e50f 100644
--- a/spec/services/test_hooks/system_service_spec.rb
+++ b/spec/services/test_hooks/system_service_spec.rb
@@ -60,5 +60,25 @@ describe TestHooks::SystemService do
         expect(service.execute).to include(success_result)
       end
     end
+
+    context 'merge_requests_events' do
+      let(:trigger) { 'merge_requests_events' }
+
+      it 'returns error message if the user does not have any repository with a merge request' do
+        expect(hook).not_to receive(:execute)
+        expect(service.execute).to include({ status: :error, message: 'Ensure one of your projects has merge requests.' })
+      end
+
+      it 'executes hook' do
+        trigger_key = :merge_request_hooks
+        sample_data = { data: 'sample' }
+        create(:project_member, user: current_user, project: project)
+        create(:merge_request, source_project: project)
+        allow_any_instance_of(MergeRequest).to receive(:to_hook_data).and_return(sample_data)
+
+        expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+        expect(service.execute).to include(success_result)
+      end
+    end
   end
 end