diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index fc460e91eb4..d46ed219a37 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -897,12 +897,8 @@ class TopicsController < ApplicationController 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.where(category_id: category_ids)).perform! category_ids.each do |category_id| - current_user - .category_users - .where(category_id: category_id) - .first_or_initialize - .update!(last_seen_at: Time.zone.now) TopicTrackingState.publish_dismiss_new(current_user.id, category_id) end else diff --git a/app/jobs/scheduled/clean_dismissed_topic_users.rb b/app/jobs/scheduled/clean_dismissed_topic_users.rb new file mode 100644 index 00000000000..c3190a58cd8 --- /dev/null +++ b/app/jobs/scheduled/clean_dismissed_topic_users.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +module Jobs + class CleanDismissedTopicUsers < ::Jobs::Scheduled + every 1.day + + def execute(args) + delete_overdue_dismissals! + delete_over_the_limit_dismissals! + end + + private + + def delete_overdue_dismissals! + sql = <<~SQL + DELETE FROM dismissed_topic_users dtu1 + USING dismissed_topic_users dtu2 + JOIN topics ON topics.id = dtu2.topic_id + JOIN users ON users.id = dtu2.user_id + JOIN categories ON categories.id = topics.category_id + LEFT JOIN user_stats ON user_stats.user_id = users.id + LEFT JOIN user_options ON user_options.user_id = users.id + WHERE 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, user_stats.new_since, :min_date) + AND dtu1.id = dtu2.id + SQL + sql = DB.sql_fragment(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) + DB.exec(sql) + end + + def delete_over_the_limit_dismissals! + user_ids = DismissedTopicUser.distinct(:user_id).pluck(:user_id) + sql = <<~SQL + DELETE FROM dismissed_topic_users + WHERE dismissed_topic_users.id NOT IN ( + SELECT valid_dtu.id FROM users + LEFT JOIN dismissed_topic_users valid_dtu ON valid_dtu.user_id = users.id + AND valid_dtu.topic_id IN ( + SELECT topic_id FROM dismissed_topic_users dtu2 + JOIN topics ON topics.id = dtu2.topic_id + WHERE dtu2.user_id = users.id + ORDER BY topics.created_at DESC + LIMIT :max_new_topics + ) + WHERE users.id IN(:user_ids) + ) + SQL + sql = DB.sql_fragment(sql, max_new_topics: SiteSetting.max_new_topics, user_ids: user_ids) + DB.exec(sql) + end + end +end diff --git a/app/models/dismissed_topic_user.rb b/app/models/dismissed_topic_user.rb new file mode 100644 index 00000000000..d6e368c61f2 --- /dev/null +++ b/app/models/dismissed_topic_user.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +class DismissedTopicUser < ActiveRecord::Base + belongs_to :user + belongs_to :topic + + def self.lookup_for(user, topics) + return [] if user.blank? || topics.blank? + + topic_ids = topics.map(&:id) + DismissedTopicUser.where(topic_id: topic_ids, user_id: user.id).pluck(:topic_id) + end +end + +# == Schema Information +# +# Table name: dismissed_topic_users +# +# id :bigint not null, primary key +# user_id :integer +# topic_id :integer +# created_at :datetime +# +# Indexes +# +# index_dismissed_topic_users_on_user_id_and_topic_id (user_id,topic_id) UNIQUE +# diff --git a/app/models/topic.rb b/app/models/topic.rb index bed3e1b1003..6b04fdd4546 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -231,6 +231,7 @@ class Topic < ActiveRecord::Base belongs_to :featured_user4, class_name: 'User', foreign_key: :featured_user4_id has_many :topic_users + has_many :dismissed_topic_users has_many :topic_links has_many :topic_invites has_many :invites, through: :topic_invites, source: :invite @@ -250,6 +251,7 @@ class Topic < ActiveRecord::Base # When we want to temporarily attach some data to a forum topic (usually before serialization) attr_accessor :user_data attr_accessor :category_user_data + attr_accessor :dismissed attr_accessor :posters # TODO: can replace with posters_summary once we remove old list code attr_accessor :participants diff --git a/app/models/topic_list.rb b/app/models/topic_list.rb index 6d24e8ba5b9..c4ecba7c87f 100644 --- a/app/models/topic_list.rb +++ b/app/models/topic_list.rb @@ -84,6 +84,7 @@ class TopicList < DraftableList # Attach some data for serialization to each topic @topic_lookup = TopicUser.lookup_for(@current_user, @topics) if @current_user + @dismissed_topic_users_lookup = DismissedTopicUser.lookup_for(@current_user, @topics) if @current_user post_action_type = if @current_user @@ -119,6 +120,8 @@ class TopicList < DraftableList ft.category_user_data = @category_user_lookup[ft.category_id] end + ft.dismissed = @current_user && @dismissed_topic_users_lookup.include?(ft.id) + if ft.user_data && post_action_lookup && actions = post_action_lookup[ft.id] ft.user_data.post_action_data = { post_action_type => actions } end diff --git a/app/models/topic_tracking_state.rb b/app/models/topic_tracking_state.rb index 69b983175bc..ad7c5a7b4d4 100644 --- a/app/models/topic_tracking_state.rb +++ b/app/models/topic_tracking_state.rb @@ -330,7 +330,7 @@ class TopicTrackingState else TopicQuery.new_filter(Topic, "xxx").where_clause.ast.to_sql.gsub!("'xxx'", treat_as_new_topic_clause) + " AND topics.created_at > :min_new_topic_date" + - " AND (category_users.last_seen_at IS NULL OR topics.created_at > category_users.last_seen_at)" + " AND dismissed_topic_users.id IS NULL" end select = (opts[:select]) || " @@ -396,6 +396,7 @@ class TopicTrackingState JOIN categories c ON c.id = topics.category_id LEFT JOIN topic_users tu ON tu.topic_id = topics.id AND tu.user_id = u.id LEFT JOIN category_users ON category_users.category_id = topics.category_id AND category_users.user_id = #{opts[:user].id} + LEFT JOIN dismissed_topic_users ON dismissed_topic_users.topic_id = topics.id AND dismissed_topic_users.user_id = #{opts[:user].id} WHERE u.id = :user_id AND #{filter_old_unread} topics.archetype <> 'private_message' AND diff --git a/app/serializers/listable_topic_serializer.rb b/app/serializers/listable_topic_serializer.rb index 184a2ca4744..884884bf7d1 100644 --- a/app/serializers/listable_topic_serializer.rb +++ b/app/serializers/listable_topic_serializer.rb @@ -75,7 +75,7 @@ class ListableTopicSerializer < BasicTopicSerializer def seen return true if !scope || !scope.user return true if object.user_data && !object.user_data.last_read_post_number.nil? - return true if object.category_user_data&.last_seen_at && object.created_at < object.category_user_data.last_seen_at + return true if object.dismissed return true if object.created_at < scope.user.user_option.treat_as_new_topic_start_date false end diff --git a/app/services/dismiss_topics.rb b/app/services/dismiss_topics.rb new file mode 100644 index 00000000000..b8ce5bc86d5 --- /dev/null +++ b/app/services/dismiss_topics.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +class DismissTopics + def initialize(user, topics_scope) + @user = user + @topics_scope = topics_scope + end + + def perform! + DismissedTopicUser.insert_all(rows) if rows.present? + end + + private + + def rows + @rows ||= @topics_scope.where("created_at >= ?", since_date).order(created_at: :desc).limit(SiteSetting.max_new_topics).map do |topic| + { + topic_id: topic.id, + user_id: @user.id, + created_at: Time.zone.now + } + end + end + + def since_date + new_topic_duration_minutes = @user.user_option&.new_topic_duration_minutes || SiteSetting.default_other_new_topic_duration_minutes + setting_date = + case new_topic_duration_minutes + when User::NewTopicDuration::LAST_VISIT + @user.previous_visit_at || @user.created_at + when User::NewTopicDuration::ALWAYS + @user.created_at + 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 + end +end diff --git a/db/migrate/20210131221311_create_dismissed_topic_users_table.rb b/db/migrate/20210131221311_create_dismissed_topic_users_table.rb new file mode 100644 index 00000000000..699e05deedb --- /dev/null +++ b/db/migrate/20210131221311_create_dismissed_topic_users_table.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +class CreateDismissedTopicUsersTable < ActiveRecord::Migration[6.0] + def change + create_table :dismissed_topic_users do |t| + t.integer :user_id + t.integer :topic_id + t.datetime :created_at + end + add_index :dismissed_topic_users, %i(user_id topic_id), unique: true + end +end diff --git a/db/migrate/20210201034048_move_category_last_seen_at_to_new_table.rb b/db/migrate/20210201034048_move_category_last_seen_at_to_new_table.rb new file mode 100644 index 00000000000..49414d01960 --- /dev/null +++ b/db/migrate/20210201034048_move_category_last_seen_at_to_new_table.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +class MoveCategoryLastSeenAtToNewTable < ActiveRecord::Migration[6.0] + def up + sql = <<~SQL + INSERT INTO dismissed_topic_users (user_id, topic_id, created_at) + SELECT users.id, topics.id, category_users.last_seen_at + FROM category_users + JOIN users ON users.id = category_users.user_id + JOIN categories ON categories.id = category_users.category_id + JOIN user_stats ON user_stats.user_id = users.id + JOIN user_options ON user_options.user_id = users.id + JOIN topics ON topics.category_id = category_users.category_id + WHERE category_users.last_seen_at IS NOT NULL + 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, user_stats.new_since, :min_date) + AND topics.created_at <= category_users.last_seen_at + ORDER BY topics.created_at DESC + LIMIT :max_new_topics + SQL + sql = DB.sql_fragment(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, + max_new_topics: SiteSetting.max_new_topics) + DB.exec(sql) + end + + def down + raise IrriversibleMigration + end +end diff --git a/lib/topic_query.rb b/lib/topic_query.rb index a12229e0453..d769c1d1d1f 100644 --- a/lib/topic_query.rb +++ b/lib/topic_query.rb @@ -551,7 +551,7 @@ class TopicQuery result = remove_muted_topics(result, @user) result = remove_muted_categories(result, @user, exclude: options[:category]) result = remove_muted_tags(result, @user, options) - result = remove_already_seen_for_category(result, @user) + result = remove_dismissed(result, @user) self.class.results_filter_callbacks.each do |filter_callback| result = filter_callback.call(:new, result, @user, options) @@ -900,6 +900,7 @@ class TopicQuery list = list .references("cu") .joins("LEFT JOIN category_users ON category_users.category_id = topics.category_id AND category_users.user_id = #{user.id}") + .joins("LEFT JOIN dismissed_topic_users ON dismissed_topic_users.topic_id = topics.id AND dismissed_topic_users.user_id = #{user.id}") .where("topics.category_id = :category_id OR COALESCE(category_users.notification_level, :default) <> :muted OR tu.notification_level > :regular", @@ -968,10 +969,9 @@ class TopicQuery end end - def remove_already_seen_for_category(list, user) + def remove_dismissed(list, user) if user - list = list - .where("category_users.last_seen_at IS NULL OR topics.created_at > category_users.last_seen_at") + list = list.where("dismissed_topic_users.id IS NULL") end list diff --git a/spec/components/topic_query_spec.rb b/spec/components/topic_query_spec.rb index 67ba06024f0..ec6a2f26917 100644 --- a/spec/components/topic_query_spec.rb +++ b/spec/components/topic_query_spec.rb @@ -393,14 +393,14 @@ describe TopicQuery do end end - context 'already seen categories' do + context 'already seen topics' do it 'is removed from new and visible on latest lists' do category = Fabricate(:category_with_definition) topic = Fabricate(:topic, category: category) - CategoryUser.create!(user_id: user.id, - category_id: category.id, - last_seen_at: topic.created_at - ) + DismissedTopicUser.create!(user_id: user.id, + topic_id: topic.id, + created_at: Time.zone.now + ) expect(topic_query.list_new.topics.map(&:id)).not_to include(topic.id) expect(topic_query.list_latest.topics.map(&:id)).to include(topic.id) end diff --git a/spec/fabricators/dimissed_topic_user.rb b/spec/fabricators/dimissed_topic_user.rb new file mode 100644 index 00000000000..c5801463f1c --- /dev/null +++ b/spec/fabricators/dimissed_topic_user.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +Fabricator(:dismissed_topic_user) do + user + topic + created_at { Time.zone.now } +end diff --git a/spec/jobs/clean_dismissed_topic_users_spec.rb b/spec/jobs/clean_dismissed_topic_users_spec.rb new file mode 100644 index 00000000000..dc59e32acc9 --- /dev/null +++ b/spec/jobs/clean_dismissed_topic_users_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Jobs::CleanDismissedTopicUsers do + fab!(:user) { Fabricate(:user, created_at: 1.days.ago, previous_visit_at: 1.days.ago) } + 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) + expect { described_class.new.execute({}) }.not_to change { DismissedTopicUser.count } + end + + it 'deletes when new_topic_duration_minutes is set to since last visit' do + user.user_option.update(new_topic_duration_minutes: User::NewTopicDuration::LAST_VISIT) + expect { described_class.new.execute({}) }.not_to change { DismissedTopicUser.count } + + user.update!(previous_visit_at: 1.hours.ago) + expect { described_class.new.execute({}) }.to change { DismissedTopicUser.count }.by(-1) + end + + it 'deletes when new_topic_duration_minutes is set to created in the last day' do + user.user_option.update(new_topic_duration_minutes: 1440) + expect { described_class.new.execute({}) }.not_to change { DismissedTopicUser.count } + + topic.update!(created_at: 2.days.ago) + expect { described_class.new.execute({}) }.to change { DismissedTopicUser.count }.by(-1) + end + end + + context '#delete_over_the_limit_dismissals!' do + fab!(:user2) { Fabricate(:user, created_at: 1.days.ago, previous_visit_at: 1.days.ago) } + fab!(:topic2) { Fabricate(:topic, created_at: 6.hours.ago) } + fab!(:topic3) { Fabricate(:topic, created_at: 2.hours.ago) } + fab!(:dismissed_topic_user2) { Fabricate(:dismissed_topic_user, user: user, topic: topic2) } + fab!(:dismissed_topic_user3) { Fabricate(:dismissed_topic_user, user: user, topic: topic3) } + fab!(:dismissed_topic_user4) { Fabricate(:dismissed_topic_user, user: user2, topic: topic) } + + 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 + described_class.new.execute({}) + expect(dismissed_topic_user.reload).to be_present + expect(dismissed_topic_user2.reload).to be_present + expect(dismissed_topic_user3.reload).to be_present + expect(dismissed_topic_user4.reload).to be_present + + SiteSetting.max_new_topics = 2 + described_class.new.execute({}) + expect(dismissed_topic_user.reload).to be_present + expect { dismissed_topic_user2.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect(dismissed_topic_user3.reload).to be_present + expect(dismissed_topic_user4.reload).to be_present + end + end +end diff --git a/spec/models/topic_tracking_state_spec.rb b/spec/models/topic_tracking_state_spec.rb index e84f8e85ffb..b0c119419af 100644 --- a/spec/models/topic_tracking_state_spec.rb +++ b/spec/models/topic_tracking_state_spec.rb @@ -563,7 +563,7 @@ describe TopicTrackingState do end end - it "correctly handles seen categories" do + it "correctly handles dismissed topics" do freeze_time 1.minute.ago user = Fabricate(:user) post @@ -571,6 +571,7 @@ describe TopicTrackingState do report = TopicTrackingState.report(user) expect(report.length).to eq(1) + DismissedTopicUser.create!(user_id: user.id, topic_id: post.topic_id, created_at: Time.zone.now) CategoryUser.create!(user_id: user.id, notification_level: CategoryUser.notification_levels[:regular], category_id: post.topic.category_id, @@ -579,12 +580,6 @@ describe TopicTrackingState do report = TopicTrackingState.report(user) expect(report.length).to eq(0) - - unfreeze_time - post.topic.touch(:created_at) - - report = TopicTrackingState.report(user) - expect(report.length).to eq(1) end it "correctly handles capping" do diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 89a96aae8eb..33f942adb5c 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -2867,28 +2867,24 @@ RSpec.describe TopicsController do context 'category' do fab!(:category) { Fabricate(:category) } fab!(:subcategory) { Fabricate(:category, parent_category_id: category.id) } + fab!(:category_topic) { Fabricate(:topic, category: category) } + fab!(:subcategory_topic) { Fabricate(:topic, category: subcategory) } - it 'updates last_seen_at for main category' do + it 'dismisses topics for main category' do sign_in(user) - category_user = CategoryUser.create!(category_id: category.id, user_id: user.id) - subcategory_user = CategoryUser.create!(category_id: subcategory.id, user_id: user.id) TopicTrackingState.expects(:publish_dismiss_new).with(user.id, category.id.to_s) put "/topics/reset-new.json?category_id=#{category.id}" - expect(category_user.reload.last_seen_at).not_to be_nil - expect(subcategory_user.reload.last_seen_at).to be_nil + expect(DismissedTopicUser.where(user_id: user.id).pluck(:topic_id)).to eq([category_topic.id]) end - it 'updates last_seen_at for main category and subcategories' do + it 'dismisses topics for main category and subcategories' do sign_in(user) - category_user = CategoryUser.create!(category_id: category.id, user_id: user.id) - subcategory_user = CategoryUser.create!(category_id: subcategory.id, user_id: user.id) put "/topics/reset-new.json?category_id=#{category.id}&include_subcategories=true" - expect(category_user.reload.last_seen_at).not_to be_nil - expect(subcategory_user.reload.last_seen_at).not_to be_nil + expect(DismissedTopicUser.where(user_id: user.id).pluck(:topic_id).sort).to eq([category_topic.id, subcategory_topic.id].sort) end end end diff --git a/spec/services/dismiss_topics_spec.rb b/spec/services/dismiss_topics_spec.rb new file mode 100644 index 00000000000..ad3741163f4 --- /dev/null +++ b/spec/services/dismiss_topics_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe DismissTopics do + fab!(:user) { Fabricate(:user) } + 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 '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) + + dismissed_topic_user = DismissedTopicUser.last + + expect(dismissed_topic_user.user_id).to eq(user.id) + expect(dismissed_topic_user.topic_id).to eq(topic1.id) + expect(dismissed_topic_user.created_at).not_to be_nil + end + + it 'respects new_topic_duration_minutes' do + user.user_option.update!(new_topic_duration_minutes: 70) + + expect { described_class.new(user, Topic.all).perform! }.to change { DismissedTopicUser.count }.by(1) + + dismissed_topic_user = DismissedTopicUser.last + + expect(dismissed_topic_user.user_id).to eq(user.id) + expect(dismissed_topic_user.topic_id).to eq(topic1.id) + expect(dismissed_topic_user.created_at).not_to be_nil + end + end +end