FIX: don't send empty email notifications for small_actions

This commit is contained in:
Régis Hanol 2016-02-01 19:12:10 +01:00
parent 8608b5cbc1
commit 12051f79a8
6 changed files with 31 additions and 19 deletions

View file

@ -113,14 +113,11 @@ class Notification < ActiveRecord::Base
end end
def url def url
if topic.present? topic.relative_url(post_number) if topic.present?
return topic.relative_url(post_number)
end
end end
def post def post
return if topic_id.blank? || post_number.blank? return if topic_id.blank? || post_number.blank?
Post.find_by(topic_id: topic_id, post_number: post_number) Post.find_by(topic_id: topic_id, post_number: post_number)
end end

View file

@ -61,17 +61,17 @@ class UserEmailObserver < ActiveRecord::Observer
def enqueue(type, delay=default_delay) def enqueue(type, delay=default_delay)
return unless notification.user.email_direct? return unless notification.user.email_direct?
perform_enqueue(type,delay) perform_enqueue(type, delay)
end end
def enqueue_private(type, delay=private_delay) def enqueue_private(type, delay=private_delay)
return unless notification.user.email_private_messages? return unless notification.user.email_private_messages?
perform_enqueue(type,delay) perform_enqueue(type, delay)
end end
def perform_enqueue(type, delay) def perform_enqueue(type, delay)
return unless notification.user.active? || notification.user.staged? 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)) Jobs.enqueue_in(delay, :user_email, self.class.notification_params(notification, type))
end end
@ -84,6 +84,14 @@ class UserEmailObserver < ActiveRecord::Observer
20.seconds 20.seconds
end 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 end
def after_commit(notification) def after_commit(notification)
@ -94,6 +102,7 @@ class UserEmailObserver < ActiveRecord::Observer
def self.process_notification(notification) def self.process_notification(notification)
email_user = EmailUser.new(notification) email_user = EmailUser.new(notification)
email_method = Notification.types[notification.notification_type] email_method = Notification.types[notification.notification_type]
email_user.send(email_method) if email_user.respond_to? email_method email_user.send(email_method) if email_user.respond_to? email_method
end end

View file

@ -234,8 +234,8 @@ class PostAlerter
collapsed = false collapsed = false
if type == Notification.types[:replied] || type == Notification.types[:posted] if type == Notification.types[:replied] || type == Notification.types[:posted]
destroy_notifications(user, Notification.types[:replied] , post.topic) destroy_notifications(user, Notification.types[:replied], post.topic)
destroy_notifications(user, Notification.types[:posted] , post.topic) destroy_notifications(user, Notification.types[:posted], post.topic)
collapsed = true collapsed = true
end end
@ -248,10 +248,12 @@ class PostAlerter
original_username = opts[:display_username] || post.username original_username = opts[:display_username] || post.username
if collapsed if collapsed
post = first_unread_post(user,post.topic) || post post = first_unread_post(user, post.topic) || post
count = unread_count(user, post.topic) count = unread_count(user, post.topic)
if count > 1
I18n.with_locale(user.effective_locale) do I18n.with_locale(user.effective_locale) do
opts[:display_username] = I18n.t('embed.replies', count: count) if count > 1 opts[:display_username] = I18n.t('embed.replies', count: count)
end
end end
end end
@ -260,6 +262,7 @@ class PostAlerter
notification_data = { notification_data = {
topic_title: post.topic.title, topic_title: post.topic.title,
original_post_id: original_post.id, original_post_id: original_post.id,
original_post_type: original_post.post_type,
original_username: original_username, original_username: original_username,
display_username: opts[:display_username] || post.user.username display_username: opts[:display_username] || post.user.username
} }
@ -276,8 +279,7 @@ class PostAlerter
post_action_id: opts[:post_action_id], post_action_id: opts[:post_action_id],
data: notification_data.to_json) 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 # we may have an invalid post somehow, dont blow up
post_url = original_post.url rescue nil post_url = original_post.url rescue nil
if post_url if post_url

View file

@ -185,7 +185,6 @@ describe Jobs::UserEmail do
expect(err.skipped_reason).to match(/notification.*already/) expect(err.skipped_reason).to match(/notification.*already/)
end end
it "does send the email if the notification has been seen but the user is set for email_always" do 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) Email::Sender.any_instance.expects(:send)
notification.update_column(:read, true) notification.update_column(:read, true)
@ -275,6 +274,4 @@ describe Jobs::UserEmail do
end end
end end

View file

@ -85,6 +85,7 @@ describe Notification do
end end
end end
describe 'unread counts' do describe 'unread counts' do
let(:user) { Fabricate(:user) } let(:user) { Fabricate(:user) }
@ -171,7 +172,6 @@ describe Notification do
Fabricate(:post, topic: @topic, user: @topic.user) Fabricate(:post, topic: @topic, user: @topic.user)
@target.reload @target.reload
expect(@target.unread_private_messages).to eq(1) expect(@target.unread_private_messages).to eq(1)
end end
end end

View file

@ -13,7 +13,6 @@ describe UserEmailObserver do
shared_examples "enqueue" do shared_examples "enqueue" do
it "enqueues a job for the email" do it "enqueues a job for the email" do
Jobs.expects(:enqueue_in).with(delay, :user_email, UserEmailObserver::EmailUser.notification_params(notification,type)) Jobs.expects(:enqueue_in).with(delay, :user_email, UserEmailObserver::EmailUser.notification_params(notification,type))
UserEmailObserver.process_notification(notification) UserEmailObserver.process_notification(notification)
@ -66,6 +65,7 @@ describe UserEmailObserver do
Jobs.expects(:enqueue_in).with(delay, :user_email, has_entry(type: type)).never Jobs.expects(:enqueue_in).with(delay, :user_email, has_entry(type: type)).never
UserEmailObserver.process_notification(notification) UserEmailObserver.process_notification(notification)
end end
end end
context 'user_mentioned' do context 'user_mentioned' do
@ -113,6 +113,13 @@ describe UserEmailObserver do
let!(:notification) { create_notification(6) } let!(:notification) { create_notification(6) }
include_examples "enqueue_private" 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 end
context 'user_invited_to_private_message' do context 'user_invited_to_private_message' do