DEV: Improving topic tracking state code (PR #12555)

GitHub

I mainly wanted this to be in the cloud™ not just my PC

BTW you can push branches to origin without a PR and it will do this!

I mainly wanted this to be in the cloud™ not just my PC

BTW you can push branches to origin without a PR and it will do this!

Yeah I know, I mainly find opening a PR helps me see the whole overview of the changes I’ve made — I really like GitHub’s diff interface for the files :slight_smile:

tags,tag_ids = post.topic.tags.pluck(:id, :name).transpose

not happy with uniq being called unconditionally. In fact maybe part of the contract shall be … refiner shall not introduce dupes … then you don’t need this at all.

got to do this “after” as well so you do not leak state

Hmm in that case I probably need to pass some data structure class to the refine method instead of the raw array, then that can control this e.g.

class TopicTrackingStateReport do
  def initialize(report_data)
    @report_data = report_data
    @report_data_map = {}
    @report_data.each do |data|
      if !@report_data_map.key?(data[:topic_id])
        @report_data_map[data[:topic_id]] = data
      end
    end
  end

  def add(data)
    if @report_data_map.key?(data.topic_id) raise NotUniqueError
    @report_data_map[data.topic_id] = data
  end

  def read
    @report_data
  end
end

I think I should turn the data array into a hash to speed up the lookups. This way refinement.call would pass through an instance of this class. Thoughts? I don’t think we want to trust the refiner methods to not introduce dupes

A simple compromise would be not to call uniq unless a refinement happened, I think that is probably the easiest way to go here.

The title of this pull request changed from “DEV: First pass improving topic tracking state” to "DEV: Improving topic tracking state code

I am not really sure if we want to set topic.deleted like we do here:

Or if we just want to remove the topic completely. This code on 720 is not currently being called.

Any reason not to use transpose here too?

No reason, just missed it :slight_smile:

1 Like

I had to add this in because we are now possibly returning topics over the new topic threshold that shouldn’t show as “new” in tracking but which can be used for total counts.

This is just reflecting what we already do in topic_query

We weren’t actually using this in sidebar, but we are now!

Added this refinement and the one for isNew to bring the JS in line with what we are doing on server-side

Added this to the query so it matches up with what we are doing for muted categories in TopicQuery discourse/topic_query.rb at 2a8a23d9280c9bfd1af6976a301a4380d1c2adf9 · discourse/discourse · GitHub

Otherwise, if someone has muted a category but has a tagged topic that is tracking or higher in that category, it will not show up in the totals.

I wonder if we should just do a report.limit(4000) here to avoid too much state craziness? Maybe even less?

TODO ?

Yeah I think some safety is in order, I think a hard limit of 5000 on refinements is a good start. as a constant.