From 12051f79a8c5b6e8c790b83a71b0f500186a852a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Mon, 1 Feb 2016 19:12:10 +0100 Subject: [PATCH] FIX: don't send empty email notifications for small_actions --- app/models/notification.rb | 5 +---- app/models/user_email_observer.rb | 15 ++++++++++++--- app/services/post_alerter.rb | 16 +++++++++------- spec/jobs/user_email_spec.rb | 3 --- spec/models/notification_spec.rb | 2 +- spec/models/user_email_observer_spec.rb | 9 ++++++++- 6 files changed, 31 insertions(+), 19 deletions(-) diff --git a/app/models/notification.rb b/app/models/notification.rb index 218f7a96c..bffb70dce 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -113,14 +113,11 @@ class Notification < ActiveRecord::Base end def url - if topic.present? - return topic.relative_url(post_number) - end + topic.relative_url(post_number) if topic.present? end def post return if topic_id.blank? || post_number.blank? - Post.find_by(topic_id: topic_id, post_number: post_number) end diff --git a/app/models/user_email_observer.rb b/app/models/user_email_observer.rb index 889f9cdd9..aab574ce7 100644 --- a/app/models/user_email_observer.rb +++ b/app/models/user_email_observer.rb @@ -61,17 +61,17 @@ class UserEmailObserver < ActiveRecord::Observer def enqueue(type, delay=default_delay) return unless notification.user.email_direct? - perform_enqueue(type,delay) + perform_enqueue(type, delay) end def enqueue_private(type, delay=private_delay) return unless notification.user.email_private_messages? - perform_enqueue(type,delay) + perform_enqueue(type, delay) end def perform_enqueue(type, delay) return unless notification.user.active? || notification.user.staged? - return unless EMAILABLE_POST_TYPES.include? notification.post.try(:post_type) + return unless EMAILABLE_POST_TYPES.include?(post_type) Jobs.enqueue_in(delay, :user_email, self.class.notification_params(notification, type)) end @@ -84,6 +84,14 @@ class UserEmailObserver < ActiveRecord::Observer 20.seconds end + def post_type + @post_type ||= begin + type = notification.data_hash["original_post_type"] if notification.data_hash + type ||= notification.post.try(:post_type) + type + end + end + end def after_commit(notification) @@ -94,6 +102,7 @@ class UserEmailObserver < ActiveRecord::Observer def self.process_notification(notification) email_user = EmailUser.new(notification) email_method = Notification.types[notification.notification_type] + email_user.send(email_method) if email_user.respond_to? email_method end diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index d81728f9a..5ee0ad8e1 100644 --- a/app/services/post_alerter.rb +++ b/app/services/post_alerter.rb @@ -234,8 +234,8 @@ class PostAlerter collapsed = false if type == Notification.types[:replied] || type == Notification.types[:posted] - destroy_notifications(user, Notification.types[:replied] , post.topic) - destroy_notifications(user, Notification.types[:posted] , post.topic) + destroy_notifications(user, Notification.types[:replied], post.topic) + destroy_notifications(user, Notification.types[:posted], post.topic) collapsed = true end @@ -248,10 +248,12 @@ class PostAlerter original_username = opts[:display_username] || post.username if collapsed - post = first_unread_post(user,post.topic) || post + post = first_unread_post(user, post.topic) || post count = unread_count(user, post.topic) - I18n.with_locale(user.effective_locale) do - opts[:display_username] = I18n.t('embed.replies', count: count) if count > 1 + if count > 1 + I18n.with_locale(user.effective_locale) do + opts[:display_username] = I18n.t('embed.replies', count: count) + end end end @@ -260,6 +262,7 @@ class PostAlerter notification_data = { topic_title: post.topic.title, original_post_id: original_post.id, + original_post_type: original_post.post_type, original_username: original_username, display_username: opts[:display_username] || post.user.username } @@ -276,8 +279,7 @@ class PostAlerter post_action_id: opts[:post_action_id], data: notification_data.to_json) - if (!existing_notification) && NOTIFIABLE_TYPES.include?(type) - + if !existing_notification && NOTIFIABLE_TYPES.include?(type) # we may have an invalid post somehow, dont blow up post_url = original_post.url rescue nil if post_url diff --git a/spec/jobs/user_email_spec.rb b/spec/jobs/user_email_spec.rb index 6b6c5a93c..8685afd50 100644 --- a/spec/jobs/user_email_spec.rb +++ b/spec/jobs/user_email_spec.rb @@ -185,7 +185,6 @@ describe Jobs::UserEmail do expect(err.skipped_reason).to match(/notification.*already/) end - it "does send the email if the notification has been seen but the user is set for email_always" do Email::Sender.any_instance.expects(:send) notification.update_column(:read, true) @@ -275,6 +274,4 @@ describe Jobs::UserEmail do end - end - diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index 1fa789f65..09b9f5843 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -85,6 +85,7 @@ describe Notification do end end + describe 'unread counts' do let(:user) { Fabricate(:user) } @@ -171,7 +172,6 @@ describe Notification do Fabricate(:post, topic: @topic, user: @topic.user) @target.reload expect(@target.unread_private_messages).to eq(1) - end end diff --git a/spec/models/user_email_observer_spec.rb b/spec/models/user_email_observer_spec.rb index 9e91271d5..340eb2ed9 100644 --- a/spec/models/user_email_observer_spec.rb +++ b/spec/models/user_email_observer_spec.rb @@ -13,7 +13,6 @@ 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) @@ -66,6 +65,7 @@ describe UserEmailObserver do Jobs.expects(:enqueue_in).with(delay, :user_email, has_entry(type: type)).never UserEmailObserver.process_notification(notification) end + end context 'user_mentioned' do @@ -113,6 +113,13 @@ describe UserEmailObserver do let!(:notification) { create_notification(6) } include_examples "enqueue_private" + + 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) + end + end context 'user_invited_to_private_message' do