FIX: Payload for webhooks should be current as of the time the event was triggered.

https://meta.discourse.org/t/group-category-tag-user-deleted-webhooks-not-firing/87752

spec/jobs/emit_web_hook_event_spec.rb

@@ -7,21 +7,41 @@ describe Jobs::EmitWebHookEvent do
   let(:user) { Fabricate(:user) }
 
   it 'raises an error when there is no web hook record' do
-    expect { subject.execute(event_type: 'post') }.to raise_error(Discourse::InvalidParameters)
+    expect do
+      subject.execute(event_type: 'post', payload: {})
+    end.to raise_error(Discourse::InvalidParameters)
   end
 
   it 'raises an error when there is no event type' do
-    expect { subject.execute(web_hook_id: 1) }.to raise_error(Discourse::InvalidParameters)
+    expect do
+      subject.execute(web_hook_id: 1, payload: {})
+    end.to raise_error(Discourse::InvalidParameters)
+  end
+
+  it 'raises an error when there is no payload' do
+    expect do
+      subject.execute(web_hook_id: 1, event_type: 'post')
+    end.to raise_error(Discourse::InvalidParameters)
   end
 
   it "doesn't emit when the hook is inactive" do
-    Jobs::EmitWebHookEvent.any_instance.expects(:web_hook_request).never
-    subject.execute(web_hook_id: inactive_hook.id, event_type: 'post', post_id: post.id)
+    subject.execute(
+      web_hook_id: inactive_hook.id,
+      event_type: 'post',
+      payload: { test: "some payload" }.to_json
+    )
   end
 
   it 'emits normally with sufficient arguments' do
-    Jobs::EmitWebHookEvent.any_instance.expects(:web_hook_request).once
-    subject.execute(web_hook_id: post_hook.id, event_type: 'post', post_id: post.id)
+    stub_request(:post, "https://meta.discourse.org/webhook_listener")
+      .with(body: "{\"post\":{\"test\":\"some payload\"}}")
+      .to_return(body: 'OK', status: 200)
+
+    subject.execute(
+      web_hook_id: post_hook.id,
+      event_type: 'post',
+      payload: { test: "some payload" }.to_json
+    )
   end
 
   context 'with category filters' do
@@ -31,69 +51,64 @@ describe Jobs::EmitWebHookEvent do
     let(:topic_hook) { Fabricate(:topic_web_hook, categories: [category]) }
 
     it "doesn't emit when event is not related with defined categories" do
-      Jobs::EmitWebHookEvent.any_instance.expects(:web_hook_request).never
-
-      subject.execute(web_hook_id: topic_hook.id,
-                      event_type: 'topic',
-                      topic_id: topic.id,
-                      user_id: user.id,
-                      category_id: topic.category.id)
+      subject.execute(
+        web_hook_id: topic_hook.id,
+        event_type: 'topic',
+        category_id: topic.category.id,
+        payload: { test: "some payload" }.to_json
+      )
     end
 
     it 'emit when event is related with defined categories' do
-      Jobs::EmitWebHookEvent.any_instance.expects(:web_hook_request).once
+      stub_request(:post, "https://meta.discourse.org/webhook_listener")
+        .with(body: "{\"topic\":{\"test\":\"some payload\"}}")
+        .to_return(body: 'OK', status: 200)
 
-      subject.execute(web_hook_id: topic_hook.id,
-                      event_type: 'topic',
-                      topic_id: topic_with_category.id,
-                      user_id: user.id,
-                      category_id: topic_with_category.category.id)
+      subject.execute(
+        web_hook_id: topic_hook.id,
+        event_type: 'topic',
+        category_id: topic_with_category.category.id,
+        payload: { test: "some payload" }.to_json
+      )
     end
   end
 
-  describe '.web_hook_request' do
+  describe '#web_hook_request' do
     it 'creates delivery event record' do
       stub_request(:post, "https://meta.discourse.org/webhook_listener")
         .to_return(body: 'OK', status: 200)
 
       WebHookEventType.all.pluck(:name).each do |name|
         web_hook_id = Fabricate("#{name}_web_hook").id
-        object_id = Fabricate(name).id
 
         expect do
-          subject.execute(web_hook_id: web_hook_id, event_type: name, "#{name}_id": object_id)
+          subject.execute(
+            web_hook_id: web_hook_id,
+            event_type: name,
+            payload: { test: "some payload" }.to_json
+          )
         end.to change(WebHookEvent, :count).by(1)
       end
     end
 
-    it 'skips silently on missing post' do
-      expect do
-        subject.execute(web_hook_id: post_hook.id, event_type: 'post', post_id: (Post.maximum(:id).to_i + 1))
-      end.not_to raise_error
-    end
-
-    it 'should not skip trashed post' do
-      stub_request(:post, "https://meta.discourse.org/webhook_listener")
-        .to_return(body: 'OK', status: 200)
-
-      expect do
-        post.trash!
-        subject.execute(web_hook_id: post_hook.id, event_type: 'post', post_id: post.id)
-      end.to change(WebHookEvent, :count).by(1)
-    end
-
     it 'sets up proper request headers' do
       stub_request(:post, "https://meta.discourse.org/webhook_listener")
         .to_return(headers: { test: 'string' }, body: 'OK', status: 200)
 
-      subject.execute(web_hook_id: post_hook.id, event_type: 'ping', event_name: 'ping')
+      subject.execute(
+        web_hook_id: post_hook.id,
+        event_type: described_class::PING_EVENT,
+        event_name: described_class::PING_EVENT,
+        payload: { test: "some payload" }.to_json
+      )
+
       event = WebHookEvent.last
       headers = MultiJson.load(event.headers)
       expect(headers['Content-Length']).to eq(13)
       expect(headers['Host']).to eq("meta.discourse.org")
       expect(headers['X-Discourse-Event-Id']).to eq(event.id)
-      expect(headers['X-Discourse-Event-Type']).to eq('ping')
-      expect(headers['X-Discourse-Event']).to eq('ping')
+      expect(headers['X-Discourse-Event-Type']).to eq(described_class::PING_EVENT)
+      expect(headers['X-Discourse-Event']).to eq(described_class::PING_EVENT)
       expect(headers['X-Discourse-Event-Signature']).to eq('sha256=162f107f6b5022353274eb1a7197885cfd35744d8d08e5bcea025d309386b7d6')
       expect(event.payload).to eq(MultiJson.dump(ping: 'OK'))
       expect(event.status).to eq(200)

spec/models/web_hook_spec.rb

@@ -42,69 +42,76 @@ describe WebHook do
       expect(post_hook.payload_url).to eq("https://example.com")
     end
 
-    describe '#find_by_type' do
+    describe '#active_web_hooks' do
       it "returns unique hooks" do
         post_hook.web_hook_event_types << WebHookEventType.find_by(name: 'topic')
         post_hook.update!(wildcard_web_hook: true)
 
-        expect(WebHook.find_by_type(:post)).to eq([post_hook])
+        expect(WebHook.active_web_hooks(:post)).to eq([post_hook])
       end
 
       it 'find relevant hooks' do
-        expect(WebHook.find_by_type(:post)).to eq([post_hook])
-        expect(WebHook.find_by_type(:topic)).to eq([topic_hook])
+        expect(WebHook.active_web_hooks(:post)).to eq([post_hook])
+        expect(WebHook.active_web_hooks(:topic)).to eq([topic_hook])
       end
 
       it 'excludes inactive hooks' do
-        post_hook.update_attributes!(active: false)
+        post_hook.update!(active: false)
 
-        expect(WebHook.find_by_type(:post)).to eq([])
-        expect(WebHook.find_by_type(:topic)).to eq([topic_hook])
+        expect(WebHook.active_web_hooks(:post)).to eq([])
+        expect(WebHook.active_web_hooks(:topic)).to eq([topic_hook])
+      end
+
+      describe 'wildcard web hooks' do
+        let!(:wildcard_hook) { Fabricate(:wildcard_web_hook) }
+
+        it 'should include wildcard hooks' do
+          expect(WebHook.active_web_hooks(:wildcard)).to eq([wildcard_hook])
+
+          expect(WebHook.active_web_hooks(:post)).to contain_exactly(
+            post_hook, wildcard_hook
+          )
+
+          expect(WebHook.active_web_hooks(:topic)).to contain_exactly(
+            topic_hook, wildcard_hook
+          )
+        end
       end
     end
 
     describe '#enqueue_hooks' do
-      it 'enqueues hooks with id and name' do
-        Jobs.expects(:enqueue).with(:emit_web_hook_event, web_hook_id: post_hook.id, event_type: 'post')
-
-        WebHook.enqueue_hooks(:post)
+      before do
+        SiteSetting.queue_jobs = true
       end
 
       it 'accepts additional parameters' do
-        Jobs.expects(:enqueue).with(:emit_web_hook_event, web_hook_id: post_hook.id, post_id: 1, event_type: 'post')
+        payload = { test: 'some payload' }.to_json
+        WebHook.enqueue_hooks(:post, payload: payload)
 
-        WebHook.enqueue_hooks(:post, post_id: 1)
-      end
-    end
+        job_args = Jobs::EmitWebHookEvent.jobs.first["args"].first
 
-    context 'includes wildcard hooks' do
-      let!(:wildcard_hook) { Fabricate(:wildcard_web_hook) }
-
-      describe '#find_by_type' do
-        it 'can find wildcard hooks' do
-          expect(WebHook.find_by_type(:wildcard)).to eq([wildcard_hook])
-        end
-
-        it 'can include wildcard hooks' do
-          expect(WebHook.find_by_type(:post).sort_by(&:id)).to eq([post_hook, wildcard_hook])
-          expect(WebHook.find_by_type(:topic).sort_by(&:id)).to eq([topic_hook, wildcard_hook])
-
-        end
+        expect(job_args["web_hook_id"]).to eq(post_hook.id)
+        expect(job_args["event_type"]).to eq('post')
+        expect(job_args["payload"]).to eq(payload)
       end
 
-      describe '#enqueue_hooks' do
-        it 'enqueues hooks with ids' do
-          Jobs.expects(:enqueue).with(:emit_web_hook_event, web_hook_id: post_hook.id, event_type: 'post')
-          Jobs.expects(:enqueue).with(:emit_web_hook_event, web_hook_id: wildcard_hook.id, event_type: 'post')
+      context 'includes wildcard hooks' do
+        let!(:wildcard_hook) { Fabricate(:wildcard_web_hook) }
 
-          WebHook.enqueue_hooks(:post)
-        end
+        describe '#enqueue_hooks' do
+          it 'enqueues hooks with ids' do
+            WebHook.enqueue_hooks(:post)
 
-        it 'accepts additional parameters' do
-          Jobs.expects(:enqueue).with(:emit_web_hook_event, web_hook_id: post_hook.id, post_id: 1, event_type: 'post')
-          Jobs.expects(:enqueue).with(:emit_web_hook_event, web_hook_id: wildcard_hook.id, post_id: 1, event_type: 'post')
+            job_args = Jobs::EmitWebHookEvent.jobs.first["args"].first
 
-          WebHook.enqueue_hooks(:post, post_id: 1)
+            expect(job_args["web_hook_id"]).to eq(post_hook.id)
+            expect(job_args["event_type"]).to eq('post')
+
+            job_args = Jobs::EmitWebHookEvent.jobs.last["args"].first
+
+            expect(job_args["web_hook_id"]).to eq(wildcard_hook.id)
+            expect(job_args["event_type"]).to eq('post')
+          end
         end
       end
     end
@@ -115,39 +122,51 @@ describe WebHook do
     let(:admin) { Fabricate(:admin) }
     let(:topic) { Fabricate(:topic, user: user) }
     let(:post) { Fabricate(:post, topic: topic, user: user) }
+    let(:topic_web_hook) { Fabricate(:topic_web_hook) }
 
     before do
       SiteSetting.queue_jobs = true
+      topic_web_hook
+    end
+
+    describe 'when there are no active hooks' do
+      it 'should not 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)
+      end
     end
 
     it 'should enqueue the right hooks for topic events' do
-      Fabricate(:topic_web_hook)
-
       post = PostCreator.create(user, raw: 'post', title: 'topic', skip_validations: true)
-      topic_id = post.topic_id
+      topic_id = post.topic.id
       job_args = Jobs::EmitWebHookEvent.jobs.last["args"].first
 
       expect(job_args["event_name"]).to eq("topic_created")
-      expect(job_args["topic_id"]).to eq(topic_id)
+      payload = JSON.parse(job_args["payload"])
+      expect(payload["id"]).to eq(topic_id)
 
       PostDestroyer.new(user, post).destroy
       job_args = Jobs::EmitWebHookEvent.jobs.last["args"].first
 
       expect(job_args["event_name"]).to eq("topic_destroyed")
-      expect(job_args["topic_id"]).to eq(topic_id)
+      payload = JSON.parse(job_args["payload"])
+      expect(payload["id"]).to eq(topic_id)
 
       PostDestroyer.new(user, post).recover
       job_args = Jobs::EmitWebHookEvent.jobs.last["args"].first
 
       expect(job_args["event_name"]).to eq("topic_recovered")
-      expect(job_args["topic_id"]).to eq(topic_id)
+      payload = JSON.parse(job_args["payload"])
+      expect(payload["id"]).to eq(topic_id)
 
       %w{archived closed visible}.each do |status|
         post.topic.update_status(status, true, topic.user)
         job_args = Jobs::EmitWebHookEvent.jobs.last["args"].first
 
         expect(job_args["event_name"]).to eq("topic_#{status}_status_updated")
-        expect(job_args["topic_id"]).to eq(topic_id)
+        payload = JSON.parse(job_args["payload"])
+        expect(payload["id"]).to eq(topic_id)
       end
     end
 
@@ -172,10 +191,7 @@ describe WebHook do
     it 'should enqueue the right hooks for post events' do
       Fabricate(:web_hook)
 
-      user
-      topic
-
-      post = PostCreator.create(user,
+      post = PostCreator.create!(user,
         raw: 'post',
         topic_id: topic.id,
         reply_to_post_number: 1,
@@ -183,30 +199,57 @@ describe WebHook do
       )
 
       job_args = Jobs::EmitWebHookEvent.jobs.last["args"].first
-      Sidekiq::Worker.clear_all
 
       expect(job_args["event_name"]).to eq("post_created")
-      expect(job_args["post_id"]).to eq(post.id)
+      payload = JSON.parse(job_args["payload"])
+      expect(payload["id"]).to eq(post.id)
+
+      Jobs::EmitWebHookEvent.jobs.clear
 
       # post destroy or recover triggers a moderator post
       expect { PostDestroyer.new(user, post).destroy }
-        .to change { Jobs::EmitWebHookEvent.jobs.count }.by(2)
+        .to change { Jobs::EmitWebHookEvent.jobs.count }.by(3)
 
-      job_args = Jobs::EmitWebHookEvent.jobs.first["args"].first
+      job_args = Jobs::EmitWebHookEvent.jobs[0]["args"].first
 
       expect(job_args["event_name"]).to eq("post_edited")
-      expect(job_args["post_id"]).to eq(post.id)
+      payload = JSON.parse(job_args["payload"])
+      expect(payload["id"]).to eq(post.id)
 
-      job_args = Jobs::EmitWebHookEvent.jobs.last["args"].first
+      job_args = Jobs::EmitWebHookEvent.jobs[1]["args"].first
 
       expect(job_args["event_name"]).to eq("post_destroyed")
-      expect(job_args["post_id"]).to eq(post.id)
+      payload = JSON.parse(job_args["payload"])
+      expect(payload["id"]).to eq(post.id)
 
-      PostDestroyer.new(user, post).recover
-      job_args = Jobs::EmitWebHookEvent.jobs.last["args"].first
+      job_args = Jobs::EmitWebHookEvent.jobs[2]["args"].first
+
+      expect(job_args["event_name"]).to eq("topic_destroyed")
+      payload = JSON.parse(job_args["payload"])
+      expect(payload["id"]).to eq(post.topic.id)
+
+      Jobs::EmitWebHookEvent.jobs.clear
+
+      expect { PostDestroyer.new(user, post).recover }
+        .to change { Jobs::EmitWebHookEvent.jobs.count }.by(3)
+
+      job_args = Jobs::EmitWebHookEvent.jobs[0]["args"].first
+
+      expect(job_args["event_name"]).to eq("post_edited")
+      payload = JSON.parse(job_args["payload"])
+      expect(payload["id"]).to eq(post.id)
+
+      job_args = Jobs::EmitWebHookEvent.jobs[1]["args"].first
 
       expect(job_args["event_name"]).to eq("post_recovered")
-      expect(job_args["post_id"]).to eq(post.id)
+      payload = JSON.parse(job_args["payload"])
+      expect(payload["id"]).to eq(post.id)
+
+      job_args = Jobs::EmitWebHookEvent.jobs[2]["args"].first
+
+      expect(job_args["event_name"]).to eq("topic_recovered")
+      payload = JSON.parse(job_args["payload"])
+      expect(payload["id"]).to eq(post.topic.id)
     end
 
     it 'should enqueue the right hooks for user events' do
@@ -216,37 +259,43 @@ describe WebHook do
       job_args = Jobs::EmitWebHookEvent.jobs.last["args"].first
 
       expect(job_args["event_name"]).to eq("user_created")
-      expect(job_args["user_id"]).to eq(user.id)
+      payload = JSON.parse(job_args["payload"])
+      expect(payload["id"]).to eq(user.id)
 
       admin
       job_args = Jobs::EmitWebHookEvent.jobs.last["args"].first
 
       expect(job_args["event_name"]).to eq("user_created")
-      expect(job_args["user_id"]).to eq(admin.id)
+      payload = JSON.parse(job_args["payload"])
+      expect(payload["id"]).to eq(admin.id)
 
       user.approve(admin)
       job_args = Jobs::EmitWebHookEvent.jobs.last["args"].first
 
       expect(job_args["event_name"]).to eq("user_approved")
-      expect(job_args["user_id"]).to eq(user.id)
+      payload = JSON.parse(job_args["payload"])
+      expect(payload["id"]).to eq(user.id)
 
       UserUpdater.new(admin, user).update(username: 'testing123')
       job_args = Jobs::EmitWebHookEvent.jobs.last["args"].first
 
       expect(job_args["event_name"]).to eq("user_updated")
-      expect(job_args["user_id"]).to eq(user.id)
+      payload = JSON.parse(job_args["payload"])
+      expect(payload["id"]).to eq(user.id)
 
       user.logged_out
       job_args = Jobs::EmitWebHookEvent.jobs.last["args"].first
 
       expect(job_args["event_name"]).to eq("user_logged_out")
-      expect(job_args["user_id"]).to eq(user.id)
+      payload = JSON.parse(job_args["payload"])
+      expect(payload["id"]).to eq(user.id)
 
       user.logged_in
       job_args = Jobs::EmitWebHookEvent.jobs.last["args"].first
 
       expect(job_args["event_name"]).to eq("user_logged_in")
-      expect(job_args["user_id"]).to eq(user.id)
+      payload = JSON.parse(job_args["payload"])
+      expect(payload["id"]).to eq(user.id)
     end
   end
 end

spec/services/web_hook_enqueuer_spec.rb

@@ -0,0 +1,28 @@
+require 'rails_helper'
+
+RSpec.describe WebHookEnqueuer do
+  describe '#find_by_type' do
+    let(:enqueuer) { WebHookEnqueuer.new }
+    let!(:post_hook) { Fabricate(:web_hook, payload_url: " https://example.com ") }
+    let!(:topic_hook) { Fabricate(:topic_web_hook) }
+
+    it "returns unique hooks" do
+      post_hook.web_hook_event_types << WebHookEventType.find_by(name: 'topic')
+      post_hook.update!(wildcard_web_hook: true)
+
+      expect(enqueuer.find_by_type(:post)).to eq([post_hook])
+    end
+
+    it 'find relevant hooks' do
+      expect(enqueuer.find_by_type(:post)).to eq([post_hook])
+      expect(enqueuer.find_by_type(:topic)).to eq([topic_hook])
+    end
+
+    it 'excludes inactive hooks' do
+      post_hook.update!(active: false)
+
+      expect(enqueuer.find_by_type(:post)).to eq([])
+      expect(enqueuer.find_by_type(:topic)).to eq([topic_hook])
+    end
+  end
+end