diff --git a/app/assets/javascripts/discourse/app/models/topic-tracking-state.js b/app/assets/javascripts/discourse/app/models/topic-tracking-state.js index 5909ed63ef6..b00e8427ba6 100644 --- a/app/assets/javascripts/discourse/app/models/topic-tracking-state.js +++ b/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, { - is_seen: true, - }); - } + 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(); diff --git a/app/assets/javascripts/discourse/tests/unit/models/topic-tracking-state-test.js b/app/assets/javascripts/discourse/tests/unit/models/topic-tracking-state-test.js index 8e5a0e70e9d..56542dfb959 100644 --- a/app/assets/javascripts/discourse/tests/unit/models/topic-tracking-state-test.js +++ b/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); }); diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 54f29248b65..29ed6cf7563 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -910,36 +910,30 @@ class TopicsController < ApplicationController end def reset_new - 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) + 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 - 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 - 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]) - 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? + 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? + Topic.joins(:tags).where(tags: { name: params[:tag_id] }) else - current_user.user_stat.update_column(:new_since, Time.zone.now) - TopicTrackingState.publish_dismiss_new(current_user.id) + if params[:tracked].to_s == "true" + TopicQuery.tracked_filter(TopicQuery.new(current_user).new_results, current_user.id) + else + current_user.user_stat.update_column(:new_since, Time.zone.now) + Topic + end 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 diff --git a/app/jobs/scheduled/clean_dismissed_topic_users.rb b/app/jobs/scheduled/clean_dismissed_topic_users.rb index c3190a58cd8..72109ffbfcc 100644 --- a/app/jobs/scheduled/clean_dismissed_topic_users.rb +++ b/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, diff --git a/app/models/topic_tracking_state.rb b/app/models/topic_tracking_state.rb index 78e4c7073d8..045ad34cece 100644 --- a/app/models/topic_tracking_state.rb +++ b/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, diff --git a/app/models/user_option.rb b/app/models/user_option.rb index 1c0cfbb52f4..8ba26acf9d6 100644 --- a/app/models/user_option.rb +++ b/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 ] diff --git a/app/services/dismiss_topics.rb b/app/services/dismiss_topics.rb index 2b439944631..fc722f878da 100644 --- a/app/services/dismiss_topics.rb +++ b/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 diff --git a/app/services/user_merger.rb b/app/services/user_merger.rb index d6a0a1eefed..c0a6b6b826b 100644 --- a/app/services/user_merger.rb +++ b/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) diff --git a/db/migrate/20210208022738_move_new_since_to_new_table.rb b/db/migrate/20210208022738_move_new_since_to_new_table.rb new file mode 100644 index 00000000000..6b975e464fa --- /dev/null +++ b/db/migrate/20210208022738_move_new_since_to_new_table.rb @@ -0,0 +1,39 @@ +# 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 + 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 dismissed_topic_users.id IS NULL + 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) + ORDER BY topics.created_at DESC + LIMIT :max_new_topics + 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 diff --git a/spec/jobs/clean_dismissed_topic_users_spec.rb b/spec/jobs/clean_dismissed_topic_users_spec.rb index dc59e32acc9..d17041f7b9a 100644 --- a/spec/jobs/clean_dismissed_topic_users_spec.rb +++ b/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 diff --git a/spec/jobs/export_user_archive_spec.rb b/spec/jobs/export_user_archive_spec.rb index efe6d5d473e..996c0c2c6de 100644 --- a/spec/jobs/export_user_archive_spec.rb +++ b/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 diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index ee3c2ddde75..dbeb2dc91b2 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/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 diff --git a/spec/services/dismiss_topics_spec.rb b/spec/services/dismiss_topics_spec.rb index 925a573051c..b61db8a2933 100644 --- a/spec/services/dismiss_topics_spec.rb +++ b/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)