Merge branch 'dm-project-system-hooks-in-transaction' into 'master'10-4-stable-patch-2-conflicts

Execute system hooks after-commit when executing project hooks

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

4 files changed, 77 insertions, 1 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 02f37135670..248ae8dae72 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -971,7 +971,17 @@ class Project < ActiveRecord::Base
       hooks.public_send(hooks_scope).each do |hook| # rubocop:disable GitlabSecurity/PublicSend
         hook.async_execute(data, hooks_scope.to_s)
       end
+<<<<<<< HEAD
     end
+||||||| parent of 956c55e2abf... Merge branch 'dm-project-system-hooks-in-transaction' into 'master'
+    end
+
+    SystemHooksService.new.execute_hooks(data, hooks_scope)
+=======
+
+      SystemHooksService.new.execute_hooks(data, hooks_scope)
+    end
+>>>>>>> 956c55e2abf... Merge branch 'dm-project-system-hooks-in-transaction' into 'master'
   end
 
   def execute_services(data, hooks_scope = :push_hooks)
diff --git a/changelogs/unreleased/dm-project-system-hooks-in-transaction.yml b/changelogs/unreleased/dm-project-system-hooks-in-transaction.yml
new file mode 100644
index 00000000000..f59021c0ec9
--- /dev/null
+++ b/changelogs/unreleased/dm-project-system-hooks-in-transaction.yml
@@ -0,0 +1,5 @@
+---
+title: Execute system hooks after-commit when executing project hooks
+merge_request:
+author:
+type: fixed
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 6f24dd6811e..ccdac7df8eb 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3211,4 +3211,63 @@ describe Project do
       expect { project.write_repository_config }.not_to raise_error
     end
   end
+<<<<<<< HEAD
+||||||| parent of 956c55e2abf... Merge branch 'dm-project-system-hooks-in-transaction' into 'master'
+
+  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
+=======
+
+  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
+
+    it 'executes the system hooks when inside a transaction' do
+      allow_any_instance_of(WebHookService).to receive(:execute)
+
+      create(:system_hook, merge_requests_events: true)
+
+      project = build(:project)
+
+      # Ideally, we'd test that `WebHookWorker.jobs.size` increased by 1,
+      # but since the entire spec run takes place in a transaction, we never
+      # actually get to the `after_commit` hook that queues these jobs.
+      expect do
+        project.transaction do
+          project.execute_hooks({ data: 'data' }, :merge_request_hooks)
+        end
+      end.not_to raise_error # Sidekiq::Worker::EnqueueFromTransactionError
+    end
+  end
+>>>>>>> 956c55e2abf... Merge branch 'dm-project-system-hooks-in-transaction' into 'master'
 end
diff --git a/spec/services/issues/move_service_spec.rb b/spec/services/issues/move_service_spec.rb
index 53ea88332fb..f0828072fb9 100644
--- a/spec/services/issues/move_service_spec.rb
+++ b/spec/services/issues/move_service_spec.rb
@@ -291,9 +291,11 @@ describe Issues::MoveService do
       end
 
       context 'project issue hooks' do
-        let(:hook) { create(:project_hook, project: old_project, issues_events: true) }
+        let!(:hook) { create(:project_hook, project: old_project, issues_events: true) }
 
         it 'executes project issue hooks' do
+          allow_any_instance_of(WebHookService).to receive(:execute)
+
           # Ideally, we'd test that `WebHookWorker.jobs.size` increased by 1,
           # but since the entire spec run takes place in a transaction, we never
           # actually get to the `after_commit` hook that queues these jobs.