FIX: Ensure suppressed categories do not produce any featured topics. (#7863)

FIX: Ensure suppressed categories do not produce any featured topics. (#7863)

diff --git a/app/models/category.rb b/app/models/category.rb
index 9ba9d91..6009164 100644
--- a/app/models/category.rb
+++ b/app/models/category.rb
@@ -72,6 +72,7 @@ class Category < ActiveRecord::Base
   after_save :clear_url_cache
   after_save :index_search
   after_save :update_reviewables
+  after_save :clear_featured_cache
 
   after_destroy :reset_topic_ids_cache
   after_destroy :publish_category_deletion
@@ -568,6 +569,10 @@ class Category < ActiveRecord::Base
     @@url_cache.clear
   end
 
+  def clear_featured_cache
+    CategoryFeaturedTopic.clear_exclude_category_ids
+  end
+
   def full_slug(separator = "-")
     start_idx = "#{Discourse.base_uri}/c/".length
     url[start_idx..-1].gsub("/", separator)
diff --git a/app/models/category_featured_topic.rb b/app/models/category_featured_topic.rb
index 9ffb283..fef24c0 100644
--- a/app/models/category_featured_topic.rb
+++ b/app/models/category_featured_topic.rb
@@ -38,6 +38,16 @@ class CategoryFeaturedTopic < ActiveRecord::Base
     end
   end
 
+  @@exclude_category_ids = DistributedCache.new('excluded_category_ids_from_featured')
+
+  def self.cached_exclude_category_ids
+    @@exclude_category_ids['ids'] ||= Category.where(suppress_from_latest: true).pluck(:id)
+  end
+
+  def self.clear_exclude_category_ids
+    @@exclude_category_ids.clear
+  end
+
   def self.clear_batch!
     $redis.del(NEXT_CATEGORY_ID_KEY)
   end
@@ -49,7 +59,8 @@ class CategoryFeaturedTopic < ActiveRecord::Base
       per_page: c.num_featured_topics,
       except_topic_ids: [c.topic_id],
       visible: true,
-      no_definitions: true
+      no_definitions: true,
+      exclude_category_ids: CategoryFeaturedTopic.cached_exclude_category_ids
     }
 
     # It may seem a bit odd that we are running 2 queries here, when admin
diff --git a/spec/models/category_featured_topic_spec.rb b/spec/models/category_featured_topic_spec.rb
index 0812237..f994fe5 100644
--- a/spec/models/category_featured_topic_spec.rb
+++ b/spec/models/category_featured_topic_spec.rb
@@ -53,6 +53,20 @@ describe CategoryFeaturedTopic do
       expect(CategoryFeaturedTopic.count).to be(1)
     end
 
+    it 'should not include topics from suppressed categories' do
+      CategoryFeaturedTopic.feature_topics_for(category)
+      expect(
+        CategoryFeaturedTopic.where(category_id: category.id).order('rank asc').pluck(:topic_id)
+      ).to contain_exactly(category_post.topic.id)
+
+      category.update(suppress_from_latest: true)
+
+      CategoryFeaturedTopic.feature_topics_for(category)
+      expect(
+        CategoryFeaturedTopic.where(category_id: category.id).order('rank asc').pluck(:topic_id)
+      ).to_not contain_exactly(category_post.topic.id)
+    end
+
     it 'should feature stuff in the correct order' do
       category = Fabricate(:category, num_featured_topics: 2)
       _t5 = Fabricate(:topic, category_id: category.id, bumped_at: 12.minutes.ago)

GitHub sha: 8e133de8

@nbianca this seems to have introduced a heisentest:

Failures:

  1) CategoryFeaturedTopic feature_topics_for should not include invisible topics
     Failure/Error: expect(CategoryFeaturedTopic.count).to be(1)

       expected #<Integer:3> => 1
            got #<Integer:1> => 0

       Compared using equal?, which compares object identity,
       but expected and actual are not the same object. Use
       `expect(actual).to eq(expected)` if you don't care about
       object identity in this example.
     # ./spec/models/category_featured_topic_spec.rb:53:in `block (3 levels) in <top (required)>'

I can reproduce with seed 42101.

1 Like

DEV: Fix heisentest.