FIX: Build correct topic list filter (#11473)

* FIX: 'false' value was treated as a truthy value

For example, latest.json?no_subcategories=false used to have set
no_subcategories to the string value of 'false', which is not false.

* DEV: Remove dead code

* FIX: Redirect to /none under the right conditions

These conditions are:
 - neither /all or /none present
 - only for default filter

* FIX: Build correct topic list filter

/none was never added to the topic list filter

* FIX: Do not show count for subcategories if 'none' category

* FIX: preload_key must contain /none if no_subcategories

app/assets/javascripts/discourse/app/models/nav-item.js

@@ -75,12 +75,13 @@ const NavItem = EmberObject.extend({
     "name",
     "category",
     "tagId",
+    "noSubcategories",
     "topicTrackingState.messageCount"
   )
-  count(name, category, tagId) {
+  count(name, category, tagId, noSubcategories) {
     const state = this.topicTrackingState;
     if (state) {
-      return state.lookupCount(name, category, tagId);
+      return state.lookupCount(name, category, tagId, noSubcategories);
     }
   },
 });

app/assets/javascripts/discourse/app/models/topic-tracking-state.js

@@ -466,8 +466,10 @@ const TopicTrackingState = EmberObject.extend({
     return new Set(result);
   },
 
-  countCategoryByState(type, categoryId, tagId) {
+  countCategoryByState(type, categoryId, tagId, noSubcategories) {
-    const subcategoryIds = this.getSubCategoryIds(categoryId);
+    const subcategoryIds = noSubcategories
+      ? new Set()
+      : this.getSubCategoryIds(categoryId);
     const mutedCategoryIds =
       this.currentUser && this.currentUser.muted_category_ids;
     let filter = type === "new" ? isNew : isUnread;
@@ -485,12 +487,17 @@ const TopicTrackingState = EmberObject.extend({
     ).length;
   },
 
-  countNew(categoryId, tagId) {
+  countNew(categoryId, tagId, noSubcategories) {
-    return this.countCategoryByState("new", categoryId, tagId);
+    return this.countCategoryByState("new", categoryId, tagId, noSubcategories);
   },
 
-  countUnread(categoryId, tagId) {
+  countUnread(categoryId, tagId, noSubcategories) {
-    return this.countCategoryByState("unread", categoryId, tagId);
+    return this.countCategoryByState(
+      "unread",
+      categoryId,
+      tagId,
+      noSubcategories
+    );
   },
 
   forEachTracked(fn) {
@@ -548,20 +555,20 @@ const TopicTrackingState = EmberObject.extend({
     return sum;
   },
 
-  lookupCount(name, category, tagId) {
+  lookupCount(name, category, tagId, noSubcategories) {
     if (name === "latest") {
       return (
-        this.lookupCount("new", category, tagId) +
+        this.lookupCount("new", category, tagId, noSubcategories) +
-        this.lookupCount("unread", category, tagId)
+        this.lookupCount("unread", category, tagId, noSubcategories)
       );
     }
 
     let categoryId = category ? get(category, "id") : null;
 
     if (name === "new") {
-      return this.countNew(categoryId, tagId);
+      return this.countNew(categoryId, tagId, noSubcategories);
     } else if (name === "unread") {
-      return this.countUnread(categoryId, tagId);
+      return this.countUnread(categoryId, tagId, noSubcategories);
     } else {
       const categoryName = name.split("/")[1];
       if (categoryName) {

app/assets/javascripts/discourse/app/routes/build-category-route.js

@@ -20,42 +20,7 @@ export default (filterArg, params) => {
   return DiscourseRoute.extend({
     queryParams,
 
-    serialize(modelParams) {
-      if (!modelParams.category_slug_path_with_id) {
-        if (modelParams.id === "none") {
-          const category_slug_path_with_id = [
-            modelParams.parentSlug,
-            modelParams.slug,
-          ].join("/");
-          const category = Category.findBySlugPathWithID(
-            category_slug_path_with_id
-          );
-          this.replaceWith("discovery.categoryNone", {
-            category,
-            category_slug_path_with_id,
-          });
-        } else if (modelParams.id === "all") {
-          modelParams.category_slug_path_with_id = [
-            modelParams.parentSlug,
-            modelParams.slug,
-          ].join("/");
-        } else {
-          modelParams.category_slug_path_with_id = [
-            modelParams.parentSlug,
-            modelParams.slug,
-            modelParams.id,
-          ]
-            .filter((x) => x)
-            .join("/");
-        }
-      }
-
-      return modelParams;
-    },
-
     model(modelParams) {
-      modelParams = this.serialize(modelParams);
-
       const category = Category.findBySlugPathWithID(
         modelParams.category_slug_path_with_id
       );
@@ -88,12 +53,11 @@ export default (filterArg, params) => {
       const { category, modelParams } = model;
 
       if (
+        (!params || params.no_subcategories === undefined) &&
         category.default_list_filter === "none" &&
-        filterArg === "default" &&
+        filterArg === "default"
-        modelParams &&
-        modelParams.id !== "all"
       ) {
-        this.replaceWith("discovery.categoryNone", {
+        return this.replaceWith("discovery.categoryNone", {
           category,
           category_slug_path_with_id: modelParams.category_slug_path_with_id,
         });
@@ -137,11 +101,14 @@ export default (filterArg, params) => {
     },
 
     _retrieveTopicList(category, transition, modelParams) {
-      const listFilter = `c/${Category.slugFor(category)}/${
+      const findOpts = filterQueryParams(modelParams, params);
-          category.id
+      const extras = { cached: this.isPoppedState(transition) };
-        }/l/${this.filter(category)}`,
+
-        findOpts = filterQueryParams(modelParams, params),
+      let listFilter = `c/${Category.slugFor(category)}/${category.id}`;
-        extras = { cached: this.isPoppedState(transition) };
+      if (findOpts.no_subcategories) {
+        listFilter += "/none";
+      }
+      listFilter += `/l/${this.filter(category)}`;
 
       return findTopicList(
         this.store,

app/models/topic_list.rb

@@ -64,7 +64,11 @@ class TopicList < DraftableList
 
   def preload_key
     if @category
+      if @opts[:no_subcategories]
+        "topic_list_#{@category.url.sub(/^\//, '')}/none/l/#{@filter}"
+      else
         "topic_list_#{@category.url.sub(/^\//, '')}/l/#{@filter}"
+      end
     else
       "topic_list_#{@filter}"
     end

lib/topic_query_params.rb

@@ -16,9 +16,7 @@ module TopicQueryParams
 
     # hacky columns get special handling
     options[:topic_ids] = param_to_integer_list(:topic_ids)
-    if options[:no_subcategories] == 'true'
+    options[:no_subcategories] = options[:no_subcategories] == 'true' if options[:no_subcategories].present?
-      options[:no_subcategories] = true
-    end
 
     options
   end

spec/models/topic_list_spec.rb

@@ -90,4 +90,18 @@ describe TopicList do
       end
     end
   end
+
+  describe "#preload_key" do
+    let(:category) { Fabricate(:category) }
+
+    it "generates correct key for categories" do
+      topic_list = TopicList.new('latest', nil, nil, category: category, category_id: category.id)
+      expect(topic_list.preload_key).to eq("topic_list_c/#{category.slug}/#{category.id}/l/latest")
+    end
+
+    it "generates correct key for 'no subcategories' option" do
+      topic_list = TopicList.new('latest', nil, nil, category: category, category_id: category.id, no_subcategories: true)
+      expect(topic_list.preload_key).to eq("topic_list_c/#{category.slug}/#{category.id}/none/l/latest")
+    end
+  end
 end

spec/requests/list_controller_spec.rb

@@ -88,6 +88,9 @@ RSpec.describe ListController do
   end
 
   describe "categories and X" do
+    let(:category) { Fabricate(:category_with_definition) }
+    let(:sub_category) { Fabricate(:category_with_definition, parent_category: category) }
+
     it "returns top topics" do
       Fabricate(:topic, like_count: 1000, posts_count: 100)
       TopTopic.refresh!
@@ -100,6 +103,12 @@ RSpec.describe ListController do
       data = response.parsed_body
       expect(data["topic_list"]["topics"].length).to eq(2)
     end
+
+    it "returns topics from subcategories when no_subcategories=false" do
+      Fabricate(:topic, category: sub_category)
+      get "/c/#{category.slug}/#{category.id}/l/latest.json?no_subcategories=false"
+      expect(response.parsed_body["topic_list"]["topics"].length).to eq(2)
+    end
   end
 
   describe 'titles for crawler layout' do