FIX: Should not generate payload until active webhooks are exist

FIX: Should not generate payload until active webhooks are exist

diff --git a/app/models/concerns/has_destroyed_web_hook.rb b/app/models/concerns/has_destroyed_web_hook.rb
index 98d165d..081189f 100644
--- a/app/models/concerns/has_destroyed_web_hook.rb
+++ b/app/models/concerns/has_destroyed_web_hook.rb
@@ -7,7 +7,7 @@ module HasDestroyedWebHook
 
   def enqueue_destroyed_web_hook
     type = self.class.name.underscore.to_sym
-    payload = WebHook.generate_payload(type, self)
+    payload = WebHook.generate_payload(type, self) if WebHook.active_web_hooks(type).exists?
     yield
     WebHook.enqueue_hooks(type, "#{type}_destroyed".to_sym,
       id: id,
diff --git a/lib/post_destroyer.rb b/lib/post_destroyer.rb
index fee0fc7..9509a96 100644
--- a/lib/post_destroyer.rb
+++ b/lib/post_destroyer.rb
@@ -45,12 +45,12 @@ class PostDestroyer
   end
 
   def destroy
-    payload = WebHook.generate_payload(:post, @post)
+    payload = WebHook.generate_payload(:post, @post) if WebHook.active_web_hooks(:post).exists?
     topic = @post.topic
 
     if @post.is_first_post? && topic
       topic_view = TopicView.new(topic.id, Discourse.system_user)
-      topic_payload = WebHook.generate_payload(:topic, topic_view, WebHookTopicViewSerializer)
+      topic_payload = WebHook.generate_payload(:topic, topic_view, WebHookTopicViewSerializer) if WebHook.active_web_hooks(:topic).exists?
     end
 
     delete_removed_posts_after = @opts[:delete_removed_posts_after] || SiteSetting.delete_removed_posts_after
diff --git a/spec/models/web_hook_spec.rb b/spec/models/web_hook_spec.rb
index 67e0f91..f424cb0 100644
--- a/spec/models/web_hook_spec.rb
+++ b/spec/models/web_hook_spec.rb
@@ -125,10 +125,13 @@ describe WebHook do
     end
 
     describe 'when there are no active hooks' do
-      it 'should not enqueue anything' do
+      it 'should not generate payload and enqueue anything' do
         topic_web_hook.destroy!
         post = PostCreator.create(user, raw: 'post', title: 'topic', skip_validations: true)
         expect(Jobs::EmitWebHookEvent.jobs.length).to eq(0)
+
+        WebHook.expects(:generate_payload).times(0)
+        PostDestroyer.new(admin, post).destroy
       end
     end
 
@@ -384,6 +387,12 @@ describe WebHook do
       expect(payload["id"]).to eq(tag.id)
     end
 
+    it 'should not generate payload if webhooks not exist' do
+      WebHook.expects(:generate_payload).times(0)
+      tag = Fabricate(:tag)
+      tag.destroy!
+    end
+
     it 'should enqueue the right hooks for flag events' do
       post = Fabricate(:post)
       admin = Fabricate(:admin)

GitHub sha: f2c34155

1 Like

Do we need to en-queue the hook if the web hook isn’t active? This code here seems very odd to me that if we’re en-queuing a hook with an empty payload which will probably end up doing nothing.

enqueue_hooks method already filtering and executing only for active_web_hooks. Do you think even we shouldn’t call it?

Yea If the web hook isn’t even active, I think we can just yield and avoid doing work that doesn’t need to be done.

1 Like

It is done.

I think we should check whether Sidekiq jobs are en-queued instead of using mocks here.

It is changed.