FIX: Generate webhook payloads before destroy events (#6325)

app/models/category.rb

@@ -8,6 +8,7 @@ class Category < ActiveRecord::Base
   include HasCustomFields
   include CategoryHashtag
   include AnonCacheInvalidator
+  include HasDestroyedWebHook
 
   REQUIRE_TOPIC_APPROVAL = 'require_topic_approval'
   REQUIRE_REPLY_APPROVAL = 'require_reply_approval'

app/models/concerns/has_destroyed_web_hook.rb

@@ -0,0 +1,17 @@
+module HasDestroyedWebHook
+  extend ActiveSupport::Concern
+
+  included do
+    around_destroy :enqueue_destroyed_web_hook
+  end
+
+  def enqueue_destroyed_web_hook
+    type = self.class.name.underscore.to_sym
+    payload = WebHook.generate_payload(type, self)
+    yield
+    WebHook.enqueue_hooks(type, "#{type}_destroyed".to_sym,
+      id: id,
+      payload: payload
+    )
+  end
+end

app/models/group.rb

@@ -5,6 +5,7 @@ require_dependency 'enum'
 class Group < ActiveRecord::Base
   include HasCustomFields
   include AnonCacheInvalidator
+  include HasDestroyedWebHook
 
   cattr_accessor :preloaded_custom_field_names
   self.preloaded_custom_field_names = Set.new

app/models/tag.rb

@@ -1,5 +1,6 @@
 class Tag < ActiveRecord::Base
   include Searchable
+  include HasDestroyedWebHook
 
   validates :name, presence: true, uniqueness: true
 

app/models/user.rb

@@ -19,6 +19,7 @@ class User < ActiveRecord::Base
   include Roleable
   include HasCustomFields
   include SecondFactorManager
+  include HasDestroyedWebHook
 
   has_many :posts
   has_many :notifications, dependent: :destroy

app/models/web_hook.rb

@@ -41,24 +41,21 @@ class WebHook < ActiveRecord::Base
       .distinct
   end
 
-  def self.enqueue_hooks(type, opts = {})
+  def self.enqueue_hooks(type, event, opts = {})
     active_web_hooks(type).each do |web_hook|
       Jobs.enqueue(:emit_web_hook_event, opts.merge(
-        web_hook_id: web_hook.id, event_type: type.to_s
+        web_hook_id: web_hook.id, event_name: event.to_s, event_type: type.to_s
       ))
     end
   end
 
   def self.enqueue_object_hooks(type, object, event, serializer = nil)
     if active_web_hooks(type).exists?
-      serializer ||= "WebHook#{type.capitalize}Serializer".constantize
+      payload = WebHook.generate_payload(type, object, serializer)
 
-      WebHook.enqueue_hooks(type,
-        event_name: event.to_s,
-        payload: serializer.new(object,
-          scope: self.guardian,
-          root: false
-        ).to_json
+      WebHook.enqueue_hooks(type, event,
+        id: object.id,
+        payload: payload
       )
     end
   end
@@ -66,31 +63,38 @@ class WebHook < ActiveRecord::Base
   def self.enqueue_topic_hooks(event, topic)
     if active_web_hooks('topic').exists?
       topic_view = TopicView.new(topic.id, Discourse.system_user)
+      payload = WebHook.generate_payload(:topic, topic_view, WebHookTopicViewSerializer)
 
-      WebHook.enqueue_hooks(:topic,
+      WebHook.enqueue_hooks(:topic, event,
+        id: topic.id,
         category_id: topic&.category_id,
-        event_name: event.to_s,
-        payload: WebHookTopicViewSerializer.new(topic_view,
-          scope: self.guardian,
-          root: false
-        ).to_json
+        payload: payload
       )
     end
   end
 
   def self.enqueue_post_hooks(event, post)
     if active_web_hooks('post').exists?
-      WebHook.enqueue_hooks(:post,
+      payload = WebHook.generate_payload(:post, post)
+
+      WebHook.enqueue_hooks(:post, event,
+        id: post.id,
         category_id: post&.topic&.category_id,
-        event_name: event.to_s,
-        payload: WebHookPostSerializer.new(post,
-          scope: self.guardian,
-          root: false
-        ).to_json
+        payload: payload
       )
     end
   end
 
+  def self.generate_payload(type, object, serializer = nil)
+    serializer ||= TagSerializer if type == :tag
+    serializer ||= "WebHook#{type.capitalize}Serializer".constantize
+
+    serializer.new(object,
+      scope: self.guardian,
+      root: false
+    ).to_json
+  end
+
   private
 
   def self.guardian

config/initializers/012-web_hook_events.rb

@@ -1,5 +1,4 @@
 %i(
-  topic_destroyed
   topic_recovered
 ).each do |event|
   DiscourseEvent.on(event) do |topic, _|
@@ -17,7 +16,6 @@ end
 
 %i(
   post_created
-  post_destroyed
   post_recovered
 ).each do |event|
   DiscourseEvent.on(event) do |post, _, _|
@@ -41,7 +39,6 @@ end
   user_logged_in
   user_approved
   user_updated
-  user_destroyed
 ).each do |event|
   DiscourseEvent.on(event) do |user|
     WebHook.enqueue_object_hooks(:user, user, event)
@@ -51,7 +48,6 @@ end
 %i(
   group_created
   group_updated
-  group_destroyed
 ).each do |event|
   DiscourseEvent.on(event) do |group|
     WebHook.enqueue_object_hooks(:group, group, event)
@@ -61,7 +57,6 @@ end
 %i(
   category_created
   category_updated
-  category_destroyed
 ).each do |event|
   DiscourseEvent.on(event) do |category|
     WebHook.enqueue_object_hooks(:category, category, event)
@@ -71,7 +66,6 @@ end
 %i(
   tag_created
   tag_updated
-  tag_destroyed
 ).each do |event|
   DiscourseEvent.on(event) do |tag|
     WebHook.enqueue_object_hooks(:tag, tag, event, TagSerializer)

lib/post_destroyer.rb

@@ -45,6 +45,14 @@ class PostDestroyer
   end
 
   def destroy
+    payload = WebHook.generate_payload(:post, @post)
+    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)
+    end
+
     delete_removed_posts_after = @opts[:delete_removed_posts_after] || SiteSetting.delete_removed_posts_after
 
     if @user.staff? || delete_removed_posts_after < 1
@@ -53,9 +61,19 @@ class PostDestroyer
       mark_for_deletion(delete_removed_posts_after)
     end
     DiscourseEvent.trigger(:post_destroyed, @post, @opts, @user)
+    WebHook.enqueue_hooks(:post, :post_destroyed,
+      id: @post.id,
+      category_id: @post&.topic&.category_id,
+      payload: payload
+    )
 
     if @post.is_first_post? && @post.topic
       DiscourseEvent.trigger(:topic_destroyed, @post.topic, @user)
+      WebHook.enqueue_hooks(:topic, :topic_destroyed,
+        id: topic.id,
+        category_id: topic&.category_id,
+        payload: topic_payload
+      )
     end
   end
 

spec/components/post_destroyer_spec.rb

@@ -424,6 +424,16 @@ describe PostDestroyer do
       }.to_not change { author.topic_count }
       expect(author.post_count).to eq(0) # also unchanged
     end
+
+    it 'triggers the extensibility events' do
+      events = DiscourseEvent.track_events { PostDestroyer.new(admin, first_post).destroy }.last(2)
+
+      expect(events[0][:event_name]).to eq(:post_destroyed)
+      expect(events[0][:params].first).to eq(first_post)
+
+      expect(events[1][:event_name]).to eq(:topic_destroyed)
+      expect(events[1][:params].first).to eq(first_post.topic)
+    end
   end
 
   context 'deleting the second post in a topic' do
@@ -502,6 +512,13 @@ describe PostDestroyer do
       it "creates a new user history entry" do
         expect { subject }.to change { UserHistory.count }.by(1)
       end
+
+      it 'triggers a extensibility event' do
+        events = DiscourseEvent.track_events { subject }
+
+        expect(events[0][:event_name]).to eq(:post_destroyed)
+        expect(events[0][:params].first).to eq(post)
+      end
     end
   end
 

spec/jobs/fix_primary_emails_for_staged_users_spec.rb

@@ -20,6 +20,10 @@ RSpec.describe Jobs::FixPrimaryEmailsForStagedUsers do
 
     UserEmail.delete_all
 
+    # since we removing `user_emails` table the `user.primary_email` value will be nil.
+    # it will raise error in https://github.com/discourse/discourse/blob/d0b027d88deeabf8bc105419f7d3fae0087091cd/app/models/user.rb#L942
+    WebHook.stubs(:generate_payload).returns(nil)
+
     expect { described_class.new.execute_onceoff({}) }
       .to change { User.count }.by(-2)
       .and change { staged_user.posts.count }.by(3)

spec/models/web_hook_spec.rb

@@ -82,7 +82,7 @@ describe WebHook do
     describe '#enqueue_hooks' do
       it 'accepts additional parameters' do
         payload = { test: 'some payload' }.to_json
-        WebHook.enqueue_hooks(:post, payload: payload)
+        WebHook.enqueue_hooks(:post, :post_created, payload: payload)
 
         job_args = Jobs::EmitWebHookEvent.jobs.first["args"].first
 
@@ -96,7 +96,7 @@ describe WebHook do
 
         describe '#enqueue_hooks' do
           it 'enqueues hooks with ids' do
-            WebHook.enqueue_hooks(:post)
+            WebHook.enqueue_hooks(:post, :post_created)
 
             job_args = Jobs::EmitWebHookEvent.jobs.first["args"].first
 
@@ -292,12 +292,15 @@ describe WebHook do
       payload = JSON.parse(job_args["payload"])
       expect(payload["id"]).to eq(user.id)
 
+      email = user.email
+      user.reload
       UserDestroyer.new(Discourse.system_user).destroy(user)
       job_args = Jobs::EmitWebHookEvent.jobs.last["args"].first
 
       expect(job_args["event_name"]).to eq("user_destroyed")
       payload = JSON.parse(job_args["payload"])
       expect(payload["id"]).to eq(user.id)
+      expect(payload["email"]).to eq(email)
     end
 
     it 'should enqueue the right hooks for category events' do