From e3977f84a3f8f1fdc0d67921a479b86c22cd2995 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Mon, 6 Mar 2023 10:13:10 +0800 Subject: [PATCH] FIX: Incorrect topic tracking state count when a new category is created (#20506) What is the problem? We have a hidden site setting `show_category_definitions_in_topic_lists` which is set to false by default. What this means is that category definition topics are not shown in the topic list by default. Only the category definition topic for the category being viewed will be shown. However, we have a bug where we would show that a category has new topics when a new child category along with its category definition topic is created even though the topic list does not list the child category's category definition topic. What is the fix here? This commit fixes the problem by shipping down an additional `is_category_topic` attribute in `TopicTrackingStateItemSerializer` when the `show_category_definitions_in_topic_lists` site setting has been set to false. With the new attribute, we can then exclude counting child categories' category definition topics when counting new and unread counts for a category. --- .../app/models/topic-tracking-state.js | 8 +++ app/models/topic_tracking_state.rb | 8 +++ .../topic_tracking_state_item_serializer.rb | 9 +++ config/site_settings.yml | 1 + .../page_objects/components/topic_list.rb | 13 +++-- spec/system/page_objects/pages/category.rb | 14 +++++ spec/system/viewing_category_spec.rb | 55 +++++++++++++++++++ 7 files changed, 104 insertions(+), 4 deletions(-) create mode 100644 spec/system/viewing_category_spec.rb 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 aff8f8699f8..bd6d49ef8aa 100644 --- a/app/assets/javascripts/discourse/app/models/topic-tracking-state.js +++ b/app/assets/javascripts/discourse/app/models/topic-tracking-state.js @@ -582,6 +582,14 @@ const TopicTrackingState = EmberObject.extend({ return false; } + if ( + categoryId && + topic.is_category_topic && + categoryId !== topic.category_id + ) { + return false; + } + if (tagId && !topic.tags?.includes(tagId)) { return false; } diff --git a/app/models/topic_tracking_state.rb b/app/models/topic_tracking_state.rb index 7c6f85c8933..82e6b0cd3e1 100644 --- a/app/models/topic_tracking_state.rb +++ b/app/models/topic_tracking_state.rb @@ -388,6 +388,13 @@ class TopicTrackingState new_filter_sql end + category_topic_id_column_select = + if SiteSetting.show_category_definitions_in_topic_lists + "" + else + "c.topic_id AS category_topic_id," + end + select_sql = select || " @@ -398,6 +405,7 @@ class TopicTrackingState #{highest_post_number_column_select(whisperer)}, last_read_post_number, c.id as category_id, + #{category_topic_id_column_select} tu.notification_level, us.first_unread_at, GREATEST( diff --git a/app/serializers/topic_tracking_state_item_serializer.rb b/app/serializers/topic_tracking_state_item_serializer.rb index b84473b0cec..31df3ad67a3 100644 --- a/app/serializers/topic_tracking_state_item_serializer.rb +++ b/app/serializers/topic_tracking_state_item_serializer.rb @@ -6,6 +6,7 @@ class TopicTrackingStateItemSerializer < ApplicationSerializer :last_read_post_number, :created_at, :category_id, + :is_category_topic, :notification_level, :created_in_new_period, :treat_as_new_topic_start_date, @@ -19,4 +20,12 @@ class TopicTrackingStateItemSerializer < ApplicationSerializer def include_tags? object.respond_to?(:tags) end + + def is_category_topic + object.topic_id == object.category_topic_id + end + + def include_is_category_topic? + object.respond_to?(:category_topic_id) + end end diff --git a/config/site_settings.yml b/config/site_settings.yml index 8a5082f32fc..1e5f21c43ca 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -2564,6 +2564,7 @@ uncategorized: show_category_definitions_in_topic_lists: default: false hidden: true + client: true create_revision_on_bulk_topic_moves: default: true diff --git a/spec/system/page_objects/components/topic_list.rb b/spec/system/page_objects/components/topic_list.rb index 1c1b86c74ff..844f9a52ea5 100644 --- a/spec/system/page_objects/components/topic_list.rb +++ b/spec/system/page_objects/components/topic_list.rb @@ -3,10 +3,15 @@ module PageObjects module Components class TopicList < PageObjects::Components::Base - TOPIC_LIST_BODY_CLASS = ".topic-list-body" + TOPIC_LIST_BODY_SELECTOR = ".topic-list-body" + TOPIC_LIST_ITEM_SELECTOR = "#{TOPIC_LIST_BODY_SELECTOR} .topic-list-item" def topic_list - TOPIC_LIST_BODY_CLASS + TOPIC_LIST_BODY_SELECTOR + end + + def has_topics?(count:) + page.has_css?(TOPIC_LIST_ITEM_SELECTOR, count: count) end def has_topic?(topic) @@ -18,13 +23,13 @@ module PageObjects end def visit_topic_with_title(title) - find(".topic-list-body a", text: title).click + find("#{TOPIC_LIST_BODY_SELECTOR} a", text: title).click end private def topic_list_item_class(topic) - "#{TOPIC_LIST_BODY_CLASS} .topic-list-item[data-topic-id='#{topic.id}']" + "#{TOPIC_LIST_ITEM_SELECTOR}[data-topic-id='#{topic.id}']" end end end diff --git a/spec/system/page_objects/pages/category.rb b/spec/system/page_objects/pages/category.rb index 2fa00475847..547d9f865cf 100644 --- a/spec/system/page_objects/pages/category.rb +++ b/spec/system/page_objects/pages/category.rb @@ -58,6 +58,20 @@ module PageObjects find(".select-kit-collection .select-kit-row", text: template_name).click find(".select-category-template").click end + + CATEGORY_NAVIGATION_NEW_NAV_ITEM_SELECTOR = ".category-navigation .nav-item_new" + + def has_no_new_topics? + page.has_no_css?(CATEGORY_NAVIGATION_NEW_NAV_ITEM_SELECTOR) + end + + def has_new_topics? + page.has_css?(CATEGORY_NAVIGATION_NEW_NAV_ITEM_SELECTOR) + end + + def click_new + page.find(CATEGORY_NAVIGATION_NEW_NAV_ITEM_SELECTOR).click + end end end end diff --git a/spec/system/viewing_category_spec.rb b/spec/system/viewing_category_spec.rb new file mode 100644 index 00000000000..a1a7628bf69 --- /dev/null +++ b/spec/system/viewing_category_spec.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +RSpec.describe "Viewing a category", type: :system, js: true do + fab!(:category) { Fabricate(:category) } + fab!(:user) { Fabricate(:user) } + let(:category_page) { PageObjects::Pages::Category.new } + let(:topic_list) { PageObjects::Components::TopicList.new } + + describe "when a new child category is created with a new category topic" do + fab!(:child_category) { Fabricate(:category, parent_category: category) } + + fab!(:child_category_topic) do + Fabricate(:topic, category: child_category).tap do |topic| + child_category.update!(topic: topic) + end + end + + it "should show a new count on the parent and child category when 'show_category_definitions_in_topic_lists' is true" do + SiteSetting.show_category_definitions_in_topic_lists = true + + sign_in(user) + + category_page.visit(category) + category_page.click_new + + expect(topic_list).to have_topics(count: 1) + expect(topic_list).to have_topic(child_category_topic) + + category_page.visit(child_category) + category_page.click_new + + expect(topic_list).to have_topics(count: 1) + expect(topic_list).to have_topic(child_category_topic) + end + + it "should only show a new count on the child category when 'show_category_definitions_in_topic_lists' site setting is false" do + SiteSetting.show_category_definitions_in_topic_lists = false + + sign_in(user) + + category_page.visit(category) + + expect(category_page).to have_no_new_topics + + category_page.visit(child_category) + + expect(category_page).to have_new_topics + + category_page.click_new + + expect(topic_list).to have_topics(count: 1) + expect(topic_list).to have_topic(child_category_topic) + end + end +end