Refactor: move active hook filter to TriggerableHooks

6 files changed, 54 insertions, 22 deletions

diff --git a/app/models/concerns/triggerable_hooks.rb b/app/models/concerns/triggerable_hooks.rb
index f55ab2fcaf3..9f2e8b420bc 100644
--- a/app/models/concerns/triggerable_hooks.rb
+++ b/app/models/concerns/triggerable_hooks.rb
@@ -28,6 +28,12 @@ module TriggerableHooks
       public_send(trigger) # rubocop:disable GitlabSecurity/PublicSend
     end
 
+    def select_active(hooks_scope, data)
+      select do |hook|
+        ActiveHookFilter.new(hook).matches?(hooks_scope, data)
+      end
+    end
+
     private
 
     def triggerable_hooks(hooks)
diff --git a/app/models/project.rb b/app/models/project.rb
index 72a53e1a50d..728bbffd671 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1163,7 +1163,7 @@ class Project < ActiveRecord::Base
 
   def execute_hooks(data, hooks_scope = :push_hooks)
     run_after_commit_or_now do
-      hooks.hooks_for(hooks_scope).select {|hook| ActiveHookFilter.new(hook).matches?(hooks_scope, data)}.each do |hook|
+      hooks.hooks_for(hooks_scope).select_active(hooks_scope, data).each do |hook|
         hook.async_execute(data, hooks_scope.to_s)
       end
       SystemHooksService.new.execute_hooks(data, hooks_scope)
diff --git a/app/validators/branch_filter_validator.rb b/app/validators/branch_filter_validator.rb
index 0965be3d101..ef482aaaa63 100644
--- a/app/validators/branch_filter_validator.rb
+++ b/app/validators/branch_filter_validator.rb
@@ -16,6 +16,7 @@ class BranchFilterValidator < ActiveModel::EachValidator
 
     if value.present?
       value_without_wildcards = value.tr('*', 'x')
+
       unless Gitlab::GitRefValidator.validate(value_without_wildcards)
         record.errors[attribute] << "is not a valid branch name"
       end
diff --git a/spec/models/concerns/triggerable_hooks_spec.rb b/spec/models/concerns/triggerable_hooks_spec.rb
index 621d2d38eae..265abd6bd72 100644
--- a/spec/models/concerns/triggerable_hooks_spec.rb
+++ b/spec/models/concerns/triggerable_hooks_spec.rb
@@ -40,4 +40,28 @@ RSpec.describe TriggerableHooks do
       end
     end
   end
+
+  describe '.select_active' do
+    it 'returns hooks that match the active filter' do
+      TestableHook.create!(url: 'http://example1.com', push_events: true)
+      TestableHook.create!(url: 'http://example2.com', push_events: true)
+      filter1 = double(:filter1)
+      filter2 = double(:filter2)
+      allow(ActiveHookFilter).to receive(:new).exactly(2).times.and_return(filter1, filter2)
+      expect(filter1).to receive(:matches?).and_return(true)
+      expect(filter2).to receive(:matches?).and_return(false)
+
+      hooks = TestableHook.push_hooks.order_id_asc
+      expect(hooks.select_active(:push_hooks, {})).to eq [hooks.first]
+    end
+
+    it 'returns empty list if no hooks match the active filter' do
+      TestableHook.create!(url: 'http://example1.com', push_events: true)
+      filter = double(:filter)
+      allow(ActiveHookFilter).to receive(:new).and_return(filter)
+      expect(filter).to receive(:matches?).and_return(false)
+
+      expect(TestableHook.push_hooks.select_active(:push_hooks, {})).to eq []
+    end
+  end
 end
diff --git a/spec/models/hooks/active_hook_filter_spec.rb b/spec/models/hooks/active_hook_filter_spec.rb
index c97003eb542..df7edda2213 100644
--- a/spec/models/hooks/active_hook_filter_spec.rb
+++ b/spec/models/hooks/active_hook_filter_spec.rb
@@ -11,28 +11,29 @@ describe ActiveHookFilter do
 
       context 'branch filter is specified' do
         let(:branch_filter) { 'master' }
+
         it 'returns true if branch matches' do
-          expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to eq(true)
+          expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to be true
         end
 
         it 'returns false if branch does not match' do
-          expect(filter.matches?(:push_hooks, { ref: 'refs/heads/my_branch' })).to eq(false)
+          expect(filter.matches?(:push_hooks, { ref: 'refs/heads/my_branch' })).to be false
         end
 
         it 'returns false if ref is nil' do
-          expect(filter.matches?(:push_hooks, {})).to eq(false)
+          expect(filter.matches?(:push_hooks, {})).to be false
         end
 
         context 'branch filter contains wildcard' do
           let(:branch_filter) { 'features/*' }
 
           it 'returns true if branch matches' do
-            expect(filter.matches?(:push_hooks, { ref: 'refs/heads/features/my-branch' })).to eq(true)
-            expect(filter.matches?(:push_hooks, { ref: 'refs/heads/features/my-branch/something' })).to eq(true)
+            expect(filter.matches?(:push_hooks, { ref: 'refs/heads/features/my-branch' })).to be true
+            expect(filter.matches?(:push_hooks, { ref: 'refs/heads/features/my-branch/something' })).to be true
           end
 
           it 'returns false if branch does not match' do
-            expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to eq(false)
+            expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to be false
           end
         end
       end
@@ -41,7 +42,7 @@ describe ActiveHookFilter do
         let(:branch_filter) { nil }
 
         it 'returns true' do
-          expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to eq(true)
+          expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to be true
         end
       end
 
@@ -49,7 +50,7 @@ describe ActiveHookFilter do
         let(:branch_filter) { '' }
 
         it 'acts like branch is not specified' do
-          expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to eq(true)
+          expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to be true
         end
       end
     end
@@ -60,7 +61,7 @@ describe ActiveHookFilter do
       end
 
       it 'returns true as branch filters are not yet supported for these' do
-        expect(filter.matches?(:issues_events, { ref: 'refs/heads/master' })).to eq(true)
+        expect(filter.matches?(:issues_events, { ref: 'refs/heads/master' })).to be true
       end
     end
   end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index cea7fa31b16..d3dc95548aa 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3672,36 +3672,36 @@ describe Project do
   describe '#execute_hooks' do
     let(:data) { { ref: 'refs/heads/master', data: 'data' } }
     it 'executes active projects hooks with the specified scope' do
-      expect_any_instance_of(ActiveHookFilter).to receive(:matches?)
-        .with(:tag_push_hooks, data)
-        .and_return(true)
-      hook = create(:project_hook, merge_requests_events: false, tag_push_events: true)
+      hook = create(:project_hook, merge_requests_events: false, push_events: true)
+      expect(ProjectHook).to receive(:select_active)
+        .with(:push_hooks, data)
+        .and_return([hook])
       project = create(:project, hooks: [hook])
 
       expect_any_instance_of(ProjectHook).to receive(:async_execute).once
 
-      project.execute_hooks(data, :tag_push_hooks)
+      project.execute_hooks(data, :push_hooks)
     end
 
     it 'does not execute project hooks that dont match the specified scope' do
-      hook = create(:project_hook, merge_requests_events: true, tag_push_events: false)
+      hook = create(:project_hook, merge_requests_events: true, push_events: false)
       project = create(:project, hooks: [hook])
 
       expect_any_instance_of(ProjectHook).not_to receive(:async_execute).once
 
-      project.execute_hooks(data, :tag_push_hooks)
+      project.execute_hooks(data, :push_hooks)
     end
 
     it 'does not execute project hooks which are not active' do
-      expect_any_instance_of(ActiveHookFilter).to receive(:matches?)
-        .with(:tag_push_hooks, data)
-        .and_return(false)
-      hook = create(:project_hook, tag_push_events: true)
+      hook = create(:project_hook, push_events: true)
+      expect(ProjectHook).to receive(:select_active)
+        .with(:push_hooks, data)
+        .and_return([])
       project = create(:project, hooks: [hook])
 
       expect_any_instance_of(ProjectHook).not_to receive(:async_execute).once
 
-      project.execute_hooks(data, :tag_push_hooks)
+      project.execute_hooks(data, :push_hooks)
     end
 
     it 'executes the system hooks with the specified scope' do