From 92e1e43104b568647e0a7e70aa0718854d46c468 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Thu, 14 Jul 2022 13:44:58 +0800 Subject: [PATCH] FIX: Improve reliability of topic tracking state (#17387) The `unread_not_too_old` attribute is a little odd because there should never be a case where the user's first_unread_at column is less than the `Topic#updated_at` column of an unread topic. The `unread_not_too_old` attribute is causing a bug where topic states synced into `TopicTrackingState` do not appear as unread because the attribute does not exsist on a normal `Topic` object and hence never set. --- .../discourse/app/models/topic-tracking-state.js | 3 +-- .../acceptance/sidebar-categories-section-test.js | 4 ---- .../tests/acceptance/sidebar-tags-section-test.js | 4 ---- .../tests/acceptance/sidebar-topics-section-test.js | 10 ---------- .../acceptance/topic-discovery-tracked-test.js | 13 ------------- .../components/widgets/hamburger-menu-test.js | 1 - .../tests/unit/models/topic-tracking-state-test.js | 7 ------- app/models/topic_tracking_state.rb | 1 - app/serializers/topic_tracking_state_serializer.rb | 6 ------ .../topic_tracking_state_serializer_spec.rb | 1 - 10 files changed, 1 insertion(+), 49 deletions(-) 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 0a2a9dac3c8..64188c3e129 100644 --- a/app/assets/javascripts/discourse/app/models/topic-tracking-state.js +++ b/app/assets/javascripts/discourse/app/models/topic-tracking-state.js @@ -23,8 +23,7 @@ function isUnread(topic) { return ( topic.last_read_post_number !== null && topic.last_read_post_number < topic.highest_post_number && - topic.notification_level >= NotificationLevels.TRACKING && - topic.unread_not_too_old + topic.notification_level >= NotificationLevels.TRACKING ); } diff --git a/app/assets/javascripts/discourse/tests/acceptance/sidebar-categories-section-test.js b/app/assets/javascripts/discourse/tests/acceptance/sidebar-categories-section-test.js index 0552c9d149c..90465993047 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/sidebar-categories-section-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/sidebar-categories-section-test.js @@ -277,7 +277,6 @@ acceptance("Sidebar - Categories Section", function (needs) { category_id: category1.id, notification_level: null, created_in_new_period: true, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", }, { @@ -288,7 +287,6 @@ acceptance("Sidebar - Categories Section", function (needs) { category_id: category1.id, notification_level: 2, created_in_new_period: false, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", }, { @@ -299,7 +297,6 @@ acceptance("Sidebar - Categories Section", function (needs) { category_id: category2.id, notification_level: 2, created_in_new_period: false, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", }, { @@ -310,7 +307,6 @@ acceptance("Sidebar - Categories Section", function (needs) { category_id: category2.id, notification_level: 2, created_in_new_period: false, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", }, ]); diff --git a/app/assets/javascripts/discourse/tests/acceptance/sidebar-tags-section-test.js b/app/assets/javascripts/discourse/tests/acceptance/sidebar-tags-section-test.js index 8743ea4443c..d3f6bd595e1 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/sidebar-tags-section-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/sidebar-tags-section-test.js @@ -219,7 +219,6 @@ acceptance("Sidebar - Tags section", function (needs) { category_id: 1, notification_level: null, created_in_new_period: true, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", tags: ["tag1"], }, @@ -231,7 +230,6 @@ acceptance("Sidebar - Tags section", function (needs) { category_id: 2, notification_level: 2, created_in_new_period: false, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", tags: ["tag1"], }, @@ -243,7 +241,6 @@ acceptance("Sidebar - Tags section", function (needs) { category_id: 3, notification_level: 2, created_in_new_period: false, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", tags: ["tag2"], }, @@ -255,7 +252,6 @@ acceptance("Sidebar - Tags section", function (needs) { category_id: 4, notification_level: 2, created_in_new_period: false, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", tags: ["tag4"], }, diff --git a/app/assets/javascripts/discourse/tests/acceptance/sidebar-topics-section-test.js b/app/assets/javascripts/discourse/tests/acceptance/sidebar-topics-section-test.js index f393a7d5eee..c390a709dc2 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/sidebar-topics-section-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/sidebar-topics-section-test.js @@ -287,7 +287,6 @@ acceptance("Sidebar - Topics Section", function (needs) { category_id: 1, notification_level: null, created_in_new_period: true, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", }, { @@ -298,7 +297,6 @@ acceptance("Sidebar - Topics Section", function (needs) { category_id: 2, notification_level: 2, created_in_new_period: false, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", }, { @@ -309,7 +307,6 @@ acceptance("Sidebar - Topics Section", function (needs) { category_id: 3, notification_level: 2, created_in_new_period: false, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", }, { @@ -320,7 +317,6 @@ acceptance("Sidebar - Topics Section", function (needs) { category_id: 4, notification_level: 2, created_in_new_period: false, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", }, ]); @@ -484,7 +480,6 @@ acceptance("Sidebar - Topics Section", function (needs) { category_id: category.id, notification_level: NotificationLevels.TRACKING, created_in_new_period: true, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", }, { @@ -495,7 +490,6 @@ acceptance("Sidebar - Topics Section", function (needs) { category_id: category.subcategories[0].id, notification_level: NotificationLevels.TRACKING, created_in_new_period: false, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", }, { @@ -506,7 +500,6 @@ acceptance("Sidebar - Topics Section", function (needs) { category_id: category.subcategories[0].subcategories[0].id, notification_level: NotificationLevels.TRACKING, created_in_new_period: false, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", }, { @@ -517,7 +510,6 @@ acceptance("Sidebar - Topics Section", function (needs) { category_id: 3, notification_level: NotificationLevels.TRACKING, created_in_new_period: false, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", }, { @@ -528,7 +520,6 @@ acceptance("Sidebar - Topics Section", function (needs) { category_id: 3, notification_level: null, created_in_new_period: true, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", }, { @@ -539,7 +530,6 @@ acceptance("Sidebar - Topics Section", function (needs) { category_id: 1234, notification_level: NotificationLevels.TRACKING, created_in_new_period: false, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", tags: ["tag3"], }, diff --git a/app/assets/javascripts/discourse/tests/acceptance/topic-discovery-tracked-test.js b/app/assets/javascripts/discourse/tests/acceptance/topic-discovery-tracked-test.js index 357a5dd7c37..97374995e01 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/topic-discovery-tracked-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/topic-discovery-tracked-test.js @@ -49,7 +49,6 @@ acceptance("Topic Discovery Tracked", function (needs) { category_id: 1, notification_level: null, created_in_new_period: true, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", }, { @@ -60,7 +59,6 @@ acceptance("Topic Discovery Tracked", function (needs) { category_id: 2, notification_level: NotificationLevels.TRACKING, created_in_new_period: false, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", }, ]); @@ -116,7 +114,6 @@ acceptance("Topic Discovery Tracked", function (needs) { category_id: category.id, notification_level: NotificationLevels.TRACKING, created_in_new_period: true, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", }, { @@ -127,7 +124,6 @@ acceptance("Topic Discovery Tracked", function (needs) { category_id: category.subcategories[0].id, notification_level: NotificationLevels.TRACKING, created_in_new_period: false, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", }, { @@ -138,7 +134,6 @@ acceptance("Topic Discovery Tracked", function (needs) { category_id: category.subcategories[0].subcategories[0].id, notification_level: NotificationLevels.TRACKING, created_in_new_period: false, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", }, { @@ -149,7 +144,6 @@ acceptance("Topic Discovery Tracked", function (needs) { category_id: 3, notification_level: NotificationLevels.TRACKING, created_in_new_period: false, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", }, { @@ -160,7 +154,6 @@ acceptance("Topic Discovery Tracked", function (needs) { category_id: 3, notification_level: null, created_in_new_period: true, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", }, { @@ -171,7 +164,6 @@ acceptance("Topic Discovery Tracked", function (needs) { category_id: 1234, notification_level: NotificationLevels.TRACKING, created_in_new_period: false, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", tags: ["tag3"], }, @@ -250,7 +242,6 @@ acceptance("Topic Discovery Tracked", function (needs) { category_id: 1, notification_level: null, created_in_new_period: true, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", tags: ["someothertag"], }, @@ -299,7 +290,6 @@ acceptance("Topic Discovery Tracked", function (needs) { category_id: category.id, notification_level: null, created_in_new_period: true, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", }, { @@ -310,7 +300,6 @@ acceptance("Topic Discovery Tracked", function (needs) { category_id: category.id, notification_level: NotificationLevels.TRACKING, created_in_new_period: false, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", }, { @@ -321,7 +310,6 @@ acceptance("Topic Discovery Tracked", function (needs) { category_id: 3, notification_level: NotificationLevels.TRACKING, created_in_new_period: false, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", }, { @@ -332,7 +320,6 @@ acceptance("Topic Discovery Tracked", function (needs) { category_id: 1234, notification_level: NotificationLevels.TRACKING, created_in_new_period: false, - unread_not_too_old: true, treat_as_new_topic_start_date: "2022-05-09T03:17:34.286Z", tags: ["tag3"], }, diff --git a/app/assets/javascripts/discourse/tests/integration/components/widgets/hamburger-menu-test.js b/app/assets/javascripts/discourse/tests/integration/components/widgets/hamburger-menu-test.js index 5f3858f23fd..ac19903a9f0 100644 --- a/app/assets/javascripts/discourse/tests/integration/components/widgets/hamburger-menu-test.js +++ b/app/assets/javascripts/discourse/tests/integration/components/widgets/hamburger-menu-test.js @@ -136,7 +136,6 @@ module("Integration | Component | Widget | hamburger-menu", function (hooks) { last_read_post_number: 1, highest_post_number: 2, notification_level: NotificationLevels.TRACKING, - unread_not_too_old: true, }); } } else { 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 c79036fe797..60ec6afff81 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 @@ -67,7 +67,6 @@ discourseModule("Unit | Model | topic-tracking-state", function (hooks) { highest_post_number: 7, tags: ["pending"], notification_level: NotificationLevels.TRACKING, - unread_not_too_old: true, }, { topic_id: 5, @@ -75,7 +74,6 @@ discourseModule("Unit | Model | topic-tracking-state", function (hooks) { highest_post_number: 7, tags: ["bar", "pending"], notification_level: NotificationLevels.TRACKING, - unread_not_too_old: true, }, { topic_id: 6, @@ -133,7 +131,6 @@ discourseModule("Unit | Model | topic-tracking-state", function (hooks) { highest_post_number: 7, tags: ["pending"], notification_level: NotificationLevels.TRACKING, - unread_not_too_old: true, }, { topic_id: 5, @@ -141,7 +138,6 @@ discourseModule("Unit | Model | topic-tracking-state", function (hooks) { highest_post_number: 7, tags: ["bar", "pending"], notification_level: NotificationLevels.TRACKING, - unread_not_too_old: true, }, { topic_id: 6, @@ -219,7 +215,6 @@ discourseModule("Unit | Model | topic-tracking-state", function (hooks) { category_id: 7, tags: ["bug"], notification_level: NotificationLevels.TRACKING, - unread_not_too_old: true, }, { topic_id: 5, @@ -228,7 +223,6 @@ discourseModule("Unit | Model | topic-tracking-state", function (hooks) { tags: ["bar", "bug"], category_id: 7, notification_level: NotificationLevels.TRACKING, - unread_not_too_old: true, }, { topic_id: 6, @@ -455,7 +449,6 @@ discourseModule("Unit | Model | topic-tracking-state", function (hooks) { last_read_post_number: 4, highest_post_number: 5, notification_level: NotificationLevels.TRACKING, - unread_not_too_old: true, }, { topic_id: 222, diff --git a/app/models/topic_tracking_state.rb b/app/models/topic_tracking_state.rb index 61b6903a8b1..f251a5b33af 100644 --- a/app/models/topic_tracking_state.rb +++ b/app/models/topic_tracking_state.rb @@ -171,7 +171,6 @@ class TopicTrackingState created_at: post.created_at, category_id: post.topic.category_id, archetype: post.topic.archetype, - unread_not_too_old: true } if tags diff --git a/app/serializers/topic_tracking_state_serializer.rb b/app/serializers/topic_tracking_state_serializer.rb index 473997204f2..72736a13be0 100644 --- a/app/serializers/topic_tracking_state_serializer.rb +++ b/app/serializers/topic_tracking_state_serializer.rb @@ -8,7 +8,6 @@ class TopicTrackingStateSerializer < ApplicationSerializer :category_id, :notification_level, :created_in_new_period, - :unread_not_too_old, :treat_as_new_topic_start_date, :tags @@ -17,11 +16,6 @@ class TopicTrackingStateSerializer < ApplicationSerializer object.created_at >= treat_as_new_topic_start_date end - def unread_not_too_old - return true if object.first_unread_at.blank? - object.updated_at >= object.first_unread_at - end - def include_tags? object.respond_to?(:tags) end diff --git a/spec/serializers/topic_tracking_state_serializer_spec.rb b/spec/serializers/topic_tracking_state_serializer_spec.rb index c826daad109..12fddba71da 100644 --- a/spec/serializers/topic_tracking_state_serializer_spec.rb +++ b/spec/serializers/topic_tracking_state_serializer_spec.rb @@ -14,7 +14,6 @@ describe TopicTrackingStateSerializer do expect(serialized[:created_at]).to be_present expect(serialized[:notification_level]).to eq(nil) expect(serialized[:created_in_new_period]).to eq(true) - expect(serialized[:unread_not_too_old]).to eq(true) expect(serialized[:treat_as_new_topic_start_date]).to be_present expect(serialized.has_key?(:tags)).to eq(false) end