FIX: category list order using category featured topics (#7283)

app/models/category_list.rb

@@ -43,6 +43,19 @@ class CategoryList
     "categories_list".freeze
   end
 
+  def self.order_categories(categories)
+    if SiteSetting.fixed_category_positions
+      categories.order(:position, :id)
+    else
+      allowed_category_ids = categories.pluck(:id) << nil # `nil` is necessary to include categories without any associated topics
+      categories.left_outer_joins(:featured_topics)
+        .where(topics: { category_id: allowed_category_ids })
+        .group('categories.id')
+        .order("max(topics.bumped_at) DESC NULLS LAST")
+        .order('categories.id ASC')
+    end
+  end
+
   private
 
   def find_relevant_topics
@@ -75,11 +88,7 @@ class CategoryList
 
     @categories = @categories.where("categories.parent_category_id = ?", @options[:parent_category_id].to_i) if @options[:parent_category_id].present?
 
-    if SiteSetting.fixed_category_positions
-      @categories = @categories.order(:position, :id)
-    else
-      @categories = @categories.includes(:latest_post).order("posts.created_at DESC NULLS LAST").order('categories.id ASC')
-    end
+    @categories = self.class.order_categories(@categories)
 
     @categories = @categories.to_a
 

spec/models/category_list_spec.rb

@@ -116,7 +116,14 @@ describe CategoryList do
   end
 
   describe 'category order' do
-    let(:category_ids) { CategoryList.new(Guardian.new(admin)).categories.map(&:id) - [SiteSetting.uncategorized_category_id] }
+    def ordered_category_list(some_user)
+      categories = Category.secured(Guardian.new(some_user))
+      subcategories = categories.where.not(parent_category_id: nil).pluck(:id)
+      CategoryList.order_categories(categories).pluck(:id) - subcategories.push(SiteSetting.uncategorized_category_id)
+    end
+
+    let(:category_ids_admin) { ordered_category_list(admin) }
+    let(:category_ids_user) { ordered_category_list(user) }
 
     before do
       uncategorized = Category.find(SiteSetting.uncategorized_category_id)
@@ -130,17 +137,21 @@ describe CategoryList do
       end
 
       it "returns categories in specified order" do
-        cat1, cat2 = Fabricate(:category, position: 1), Fabricate(:category, position: 0)
-        expect(category_ids).to eq([cat2.id, cat1.id])
+        cat1 = Fabricate(:category, position: 1)
+        cat2 = Fabricate(:category, position: 0)
+        expect(category_ids_admin).to eq([cat2.id, cat1.id])
       end
 
       it "handles duplicate position values" do
-        cat1, cat2, cat3, cat4 = Fabricate(:category, position: 0), Fabricate(:category, position: 0), Fabricate(:category, position: nil), Fabricate(:category, position: 0)
-        first_three = category_ids[0, 3] # The order is not deterministic
+        cat1 = Fabricate(:category, position: 0)
+        cat2 = Fabricate(:category, position: 0)
+        cat3 = Fabricate(:category, position: nil)
+        cat4 = Fabricate(:category, position: 0)
+        first_three = category_ids_admin[0, 3] # The order is not deterministic
         expect(first_three).to include(cat1.id)
         expect(first_three).to include(cat2.id)
         expect(first_three).to include(cat4.id)
-        expect(category_ids[-1]).to eq(cat3.id)
+        expect(category_ids_admin[-1]).to eq(cat3.id)
       end
     end
 
@@ -150,18 +161,44 @@ describe CategoryList do
       end
 
       it "returns categories in order of activity" do
-        post1 = Fabricate(:post, created_at: 1.hour.ago)
-        post2 = Fabricate(:post, created_at: 1.day.ago)
-        post3 = Fabricate(:post, created_at: 1.week.ago)
-        cat1 = Fabricate(:category, position: 0, latest_post_id: post2.id)
-        cat2 = Fabricate(:category, position: 1, latest_post_id: post3.id)
-        cat3 = Fabricate(:category, position: 1, latest_post_id: post1.id)
-        expect(category_ids).to eq([cat3.id, cat1.id, cat2.id])
+        cat1 = Fabricate(:category, position: 0)
+        cat2 = Fabricate(:category, position: 1)
+        cat3 = Fabricate(:category, position: 2)
+        cat4 = Fabricate(:category, position: 3)
+        cat5 = Fabricate(:category, parent_category_id: cat2.id)
+
+        Fabricate(:topic, category_id: cat3.id, bumped_at: 1.minutes.ago)
+        Fabricate(:topic, category_id: cat5.id, bumped_at: 2.minutes.ago)
+        Fabricate(:topic, category_id: cat1.id, bumped_at: 3.minutes.ago)
+        Fabricate(:topic, category_id: cat2.id, bumped_at: 5.minutes.ago)
+
+        CategoryFeaturedTopic.feature_topics
+
+        expect(category_ids_admin).to eq([cat3.id, cat2.id, cat1.id, cat4.id])
       end
 
       it "returns categories in order of id when there's no activity" do
-        cat1, cat2 = Fabricate(:category, position: 1), Fabricate(:category, position: 0)
-        expect(category_ids).to eq([cat1.id, cat2.id])
+        cat1 = Fabricate(:category, position: 2)
+        cat2 = Fabricate(:category, position: 1)
+        cat3 = Fabricate(:category, position: 0)
+        expect(category_ids_admin).to eq([cat1.id, cat2.id, cat3.id])
+      end
+
+      it "shows correct order when a topic in a private category is bumped" do
+        public_cat = Fabricate(:category)
+        public_cat2 = Fabricate(:category)
+        sub_cat_private = Fabricate(:category, parent_category_id: public_cat2.id)
+        sub_cat_private.set_permissions(admins: :full)
+        sub_cat_private.save
+
+        Fabricate(:topic, category: sub_cat_private, bumped_at: 1.minutes.ago)
+        Fabricate(:topic, category: public_cat, bumped_at: 3.minutes.ago)
+        Fabricate(:topic, category: public_cat2, bumped_at: 4.minutes.ago)
+
+        CategoryFeaturedTopic.feature_topics
+
+        expect(category_ids_user).to eq([public_cat.id, public_cat2.id])
+        expect(category_ids_admin).to eq([public_cat2.id, public_cat.id])
       end
     end
   end