DEV: Improving topic tracking state code (#12555)

DEV: Improving topic tracking state code (#12555)

The aim of this PR is to improve the topic tracking state JavaScript code and test coverage so further modifications can be made in plugins and in core. This is focused on making topic tracking state changes easier to respond to with callbacks, and changing it so all state modifications go through a single method instead of modifying this.state all over the place. I have also tried to improve documentation, make the code clearer and easier to follow, and make it clear what are public and private methods.

The changes I have made here should not break backwards compatibility, though there is no way to tell for sure if other plugin/theme authors are using tracking state methods that are essentially private methods. Any name changes made in the tracking-state.js code have been reflected in core.


We now have a _trackedTopicLimit in the tracking state. Previously, if a topic was neither new nor unread it was removed from the tracking state; now it is only removed if we are tracking more than _trackedTopicLimit topics (which is set to 4000). This is so plugins/themes adding topics with TopicTrackingState.register_refine_method can add topics to track that aren’t necessarily new or unread, e.g. for totals counts.

Anywhere where we were doing tracker.states["t" + data.topic_id] = newObject has now been changed to flow through central modifyState and modifyStateProp methods. This is so state objects are not modified until they need to be (e.g. sometimes properties are set based on certain conditions) and also so we can run callback functions when the state is modified.

I added onStateChange and onMessageIncrement methods to register callbacks that are called when the state is changed and when the message count is incremented, respectively. This was done so we no longer need to do things like @observes("trackingState.states") in other Ember classes.

I split up giant functions like sync and establishChannels into smaller functions for readability and testability, and renamed many small functions to _functionName to designate them as private functions which not be called by consumers of topicTrackingState. Public functions are now all documented (well…at least ones that are not immediately obvious).


On the backend side, I have changed the MessageBus publish events for TopicTrackingState to send back tags and tag IDs for more channels, and done some extra code cleanup and refactoring. Plugins may override TopicTrackingState.report so I have made its footprint as small as possible and externalised the main parts of it into other methods.

diff --git a/app/assets/javascripts/discourse/app/components/discovery-topics-list.js b/app/assets/javascripts/discourse/app/components/discovery-topics-list.js
index 8665981..9b414b9 100644
--- a/app/assets/javascripts/discourse/app/components/discovery-topics-list.js
+++ b/app/assets/javascripts/discourse/app/components/discovery-topics-list.js
@@ -21,8 +21,19 @@ const DiscoveryTopicsListComponent = Component.extend(UrlRefresh, LoadMore, {
     }
   },
 
-  @observes("topicTrackingState.states")
-  _updateTopics() {
+  @on("didInsertElement")
+  _monitorTrackingState() {
+    this.stateChangeCallbackId = this.topicTrackingState.onStateChange(
+      this._updateTrackingTopics.bind(this)
+    );
+  },
+
+  @on("willDestroyElement")
+  _removeTrackingStateChangeMonitor() {
+    this.topicTrackingState.offStateChange(this.stateChangeCallbackId);
+  },
+
+  _updateTrackingTopics() {
     this.topicTrackingState.updateTopics(this.model.topics);
   },
 
diff --git a/app/assets/javascripts/discourse/app/mixins/bulk-topic-selection.js b/app/assets/javascripts/discourse/app/mixins/bulk-topic-selection.js
index 7ba734c..8562aa9 100644
--- a/app/assets/javascripts/discourse/app/mixins/bulk-topic-selection.js
+++ b/app/assets/javascripts/discourse/app/mixins/bulk-topic-selection.js
@@ -44,9 +44,7 @@ export default Mixin.create({
 
       promise.then((result) => {
         if (result && result.topic_ids) {
-          const tracker = this.topicTrackingState;
-          result.topic_ids.forEach((t) => tracker.removeTopic(t));
-          tracker.incrementMessageCount();
+          this.topicTrackingState.removeTopics(result.topic_ids);
         }
 
         this.send("closeModal");
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 b00e842..ee59b9e 100644
--- a/app/assets/javascripts/discourse/app/models/topic-tracking-state.js
+++ b/app/assets/javascripts/discourse/app/models/topic-tracking-state.js
@@ -1,27 +1,39 @@
 import EmberObject, { get } from "@ember/object";
 import discourseComputed, { on } from "discourse-common/utils/decorators";
 import Category from "discourse/models/category";
+import { deepEqual, deepMerge } from "discourse-common/lib/object";
 import DiscourseURL from "discourse/lib/url";
 import { NotificationLevels } from "discourse/lib/notification-levels";
 import PreloadStore from "discourse/lib/preload-store";
 import User from "discourse/models/user";
-import { deepEqual } from "discourse-common/lib/object";
 import { isEmpty } from "@ember/utils";
 
-function isNew(topic) {
+function isNew(topic, currentUser) {
+  let createdInNewPeriod = true;
+  if (currentUser) {
+    createdInNewPeriod =
+      moment(topic.created_at) >=
+      moment(currentUser.treat_as_new_topic_start_date);
+  }
   return (
     topic.last_read_post_number === null &&
     ((topic.notification_level !== 0 && !topic.notification_level) ||
       topic.notification_level >= NotificationLevels.TRACKING) &&
+    createdInNewPeriod &&
     isUnseen(topic)
   );
 }
 
 function isUnread(topic) {
+  let unreadNotTooOld = true;
+  if (topic.first_unread_at) {
+    unreadNotTooOld = moment(topic.updated_at) >= moment(topic.first_unread_at);
+  }
   return (
     topic.last_read_post_number !== null &&
     topic.last_read_post_number < topic.highest_post_number &&
-    topic.notification_level >= NotificationLevels.TRACKING
+    topic.notification_level >= NotificationLevels.TRACKING &&
+    unreadNotTooOld
   );
 }
 
@@ -49,104 +61,46 @@ const TopicTrackingState = EmberObject.extend({
     this.unreadSequence = [];
     this.newSequence = [];
     this.states = {};
+    this.messageIncrementCallbacks = {};
+    this.stateChangeCallbacks = {};
+    this._trackedTopicLimit = 4000;
   },
 
+  /**
+   * Subscribe to MessageBus channels which are used for publishing changes
+   * to the tracking state. Each message received will modify state for
+   * a particular topic.
+   *
+   * See app/models/topic_tracking_state.rb for the data payloads published
+   * to each of the channels.
+   *
+   * @method establishChannels
+   */
   establishChannels() {
-    const tracker = this;
-
-    const process = (data) => {
-      if (["muted", "unmuted"].includes(data.message_type)) {
-        tracker.trackMutedOrUnmutedTopic(data);
-        return;
-      }
-
-      tracker.pruneOldMutedAndUnmutedTopics();
-
-      if (tracker.isMutedTopic(data.topic_id)) {
-        return;
-      }
-
-      if (
-        this.siteSettings.mute_all_categories_by_default &&
-        !tracker.isUnmutedTopic(data.topic_id)
-      ) {
-        return;
-      }
-
-      if (data.message_type === "delete") {
-        tracker.removeTopic(data.topic_id);
-        tracker.incrementMessageCount();
-      }
-
-      if (["new_topic", "latest"].includes(data.message_type)) {
-        const muted_category_ids = User.currentProp("muted_category_ids");
-        if (
-          muted_category_ids &&
-          muted_category_ids.includes(data.payload.category_id)
-        ) {
-          return;
-        }
-      }
-
-      if (["new_topic", "latest"].includes(data.message_type)) {
-        const mutedTagIds = User.currentProp("muted_tag_ids");
-        if (
-          hasMutedTags(
-            data.payload.topic_tag_ids,
-            mutedTagIds,
-            this.siteSettings
-          )
-        ) {
-          return;
-        }
-      }
-
-      if (data.message_type === "latest") {
-        tracker.notify(data);
-      }
-
-      if (data.message_type === "dismiss_new") {
-        tracker.dismissNewTopic(data);
-      }
-
-      if (["new_topic", "unread", "read"].includes(data.message_type)) {
-        tracker.notify(data);
-        const old = tracker.states["t" + data.topic_id];
-        if (!deepEqual(old, data.payload)) {
-          tracker.states["t" + data.topic_id] = data.payload;
-          tracker.notifyPropertyChange("states");
-          tracker.incrementMessageCount();
-        }
-      }
-    };
-
-    this.messageBus.subscribe("/new", process);
-    this.messageBus.subscribe("/latest", process);
+    this.messageBus.subscribe("/new", this._processChannelPayload.bind(this));
+    this.messageBus.subscribe(
+      "/latest",
+      this._processChannelPayload.bind(this)
+    );
     if (this.currentUser) {
       this.messageBus.subscribe(
         "/unread/" + this.currentUser.get("id"),
-        process
+        this._processChannelPayload.bind(this)
       );
     }
 
     this.messageBus.subscribe("/delete", (msg) => {
-      const old = tracker.states["t" + msg.topic_id];
-      if (old) {
-        old.deleted = true;
-      }
-      tracker.incrementMessageCount();
+      this.modifyStateProp(msg, "deleted", true);
+      this.incrementMessageCount();
     });
 
     this.messageBus.subscribe("/recover", (msg) => {
-      const old = tracker.states["t" + msg.topic_id];
-      if (old) {
-        delete old.deleted;
-      }
-      tracker.incrementMessageCount();
+      this.modifyStateProp(msg, "deleted", false);
+      this.incrementMessageCount();
     });
 
     this.messageBus.subscribe("/destroy", (msg) => {
-      tracker.incrementMessageCount();
+      this.incrementMessageCount();
       const currentRoute = DiscourseURL.router.currentRoute.parent;
       if (
         currentRoute.name === "topic" &&
@@ -181,17 +135,6 @@ const TopicTrackingState = EmberObject.extend({
     this.currentUser && this.currentUser.set(key, topics);
   },
 
-  dismissNewTopic(data) {
-    data.payload.topic_ids.forEach((k) => {
-      const topic = this.states[`t${k}`];
-      this.states[`t${k}`] = Object.assign({}, topic, {
-        is_seen: true,
-      });
-    });
-    this.notifyPropertyChange("states");
-    this.incrementMessageCount();
-  },
-
   pruneOldMutedAndUnmutedTopics() {
     const now = Date.now();
     let mutedTopics = this.mutedTopics().filter(

[... diff too long, it was truncated ...]

GitHub sha: 45df579d

This commit appears in #12555 which was approved by eviltrout. It was merged by martin.