Merge branch '328048-group-hook-push-event-branch-filter-does-not-apply' into 'master'

Support push branch filters in group hook

See merge request gitlab-org/gitlab!82531

app/models/project.rb

@@ -1567,14 +1567,17 @@ class Project < ApplicationRecord
   # rubocop: disable CodeReuse/ServiceClass
   def execute_hooks(data, hooks_scope = :push_hooks)
     run_after_commit_or_now do
-      hooks.hooks_for(hooks_scope).select_active(hooks_scope, data).each do |hook|
-        hook.async_execute(data, hooks_scope.to_s)
-      end
+      triggered_hooks(hooks_scope, data).execute
       SystemHooksService.new.execute_hooks(data, hooks_scope)
     end
   end
   # rubocop: enable CodeReuse/ServiceClass
 
+  def triggered_hooks(hooks_scope, data)
+    triggered = ::Projects::TriggeredHooks.new(hooks_scope, data)
+    triggered.add_hooks(hooks)
+  end
+
   def execute_integrations(data, hooks_scope = :push_hooks)
     # Call only service hooks that are active for this scope
     run_after_commit_or_now do

app/models/projects/triggered_hooks.rb

+# frozen_string_literal: true
+
+module Projects
+  class TriggeredHooks
+    def initialize(scope, data)
+      @scope = scope
+      @data = data
+      @relations = []
+    end
+
+    def add_hooks(relation)
+      @relations << relation
+      self
+    end
+
+    def execute
+      # Assumes that the relations implement TriggerableHooks
+      @relations.each do |hooks|
+        hooks.hooks_for(@scope).select_active(@scope, @data).each do |hook|
+          hook.async_execute(@data, @scope.to_s)
+        end
+      end
+    end
+  end
+end

ee/app/controllers/groups/hooks_controller.rb

@@ -77,6 +77,7 @@ class Groups::HooksController < Groups::ApplicationController
       :enable_ssl_verification,
       :token,
       :url,
+      :push_events_branch_filter,
       *GroupHook.triggers.values
     )
   end

ee/app/models/ee/project.rb

@@ -459,17 +459,12 @@ module EE
       end
     end
 
-    override :execute_hooks
-    def execute_hooks(data, hooks_scope = :push_hooks)
-      super
+    override :triggered_hooks
+    def triggered_hooks(scope, data)
+      triggered = super
+      triggered.add_hooks(group_hooks) if group && feature_available?(:group_webhooks)
 
-      if group && feature_available?(:group_webhooks)
-        run_after_commit_or_now do
-          group_hooks.hooks_for(hooks_scope).each do |hook|
-            hook.async_execute(data, hooks_scope.to_s)
-          end
-        end
-      end
+      triggered
     end
 
     # No need to have a Kerberos Web url. Kerberos URL will be used only to

ee/lib/api/group_hooks.rb

@@ -13,6 +13,7 @@ module API
       params :group_hook_properties do
         requires :url, type: String, desc: "The URL to send the request to"
         optional :push_events, type: Boolean, desc: "Trigger hook on push events"
+        optional :push_events_branch_filter, type: String, desc: "Respond to push events only on branches that match this filter"
         optional :issues_events, type: Boolean, desc: "Trigger hook on issues events"
         optional :confidential_issues_events, type: Boolean, desc: "Trigger hook on confidential issues events"
         optional :merge_requests_events, type: Boolean, desc: "Trigger hook on merge request events"

ee/lib/ee/api/entities/group_hook.rb

@@ -7,6 +7,7 @@ module EE
         expose :group_id, :issues_events, :confidential_issues_events,
                :note_events, :confidential_note_events, :pipeline_events, :wiki_page_events,
                :job_events, :deployment_events, :releases_events, :subgroup_events
+        expose :push_events_branch_filter
       end
     end
   end

ee/spec/controllers/groups/hooks_controller_spec.rb

@@ -17,7 +17,7 @@ RSpec.describe Groups::HooksController do
     end
 
     describe 'GET #index' do
-      it 'is successfull' do
+      it 'is successful' do
         get :index, params: { group_id: group.to_param }
 
         expect(response).to have_gitlab_http_status(:ok)
@@ -27,21 +27,24 @@ RSpec.describe Groups::HooksController do
     describe 'POST #create' do
       it 'sets all parameters' do
         hook_params = {
+          # triggers
           job_events: true,
           confidential_issues_events: true,
-          enable_ssl_verification: true,
           issues_events: true,
           merge_requests_events: true,
           note_events: true,
           pipeline_events: true,
           push_events: true,
           tag_push_events: true,
-          token: 'TEST TOKEN',
-          url: 'http://example.com',
           wiki_page_events: true,
           deployment_events: true,
           member_events: true,
-          subgroup_events: true
+          subgroup_events: true,
+          # editable attributes
+          enable_ssl_verification: true,
+          token: 'TEST TOKEN',
+          url: 'http://example.com',
+          push_events_branch_filter: 'filter-branch'
         }
 
         post :create, params: { group_id: group.to_param, hook: hook_params }
@@ -55,7 +58,7 @@ RSpec.describe Groups::HooksController do
     describe 'GET #edit' do
       let(:hook) { create(:group_hook, group: group) }
 
-      it 'is successfull' do
+      it 'is successful' do
         get :edit, params: { group_id: group.to_param, id: hook }
 
         expect(response).to have_gitlab_http_status(:ok)

ee/spec/features/groups/hooks/user_edits_hooks_spec.rb

@@ -23,10 +23,17 @@ RSpec.describe 'User edits hooks' do
     expect(page).to have_current_path(edit_group_hook_path(group, hook), ignore_query: true)
 
     fill_in('URL', with: url)
+    fill_in('hook[push_events_branch_filter]', with: 'notify-on-branch')
+    page.check('hook[push_events]')
 
     click_button('Save changes')
 
-    expect(hook.reload.url).to eq(url)
+    expect(hook.reload).to have_attributes(
+      url: eq(url),
+      push_events_branch_filter: eq('notify-on-branch'),
+      push_events: eq(true)
+    )
+
     expect(page).to have_current_path(group_hooks_path(group), ignore_query: true)
     expect(page).to have_selector('[data-testid="alert-info"]', text: 'Hook was successfully updated.')
   end

ee/spec/fixtures/api/schemas/public_api/v4/group_hook.json

@@ -5,6 +5,7 @@
     "url",
     "created_at",
     "push_events",
+    "push_events_branch_filter",
     "tag_push_events",
     "merge_requests_events",
     "repository_update_events",
@@ -27,6 +28,7 @@
     "url": { "type": "string" },
     "created_at": { "type": "string", "format": "date-time" },
     "push_events": { "type": "boolean" },
+    "push_events_branch_filter": { "type": ["string", "null"] },
     "tag_push_events": { "type": "boolean" },
     "merge_requests_events": { "type": "boolean" },
     "repository_update_events": { "type": "boolean" },

ee/spec/models/project_spec.rb

@@ -984,8 +984,8 @@ RSpec.describe Project do
       it 'does not execute the hook when the feature is disabled' do
         stub_licensed_features(group_webhooks: false)
 
-        expect(WebHookService).not_to receive(:new)
-                                        .with(group_hook, { some: 'info' }, 'push_hooks')
+        expect(project).not_to receive(:group_hooks)
+        expect(WebHookService).not_to receive(:new).with(instance_of(GroupHook), anything, anything)
 
         project.execute_hooks(some: 'info')
       end
@@ -994,19 +994,36 @@ RSpec.describe Project do
         before do
           stub_licensed_features(group_webhooks: true)
         end
-        let(:fake_integration) { double }
+        let(:fake_wh_service) { double }
 
         shared_examples 'triggering group webhook' do
           it 'executes the hook' do
-            expect(fake_integration).to receive(:async_execute).once
+            expect(fake_wh_service).to receive(:async_execute).once
 
             expect(WebHookService)
-              .to receive(:new).with(group_hook, { some: 'info' }, 'push_hooks') { fake_integration }
+              .to receive(:new).with(group_hook, { some: 'info' }, 'push_hooks') { fake_wh_service }
 
             project.execute_hooks(some: 'info')
           end
         end
 
+        context 'when the hook defines a branch filter for push events' do
+          let(:wh_service) { double(async_execute: true) }
+          let(:selective_hook) { create(:group_hook, group: group, push_events: true, push_events_branch_filter: 'on-this-branch-only') }
+
+          it 'respects the branch filter' do
+            expect(WebHookService)
+              .to receive(:new).twice.with(group_hook, Hash, 'push_hooks').and_return(wh_service)
+
+            expect(WebHookService)
+              .to receive(:new).once.with(selective_hook, a_hash_including(note: 'matches-filter'), 'push_hooks').and_return(wh_service)
+
+            project.execute_hooks({ note: 'matches-filter', ref: 'refs/heads/on-this-branch-only' }, :push_hooks)
+            project.execute_hooks({ note: 'default-branch', ref: 'refs/heads/master' }, :push_hooks)
+            project.execute_hooks({ note: 'not-push', ref: 'refs/heads/on-this-branch-only' }, :deployment_hooks)
+          end
+        end
+
         it_behaves_like 'triggering group webhook'
 
         context 'in sub group' do

ee/spec/requests/api/group_hooks_spec.rb

@@ -110,6 +110,7 @@ RSpec.describe API::GroupHooks do
       {
         url: "http://example.com",
         push_events: true,
+        push_events_branch_filter: 'only-on-this-branch',
         issues_events: true,
         confidential_issues_events: true,
         merge_requests_events: true,
@@ -138,6 +139,7 @@ RSpec.describe API::GroupHooks do
         expect(json_response['issues_events']).to eq(true)
         expect(json_response['confidential_issues_events']).to eq(true)
         expect(json_response['push_events']).to eq(true)
+        expect(json_response['push_events_branch_filter']).to eq('only-on-this-branch')
         expect(json_response['merge_requests_events']).to eq(true)
         expect(json_response['tag_push_events']).to eq(true)
         expect(json_response['note_events']).to eq(true)
@@ -189,13 +191,23 @@ RSpec.describe API::GroupHooks do
   end
 
   describe "PUT /groups/:id/hooks/:hook_id" do
+    before do
+      hook_params.merge!(
+        push_events: true,
+        push_events_branch_filter: 'updated-branch-filter',
+        issues_events: false
+      )
+    end
+
     context "authorized user" do
       it "updates the hook" do
-        make_put_group_hook_request(group.id, hook.id, group_admin, hook_params.merge({ push_events: false }))
+        make_put_group_hook_request(group.id, hook.id, group_admin, hook_params)
 
         expect(response).to have_gitlab_http_status(:ok)
         expect(response).to match_response_schema('public_api/v4/group_hook', dir: 'ee')
-        expect(json_response['push_events']).to eq(false)
+        expect(json_response['push_events']).to eq(true)
+        expect(json_response['issues_events']).to eq(false)
+        expect(json_response['push_events_branch_filter']).to eq('updated-branch-filter')
       end
 
       it "returns 422 if url is not valid" do

spec/factories/project_hooks.rb

@@ -25,5 +25,9 @@ FactoryBot.define do
       feature_flag_events { true }
       releases_events { true }
     end
+
+    trait :with_push_branch_filter do
+      push_events_branch_filter { 'my-branch-*' }
+    end
   end
 end

spec/models/projects/triggered_hooks_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Projects::TriggeredHooks do
+  let_it_be(:project) { create(:project) }
+
+  let_it_be(:universal_push_hook) { create(:project_hook, project: project, push_events: true) }
+  let_it_be(:selective_push_hook) { create(:project_hook, :with_push_branch_filter, project: project, push_events: true) }
+  let_it_be(:issues_hook) { create(:project_hook, project: project, issues_events: true, push_events: false) }
+
+  let(:wh_service) { instance_double(::WebHookService, async_execute: true) }
+
+  def run_hooks(scope, data)
+    hooks = described_class.new(scope, data)
+    hooks.add_hooks(ProjectHook.all)
+    hooks.execute
+  end
+
+  it 'executes hooks by scope' do
+    data = { some: 'data', as: 'json' }
+
+    expect_hook_execution(issues_hook, data, 'issue_hooks')
+
+    run_hooks(:issue_hooks, data)
+  end
+
+  it 'applies branch filters, when they match' do
+    data = { some: 'data', as: 'json', ref: "refs/heads/#{generate(:branch)}" }
+
+    expect_hook_execution(universal_push_hook, data, 'push_hooks')
+    expect_hook_execution(selective_push_hook, data, 'push_hooks')
+
+    run_hooks(:push_hooks, data)
+  end
+
+  it 'applies branch filters, when they do not match' do
+    data = { some: 'data', as: 'json', ref: "refs/heads/master}" }
+
+    expect_hook_execution(universal_push_hook, data, 'push_hooks')
+
+    run_hooks(:push_hooks, data)
+  end
+
+  def expect_hook_execution(hook, data, scope)
+    expect(WebHookService).to receive(:new).with(hook, data, scope).and_return(wh_service)
+  end
+end