UX: order categories based on latest activity for all page styles (#7196)

UX: order categories based on latest activity for all page styles (#7196)

follow up on 32db3ac2

diff --git a/app/models/category_list.rb b/app/models/category_list.rb
index 9be710e..fb0388f 100644
--- a/app/models/category_list.rb
+++ b/app/models/category_list.rb
@@ -77,13 +77,8 @@ class CategoryList
 
     if SiteSetting.fixed_category_positions
       @categories = @categories.order(:position, :id)
-    elsif !SiteSetting.fixed_category_positions && SiteSetting.desktop_category_page_style == "categories_and_latest_topics"
-      @categories = @categories.includes(:latest_post).order("posts.created_at DESC NULLS LAST").order('categories.id ASC')
     else
-      @categories = @categories.order('COALESCE(categories.posts_week, 0) DESC')
-        .order('COALESCE(categories.posts_month, 0) DESC')
-        .order('COALESCE(categories.posts_year, 0) DESC')
-        .order('id ASC')
+      @categories = @categories.includes(:latest_post).order("posts.created_at DESC NULLS LAST").order('categories.id ASC')
     end
 
     @categories = @categories.to_a
diff --git a/spec/models/category_list_spec.rb b/spec/models/category_list_spec.rb
index 8cd2450..cda6780 100644
--- a/spec/models/category_list_spec.rb
+++ b/spec/models/category_list_spec.rb
@@ -147,35 +147,22 @@ describe CategoryList do
     context 'fixed_category_positions is disabled' do
       before do
         SiteSetting.fixed_category_positions = false
-        SiteSetting.desktop_category_page_style = "categories_and_top_topics"
       end
 
       it "returns categories in order of activity" do
-        cat1 = Fabricate(:category, position: 0, posts_week: 1, posts_month: 1, posts_year: 1)
-        cat2 = Fabricate(:category, position: 1, posts_week: 2, posts_month: 1, posts_year: 1)
-        expect(category_ids).to eq([cat2.id, cat1.id])
+        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])
       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])
       end
-
-      context "when using categories_and_latest_topics layout" do
-        before do
-          SiteSetting.desktop_category_page_style = "categories_and_latest_topics"
-        end
-
-        it "returns categories in order of latest 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])
-        end
-      end
     end
   end

GitHub sha: 714a0d87

2 Likes

Oh I know why this does not work. We need to handle sub categories as well so this is actually a fairly tricky change.

Each category now keeps track of latest_post in the category, BUT it does not keep track of latest_post in any of the sub categories.

To add more complexity here, we have to apply security to the category_featured_topics list so we can’t simply join to that without scoping.

@ZogStriP / @nlalonde any thought here?

I see 3 options

  1. We roll back this change and just go with previous implementation

  2. We left join into category_featured_topics for non leaf categories grouping by all the right things mixing in security and order on max bumped_at of the topic descending.

  3. We restructure everything so we reorder the category list later (or maybe even just do it on the client)

(2) and (3) have a minor downside that if you have a LOT of pinned topics your category is always going to be buried, which is not great. But then again pinning more than 3 topic per category is opting for pain, so you can just use fixed ordering.

I am not sure what the perfect thing is to do here.

2 Likes

Similar to option 2, but we can still use the latest_posts with something like this…

SELECT c.parent_category_id, MAX(p.created_at)
FROM categories c
INNER JOIN posts p
ON c.latest_post_id = p.id
WHERE NOT c.read_restricted OR c.id IN (?guardian.secure_category_ids?)
GROUP BY c.parent_category_id

(That’s only subcategories.)

1 Like

Has been followed up