From 207cb2052fd9553df984a777596a8262e2e0c7a4 Mon Sep 17 00:00:00 2001 From: Sam Date: Fri, 23 Feb 2024 17:11:39 +1100 Subject: [PATCH] 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 --- .../app/controllers/preferences/interface.js | 2 +- app/jobs/scheduled/update_topic_hot_scores.rb | 7 +++- app/models/topic_list.rb | 2 +- app/models/user_option.rb | 8 +++-- config/site_settings.yml | 4 --- lib/topic_query.rb | 4 +-- spec/jobs/update_topic_hot_scores_spec.rb | 34 +++++++++++++++++++ spec/lib/topic_query_spec.rb | 15 ++++++++ spec/requests/tags_controller_spec.rb | 24 ++++++------- 9 files changed, 75 insertions(+), 25 deletions(-) create mode 100644 spec/jobs/update_topic_hot_scores_spec.rb diff --git a/app/assets/javascripts/discourse/app/controllers/preferences/interface.js b/app/assets/javascripts/discourse/app/controllers/preferences/interface.js index 19003db66bd..d18cd8912ab 100644 --- a/app/assets/javascripts/discourse/app/controllers/preferences/interface.js +++ b/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"; } }, diff --git a/app/jobs/scheduled/update_topic_hot_scores.rb b/app/jobs/scheduled/update_topic_hot_scores.rb index 59df23f31f7..c9eecedc1a5 100644 --- a/app/jobs/scheduled/update_topic_hot_scores.rb +++ b/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 diff --git a/app/models/topic_list.rb b/app/models/topic_list.rb index a9771d1181d..6c2fbc776ca 100644 --- a/app/models/topic_list.rb +++ b/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 diff --git a/app/models/user_option.rb b/app/models/user_option.rb index ef3a9a6a03e..e20a556459b 100644 --- a/app/models/user_option.rb +++ b/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 diff --git a/config/site_settings.yml b/config/site_settings.yml index 64bb9209290..da0d658fc75 100644 --- a/config/site_settings.yml +++ b/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 diff --git a/lib/topic_query.rb b/lib/topic_query.rb index 021a351ce99..76d40ad0e8a 100644 --- a/lib/topic_query.rb +++ b/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)) diff --git a/spec/jobs/update_topic_hot_scores_spec.rb b/spec/jobs/update_topic_hot_scores_spec.rb new file mode 100644 index 00000000000..2c83da4db47 --- /dev/null +++ b/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 diff --git a/spec/lib/topic_query_spec.rb b/spec/lib/topic_query_spec.rb index 77b69e4525f..dcdcb0c5371 100644 --- a/spec/lib/topic_query_spec.rb +++ b/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 diff --git a/spec/requests/tags_controller_spec.rb b/spec/requests/tags_controller_spec.rb index be5bac04b0c..5b247625b12 100644 --- a/spec/requests/tags_controller_spec.rb +++ b/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)