Periodic Digest Email frequency should be in days (#15904)

* Use the most recent timestamp from sent digest messages

The original logic here had queried for up to 10 (unordered) messages
from the database for this user, perhaps leveraging the knowledge that
we purge old messages after 90 days to ensure 10 was enough.

Since the last message in the limited and unordered list could have
had any sent_at time in the past 90 days, it's possible users could
pass the "should send email" check multiple times in a day (and get
multiple digest emails, I'm not certain when the rake task runs but it
could be as frequently as once per deployment?)

Since we're only using this list of messages to find the most recently
sent message, select the maximum sent time.

* convert periodic_email_digest setting to days

Time.current - last_email_sent_at gives an integer count of elapsed
seconds

Settings::General.periodic_email_digest is an integer count of days
between digests.

Convert to days (so we're only sending digests when enough time has
elapsed) instead of defaulting to once every 2 seconds (or at least
several times per day).

* Clean up comparison

Rather than subtracting the current time from the last time, and
checking that the difference in time is greater than the periodic
setting, use days.ago to get the timestamp far enough in the past, and
ensure last email was sent before that ( `sent_at < n.days.ago` ).

This seems like it reads clearer than the original implementation.

* Prefer Time.before? to numeric comparison

Change the method name from last_email_sent_at to last_email_sent, so
that the comparison reads like English (the other callers use it as a
number)

And since the code reveals its intention clearly, there's no need for
an inline comment about the next line any more.
This commit is contained in:
Daniel Uber 2021-12-30 09:23:24 -06:00 committed by GitHub
parent 3acd4e9deb
commit cd835c281f
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23

View file

@ -48,21 +48,23 @@ class EmailDigestArticleCollector
private
def should_receive_email?
return true unless last_email_sent_at
return true unless last_email_sent
# Has it been at least x days since @user received an email?
Time.current - last_email_sent_at >= Settings::General.periodic_email_digest
last_email_sent.before? Settings::General.periodic_email_digest.days.ago
end
def last_email_sent_at
last_user_emails.last&.sent_at
def last_email_sent
@last_email_sent ||=
@user.email_messages
.where(mailer: "DigestMailer#digest_email")
.maximum(:sent_at)
end
def cutoff_date
a_few_days_ago = 4.days.ago.utc
return a_few_days_ago unless last_email_sent_at
return a_few_days_ago unless last_email_sent
[a_few_days_ago, last_email_sent_at].max
[a_few_days_ago, last_email_sent].max
end
def user_has_followings?
@ -70,8 +72,4 @@ class EmailDigestArticleCollector
@user.cached_followed_tag_names.any? ||
@user.cached_antifollowed_tag_names.any?
end
def last_user_emails
@last_user_emails ||= @user.email_messages.select(:sent_at).where(mailer: "DigestMailer#digest_email").limit(10)
end
end