From f27de87bf3acbc6ed32bd33306ee1ecf9dc62633 Mon Sep 17 00:00:00 2001
From: Guo Xiang Tan <gxtan1990@gmail.com>
Date: Thu, 10 Sep 2020 17:13:11 +0800
Subject: [PATCH] FIX: Update first_pm_unread_at of user's groups without
 unread.

If a user always read all group messages, we will never update the
`first_pm_unread_at` column since the previous query will not return the
group_user. Instead, we should update `first_pm_unread_at` to the
current timestamp if the user has read everything.

Follow-up to 9b75d95fc616ea51181d622182b0f74dea8694ac
---
 app/models/group_user.rb       | 43 ++++++++++++++++++++--------------
 spec/models/group_user_spec.rb | 10 +++++---
 2 files changed, 32 insertions(+), 21 deletions(-)

diff --git a/app/models/group_user.rb b/app/models/group_user.rb
index d83b0606b5a..820bbcc1127 100644
--- a/app/models/group_user.rb
+++ b/app/models/group_user.rb
@@ -24,7 +24,7 @@ class GroupUser < ActiveRecord::Base
   end
 
   def self.update_first_unread_pm(last_seen, limit: 10_000)
-    DB.exec(<<~SQL, archetype: Archetype.private_message, last_seen: last_seen, limit: limit)
+    DB.exec(<<~SQL, archetype: Archetype.private_message, last_seen: last_seen, limit: limit, now: 10.minutes.ago)
     UPDATE group_users gu
     SET first_unread_pm_at = Y.min_date
     FROM (
@@ -34,23 +34,30 @@ class GroupUser < ActiveRecord::Base
         X.min_date
       FROM (
         SELECT
-          gu2.group_id,
-          gu2.user_id,
-          MIN(t.updated_at) min_date
-        FROM group_users gu2
-        INNER JOIN topic_allowed_groups tag ON tag.group_id = gu2.group_id
-        INNER JOIN topics t ON t.id = tag.topic_id
-        INNER JOIN users u ON u.id = gu2.user_id
-        LEFT JOIN topic_users tu ON t.id = tu.topic_id AND tu.user_id = gu2.user_id
-        WHERE t.deleted_at IS NULL
-        AND t.archetype = :archetype
-        AND tu.last_read_post_number < CASE
-                                       WHEN u.admin OR u.moderator
-                                       THEN t.highest_staff_post_number
-                                       ELSE t.highest_post_number
-                                       END
-        AND (COALESCE(tu.notification_level, 1) >= 2)
-        GROUP BY gu2.user_id, gu2.group_id
+          gu.group_id,
+          gu.user_id,
+          COALESCE(Z.min_date, :now) min_date
+        FROM group_users gu
+        LEFT JOIN (
+          SELECT
+            gu2.group_id,
+            gu2.user_id,
+            MIN(t.updated_at) min_date
+          FROM group_users gu2
+          INNER JOIN topic_allowed_groups tag ON tag.group_id = gu2.group_id
+          INNER JOIN topics t ON t.id = tag.topic_id
+          INNER JOIN users u ON u.id = gu2.user_id
+          LEFT JOIN topic_users tu ON t.id = tu.topic_id AND tu.user_id = gu2.user_id
+          WHERE t.deleted_at IS NULL
+          AND t.archetype = :archetype
+          AND tu.last_read_post_number < CASE
+                                         WHEN u.admin OR u.moderator
+                                         THEN t.highest_staff_post_number
+                                         ELSE t.highest_post_number
+                                         END
+          AND (COALESCE(tu.notification_level, 1) >= 2)
+          GROUP BY gu2.user_id, gu2.group_id
+        ) AS Z ON Z.user_id = gu.user_id AND Z.group_id = gu.group_id
       ) AS X
       WHERE X.user_id IN (
         SELECT id
diff --git a/spec/models/group_user_spec.rb b/spec/models/group_user_spec.rb
index fca2c2c7e6d..2f7a53a740d 100644
--- a/spec/models/group_user_spec.rb
+++ b/spec/models/group_user_spec.rb
@@ -163,6 +163,7 @@ describe GroupUser do
 
   describe '#ensure_consistency!' do
     fab!(:group) { Fabricate(:group) }
+    fab!(:group_2) { Fabricate(:group) }
 
     fab!(:pm_post) { Fabricate(:private_message_post) }
 
@@ -173,6 +174,7 @@ describe GroupUser do
     fab!(:user) do
       Fabricate(:user, last_seen_at: Time.zone.now).tap do |u|
         group.add(u)
+        group_2.add(u)
 
         TopicUser.change(u.id, pm_topic.id,
           notification_level: TopicUser.notification_levels[:tracking],
@@ -213,11 +215,13 @@ describe GroupUser do
         topic_id: pm_topic.id
       )
 
-      GroupUser.ensure_consistency!
+      expect { GroupUser.ensure_consistency! }
+        .to_not change { group.group_users.find_by(user_id: user_3.id).first_unread_pm_at }
 
+      expect(post.topic.updated_at).to_not eq(10.minutes.ago)
       expect(group.group_users.find_by(user_id: user.id).first_unread_pm_at).to eq_time(post.topic.updated_at)
-      expect(group.group_users.find_by(user_id: user_2.id).first_unread_pm_at).to_not eq_time(post.topic.updated_at)
-      expect(group.group_users.find_by(user_id: user_3.id).first_unread_pm_at).to_not eq_time(post.topic.updated_at)
+      expect(group_2.group_users.find_by(user_id: user.id).first_unread_pm_at).to eq_time(10.minutes.ago)
+      expect(group.group_users.find_by(user_id: user_2.id).first_unread_pm_at).to eq_time(10.minutes.ago)
     end
   end
 end