Merge branch 'execute_parent_group_webhooks' into 'master'

Fix group hook triggering from subgroup project

Closes #2372 and #12097

See merge request gitlab-org/gitlab!23333

db/migrate/20200128141125_add_index_web_hooks_on_group_id.rb

+# frozen_string_literal: true
+
+class AddIndexWebHooksOnGroupId < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :web_hooks, :group_id, where: "type = 'GroupHook'"
+  end
+
+  def down
+    remove_concurrent_index :web_hooks, :group_id, where: "type = 'GroupHook'"
+  end
+end

db/schema.rb

@@ -4477,6 +4477,7 @@ ActiveRecord::Schema.define(version: 2020_02_05_143231) do
     t.string "encrypted_token_iv"
     t.string "encrypted_url"
     t.string "encrypted_url_iv"
+    t.index ["group_id"], name: "index_web_hooks_on_group_id", where: "((type)::text = 'GroupHook'::text)"
     t.index ["project_id"], name: "index_web_hooks_on_project_id"
     t.index ["type"], name: "index_web_hooks_on_type"
   end

ee/app/models/ee/project.rb

@@ -358,7 +358,7 @@ module EE
     def has_group_hooks?(hooks_scope = :push_hooks)
       return unless group && feature_available?(:group_webhooks)
 
-      group.hooks.hooks_for(hooks_scope).any?
+      group_hooks.hooks_for(hooks_scope).any?
     end
 
     def execute_hooks(data, hooks_scope = :push_hooks)
@@ -366,7 +366,7 @@ module EE
 
       if group && feature_available?(:group_webhooks)
         run_after_commit_or_now do
-          group.hooks.hooks_for(hooks_scope).each do |hook|
+          group_hooks.hooks_for(hooks_scope).each do |hook|
             hook.async_execute(data, hooks_scope.to_s)
           end
         end
@@ -737,6 +737,12 @@ module EE
 
     private
 
+    def group_hooks
+      return group.hooks unless ::Feature.enabled?(:sub_group_webhooks, self)
+
+      GroupHook.where(group_id: group.self_and_ancestors)
+    end
+
     def set_override_pull_mirror_available
       self.pull_mirror_available_overridden = read_attribute(:mirror)
       true

ee/changelogs/unreleased/execute_parent_group_webhooks.yml

+---
+title: Fix group hook triggering from subgroup project
+merge_request: 23333
+author:
+type: fixed

ee/spec/models/project_spec.rb

@@ -506,23 +506,71 @@ describe Project do
     end
   end
 
-  describe "#execute_hooks" do
-    context "group hooks" do
+  describe '#has_group_hooks?' do
+    subject { project.has_group_hooks? }
+
+    let(:project) { create(:project) }
+
+    it { is_expected.to eq(nil) }
+
+    context 'project is in a group' do
       let(:group) { create(:group) }
       let(:project) { create(:project, namespace: group) }
-      let(:group_hook) { create(:group_hook, group: group, push_events: true) }
 
-      it 'executes the hook when the feature is enabled' do
-        stub_licensed_features(group_webhooks: true)
+      shared_examples 'returns nil when the feature is not available' do
+        specify do
+          stub_licensed_features(group_webhooks: false)
 
-        fake_service = double
-        expect(WebHookService).to receive(:new)
-                                    .with(group_hook, { some: 'info' }, 'push_hooks') { fake_service }
-        expect(fake_service).to receive(:async_execute)
+          expect(subject).to eq(nil)
+        end
+      end
 
-        project.execute_hooks(some: 'info')
+      it_behaves_like 'returns nil when the feature is not available'
+
+      it { is_expected.to eq(false) }
+
+      context 'the group has hooks' do
+        let!(:group_hook) { create(:group_hook, group: group, push_events: true) }
+
+        it { is_expected.to eq(true) }
+
+        it_behaves_like 'returns nil when the feature is not available'
+
+        context 'but the hook is not in scope' do
+          subject { project.has_group_hooks?(:issue_hooks) }
+
+          it_behaves_like 'returns nil when the feature is not available'
+
+          it { is_expected.to eq(false) }
+        end
       end
 
+      context 'the group inherits a hook' do
+        let(:parent_group) { create(:group) }
+        let!(:group_hook) { create(:group_hook, group: parent_group) }
+        let(:group) { create(:group, parent: parent_group) }
+
+        it_behaves_like 'returns nil when the feature is not available'
+
+        it { is_expected.to eq(true) }
+
+        context 'when sub_group_webhooks feature flag is disabled' do
+          before do
+            stub_feature_flags(sub_group_webhooks: false)
+          end
+
+          it { is_expected.to eq(false) }
+        end
+      end
+    end
+  end
+
+  describe "#execute_hooks" do
+    context "group hooks" do
+      let(:group) { create(:group) }
+      let(:project) { create(:project, namespace: group) }
+      let(:group_hook) { create(:group_hook, group: group, push_events: true) }
+
       it 'does not execute the hook when the feature is disabled' do
         stub_licensed_features(group_webhooks: false)
 
@@ -531,23 +579,54 @@ describe Project do
 
         project.execute_hooks(some: 'info')
       end
-    end
-  end
 
-  describe '#execute_hooks' do
-    it "triggers project and group hooks" do
-      group = create :group, name: 'gitlab'
-      project = create(:project, name: 'gitlabhq', namespace: group)
-      project_hook = create(:project_hook, push_events: true, project: project)
-      group_hook = create(:group_hook, push_events: true, group: group)
+      context 'when group_webhooks frature is enabled' do
+        before do
+          stub_licensed_features(group_webhooks: true)
+        end
+        let(:fake_service) { double }
+
+        shared_examples 'triggering group webhook' do
+          it 'executes the hook' do
+            expect(fake_service).to receive(:async_execute).once
 
-      stub_request(:post, project_hook.url)
-      stub_request(:post, group_hook.url)
+            expect(WebHookService).to receive(:new)
+                                        .with(group_hook, { some: 'info' }, 'push_hooks') { fake_service }
+
+            project.execute_hooks(some: 'info')
+          end
+        end
 
-      expect_any_instance_of(GroupHook).to receive(:async_execute).and_return(true)
-      expect_any_instance_of(ProjectHook).to receive(:async_execute).and_return(true)
+        it_behaves_like 'triggering group webhook'
 
-      project.execute_hooks({}, :push_hooks)
+        context 'when sub_group_webhooks feature flag is disabled' do
+          before do
+            stub_feature_flags(sub_group_webhooks: false)
+          end
+
+          it_behaves_like 'triggering group webhook'
+        end
+
+        context 'in sub group' do
+          let(:sub_group) { create :group, parent: group }
+          let(:sub_sub_group) { create :group, parent: sub_group }
+          let(:project) { create(:project, namespace: sub_sub_group) }
+
+          it_behaves_like 'triggering group webhook'
+
+          context 'when sub_group_webhooks feature flag is disabled' do
+            before do
+              stub_feature_flags(sub_group_webhooks: false)
+            end
+
+            it 'does not execute the hook' do
+              expect(WebHookService).not_to receive(:new).with(group_hook, { some: 'info' }, 'push_hooks')
+
+              project.execute_hooks(some: 'info')
+            end
+          end
+        end
+      end
     end
   end