FIX: only ever send users 1 email per post

in the past ninja editing a post to add a mention could trigger duplicate
emails to a user (and a few other edge cases)
This commit is contained in:
Sam 2016-04-15 15:59:01 +10:00
parent a1d65ae8f6
commit 0119a2f980
4 changed files with 48 additions and 2 deletions

View file

@ -48,7 +48,11 @@ module Jobs
) )
else else
message = UserNotifications.mailing_list_notify(user, post) message = UserNotifications.mailing_list_notify(user, post)
Email::Sender.new(message, :mailing_list, user).send if message if message
EmailLog.unique_email_per_post(post, user) do
Email::Sender.new(message, :mailing_list, user).send
end
end
end end
rescue => e rescue => e
Discourse.handle_job_exception(e, error_context(args, "Sending post to mailing list subscribers", { user_id: user.id, user_email: user.email })) Discourse.handle_job_exception(e, error_context(args, "Sending post to mailing list subscribers", { user_id: user.id, user_email: user.email }))

View file

@ -117,7 +117,9 @@ module Jobs
return skip_message(I18n.t('email_log.exceeded_limit')) return skip_message(I18n.t('email_log.exceeded_limit'))
end end
message = UserNotifications.send(type, user, email_args) message = EmailLog.unique_email_per_post(post, user) do
UserNotifications.send(type, user, email_args)
end
# Update the to address if we have a custom one # Update the to address if we have a custom one
if message && to_address.present? if message && to_address.present?

View file

@ -1,3 +1,5 @@
require_dependency 'distributed_mutex'
class EmailLog < ActiveRecord::Base class EmailLog < ActiveRecord::Base
belongs_to :user belongs_to :user
belongs_to :post belongs_to :post
@ -13,6 +15,18 @@ class EmailLog < ActiveRecord::Base
User.where(id: user_id).update_all("last_emailed_at = CURRENT_TIMESTAMP") if user_id.present? && !skipped User.where(id: user_id).update_all("last_emailed_at = CURRENT_TIMESTAMP") if user_id.present? && !skipped
end end
def self.unique_email_per_post(post, user)
return yield unless post && user
DistributedMutex.synchronize("email_log_#{post.id}_#{user.id}") do
if where(post_id: post.id, user_id: user.id, skipped: false).exists?
nil
else
yield
end
end
end
def self.reached_max_emails?(user) def self.reached_max_emails?(user)
return false if SiteSetting.max_emails_per_day_per_user == 0 return false if SiteSetting.max_emails_per_day_per_user == 0

View file

@ -8,6 +8,32 @@ describe EmailLog do
let(:user) { Fabricate(:user) } let(:user) { Fabricate(:user) }
context 'unique email per post' do
it 'only allows through one email per post' do
post = Fabricate(:post)
user = post.user
# skipped emails do not matter
user.email_logs.create(email_type: 'blah', post_id: post.id, to_address: user.email, user_id: user.id, skipped: true)
ran = EmailLog.unique_email_per_post(post, user) do
true
end
expect(ran).to eq(true)
user.email_logs.create(email_type: 'blah', post_id: post.id, to_address: user.email, user_id: user.id)
ran = EmailLog.unique_email_per_post(post, user) do
true
end
expect(ran).to be_falsy
end
end
context 'after_create' do context 'after_create' do
context 'with user' do context 'with user' do
it 'updates the last_emailed_at value for the user' do it 'updates the last_emailed_at value for the user' do