From bf78c228f439097aaea05ddc29e3f3dfeaa2cd3f Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Thu, 16 Mar 2017 14:44:09 +0800 Subject: [PATCH] FIX: User created web hook being enqueued before record has been saved. * Improve web hook tests as well. --- app/models/user.rb | 2 +- app/models/web_hook.rb | 1 + app/services/user_updater.rb | 9 +- config/application.rb | 2 + spec/fabricators/web_hook_fabricator.rb | 8 ++ spec/models/web_hook_spec.rb | 110 ++++++++++++++++++------ 6 files changed, 104 insertions(+), 28 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index de36555acde..c6a24583226 100644 --- a/app/models/user.rb +++ b/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: diff --git a/app/models/web_hook.rb b/app/models/web_hook.rb index dc308736d2c..4159dfbc7cf 100644 --- a/app/models/web_hook.rb +++ b/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 diff --git a/app/services/user_updater.rb b/app/services/user_updater.rb index 4d1f6f44984..cbb3c479c49 100644 --- a/app/services/user_updater.rb +++ b/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) diff --git a/config/application.rb b/config/application.rb index 49b35de4743..258ca0c276b 100644 --- a/config/application.rb +++ b/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 diff --git a/spec/fabricators/web_hook_fabricator.rb b/spec/fabricators/web_hook_fabricator.rb index 3d10b267a9f..d9354fb59e6 100644 --- a/spec/fabricators/web_hook_fabricator.rb +++ b/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 diff --git a/spec/models/web_hook_spec.rb b/spec/models/web_hook_spec.rb index 6c4c8fdfaa6..f71ac30e11d 100644 --- a/spec/models/web_hook_spec.rb +++ b/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