From 2a4db15544b781e227cdd2452ef16f4b7600ca1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Wed, 27 May 2020 17:09:40 +0200 Subject: [PATCH] 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. --- app/jobs/regular/user_email.rb | 7 ++++--- app/jobs/scheduled/enqueue_digest_emails.rb | 8 +++++--- app/models/skipped_email_log.rb | 3 ++- config/locales/server.en.yml | 1 + spec/jobs/enqueue_digest_emails_spec.rb | 8 ++++++++ 5 files changed, 20 insertions(+), 7 deletions(-) diff --git a/app/jobs/regular/user_email.rb b/app/jobs/regular/user_email.rb index afa1105e4fc..bee715099d2 100644 --- a/app/jobs/regular/user_email.rb +++ b/app/jobs/regular/user_email.rb @@ -26,16 +26,17 @@ module Jobs notification = nil type = args[:type] 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]) - 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? post = Post.find_by(id: args[:post_id]) - unless post.present? + if post.blank? return skip(SkippedEmailLog.reason_types[:user_email_post_not_found]) end diff --git a/app/jobs/scheduled/enqueue_digest_emails.rb b/app/jobs/scheduled/enqueue_digest_emails.rb index 18db50135ac..b126dc15534 100644 --- a/app/jobs/scheduled/enqueue_digest_emails.rb +++ b/app/jobs/scheduled/enqueue_digest_emails.rb @@ -14,13 +14,15 @@ module Jobs def target_user_ids # Users who want to receive digest email within their chosen digest email frequency - query = User.real - .not_suspended + query = User + .real .activated + .not_suspended .where(staged: false) - .joins(:user_option, :user_stat) + .joins(:user_option, :user_stat, :user_emails) .where("user_options.email_digests") .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_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})") diff --git a/app/models/skipped_email_log.rb b/app/models/skipped_email_log.rb index 9ffa2230e97..1a4ec415aa8 100644 --- a/app/models/skipped_email_log.rb +++ b/app/models/skipped_email_log.rb @@ -37,7 +37,8 @@ class SkippedEmailLog < ActiveRecord::Base sender_post_deleted: 20, sender_message_to_invalid: 21, 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 # when you add a new enum value ) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 0433fd08b58..c62622dbb9c 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -3803,6 +3803,7 @@ en: user_email_user_suspended: "user was suspended" user_email_already_read: "user has already read 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_to_blank: "message.to is blank" sender_text_part_body_blank: "text_part.body is blank" diff --git a/spec/jobs/enqueue_digest_emails_spec.rb b/spec/jobs/enqueue_digest_emails_spec.rb index 029be5a814d..3c9fcad5d66 100644 --- a/spec/jobs/enqueue_digest_emails_spec.rb +++ b/spec/jobs/enqueue_digest_emails_spec.rb @@ -107,6 +107,14 @@ describe Jobs::EnqueueDigestEmails do 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 describe '#execute' do