From 6e1fe22a9dded8f711fb18b8b14dad0dd921dc90 Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Thu, 14 Nov 2019 11:16:13 +1100 Subject: [PATCH] FEATURE: Dismiss new per category (#8330) Ability to dismiss new topics per category. --- .../controllers/discovery/topics.js.es6 | 4 +- .../javascripts/discourse/models/topic.js.es6 | 5 +- app/controllers/topics_controller.rb | 18 +++++- app/models/category_user.rb | 10 ++++ app/models/topic.rb | 1 + app/models/topic_list.rb | 2 + app/serializers/listable_topic_serializer.rb | 1 + ...7025041_add_last_seen_to_category_users.rb | 7 +++ ...index_to_last_seen_at_on_category_users.rb | 15 +++++ ...0191107032231_change_notification_level.rb | 11 ++++ lib/topic_query.rb | 57 +++++++++---------- spec/components/topic_query_spec.rb | 13 +++++ spec/requests/topics_controller_spec.rb | 25 ++++++++ 13 files changed, 135 insertions(+), 34 deletions(-) create mode 100644 db/migrate/20191107025041_add_last_seen_to_category_users.rb create mode 100644 db/migrate/20191107025140_add_index_to_last_seen_at_on_category_users.rb create mode 100644 db/post_migrate/20191107032231_change_notification_level.rb diff --git a/app/assets/javascripts/discourse/controllers/discovery/topics.js.es6 b/app/assets/javascripts/discourse/controllers/discovery/topics.js.es6 index 9e4d81ceddc..5501959bda1 100644 --- a/app/assets/javascripts/discourse/controllers/discovery/topics.js.es6 +++ b/app/assets/javascripts/discourse/controllers/discovery/topics.js.es6 @@ -84,7 +84,7 @@ const controllerOpts = { resetNew() { this.topicTrackingState.resetNew(); - Topic.resetNew().then(() => this.send("refresh")); + Topic.resetNew(this.category, !this.noSubcategories).then(() => this.send("refresh")); }, dismissReadPosts() { @@ -106,7 +106,7 @@ const controllerOpts = { @discourseComputed("model.filter", "model.topics.length") showResetNew(filter, topicsLength) { - return filter === "new" && topicsLength > 0; + return this.isFilterPage(filter, "new") && topicsLength > 0; }, @discourseComputed("model.filter", "model.topics.length") diff --git a/app/assets/javascripts/discourse/models/topic.js.es6 b/app/assets/javascripts/discourse/models/topic.js.es6 index 6ad8bec5778..21abea9de66 100644 --- a/app/assets/javascripts/discourse/models/topic.js.es6 +++ b/app/assets/javascripts/discourse/models/topic.js.es6 @@ -765,8 +765,9 @@ Topic.reopenClass({ }); }, - resetNew() { - return ajax("/topics/reset-new", { type: "PUT" }); + resetNew(category, include_subcategories) { + const data = category ? { category_id: category.id, include_subcategories } : {}; + return ajax("/topics/reset-new", { type: "PUT", data }); }, idForSlug(slug) { diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 416e84f19a5..2eda0820cbc 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -847,7 +847,23 @@ class TopicsController < ApplicationController end def reset_new - current_user.user_stat.update_column(:new_since, Time.now) + 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 + + 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) + end + else + current_user.user_stat.update_column(:new_since, Time.now) + end render body: nil end diff --git a/app/models/category_user.rb b/app/models/category_user.rb index 1d9b2bfe223..fea612700be 100644 --- a/app/models/category_user.rb +++ b/app/models/category_user.rb @@ -217,6 +217,16 @@ class CategoryUser < ActiveRecord::Base Hash[*notification_levels.flatten] end + def self.lookup_for(user, category_ids) + return {} if user.blank? || category_ids.blank? + create_lookup(CategoryUser.where(category_id: category_ids, user_id: user.id)) + end + + def self.create_lookup(category_users) + category_users.each_with_object({}) do |category_user, acc| + acc[category_user.category_id] = category_user + end + end end # == Schema Information diff --git a/app/models/topic.rb b/app/models/topic.rb index 1af8598d2d2..8531738b6a2 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -135,6 +135,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 :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 27730561d7e..52b0ee00708 100644 --- a/app/models/topic_list.rb +++ b/app/models/topic_list.rb @@ -83,6 +83,7 @@ class TopicList # Attach some data for serialization to each topic @topic_lookup = TopicUser.lookup_for(@current_user, @topics) if @current_user + @category_user_lookup = CategoryUser.lookup_for(@current_user, @topics.map(&:category_id).uniq) if @current_user post_action_type = if @current_user @@ -114,6 +115,7 @@ class TopicList @topics.each do |ft| ft.user_data = @topic_lookup[ft.id] if @topic_lookup.present? + ft.category_user_data = @category_user_lookup[ft.category_id] if @category_user_lookup.present? if ft.user_data && post_action_lookup && actions = post_action_lookup[ft.id] ft.user_data.post_action_data = { post_action_type => actions } diff --git a/app/serializers/listable_topic_serializer.rb b/app/serializers/listable_topic_serializer.rb index 1336a92c14a..4e5829df02d 100644 --- a/app/serializers/listable_topic_serializer.rb +++ b/app/serializers/listable_topic_serializer.rb @@ -59,6 +59,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.created_at < scope.user.user_option.treat_as_new_topic_start_date false end diff --git a/db/migrate/20191107025041_add_last_seen_to_category_users.rb b/db/migrate/20191107025041_add_last_seen_to_category_users.rb new file mode 100644 index 00000000000..7ed1b506a85 --- /dev/null +++ b/db/migrate/20191107025041_add_last_seen_to_category_users.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddLastSeenToCategoryUsers < ActiveRecord::Migration[6.0] + def change + add_column :category_users, :last_seen_at, :datetime + end +end diff --git a/db/migrate/20191107025140_add_index_to_last_seen_at_on_category_users.rb b/db/migrate/20191107025140_add_index_to_last_seen_at_on_category_users.rb new file mode 100644 index 00000000000..e26839625db --- /dev/null +++ b/db/migrate/20191107025140_add_index_to_last_seen_at_on_category_users.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class AddIndexToLastSeenAtOnCategoryUsers < ActiveRecord::Migration[6.0] + disable_ddl_transaction! + + def up + if !index_exists?(:category_users, [:user_id, :last_seen_at]) + add_index :category_users, [:user_id, :last_seen_at], algorithm: :concurrently + end + end + + def down + remove_index :category_users, [:user_id, :last_seen_at] + end +end diff --git a/db/post_migrate/20191107032231_change_notification_level.rb b/db/post_migrate/20191107032231_change_notification_level.rb new file mode 100644 index 00000000000..6f88d41809c --- /dev/null +++ b/db/post_migrate/20191107032231_change_notification_level.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class ChangeNotificationLevel < ActiveRecord::Migration[6.0] + def up + change_column :category_users, :notification_level, :integer, null: true + end + + def down + change_column :category_users, :notification_level, :integer, null: false + end +end diff --git a/lib/topic_query.rb b/lib/topic_query.rb index dec1268030d..c9b6348dd74 100644 --- a/lib/topic_query.rb +++ b/lib/topic_query.rb @@ -517,6 +517,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) self.class.results_filter_callbacks.each do |filter_callback| result = filter_callback.call(:new, result, @user, options) @@ -871,21 +872,15 @@ class TopicQuery if SiteSetting.mute_all_categories_by_default if user - list = list.references("cu") - .where(" - NOT EXISTS ( - SELECT 1 - FROM categories c - LEFT OUTER JOIN category_users cu - ON c.id = cu.category_id AND cu.user_id = :user_id - WHERE c.id = topics.category_id - AND c.id <> :category_id - AND (COALESCE(cu.notification_level, :muted) = :muted) - AND (COALESCE(tu.notification_level, :regular) <= :regular) - )", user_id: user.id, - muted: CategoryUser.notification_levels[:muted], - regular: TopicUser.notification_levels[:regular], - category_id: category_id || -1) + list = list + .references("cu") + .joins("LEFT JOIN category_users ON category_users.category_id = topics.category_id AND category_users.user_id = #{user.id}") + .where("topics.category_id = :category_id + OR COALESCE(category_users.notification_level, :muted) <> :muted + OR tu.notification_level > :regular", + muted: CategoryUser.notification_levels[:muted], + regular: TopicUser.notification_levels[:regular], + category_id: category_id || -1) else category_ids = [ SiteSetting.default_categories_watching.split("|"), @@ -897,20 +892,15 @@ class TopicQuery list = list.where("topics.category_id IN (?)", category_ids) if category_ids.present? end elsif user - list = list.references("cu") - .where(" - NOT EXISTS ( - SELECT 1 - FROM category_users cu - WHERE cu.user_id = :user_id - AND cu.category_id = topics.category_id - AND cu.notification_level = :muted - AND cu.category_id <> :category_id - AND (tu.notification_level IS NULL OR tu.notification_level < :tracking) - )", user_id: user.id, - muted: CategoryUser.notification_levels[:muted], - tracking: TopicUser.notification_levels[:tracking], - category_id: category_id || -1) + list = list + .references("cu") + .joins("LEFT JOIN category_users ON category_users.category_id = topics.category_id AND category_users.user_id = #{user.id}") + .where("COALESCE(category_users.notification_level, :regular) <> :muted + OR category_users.category_id = :category_id OR tu.notification_level >= :tracking", + muted: CategoryUser.notification_levels[:muted], + regular: CategoryUser.notification_levels[:regular], + tracking: TopicUser.notification_levels[:tracking], + category_id: category_id || -1) end list @@ -949,6 +939,15 @@ class TopicQuery end end + def remove_already_seen_for_category(list, user) + if user + list = list + .where("category_users.last_seen_at IS NULL OR topics.created_at > category_users.last_seen_at") + end + + list + end + def new_messages(params) query = TopicQuery .new_filter(messages_for_groups_or_user(params[:my_group_ids]), Time.at(SiteSetting.min_new_topics_time).to_datetime) diff --git a/spec/components/topic_query_spec.rb b/spec/components/topic_query_spec.rb index aac94f9b67a..c29204fb8f3 100644 --- a/spec/components/topic_query_spec.rb +++ b/spec/components/topic_query_spec.rb @@ -273,6 +273,19 @@ describe TopicQuery do end end + context 'already seen categories' 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 + ) + 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 + end + context 'muted tags' do it 'is removed from new and latest lists' do SiteSetting.tagging_enabled = true diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 3e7c784452b..5112da7ca38 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -2347,6 +2347,31 @@ RSpec.describe TopicsController do user.reload expect(user.user_stat.new_since.to_date).not_to eq(old_date.to_date) end + + context 'category' do + fab!(:category) { Fabricate(:category) } + fab!(:subcategory) { Fabricate(:category, parent_category_id: category.id) } + + it 'updates last_seen_at 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) + 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 + end + + it 'updates last_seen_at 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 + end + end end describe '#feature_stats' do