FIX: muted tags breaking hot page when filtered to tags (#25824)

Also, remove experimental setting and simply use top_menu for feature detection

This means that when people eventually enable the hot top menu, there will
be topics in it


Co-authored-by: Alan Guo Xiang Tan <gxtan1990@gmail.com>

app/assets/javascripts/discourse/app/controllers/preferences/interface.js

@@ -45,7 +45,7 @@ export default Controller.extend({
 
     this.set("selectedDarkColorSchemeId", this.session.userDarkSchemeId);
 
-    if (this.siteSettings.experimental_hot_topics) {
+    if (this.siteSettings.top_menu.split("|").includes("hot")) {
       USER_HOMES[8] = "hot";
     }
   },

app/jobs/scheduled/update_topic_hot_scores.rb

@@ -4,8 +4,13 @@ module Jobs
   class UpdateTopicHotScores < ::Jobs::Scheduled
     every 10.minutes
 
+    HOT_SCORE_UPDATE_REDIS_KEY = "hot_score_6_hourly"
+
     def execute(args)
-      TopicHotScore.update_scores if SiteSetting.experimental_hot_topics
+      if SiteSetting.top_menu_map.include?("hot") ||
+           Discourse.redis.set(HOT_SCORE_UPDATE_REDIS_KEY, 1, ex: 6.hour, nx: true)
+        TopicHotScore.update_scores
+      end
     end
   end
 end

app/models/topic_list.rb

@@ -55,7 +55,7 @@ class TopicList
 
     @category = Category.find_by(id: @opts[:category_id]) if @opts[:category]
 
-    @tags = Tag.where(id: @opts[:tags]).all if @opts[:tags]
+    @tags = Tag.where(id: @opts[:tag_ids]).all if @opts[:tag_ids].present?
 
     @publish_read_state = !!@opts[:publish_read_state]
   end

app/models/user_option.rb

@@ -182,9 +182,11 @@ class UserOption < ActiveRecord::Base
   def homepage
     return HOMEPAGES[homepage_id] if HOMEPAGES.keys.include?(homepage_id)
 
-    return SiteSetting.experimental_hot_topics ? "hot" : SiteSetting.homepage if homepage_id == 8
-
-    SiteSetting.homepage
+    if homepage_id == 8 && SiteSetting.top_menu_map.include?("hot")
+      "hot"
+    else
+      SiteSetting.homepage
+    end
   end
 
   def text_size

config/site_settings.yml

@@ -3141,10 +3141,6 @@ dashboard:
   verbose_user_stat_count_logging:
     hidden: true
     default: false
-  experimental_hot_topics:
-    hidden: true
-    default: false
-    client: true
   hot_topics_gravity:
     hidden: true
     default: 1.2

lib/topic_query.rb

@@ -1285,9 +1285,7 @@ class TopicQuery
         result = result.joins(:tags).where("tags.id in (?)", tags)
       end
 
-      # TODO: this is very side-effecty and should be changed
-      # It is done cause further up we expect normalized tags
-      @options[:tags] = tags
+      @options[:tag_ids] = tags
     elsif @options[:no_tags]
       # the following will do: ("topics"."id" NOT IN (SELECT DISTINCT "topic_tags"."topic_id" FROM "topic_tags"))
       result = result.where.not(id: TopicTag.distinct.select(:topic_id))

spec/jobs/update_topic_hot_scores_spec.rb

@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+require "file_store/s3_store"
+
+RSpec.describe Jobs::UpdateTopicHotScores do
+  use_redis_snapshotting
+  let(:job) { subject }
+
+  fab!(:topic) { Fabricate(:topic, created_at: 1.day.ago) }
+
+  it "runs an update even if hot is missing from top_menu (once every 6 hours)" do
+    SiteSetting.top_menu = "latest"
+    job.execute({})
+
+    expect(TopicHotScore.where(topic_id: topic.id).count).to eq(1)
+
+    topic2 = Fabricate(:topic, created_at: 1.hour.ago)
+    job.execute({})
+
+    expect(TopicHotScore.where(topic_id: topic2.id).count).to eq(0)
+  end
+
+  it "runs an update unconditionally if hot is in top menu" do
+    SiteSetting.top_menu = "latest|hot"
+    job.execute({})
+
+    expect(TopicHotScore.where(topic_id: topic.id).count).to eq(1)
+
+    topic2 = Fabricate(:topic, created_at: 1.hour.ago)
+    job.execute({})
+
+    expect(TopicHotScore.where(topic_id: topic2.id).count).to eq(1)
+  end
+end

spec/lib/topic_query_spec.rb

@@ -93,6 +93,21 @@ RSpec.describe TopicQuery do
       TopicHotScore.update_scores(2)
 
       expect(TopicQuery.new(nil).list_hot.topics.map(&:id)).to eq([pinned_topic.id, topic.id])
+
+      SiteSetting.tagging_enabled = true
+      user = Fabricate(:user)
+      tag = Fabricate(:tag)
+
+      TagUser.create!(
+        user_id: user.id,
+        tag_id: tag.id,
+        notification_level: NotificationLevels.all[:muted],
+      )
+
+      topic.update!(tags: [tag])
+
+      # even though it is muted, we should still show it cause we are filtered to it
+      expect(TopicQuery.new(user, { tags: [tag.name] }).list_hot.topics.map(&:id)).to eq([topic.id])
     end
 
     it "excludes muted categories and topics" do

spec/requests/tags_controller_spec.rb

@@ -166,7 +166,7 @@ RSpec.describe TagsController do
       include_examples "retrieves the right tags"
 
       it "works for tags in groups" do
-        tag_group = Fabricate(:tag_group, tags: [test_tag, topic_tag, synonym])
+        _tag_group = Fabricate(:tag_group, tags: [test_tag, topic_tag, synonym])
 
         get "/tags.json"
 
@@ -405,7 +405,6 @@ RSpec.describe TagsController do
       expect(response.status).to eq(200)
 
       json = response.parsed_body
-
       topic_list = json["topic_list"]
 
       expect(topic_list["tags"].map { |t| t["id"] }).to contain_exactly(tag.id)
@@ -423,7 +422,7 @@ RSpec.describe TagsController do
     end
 
     it "does not show staff-only tags" do
-      tag_group = Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: ["test"])
+      _tag_group = Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: ["test"])
 
       get "/tag/test"
       expect(response.status).to eq(404)
@@ -558,14 +557,14 @@ RSpec.describe TagsController do
     end
 
     it "returns 404 if tag is staff-only" do
-      tag_group = Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: ["test"])
+      _tag_group = Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: ["test"])
       get "/tag/test/info.json"
       expect(response.status).to eq(404)
     end
 
     it "staff-only tags can be retrieved for staff user" do
       sign_in(admin)
-      tag_group = Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: ["test"])
+      _tag_group = Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: ["test"])
       get "/tag/test/info.json"
       expect(response.status).to eq(200)
     end
@@ -575,7 +574,7 @@ RSpec.describe TagsController do
       category2 = Fabricate(:category)
       tag_group = Fabricate(:tag_group, tags: [tag])
       category2.update!(tag_groups: [tag_group])
-      staff_category = Fabricate(:private_category, group: Fabricate(:group), tags: [tag])
+      _staff_category = Fabricate(:private_category, group: Fabricate(:group), tags: [tag])
       get "/tag/#{tag.name}/info.json"
       expect(response.parsed_body.dig("tag_info", "category_ids")).to contain_exactly(
         category.id,
@@ -852,7 +851,7 @@ RSpec.describe TagsController do
         end
 
         it "returns a 404 when tag is restricted" do
-          tag_group = Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: ["test"])
+          _tag_group = Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: ["test"])
 
           get "/tag/test/l/latest.json"
           expect(response.status).to eq(404)
@@ -944,7 +943,7 @@ RSpec.describe TagsController do
     end
 
     it "returns a 404 if tag is restricted" do
-      tag_group = Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: ["test"])
+      _tag_group = Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: ["test"])
 
       get "/tag/test/l/top.json"
       expect(response.status).to eq(404)
@@ -1019,7 +1018,7 @@ RSpec.describe TagsController do
         end
 
         it "can filter on category without q param" do
-          nope = Fabricate(:tag, name: "nope")
+          Fabricate(:tag, name: "nope")
           get "/tags/filter/search.json", params: { categoryId: category.id }
           expect(response.status).to eq(200)
           expect(response.parsed_body["results"].map { |j| j["id"] }.sort).to eq([yup.name])
@@ -1057,7 +1056,8 @@ RSpec.describe TagsController do
       end
 
       it "matches tags after sanitizing input" do
-        yup, nope = Fabricate(:tag, name: "yup"), Fabricate(:tag, name: "nope")
+        Fabricate(:tag, name: "yup")
+        Fabricate(:tag, name: "nope")
         get "/tags/filter/search.json", params: { q: "N/ope" }
         expect(response.status).to eq(200)
         expect(response.parsed_body["results"].map { |j| j["id"] }.sort).to eq(["nope"])
@@ -1086,7 +1086,7 @@ RSpec.describe TagsController do
 
       it "can return all the results" do
         tag_group1 = Fabricate(:tag_group, tag_names: %w[common1 common2 group1tag group1tag2])
-        tag_group2 = Fabricate(:tag_group, tag_names: %w[common1 common2])
+        _tag_group2 = Fabricate(:tag_group, tag_names: %w[common1 common2])
         category = Fabricate(:category, tag_groups: [tag_group1])
         get "/tags/filter/search.json",
             params: {
@@ -1324,7 +1324,7 @@ RSpec.describe TagsController do
       end
 
       it "can return errors" do
-        tag2 = Fabricate(:tag, target_tag: tag)
+        _tag2 = Fabricate(:tag, target_tag: tag)
         tag3 = Fabricate(:tag)
         post "/tag/#{tag3.name}/synonyms.json", params: { synonyms: [tag.name] }
         expect(response.status).to eq(200)