FIX: Dismiss new with better migration (#12062)

Original PR was reverted because of broken migration https://github.com/discourse/discourse/pull/12058

I fixed it by adding this line
```
          AND topics.id IN(SELECT id FROM topics ORDER BY created_at DESC LIMIT :max_new_topics)
```

This time it is left joining a limited amount of topics. I tested it on few databases and it worked quite smooth

app/assets/javascripts/discourse/app/models/topic-tracking-state.js

@@ -182,17 +182,11 @@ const TopicTrackingState = EmberObject.extend({
   },
 
   dismissNewTopic(data) {
-    Object.keys(this.states).forEach((k) => {
-      const topic = this.states[k];
-      if (
-        (!data.payload.category_id ||
-          topic.category_id === parseInt(data.payload.category_id, 10)) &&
-        (!data.payload.tag_id || topic.tags.includes(data.payload.tag_id))
-      ) {
-        this.states[k] = Object.assign({}, topic, {
+    data.payload.topic_ids.forEach((k) => {
+      const topic = this.states[`t${k}`];
+      this.states[`t${k}`] = Object.assign({}, topic, {
         is_seen: true,
       });
-      }
     });
     this.notifyPropertyChange("states");
     this.incrementMessageCount();

app/assets/javascripts/discourse/tests/unit/models/topic-tracking-state-test.js

@@ -346,36 +346,7 @@ module("Unit | Model | topic-tracking-state", function (hooks) {
 
     state.dismissNewTopic({
       message_type: "dismiss_new",
-      topic_id: 112,
-      payload: { category_id: 2 },
-    });
-    assert.equal(state.states["t112"].is_seen, false);
-    state.dismissNewTopic({
-      message_type: "dismiss_new",
-      topic_id: 112,
-      payload: { category_id: 1 },
-    });
-    assert.equal(state.states["t112"].is_seen, true);
-
-    state.states["t112"].is_seen = false;
-    state.dismissNewTopic({
-      message_type: "dismiss_new",
-      topic_id: 112,
-      payload: { tag_id: "bar" },
-    });
-    assert.equal(state.states["t112"].is_seen, false);
-    state.dismissNewTopic({
-      message_type: "dismiss_new",
-      topic_id: 112,
-      payload: { tag_id: "foo" },
-    });
-    assert.equal(state.states["t112"].is_seen, true);
-
-    state.states["t112"].is_seen = false;
-    state.dismissNewTopic({
-      message_type: "dismiss_new",
-      topic_id: 112,
-      payload: {},
+      payload: { topic_ids: [112] },
     });
     assert.equal(state.states["t112"].is_seen, true);
   });

app/controllers/topics_controller.rb

@@ -914,36 +914,30 @@ class TopicsController < ApplicationController
   end
 
   def reset_new
+    topic_scope =
       if params[:category_id].present?
         category_ids = [params[:category_id]]
         if params[:include_subcategories] == 'true'
           category_ids = category_ids.concat(Category.where(parent_category_id: params[:category_id]).pluck(:id))
         end
 
-      topic_scope =
-        if params[:tag_id]
-          Topic.where(category_id: category_ids).joins(:tags).where(tags: { name: params[:tag_id] })
-        else
-          Topic.where(category_id: category_ids)
-        end
-
-      DismissTopics.new(current_user, topic_scope).perform!
-      category_ids.each do |category_id|
-        TopicTrackingState.publish_dismiss_new(current_user.id, category_id: category_id, tag_id: params[:tag_id])
-      end
+        scope = Topic.where(category_id: category_ids)
+        scope = scope.joins(:tags).where(tags: { name: params[:tag_id] }) if params[:tag_id]
+        scope
       elsif params[:tag_id].present?
-      DismissTopics.new(current_user, Topic.joins(:tags).where(tags: { name: params[:tag_id] })).perform!
-      TopicTrackingState.publish_dismiss_new(current_user.id, tag_id: params[:tag_id])
+        Topic.joins(:tags).where(tags: { name: params[:tag_id] })
       else
         if params[:tracked].to_s == "true"
-        topics = TopicQuery.tracked_filter(TopicQuery.new(current_user).new_results, current_user.id)
-        topic_users = topics.map { |topic| { topic_id: topic.id, user_id: current_user.id, last_read_post_number: 0 } }
-        TopicUser.insert_all(topic_users) if !topic_users.empty?
+          TopicQuery.tracked_filter(TopicQuery.new(current_user).new_results, current_user.id)
         else
           current_user.user_stat.update_column(:new_since, Time.zone.now)
-        TopicTrackingState.publish_dismiss_new(current_user.id)
+          Topic
         end
       end
+
+    dismissed_topic_ids = DismissTopics.new(current_user, topic_scope).perform!
+    TopicTrackingState.publish_dismiss_new(current_user.id, topic_ids: dismissed_topic_ids)
+
     render body: nil
   end
 

app/jobs/scheduled/clean_dismissed_topic_users.rb

@@ -24,7 +24,7 @@ module Jobs
                     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, user_stats.new_since, :min_date)
+                 END, users.created_at, :min_date)
         AND dtu1.id = dtu2.id
       SQL
       sql = DB.sql_fragment(sql,

app/models/topic_tracking_state.rb

@@ -215,13 +215,12 @@ class TopicTrackingState
     MessageBus.publish(self.unread_channel_key(user_id), message.as_json, user_ids: [user_id])
   end
 
-  def self.publish_dismiss_new(user_id, category_id: nil, tag_id: nil)
-    payload = {}
-    payload[:category_id] = category_id if category_id
-    payload[:tag_id] = tag_id if tag_id
+  def self.publish_dismiss_new(user_id, topic_ids: [])
     message = {
       message_type: "dismiss_new",
-      payload: payload
+      payload: {
+        topic_ids: topic_ids
+      }
     }
     MessageBus.publish(self.unread_channel_key(user_id), message.as_json, user_ids: [user_id])
   end
@@ -231,7 +230,7 @@ class TopicTrackingState
                   WHEN COALESCE(uo.new_topic_duration_minutes, :default_duration) = :always THEN u.created_at
                   WHEN COALESCE(uo.new_topic_duration_minutes, :default_duration) = :last_visit THEN COALESCE(u.previous_visit_at,u.created_at)
                   ELSE (:now::timestamp - INTERVAL '1 MINUTE' * COALESCE(uo.new_topic_duration_minutes, :default_duration))
-               END, us.new_since, :min_date)",
+               END, u.created_at, :min_date)",
                 now: DateTime.now,
                 last_visit: User::NewTopicDuration::LAST_VISIT,
                 always: User::NewTopicDuration::ALWAYS,

app/models/user_option.rb

@@ -149,7 +149,7 @@ class UserOption < ActiveRecord::Base
       else
         duration.minutes.ago
       end,
-      user.user_stat.new_since,
+      user.created_at,
       Time.at(SiteSetting.min_new_topics_time).to_datetime
     ]
 

app/services/dismiss_topics.rb

@@ -8,6 +8,7 @@ class DismissTopics
 
   def perform!
     DismissedTopicUser.insert_all(rows) if rows.present?
+    @rows.map { |row| row[:topic_id] }
   end
 
   private
@@ -17,6 +18,7 @@ class DismissTopics
       .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("topics.archetype <> ?", Archetype.private_message)
       .order("topics.created_at DESC")
       .limit(SiteSetting.max_new_topics).map do |topic|
       {
@@ -38,6 +40,6 @@ class DismissTopics
       else
         new_topic_duration_minutes.minutes.ago
       end
-    [setting_date, @user.user_stat.new_since, Time.at(SiteSetting.min_new_topics_time).to_datetime].max
+    [setting_date, @user.created_at, Time.at(SiteSetting.min_new_topics_time).to_datetime].max
   end
 end

app/services/user_merger.rb

@@ -266,6 +266,8 @@ class UserMerger
     update_user_id(:draft_sequences, conditions: "x.draft_key = y.draft_key")
     update_user_id(:drafts, conditions: "x.draft_key = y.draft_key")
 
+    update_user_id(:dismissed_topic_users, conditions: "x.topic_id = y.topic_id")
+
     EmailLog.where(user_id: @source_user.id).update_all(user_id: @target_user.id)
 
     GroupHistory.where(acting_user_id: @source_user.id).update_all(acting_user_id: @target_user.id)

db/post_migrate/20210208022738_move_new_since_to_new_table.rb

@@ -0,0 +1,40 @@
+# 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

spec/jobs/clean_dismissed_topic_users_spec.rb

@@ -7,10 +7,6 @@ describe Jobs::CleanDismissedTopicUsers do
   fab!(:topic) { Fabricate(:topic, created_at: 5.hours.ago) }
   fab!(:dismissed_topic_user) { Fabricate(:dismissed_topic_user, user: user, topic: topic) }
 
-  before do
-    user.user_stat.update!(new_since: 1.days.ago)
-  end
-
   context '#delete_overdue_dismissals!' do
     it 'does not delete when new_topic_duration_minutes is set to always' do
       user.user_option.update(new_topic_duration_minutes: User::NewTopicDuration::ALWAYS)
@@ -45,8 +41,6 @@ describe Jobs::CleanDismissedTopicUsers do
     before do
       user.user_option.update(new_topic_duration_minutes: User::NewTopicDuration::ALWAYS)
       user2.user_option.update(new_topic_duration_minutes: User::NewTopicDuration::ALWAYS)
-      user.user_stat.update!(new_since: 1.days.ago)
-      user2.user_stat.update!(new_since: 1.days.ago)
     end
 
     it 'deletes over the limit dismissals' do

spec/jobs/export_user_archive_spec.rb

@@ -317,7 +317,6 @@ describe Jobs::ExportUserArchive do
           .where(category_id: category_id)
           .first_or_initialize
           .update!(last_seen_at: reset_at)
-        #TopicTrackingState.publish_dismiss_new(user.id, category_id)
       end
 
       # Set Watching First Post on announcements, Tracking on subcategory, Muted on deleted, nothing on subsubcategory

spec/requests/topics_controller_spec.rb

@@ -2832,8 +2832,9 @@ RSpec.describe TopicsController do
 
       old_date = 2.years.ago
       user.user_stat.update_column(:new_since, old_date)
+      user.update_column(:created_at, old_date)
 
-      TopicTrackingState.expects(:publish_dismiss_new).with(user.id)
+      TopicTrackingState.expects(:publish_dismiss_new).with(user.id, topic_ids: [topic.id])
 
       put "/topics/reset-new.json"
       expect(response.status).to eq(200)
@@ -2866,7 +2867,7 @@ RSpec.describe TopicsController do
         tracked_topic.update!(category_id: tracked_category.id)
 
         create_post # This is a new post, but is not tracked so a record will not be created for it
-        expect { put "/topics/reset-new.json?tracked=true" }.to change { TopicUser.where(user_id: user.id, last_read_post_number: 0).count }.by(1)
+        expect { put "/topics/reset-new.json?tracked=true" }.to change { DismissedTopicUser.where(user_id: user.id).count }.by(1)
       end
     end
 
@@ -2879,7 +2880,7 @@ RSpec.describe TopicsController do
       it 'dismisses topics for main category' do
         sign_in(user)
 
-        TopicTrackingState.expects(:publish_dismiss_new).with(user.id, category_id: category.id.to_s, tag_id: nil)
+        TopicTrackingState.expects(:publish_dismiss_new).with(user.id, topic_ids: [category_topic.id])
 
         put "/topics/reset-new.json?category_id=#{category.id}"
 
@@ -2888,6 +2889,9 @@ RSpec.describe TopicsController do
 
       it 'dismisses topics for main category and subcategories' do
         sign_in(user)
+
+        TopicTrackingState.expects(:publish_dismiss_new).with(user.id, topic_ids: [subcategory_topic.id, category_topic.id])
+
         put "/topics/reset-new.json?category_id=#{category.id}&include_subcategories=true"
 
         expect(DismissedTopicUser.where(user_id: user.id).pluck(:topic_id).sort).to eq([category_topic.id, subcategory_topic.id].sort)
@@ -2902,7 +2906,7 @@ RSpec.describe TopicsController do
 
       it 'dismisses topics for tag' do
         sign_in(user)
-        TopicTrackingState.expects(:publish_dismiss_new).with(user.id, tag_id: tag.name)
+        TopicTrackingState.expects(:publish_dismiss_new).with(user.id, topic_ids: [tag_topic.id])
         put "/topics/reset-new.json?tag_id=#{tag.name}"
         expect(DismissedTopicUser.where(user_id: user.id).pluck(:topic_id)).to eq([tag_topic.id])
       end
@@ -2918,7 +2922,7 @@ RSpec.describe TopicsController do
 
       it 'dismisses topics for tag' do
         sign_in(user)
-        TopicTrackingState.expects(:publish_dismiss_new).with(user.id, tag_id: tag.name, category_id: category.id.to_s)
+        TopicTrackingState.expects(:publish_dismiss_new).with(user.id, topic_ids: [tag_and_category_topic.id])
         put "/topics/reset-new.json?tag_id=#{tag.name}&category_id=#{category.id}"
         expect(DismissedTopicUser.where(user_id: user.id).pluck(:topic_id)).to eq([tag_and_category_topic.id])
       end

spec/services/dismiss_topics_spec.rb

@@ -3,20 +3,20 @@
 require 'rails_helper'
 
 describe DismissTopics do
-  fab!(:user) { Fabricate(:user) }
+  fab!(:user) { Fabricate(:user, created_at: 1.days.ago) }
   fab!(:category) { Fabricate(:category) }
   fab!(:topic1) { Fabricate(:topic, category: category, created_at: 60.minutes.ago) }
   fab!(:topic2) { Fabricate(:topic, category: category, created_at: 120.minutes.ago) }
 
-  before do
-    user.user_stat.update!(new_since: 1.days.ago)
-  end
-
   describe '#perform!' do
     it 'dismisses two topics' do
       expect { described_class.new(user, Topic.all).perform! }.to change { DismissedTopicUser.count }.by(2)
     end
 
+    it 'returns dismissed topic ids' do
+      expect(described_class.new(user, Topic.all).perform!.sort).to eq([topic1.id, topic2.id])
+    end
+
     it 'respects max_new_topics limit' do
       SiteSetting.max_new_topics = 1
       expect { described_class.new(user, Topic.all).perform! }.to change { DismissedTopicUser.count }.by(1)