From cd835c281fcb3309fe404426c093de1ea95aa9b9 Mon Sep 17 00:00:00 2001
From: Daniel Uber <djuber@gmail.com>
Date: Thu, 30 Dec 2021 09:23:24 -0600
Subject: [PATCH] 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.
---
 .../email_digest_article_collector.rb         | 20 +++++++++----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/app/services/email_digest_article_collector.rb b/app/services/email_digest_article_collector.rb
index 8601aeadc..71565936e 100644
--- a/app/services/email_digest_article_collector.rb
+++ b/app/services/email_digest_article_collector.rb
@@ -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