Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

FEATURE: New way to dismiss new topics (#11927) 

This is a try to simplify logic around dismiss new topics to have one solution to work in all places - dismiss all-new, dismiss new in a specific category or even in a specific tag.
This commit is contained in:
Krzysztof Kotlarek 2021-02-04 11:27:34 +11:00 committed by GitHub
parent 151193bb11
commit f39e7fe81d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
17 changed files with 317 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
app/controllers/topics_controller.rb
View File

@ -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

60
app/jobs/scheduled/clean_dismissed_topic_users.rb Normal file
View File

@ -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

27
app/models/dismissed_topic_user.rb Normal file
View File

@ -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
#

2
app/models/topic.rb
View File

@ -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

3
app/models/topic_list.rb
View File

@ -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

3
app/models/topic_tracking_state.rb
View File

@ -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

2
app/serializers/listable_topic_serializer.rb
View File

@ -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

38
app/services/dismiss_topics.rb Normal file
View File

@ -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

12
db/migrate/20210131221311_create_dismissed_topic_users_table.rb Normal file
View File

@ -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

37
db/migrate/20210201034048_move_category_last_seen_at_to_new_table.rb Normal file
View File

@ -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

8
lib/topic_query.rb
View File

@ -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

10
spec/components/topic_query_spec.rb
View File

@ -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

7
spec/fabricators/dimissed_topic_user.rb Normal file
View File

@ -0,0 +1,7 @@
# frozen_string_literal: true
Fabricator(:dismissed_topic_user) do
user
topic
created_at { Time.zone.now }
end

67
spec/jobs/clean_dismissed_topic_users_spec.rb Normal file
View File

@ -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

9
spec/models/topic_tracking_state_spec.rb
View File

@ -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

16
spec/requests/topics_controller_spec.rb
View File

@ -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

43
spec/services/dismiss_topics_spec.rb Normal file
View File

@ -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