FIX: User created web hook being enqueued before record has been saved.

* Improve web hook tests as well.

app/models/user.rb

@@ -93,7 +93,6 @@ class User < ActiveRecord::Base
   after_create :create_user_profile
   after_create :ensure_in_trust_level_group
   after_create :set_default_categories_preferences
-  after_create :trigger_user_created_event
 
   before_save :update_username_lower
   before_save :ensure_password_is_hashed
@@ -105,6 +104,7 @@ class User < ActiveRecord::Base
   after_save :badge_grant
   after_save :expire_old_email_tokens
   after_save :index_search
+  after_save :trigger_user_created_event
 
   before_destroy do
     # These tables don't have primary keys, so destroying them with activerecord is tricky:

app/models/web_hook.rb

@@ -61,6 +61,7 @@ class WebHook < ActiveRecord::Base
   %i(post_created
      post_destroyed
      post_recovered).each do |event|
+
     DiscourseEvent.on(event) do |post, _, user|
       WebHook.enqueue_post_hooks(event, post, user)
     end

app/services/user_updater.rb

@@ -96,15 +96,16 @@ class UserUpdater
       user.custom_fields = user.custom_fields.merge(fields)
     end
 
+    saved = nil
+
     User.transaction do
       if attributes.key?(:muted_usernames)
         update_muted_users(attributes[:muted_usernames])
       end
 
       saved = (!save_options || user.user_option.save) && user_profile.save && user.save
-      if saved
-        DiscourseEvent.trigger(:user_updated, user)
 
+      if saved
         # log name changes
         if attributes[:name].present? && old_user_name.downcase != attributes.fetch(:name).downcase
           StaffActionLogger.new(@actor).log_name_change(user.id, old_user_name, attributes.fetch(:name))
@@ -112,8 +113,10 @@ class UserUpdater
           StaffActionLogger.new(@actor).log_name_change(user.id, old_user_name, "")
         end
       end
-      saved
     end
+
+    DiscourseEvent.trigger(:user_updated, user) if saved
+    saved
   end
 
   def update_muted_users(usernames)

config/application.rb

@@ -185,6 +185,8 @@ module Discourse
       require_dependency 'group'
       require_dependency 'user_field'
       require_dependency 'post_action_type'
+      # Ensure that Discourse event triggers for web hooks are loaded
+      require_dependency 'web_hook'
 
       # So open id logs somewhere sane
       OpenID::Util.logger = Rails.logger

spec/fabricators/web_hook_fabricator.rb

@@ -28,3 +28,11 @@ Fabricator(:topic_web_hook, from: :web_hook) do
     web_hook.web_hook_event_types = [transients[:topic_hook]]
   end
 end
+
+Fabricator(:user_web_hook, from: :web_hook) do
+  transient user_hook: WebHookEventType.find_by(name: 'user')
+
+  after_build do |web_hook, transients|
+    web_hook.web_hook_event_types = [transients[:user_hook]]
+  end
+end

spec/models/web_hook_spec.rb

@@ -1,4 +1,5 @@
 require 'rails_helper'
+require 'sidekiq/testing'
 
 describe WebHook do
   it { is_expected.to validate_presence_of :payload_url }
@@ -100,49 +101,110 @@ describe WebHook do
   end
 
   describe 'enqueues hooks' do
-    let!(:post_hook) { Fabricate(:web_hook) }
-    let!(:topic_hook) { Fabricate(:topic_web_hook) }
     let(:user) { Fabricate(:user) }
     let(:admin) { Fabricate(:admin) }
     let(:topic) { Fabricate(:topic, user: user) }
     let(:post) { Fabricate(:post, topic: topic, user: user) }
-    let(:post2) { Fabricate(:post, topic: topic, user: user) }
+
+    before do
+      SiteSetting.queue_jobs = true
+    end
 
     it 'should enqueue the right hooks for topic events' do
-      WebHook.expects(:enqueue_topic_hooks).once
-      PostCreator.create(user, { raw: 'post', title: 'topic', skip_validations: true })
+      Fabricate(:topic_web_hook)
 
-      WebHook.expects(:enqueue_topic_hooks).once
-      PostDestroyer.new(user, post).destroy
+      Sidekiq::Testing.fake! do
+        post = PostCreator.create(user, { raw: 'post', title: 'topic', skip_validations: true })
+        topic_id = post.topic_id
+        job_args = Jobs::EmitWebHookEvent.jobs.last["args"].first
 
-      WebHook.expects(:enqueue_topic_hooks).once
-      PostDestroyer.new(user, post).recover
+        expect(job_args["event_name"]).to eq("topic_created")
+        expect(job_args["topic_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)
+
+        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)
+      end
     end
 
     it 'should enqueue the right hooks for post events' do
-      WebHook.expects(:enqueue_post_hooks).once
-      PostCreator.create(user, { raw: 'post', topic_id: topic.id, reply_to_post_number: 1, skip_validations: true })
+      Fabricate(:web_hook)
 
-      # post destroy or recover triggers a moderator post
-      WebHook.expects(:enqueue_post_hooks).twice
-      PostDestroyer.new(user, post2).destroy
+      Sidekiq::Testing.fake! do
+        user
+        topic
 
-      WebHook.expects(:enqueue_post_hooks).twice
-      PostDestroyer.new(user, post2).recover
+        post = PostCreator.create(user,
+          raw: 'post',
+          topic_id: topic.id,
+          reply_to_post_number: 1,
+          skip_validations: true
+        )
+
+        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)
+
+        # post destroy or recover triggers a moderator post
+        expect { PostDestroyer.new(user, post).destroy }
+          .to change { Jobs::EmitWebHookEvent.jobs.count }.by(2)
+
+        job_args = Jobs::EmitWebHookEvent.jobs.first["args"].first
+
+        expect(job_args["event_name"]).to eq("post_edited")
+        expect(job_args["post_id"]).to eq(post.id)
+
+        job_args = Jobs::EmitWebHookEvent.jobs.last["args"].first
+
+        expect(job_args["event_name"]).to eq("post_destroyed")
+        expect(job_args["post_id"]).to eq(post.id)
+
+        PostDestroyer.new(user, post).recover
+        job_args = Jobs::EmitWebHookEvent.jobs.last["args"].first
+
+        expect(job_args["event_name"]).to eq("post_recovered")
+        expect(job_args["post_id"]).to eq(post.id)
+      end
     end
 
     it 'should enqueue the right hooks for user events' do
-      WebHook.expects(:enqueue_hooks).once
-      user
+      user_web_hook = Fabricate(:user_web_hook, active: true)
 
-      WebHook.expects(:enqueue_hooks).once
-      admin
+      Sidekiq::Testing.fake! do
+        user
+        job_args = Jobs::EmitWebHookEvent.jobs.last["args"].first
 
-      WebHook.expects(:enqueue_hooks).once
-      user.approve(admin)
+        expect(job_args["event_name"]).to eq("user_created")
+        expect(job_args["user_id"]).to eq(user.id)
 
-      WebHook.expects(:enqueue_hooks).once
-      UserUpdater.new(admin, user).update(username: 'testing123')
+        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)
+
+        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)
+
+        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)
+      end
     end
   end
 end