FIX: don't send digests to users with no primary email
It might happen that some User records have no associated primary emails. In which case we don't ever want to send them a digest. Also added a new "user_email_no_email" skipped email log to ensure these cases are properly handled and surfaced.
This commit is contained in:
parent
2152e70e0d
commit
2a4db15544
|
@ -26,16 +26,17 @@ module Jobs
|
||||||
notification = nil
|
notification = nil
|
||||||
type = args[:type]
|
type = args[:type]
|
||||||
user = User.find_by(id: args[:user_id])
|
user = User.find_by(id: args[:user_id])
|
||||||
to_address = args[:to_address].presence || user.try(:email).presence || "no_email_found"
|
to_address = args[:to_address].presence || user&.primary_email&.email.presence || "no_email_found"
|
||||||
|
|
||||||
set_skip_context(type, args[:user_id], to_address, args[:post_id])
|
set_skip_context(type, args[:user_id], to_address, args[:post_id])
|
||||||
|
|
||||||
return skip(SkippedEmailLog.reason_types[:user_email_no_user]) unless user
|
return skip(SkippedEmailLog.reason_types[:user_email_no_user]) if !user
|
||||||
|
return skip(SkippedEmailLog.reason_types[:user_email_no_email]) if to_address == "no_email_found"
|
||||||
|
|
||||||
if args[:post_id].present?
|
if args[:post_id].present?
|
||||||
post = Post.find_by(id: args[:post_id])
|
post = Post.find_by(id: args[:post_id])
|
||||||
|
|
||||||
unless post.present?
|
if post.blank?
|
||||||
return skip(SkippedEmailLog.reason_types[:user_email_post_not_found])
|
return skip(SkippedEmailLog.reason_types[:user_email_post_not_found])
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
|
@ -14,13 +14,15 @@ module Jobs
|
||||||
|
|
||||||
def target_user_ids
|
def target_user_ids
|
||||||
# Users who want to receive digest email within their chosen digest email frequency
|
# Users who want to receive digest email within their chosen digest email frequency
|
||||||
query = User.real
|
query = User
|
||||||
.not_suspended
|
.real
|
||||||
.activated
|
.activated
|
||||||
|
.not_suspended
|
||||||
.where(staged: false)
|
.where(staged: false)
|
||||||
.joins(:user_option, :user_stat)
|
.joins(:user_option, :user_stat, :user_emails)
|
||||||
.where("user_options.email_digests")
|
.where("user_options.email_digests")
|
||||||
.where("user_stats.bounce_score < #{SiteSetting.bounce_score_threshold}")
|
.where("user_stats.bounce_score < #{SiteSetting.bounce_score_threshold}")
|
||||||
|
.where("user_emails.primary")
|
||||||
.where("COALESCE(last_emailed_at, '2010-01-01') <= CURRENT_TIMESTAMP - ('1 MINUTE'::INTERVAL * user_options.digest_after_minutes)")
|
.where("COALESCE(last_emailed_at, '2010-01-01') <= CURRENT_TIMESTAMP - ('1 MINUTE'::INTERVAL * user_options.digest_after_minutes)")
|
||||||
.where("COALESCE(last_seen_at, '2010-01-01') <= CURRENT_TIMESTAMP - ('1 MINUTE'::INTERVAL * user_options.digest_after_minutes)")
|
.where("COALESCE(last_seen_at, '2010-01-01') <= CURRENT_TIMESTAMP - ('1 MINUTE'::INTERVAL * user_options.digest_after_minutes)")
|
||||||
.where("COALESCE(last_seen_at, '2010-01-01') >= CURRENT_TIMESTAMP - ('1 DAY'::INTERVAL * #{SiteSetting.suppress_digest_email_after_days})")
|
.where("COALESCE(last_seen_at, '2010-01-01') >= CURRENT_TIMESTAMP - ('1 DAY'::INTERVAL * #{SiteSetting.suppress_digest_email_after_days})")
|
||||||
|
|
|
@ -37,7 +37,8 @@ class SkippedEmailLog < ActiveRecord::Base
|
||||||
sender_post_deleted: 20,
|
sender_post_deleted: 20,
|
||||||
sender_message_to_invalid: 21,
|
sender_message_to_invalid: 21,
|
||||||
user_email_access_denied: 22,
|
user_email_access_denied: 22,
|
||||||
sender_topic_deleted: 23
|
sender_topic_deleted: 23,
|
||||||
|
user_email_no_email: 24,
|
||||||
# you need to add the reason in server.en.yml below the "skipped_email_log" key
|
# you need to add the reason in server.en.yml below the "skipped_email_log" key
|
||||||
# when you add a new enum value
|
# when you add a new enum value
|
||||||
)
|
)
|
||||||
|
|
|
@ -3803,6 +3803,7 @@ en:
|
||||||
user_email_user_suspended: "user was suspended"
|
user_email_user_suspended: "user was suspended"
|
||||||
user_email_already_read: "user has already read this post"
|
user_email_already_read: "user has already read this post"
|
||||||
user_email_access_denied: "user is not allowed to see this post"
|
user_email_access_denied: "user is not allowed to see this post"
|
||||||
|
user_email_no_email: "No email associated with user id %{user_id}"
|
||||||
sender_message_blank: "message is blank"
|
sender_message_blank: "message is blank"
|
||||||
sender_message_to_blank: "message.to is blank"
|
sender_message_to_blank: "message.to is blank"
|
||||||
sender_text_part_body_blank: "text_part.body is blank"
|
sender_text_part_body_blank: "text_part.body is blank"
|
||||||
|
|
|
@ -107,6 +107,14 @@ describe Jobs::EnqueueDigestEmails do
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
context "no primary email" do
|
||||||
|
let!(:user) { Fabricate(:active_user, last_seen_at: 2.months.ago) }
|
||||||
|
|
||||||
|
it "doesn't return users with no primary emails" do
|
||||||
|
UserEmail.where(user: user).delete_all
|
||||||
|
expect(Jobs::EnqueueDigestEmails.new.target_user_ids.include?(user.id)).to eq(false)
|
||||||
|
end
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
describe '#execute' do
|
describe '#execute' do
|
||||||
|
|
Loading…
Reference in New Issue