From 5f6b6e9818eb40a6ed133c1f1b2dac97f9cacf40 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9gis=20Hanol?= <regis@hanol.fr>
Date: Mon, 27 May 2024 17:33:41 +0200
Subject: [PATCH] FIX: correctly compute the window for email summaries
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

In https://github.com/discourse/discourse/commit/958437e7ddce6e015725522d25c3f8327ff4064a we ensured that the email summaries are properly sent based on 'digest_attempted_at' for people who barely/never visit the forum.

This fixed the "frequency" of the email summaries but introduced a bug where the digest would be sent even though there wasn't anything new since for some users.

The logic we use to compute the threshold date for the content to be included in the digest was

```ruby
@since = opts[:since] || user.last_seen_at || user.user_stat&.digest_attempted_at || 1.month.ago
```

It was working as expected for users who haven never been seen but for users who have connected at least once, we would use their "last_seen_at" date as the "threshold date" for the content to be sent in a summary 😬

This fix changes the logic to be the most recent date amongst the `last_seen_at`, `digest_attempted_at` and `1.month.ago` so it's correctly handling cases where

- user has never been seen nor emailed a summary
- user has been seen in a while but has recently been sent a summary
- user has been sent a summary recently but hasn't been seen in a while.
---
 app/mailers/user_notifications.rb       |  3 ++-
 spec/mailers/user_notifications_spec.rb | 23 +++++++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb
index 724d9172afd..0f33c4021ab 100644
--- a/app/mailers/user_notifications.rb
+++ b/app/mailers/user_notifications.rb
@@ -224,7 +224,8 @@ class UserNotifications < ActionMailer::Base
     build_summary_for(user)
     @unsubscribe_key = UnsubscribeKey.create_key_for(@user, UnsubscribeKey::DIGEST_TYPE)
 
-    @since = opts[:since] || user.last_seen_at || user.user_stat&.digest_attempted_at || 1.month.ago
+    @since = opts[:since].presence
+    @since ||= [user.last_seen_at, user.user_stat&.digest_attempted_at, 1.month.ago].compact.max
 
     # Fetch some topics and posts to show
     digest_opts = {
diff --git a/spec/mailers/user_notifications_spec.rb b/spec/mailers/user_notifications_spec.rb
index adc4f2137b2..5ab50623a9c 100644
--- a/spec/mailers/user_notifications_spec.rb
+++ b/spec/mailers/user_notifications_spec.rb
@@ -450,6 +450,29 @@ RSpec.describe UserNotifications do
         expect(html).to match(' lang="pl-PL"')
         expect(html).to match(' xml:lang="pl-PL"')
       end
+
+      it "uses digest_attempted_at when user hasn't been seen in a while" do
+        user.update!(last_seen_at: 7.days.ago)
+        user.user_stat.update!(digest_attempted_at: 30.minutes.ago)
+        expect(email.to).to be_nil
+      end
+
+      it "uses last_seen_at when user has been sent a digest in a while" do
+        user.update!(last_seen_at: 30.minutes.ago)
+        user.user_stat.update!(digest_attempted_at: 7.days.ago)
+        expect(email.to).to be_nil
+      end
+
+      it "caps at 1 month when user has never been seen or sent a digest" do
+        old_topic = Fabricate(:topic, created_at: 2.months.ago)
+
+        user.update!(last_seen_at: nil)
+        user.user_stat.update!(digest_attempted_at: nil)
+        expect(email.to).to contain_exactly(user.email)
+
+        html = email.html_part.body.to_s
+        expect(html).not_to include(old_topic.title)
+      end
     end
   end