From 37f4022f73a791b6a0f720dc8292be2f95a441ef Mon Sep 17 00:00:00 2001 From: Ricardo Mendes Date: Sat, 24 Aug 2013 12:21:39 +0100 Subject: [PATCH] Refactors UserEmailObserver to improve Code Climate score - Extracts certain logic to private methods and remove unnecessary comments - Extracts email enqueueing methods into a separate class - Fix specs involving UserEmailObserver to call #after_commit instead of the specific methods --- app/models/user_email_observer.rb | 124 ++++++++++++------------ spec/models/notification_spec.rb | 4 +- spec/models/user_email_observer_spec.rb | 28 +++--- 3 files changed, 78 insertions(+), 78 deletions(-) diff --git a/app/models/user_email_observer.rb b/app/models/user_email_observer.rb index 23c3999b0c2..ed8a14941e4 100644 --- a/app/models/user_email_observer.rb +++ b/app/models/user_email_observer.rb @@ -1,78 +1,78 @@ class UserEmailObserver < ActiveRecord::Observer observe :notification - def after_commit(notification) - if rails4? - if notification.send(:transaction_include_any_action?, [:create]) - notification_type = Notification.types[notification.notification_type] + class EmailUser + attr_reader :notification - # Delegate to email_user_{{NOTIFICATION_TYPE}} if exists - email_method = :"email_user_#{notification_type.to_s}" - send(email_method, notification) if respond_to?(email_method) - end - else - if notification.send(:transaction_include_action?, :create) - notification_type = Notification.types[notification.notification_type] + def initialize(notification) + @notification = notification + end - # Delegate to email_user_{{NOTIFICATION_TYPE}} if exists - email_method = :"email_user_#{notification_type.to_s}" + def mentioned + enqueue :user_mentioned + end - send(email_method, notification) if respond_to?(email_method) - end + def posted + enqueue :user_posted + end + + def quoted + enqueue :user_quoted + end + + def replied + enqueue :user_replied + end + + def private_message + enqueue_private :user_private_message + end + + def invited_to_private_message + enqueue :user_invited_to_private_message + end + + private + + def enqueue(type) + return unless notification.user.email_direct? + Jobs.enqueue_in(SiteSetting.email_time_window_mins.minutes, + :user_email, + type: type, + user_id: notification.user_id, + notification_id: notification.id) + end + + def enqueue_private(type) + return unless (notification.user.email_direct? && notification.user.email_private_messages?) + Jobs.enqueue_in(SiteSetting.email_time_window_mins.minutes, + :user_email, + type: type, + user_id: notification.user_id, + notification_id: notification.id) end end - def email_user_mentioned(notification) - return unless notification.user.email_direct? - Jobs.enqueue_in(SiteSetting.email_time_window_mins.minutes, - :user_email, - type: :user_mentioned, - user_id: notification.user_id, - notification_id: notification.id) + def after_commit(notification) + transaction_includes_action = if rails4? + notification.send(:transaction_include_any_action?, [:create]) + else + notification.send(:transaction_include_action?, :create) + end + + delegate_to_email_user notification if transaction_includes_action end - def email_user_posted(notification) - return unless notification.user.email_direct? - Jobs.enqueue_in(SiteSetting.email_time_window_mins.minutes, - :user_email, - type: :user_posted, - user_id: notification.user_id, - notification_id: notification.id) + private + + def extract_notification_type(notification) + Notification.types[notification.notification_type] end - def email_user_quoted(notification) - return unless notification.user.email_direct? - Jobs.enqueue_in(SiteSetting.email_time_window_mins.minutes, - :user_email, - type: :user_quoted, - user_id: notification.user_id, - notification_id: notification.id) - end + def delegate_to_email_user(notification) + email_user = EmailUser.new(notification) + email_method = extract_notification_type notification - def email_user_replied(notification) - return unless notification.user.email_direct? - Jobs.enqueue_in(SiteSetting.email_time_window_mins.minutes, - :user_email, - type: :user_replied, - user_id: notification.user_id, - notification_id: notification.id) - end - - def email_user_private_message(notification) - return unless (notification.user.email_direct? && notification.user.email_private_messages?) - Jobs.enqueue_in(SiteSetting.email_time_window_mins.minutes, - :user_email, - type: :user_private_message, - user_id: notification.user_id, - notification_id: notification.id) - end - - def email_user_invited_to_private_message(notification) - return unless notification.user.email_direct? - Jobs.enqueue_in(SiteSetting.email_time_window_mins.minutes, - :user_email, - type: :user_invited_to_private_message, - user_id: notification.user_id, - notification_id: notification.id) + email_user.send(email_method) if email_user.respond_to? email_method end end diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index 13472589031..51072222a9c 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -109,7 +109,7 @@ describe Notification do describe '@mention' do it "calls email_user_mentioned on creating a notification" do - UserEmailObserver.any_instance.expects(:email_user_mentioned).with(instance_of(Notification)) + UserEmailObserver.any_instance.expects(:after_commit).with(instance_of(Notification)) Fabricate(:notification) end @@ -117,7 +117,7 @@ describe Notification do describe '@mention' do it "calls email_user_quoted on creating a quote notification" do - UserEmailObserver.any_instance.expects(:email_user_quoted).with(instance_of(Notification)) + UserEmailObserver.any_instance.expects(:after_commit).with(instance_of(Notification)) Fabricate(:quote_notification) end end diff --git a/spec/models/user_email_observer_spec.rb b/spec/models/user_email_observer_spec.rb index 4e9d382ee47..f56ac08b41b 100644 --- a/spec/models/user_email_observer_spec.rb +++ b/spec/models/user_email_observer_spec.rb @@ -9,13 +9,13 @@ describe UserEmailObserver do it "enqueues a job for the email" do Jobs.expects(:enqueue_in).with(SiteSetting.email_time_window_mins.minutes, :user_email, type: :user_mentioned, user_id: notification.user_id, notification_id: notification.id) - UserEmailObserver.send(:new).email_user_mentioned(notification) + UserEmailObserver.send(:new).after_commit(notification) end it "doesn't enqueue an email if the user has mention emails disabled" do user.expects(:email_direct?).returns(false) Jobs.expects(:enqueue_in).with(SiteSetting.email_time_window_mins.minutes, :user_email, has_entry(type: :user_mentioned)).never - UserEmailObserver.send(:new).email_user_mentioned(notification) + UserEmailObserver.send(:new).after_commit(notification) end end @@ -23,17 +23,17 @@ describe UserEmailObserver do context 'posted' do let(:user) { Fabricate(:user) } - let!(:notification) { Fabricate(:notification, user: user) } + let!(:notification) { Fabricate(:notification, user: user, notification_type: 9) } it "enqueues a job for the email" do Jobs.expects(:enqueue_in).with(SiteSetting.email_time_window_mins.minutes, :user_email, type: :user_posted, user_id: notification.user_id, notification_id: notification.id) - UserEmailObserver.send(:new).email_user_posted(notification) + UserEmailObserver.send(:new).after_commit(notification) end it "doesn't enqueue an email if the user has mention emails disabled" do user.expects(:email_direct?).returns(false) Jobs.expects(:enqueue_in).with(SiteSetting.email_time_window_mins.minutes, :user_email, has_entry(type: :user_posted)).never - UserEmailObserver.send(:new).email_user_posted(notification) + UserEmailObserver.send(:new).after_commit(notification) end end @@ -41,17 +41,17 @@ describe UserEmailObserver do context 'user_replied' do let(:user) { Fabricate(:user) } - let!(:notification) { Fabricate(:notification, user: user) } + let!(:notification) { Fabricate(:notification, user: user, notification_type: 2) } it "enqueues a job for the email" do Jobs.expects(:enqueue_in).with(SiteSetting.email_time_window_mins.minutes, :user_email, type: :user_replied, user_id: notification.user_id, notification_id: notification.id) - UserEmailObserver.send(:new).email_user_replied(notification) + UserEmailObserver.send(:new).after_commit(notification) end it "doesn't enqueue an email if the user has mention emails disabled" do user.expects(:email_direct?).returns(false) Jobs.expects(:enqueue_in).with(SiteSetting.email_time_window_mins.minutes, :user_email, has_entry(type: :user_replied)).never - UserEmailObserver.send(:new).email_user_replied(notification) + UserEmailObserver.send(:new).after_commit(notification) end end @@ -59,17 +59,17 @@ describe UserEmailObserver do context 'user_quoted' do let(:user) { Fabricate(:user) } - let!(:notification) { Fabricate(:notification, user: user) } + let!(:notification) { Fabricate(:notification, user: user, notification_type: 3) } it "enqueues a job for the email" do Jobs.expects(:enqueue_in).with(SiteSetting.email_time_window_mins.minutes, :user_email, type: :user_quoted, user_id: notification.user_id, notification_id: notification.id) - UserEmailObserver.send(:new).email_user_quoted(notification) + UserEmailObserver.send(:new).after_commit(notification) end it "doesn't enqueue an email if the user has mention emails disabled" do user.expects(:email_direct?).returns(false) Jobs.expects(:enqueue_in).with(SiteSetting.email_time_window_mins.minutes, :user_email, has_entry(type: :user_quoted)).never - UserEmailObserver.send(:new).email_user_quoted(notification) + UserEmailObserver.send(:new).after_commit(notification) end end @@ -77,17 +77,17 @@ describe UserEmailObserver do context 'email_user_invited_to_private_message' do let(:user) { Fabricate(:user) } - let!(:notification) { Fabricate(:notification, user: user) } + let!(:notification) { Fabricate(:notification, user: user, notification_type: 7) } it "enqueues a job for the email" do Jobs.expects(:enqueue_in).with(SiteSetting.email_time_window_mins.minutes, :user_email, type: :user_invited_to_private_message, user_id: notification.user_id, notification_id: notification.id) - UserEmailObserver.send(:new).email_user_invited_to_private_message(notification) + UserEmailObserver.send(:new).after_commit(notification) end it "doesn't enqueue an email if the user has mention emails disabled" do user.expects(:email_direct?).returns(false) Jobs.expects(:enqueue_in).with(SiteSetting.email_time_window_mins.minutes, :user_email, has_entry(type: :user_invited_to_private_message)).never - UserEmailObserver.send(:new).email_user_invited_to_private_message(notification) + UserEmailObserver.send(:new).after_commit(notification) end end