diff --git a/app/models/category_list.rb b/app/models/category_list.rb index fb0388fd90f..5bb50132ad8 100644 --- a/app/models/category_list.rb +++ b/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 diff --git a/spec/models/category_list_spec.rb b/spec/models/category_list_spec.rb index cda6780e9c0..3bba4856c91 100644 --- a/spec/models/category_list_spec.rb +++ b/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