From 019f1a1d06d35d8d2a6b3e15a2022cd25713cf99 Mon Sep 17 00:00:00 2001 From: Sam Date: Thu, 22 Dec 2016 15:29:34 +1100 Subject: [PATCH] UserEmailObserver is now removed no big surprises here was pretty straightforward after_commit semantics sure are weird though --- app/models/notification.rb | 7 +++++ .../notification_emailer.rb} | 14 +++++---- config/application.rb | 1 - spec/models/category_user_spec.rb | 1 + spec/models/notification_spec.rb | 18 ++--------- spec/rails_helper.rb | 1 + .../notification_emailer_spec.rb} | 30 +++++++++++-------- 7 files changed, 38 insertions(+), 34 deletions(-) rename app/{models/user_email_observer.rb => services/notification_emailer.rb} (90%) rename spec/{models/user_email_observer_spec.rb => services/notification_emailer_spec.rb} (82%) diff --git a/app/models/notification.rb b/app/models/notification.rb index 9d0814c8615..cdac6ed46e3 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -15,6 +15,8 @@ class Notification < ActiveRecord::Base after_save :refresh_notification_count after_destroy :refresh_notification_count + after_commit :send_email + def self.ensure_consistency! Notification.exec_sql(" DELETE FROM Notifications n WHERE notification_type = :id AND @@ -196,6 +198,11 @@ class Notification < ActiveRecord::Base user.publish_notifications_state end + def send_email + transaction_includes_action = self.send(:transaction_include_any_action?, [:create]) + NotificationEmailer.process_notification(self) if transaction_includes_action + end + end # == Schema Information diff --git a/app/models/user_email_observer.rb b/app/services/notification_emailer.rb similarity index 90% rename from app/models/user_email_observer.rb rename to app/services/notification_emailer.rb index bb410294da7..534d4037a13 100644 --- a/app/models/user_email_observer.rb +++ b/app/services/notification_emailer.rb @@ -1,5 +1,4 @@ -class UserEmailObserver < ActiveRecord::Observer - observe :notification +class NotificationEmailer class EmailUser attr_reader :notification @@ -105,12 +104,17 @@ class UserEmailObserver < ActiveRecord::Observer end - def after_commit(notification) - transaction_includes_action = notification.send(:transaction_include_any_action?, [:create]) - self.class.process_notification(notification) if transaction_includes_action + def self.disable + @disabled = true + end + + def self.enable + @disabled = false end def self.process_notification(notification) + return if @disabled + email_user = EmailUser.new(notification) email_method = Notification.types[notification.notification_type] diff --git a/config/application.rb b/config/application.rb index e8189eed1b7..38dd39d7012 100644 --- a/config/application.rb +++ b/config/application.rb @@ -82,7 +82,6 @@ module Discourse # Activate observers that should always be running. config.active_record.observers = [ - :user_email_observer, :post_alert_observer, ] diff --git a/spec/models/category_user_spec.rb b/spec/models/category_user_spec.rb index ab3acbff02b..3bdb11767fa 100644 --- a/spec/models/category_user_spec.rb +++ b/spec/models/category_user_spec.rb @@ -69,6 +69,7 @@ describe CategoryUser do context 'integration' do before do ActiveRecord::Base.observers.enable :all + NotificationEmailer.enable end it 'should operate correctly' do diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index ef64bcfbe39..4d00ab80e3e 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -3,6 +3,7 @@ require 'rails_helper' describe Notification do before do ActiveRecord::Base.observers.enable :all + NotificationEmailer.enable end it { is_expected.to validate_presence_of :notification_type } @@ -139,21 +140,6 @@ describe Notification do end end - describe '@mention' do - - it "calls email_user_mentioned on creating a notification" do - UserEmailObserver.any_instance.expects(:after_commit).with(instance_of(Notification)) - Fabricate(:notification) - end - - end - - describe '@mention' do - it "calls email_user_quoted on creating a quote notification" do - UserEmailObserver.any_instance.expects(:after_commit).with(instance_of(Notification)) - Fabricate(:quote_notification) - end - end describe 'private message' do before do @@ -250,6 +236,8 @@ describe Notification do it 'deletes notifications if post is missing or deleted' do ActiveRecord::Base.observers.disable :all + NotificationEmailer.disable + p = Fabricate(:post) p2 = Fabricate(:post) diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 7bfdeb29bcb..2ecb556648a 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -109,6 +109,7 @@ Spork.prefork do ActiveRecord::Base.observers.disable :all SearchIndexer.disable UserActionCreator.disable + NotificationEmailer.disable SiteSetting.provider.all.each do |setting| SiteSetting.remove_override!(setting.name) diff --git a/spec/models/user_email_observer_spec.rb b/spec/services/notification_emailer_spec.rb similarity index 82% rename from spec/models/user_email_observer_spec.rb rename to spec/services/notification_emailer_spec.rb index 013b1db9413..8e44538af3a 100644 --- a/spec/models/user_email_observer_spec.rb +++ b/spec/services/notification_emailer_spec.rb @@ -1,6 +1,10 @@ require 'rails_helper' -describe UserEmailObserver do +describe NotificationEmailer do + + before do + NotificationEmailer.enable + end let(:topic) { Fabricate(:topic) } let(:post) { Fabricate(:post, topic: topic) } @@ -18,8 +22,8 @@ describe UserEmailObserver do shared_examples "enqueue" do it "enqueues a job for the email" do - Jobs.expects(:enqueue_in).with(delay, :user_email, UserEmailObserver::EmailUser.notification_params(notification,type)) - UserEmailObserver.process_notification(notification) + Jobs.expects(:enqueue_in).with(delay, :user_email, NotificationEmailer::EmailUser.notification_params(notification,type)) + NotificationEmailer.process_notification(notification) end context "inactive user" do @@ -27,13 +31,13 @@ describe UserEmailObserver do it "doesn't enqueue a job" do Jobs.expects(:enqueue_in).with(delay, :user_email, has_entry(type: type)).never - UserEmailObserver.process_notification(notification) + NotificationEmailer.process_notification(notification) end it "enqueues a job if the user is staged" do notification.user.staged = true - Jobs.expects(:enqueue_in).with(delay, :user_email, UserEmailObserver::EmailUser.notification_params(notification,type)) - UserEmailObserver.process_notification(notification) + Jobs.expects(:enqueue_in).with(delay, :user_email, NotificationEmailer::EmailUser.notification_params(notification,type)) + NotificationEmailer.process_notification(notification) end end @@ -46,7 +50,7 @@ describe UserEmailObserver do it "doesn't enqueue a job" do Jobs.expects(:enqueue_in).with(delay, :user_email, has_entry(type: type)).never - UserEmailObserver.process_notification(notification) + NotificationEmailer.process_notification(notification) end end @@ -55,7 +59,7 @@ describe UserEmailObserver do it "doesn't enqueue a job" do Post.any_instance.expects(:post_type).returns(Post.types[:small_action]) Jobs.expects(:enqueue_in).with(delay, :user_email, has_entry(type: type)).never - UserEmailObserver.process_notification(notification) + NotificationEmailer.process_notification(notification) end end @@ -68,7 +72,7 @@ describe UserEmailObserver do it "doesn't enqueue a job if the user has mention emails disabled" do notification.user.user_option.update_columns(email_direct: false) Jobs.expects(:enqueue_in).with(delay, :user_email, has_entry(type: type)).never - UserEmailObserver.process_notification(notification) + NotificationEmailer.process_notification(notification) end end @@ -78,7 +82,7 @@ describe UserEmailObserver do it "doesn't enqueue a job if the user has private message emails disabled" do notification.user.user_option.update_columns(email_private_messages: false) Jobs.expects(:enqueue_in).with(delay, :user_email, has_entry(type: type)).never - UserEmailObserver.process_notification(notification) + NotificationEmailer.process_notification(notification) end end @@ -92,8 +96,8 @@ describe UserEmailObserver do it "enqueue a delayed job for users that are online" do notification.user.last_seen_at = 1.minute.ago - Jobs.expects(:enqueue_in).with(delay, :user_email, UserEmailObserver::EmailUser.notification_params(notification,type)) - UserEmailObserver.process_notification(notification) + Jobs.expects(:enqueue_in).with(delay, :user_email, NotificationEmailer::EmailUser.notification_params(notification,type)) + NotificationEmailer.process_notification(notification) end end @@ -140,7 +144,7 @@ describe UserEmailObserver do it "doesn't enqueue a job for a small action" do notification.data_hash["original_post_type"] = Post.types[:small_action] Jobs.expects(:enqueue_in).with(delay, :user_email, has_entry(type: type)).never - UserEmailObserver.process_notification(notification) + NotificationEmailer.process_notification(notification) end end