Merge branch 'dm-project-system-hooks-in-transaction' into 'master'

Execute system hooks after-commit when executing project hooks

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

4 files changed, 27 insertions, 3 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 0570bbc8ee3..e19873f64ce 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -971,9 +971,9 @@ class Project < ActiveRecord::Base
       hooks.hooks_for(hooks_scope).each do |hook|
         hook.async_execute(data, hooks_scope.to_s)
       end
-    end
 
-    SystemHooksService.new.execute_hooks(data, hooks_scope)
+      SystemHooksService.new.execute_hooks(data, hooks_scope)
+    end
   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 4d10df410ab..31dcb543cbd 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3228,5 +3228,22 @@ describe Project do
       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
 end
diff --git a/spec/services/issues/move_service_spec.rb b/spec/services/issues/move_service_spec.rb
index f3c98fa5416..388c9d63c7b 100644
--- a/spec/services/issues/move_service_spec.rb
+++ b/spec/services/issues/move_service_spec.rb
@@ -297,9 +297,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.