FIX: dismiss new when topic_user exists without last read (#12103)

The bug was mentioned on meta: https://meta.discourse.org/t/pressing-dismiss-new-doesnt-clear-new-topics/179858

Problem is that sometimes the user has TopicUser records with `last_read_post_number` set as NULL. In that case, the topic is still "new" to them and should be dismissed when they click dismiss button.

In addition, I added that condition to post_migration and bumped the number to fix existing records. Migration is written to be idempotent so it will make no harm to already deployed instances.

app/services/dismiss_topics.rb

@@ -17,7 +17,7 @@ class DismissTopics
     @rows ||= @topics_scope
       .joins("LEFT JOIN topic_users ON topic_users.topic_id = topics.id AND topic_users.user_id = #{@user.id}")
       .where("topics.created_at >= ?", since_date)
-      .where("topic_users.id IS NULL")
+      .where("topic_users.id IS NULL OR topic_users.last_read_post_number IS NULL")
       .where("topics.archetype <> ?", Archetype.private_message)
       .order("topics.created_at DESC")
       .limit(SiteSetting.max_new_topics).map do |topic|

db/post_migrate/20210208022738_move_new_since_to_new_table.rb

@@ -1,40 +0,0 @@
-# frozen_string_literal: true
-
-class MoveNewSinceToNewTable < ActiveRecord::Migration[6.0]
-  def up
-    sql = <<~SQL
-    INSERT INTO dismissed_topic_users (user_id, topic_id, created_at)
-    SELECT users.id, topics.id, user_stats.new_since
-    FROM user_stats
-    JOIN users ON users.id = user_stats.user_id
-    JOIN user_options ON user_options.user_id = users.id
-    LEFT JOIN topics ON topics.created_at <= user_stats.new_since
-      AND topics.archetype <> :private_message
-      AND topics.created_at >= GREATEST(CASE
-                  WHEN COALESCE(user_options.new_topic_duration_minutes, :default_duration) = :always THEN users.created_at
-                  WHEN COALESCE(user_options.new_topic_duration_minutes, :default_duration) = :last_visit THEN COALESCE(users.previous_visit_at,users.created_at)
-                  ELSE (:now::timestamp - INTERVAL '1 MINUTE' * COALESCE(user_options.new_topic_duration_minutes, :default_duration))
-               END, :min_date)
-      AND topics.id IN(SELECT id FROM topics ORDER BY created_at DESC LIMIT :max_new_topics)
-    LEFT JOIN topic_users ON topics.id = topic_users.topic_id AND users.id = topic_users.user_id
-    LEFT JOIN dismissed_topic_users ON dismissed_topic_users.topic_id = topics.id AND users.id = dismissed_topic_users.user_id
-    WHERE user_stats.new_since IS NOT NULL
-    AND topic_users.id IS NULL
-    AND topics.id IS NOT NULL
-    AND dismissed_topic_users.id IS NULL
-    ORDER BY topics.created_at DESC
-    SQL
-    DB.exec(sql,
-            now: DateTime.now,
-            last_visit: User::NewTopicDuration::LAST_VISIT,
-            always: User::NewTopicDuration::ALWAYS,
-            default_duration: SiteSetting.default_other_new_topic_duration_minutes,
-            min_date: Time.at(SiteSetting.min_new_topics_time).to_datetime,
-            private_message: Archetype.private_message,
-            max_new_topics: SiteSetting.max_new_topics)
-  end
-
-  def down
-    raise ActiveRecord::IrreversibleMigration
-  end
-end

db/post_migrate/20210208022739_move_new_since_to_new_table.rb

@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+
+class MoveNewSinceToNewTable < ActiveRecord::Migration[6.0]
+  disable_ddl_transaction!
+  BATCH_SIZE = 30_000
+
+  def up
+    offset = 0
+    loop do
+      user_stat_user_ids = DB.query("SELECT user_id FROM user_stats LIMIT #{BATCH_SIZE} OFFSET :offset", offset: offset).map(&:user_id)
+      break if user_stat_user_ids.count.zero?
+      sql = <<~SQL
+        INSERT INTO dismissed_topic_users (user_id, topic_id, created_at)
+        SELECT users.id, topics.id, user_stats.new_since
+        FROM user_stats
+        JOIN users ON users.id = user_stats.user_id
+        JOIN user_options ON user_options.user_id = users.id
+        LEFT JOIN topics ON topics.created_at <= user_stats.new_since
+          AND topics.archetype <> :private_message
+          AND topics.created_at >= GREATEST(CASE
+                      WHEN COALESCE(user_options.new_topic_duration_minutes, :default_duration) = :always THEN users.created_at
+                      WHEN COALESCE(user_options.new_topic_duration_minutes, :default_duration) = :last_visit THEN COALESCE(users.previous_visit_at,users.created_at)
+                      ELSE (:now::timestamp - INTERVAL '1 MINUTE' * COALESCE(user_options.new_topic_duration_minutes, :default_duration))
+                   END, :min_date)
+          AND topics.id IN(SELECT id FROM topics ORDER BY created_at DESC LIMIT :max_new_topics)
+        LEFT JOIN topic_users ON topics.id = topic_users.topic_id AND users.id = topic_users.user_id
+        LEFT JOIN dismissed_topic_users ON dismissed_topic_users.topic_id = topics.id AND users.id = dismissed_topic_users.user_id
+        WHERE user_stats.new_since IS NOT NULL
+        AND user_stats.user_id IN (:user_stat_user_ids)
+        AND topic_users.last_read_post_number IS NULL
+        AND topics.id IS NOT NULL
+        AND dismissed_topic_users.id IS NULL
+        ORDER BY topics.created_at DESC
+        ON CONFLICT DO NOTHING
+      SQL
+      DB.exec(sql,
+               now: DateTime.now,
+               last_visit: User::NewTopicDuration::LAST_VISIT,
+               always: User::NewTopicDuration::ALWAYS,
+               default_duration: SiteSetting.default_other_new_topic_duration_minutes,
+               min_date: Time.at(SiteSetting.min_new_topics_time).to_datetime,
+               private_message: Archetype.private_message,
+               user_stat_user_ids: user_stat_user_ids,
+               max_new_topics: SiteSetting.max_new_topics)
+      offset = offset + BATCH_SIZE
+    end
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

spec/services/dismiss_topics_spec.rb

@@ -29,11 +29,17 @@ describe DismissTopics do
     end
 
     it 'respects seen topics' do
-      Fabricate(:topic_user, user: user, topic: topic1)
-      Fabricate(:topic_user, user: user, topic: topic2)
+      Fabricate(:topic_user, user: user, topic: topic1, last_read_post_number: 1)
+      Fabricate(:topic_user, user: user, topic: topic2, last_read_post_number: 1)
       expect { described_class.new(user, Topic.all).perform! }.to change { DismissedTopicUser.count }.by(0)
     end
 
+    it 'dismisses when topic user without last_read_post_number' do
+      Fabricate(:topic_user, user: user, topic: topic1, last_read_post_number: nil)
+      Fabricate(:topic_user, user: user, topic: topic2, last_read_post_number: nil)
+      expect { described_class.new(user, Topic.all).perform! }.to change { DismissedTopicUser.count }.by(2)
+    end
+
     it 'respects new_topic_duration_minutes' do
       user.user_option.update!(new_topic_duration_minutes: 70)