FIX: `default_list_filter = none` navigation and preloading (#20641)

When a category has default_list_filter=none, there were a number of issues which this commit resolves:

1. When using the breadcrumbs to navigate a `default_list_filter=none` category, adding a tag filter would not apply the no-subcategories filter, but the subcategories dropdown would still say 'none'. This commit adjusts `getCategoryAndTagUrl` so that `/none` is added to the URL

2. When landing on `/tags/c/{slug}/{id}/{tag}`, for a default_list_filter=none category, it would include subcategories. This commit introduces a client-side redirect to match the behavior of `/c/{slug}/{id}`

3. When directly navigating to `/c/{slug}/{id}`, it was correctly redirecting to `/c/{slug}/{id}/none`, BUT it was still using the preloaded data for the old route. This has been happening since e7a84948. Prior to that, the preloaded data was discarded and a new JSON request was made to the server. This commit restores that discarding behavior. In future we may want to look into making this more efficient.

System specs are introduced to provide end-end testing of this functionality

app/assets/javascripts/discourse/app/lib/url.js

@@ -506,9 +506,13 @@ export function getCategoryAndTagUrl(category, subcategories, tag) {
 
   if (category) {
     url = category.path;
-    if (subcategories && category.default_list_filter === "none") {
+    if (category.default_list_filter === "none" && subcategories) {
-      url += "/all";
+      if (subcategories) {
-    } else if (!subcategories && category.default_list_filter === "all") {
+        url += "/all";
+      } else {
+        url += "/none";
+      }
+    } else if (!subcategories) {
       url += "/none";
     }
   }

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

@@ -15,6 +15,7 @@ import I18n from "I18n";
 import PermissionType from "discourse/models/permission-type";
 import TopicList from "discourse/models/topic-list";
 import { action } from "@ember/object";
+import PreloadStore from "discourse/lib/preload-store";
 
 // A helper function to create a category route with parameters
 export default (filterArg, params) => {
@@ -59,6 +60,8 @@ export default (filterArg, params) => {
         filterArg === "default" &&
         modelParams
       ) {
+        // TODO: avoid throwing away preload data by redirecting on the server
+        PreloadStore.getAndRemove("topic_list");
         return this.replaceWith("discovery.categoryNone", {
           category,
           category_slug_path_with_id: modelParams.category_slug_path_with_id,

app/assets/javascripts/discourse/app/routes/tag-show.js

@@ -17,6 +17,7 @@ import { makeArray } from "discourse-common/lib/helpers";
 import { setTopicList } from "discourse/lib/topic-list-tracker";
 import showModal from "discourse/lib/show-modal";
 import { action } from "@ember/object";
+import PreloadStore from "discourse/lib/preload-store";
 
 const NONE = "none";
 const ALL = "all";
@@ -89,6 +90,20 @@ export default DiscourseRoute.extend(FilterModeMixin, {
       filter = `tag/${tagId}/l/${topicFilter}`;
     }
 
+    if (
+      this.noSubcategories === undefined &&
+      category?.default_list_filter === "none" &&
+      topicFilter === "latest"
+    ) {
+      // TODO: avoid throwing away preload data by redirecting on the server
+      PreloadStore.getAndRemove("topic_list");
+      return this.replaceWith(
+        "tags.showCategoryNone",
+        params.category_slug_path_with_id,
+        tagId
+      );
+    }
+
     const list = await findTopicList(
       this.store,
       this.topicTrackingState,
@@ -123,10 +138,7 @@ export default DiscourseRoute.extend(FilterModeMixin, {
   },
 
   setupController(controller, model) {
-    const noSubcategories =
+    const noSubcategories = this.noSubcategories;
-      this.noSubcategories === undefined
-        ? model.category?.default_list_filter === NONE
-        : this.noSubcategories;
 
     this.controllerFor("tag.show").setProperties({
       model: model.tag,

app/assets/javascripts/discourse/tests/unit/lib/url-test.js

@@ -139,7 +139,7 @@ module("Unit | Utility | url", function () {
         { path: "/c/foo/1", default_list_filter: "none" },
         false
       ),
-      "/c/foo/1"
+      "/c/foo/1/none"
     );
   });
 

spec/system/discovery_breadcrumb_navigation_spec.rb

@@ -0,0 +1,110 @@
+# frozen_string_literal: true
+
+describe "Navigating with breadcrumbs", type: :system, js: true do
+  let(:discovery) { PageObjects::Pages::Discovery.new }
+
+  fab!(:category1) { Fabricate(:category) }
+  fab!(:c1_topic) { Fabricate(:topic, category: category1) }
+
+  fab!(:category2) { Fabricate(:category) }
+  fab!(:c2_topic) { Fabricate(:topic, category: category2) }
+  fab!(:category2_child) { Fabricate(:category, parent_category: category2) }
+  fab!(:c2_child_topic) { Fabricate(:topic, category: category2_child) }
+
+  fab!(:category3) { Fabricate(:category, default_list_filter: "none") }
+  fab!(:c3_topic) { Fabricate(:topic, category: category3) }
+  fab!(:category3_child) { Fabricate(:category, parent_category: category3) }
+  fab!(:c3_child_topic) { Fabricate(:topic, category: category3_child) }
+
+  it "can navigate between categories" do
+    visit("/c/#{category1.id}")
+
+    expect(page).to have_current_path("/c/#{category1.slug}/#{category1.id}")
+    expect(discovery.topic_list).to have_topic(c1_topic)
+    expect(discovery.topic_list).to have_topics(count: 1)
+
+    expect(discovery.category_drop).to have_selected_value(category1.id)
+    discovery.category_drop.select_row_by_value(category2.id)
+
+    expect(page).to have_current_path("/c/#{category2.slug}/#{category2.id}")
+    expect(discovery.topic_list).to have_topic(c2_topic)
+    expect(discovery.topic_list).to have_topic(c2_child_topic)
+    expect(discovery.topic_list).to have_topics(count: 2)
+
+    # When using breadcrumbs for navigation, default_list_filter does not apply
+    discovery.category_drop.select_row_by_value(category3.id)
+    expect(discovery.topic_list).to have_topic(c3_topic)
+    expect(discovery.topic_list).to have_topic(c3_child_topic)
+    expect(discovery.topic_list).to have_topics(count: 2)
+
+    expect(discovery.subcategory_drop).to have_selected_value("") # all
+
+    discovery.subcategory_drop.select_row_by_value("no-categories")
+    expect(discovery.topic_list).to have_topic(c3_topic)
+    expect(discovery.topic_list).to have_topics(count: 1)
+
+    discovery.subcategory_drop.select_row_by_value(category3_child.id)
+    expect(discovery.topic_list).to have_topic(c3_child_topic)
+    expect(discovery.topic_list).to have_topics(count: 1)
+  end
+
+  context "with tags" do
+    fab!(:tag) { Fabricate(:tag) }
+    fab!(:c1_topic_tagged) { Fabricate(:topic, category: category1, tags: [tag]) }
+    fab!(:c3_topic_tagged) { Fabricate(:topic, category: category3, tags: [tag]) }
+    fab!(:c3_child_topic_tagged) { Fabricate(:topic, category: category3_child, tags: [tag]) }
+
+    it "can filter by tags" do
+      visit("/c/#{category1.id}")
+      expect(page).to have_current_path("/c/#{category1.slug}/#{category1.id}")
+      expect(discovery.topic_list).to have_topic(c1_topic)
+      expect(discovery.topic_list).to have_topic(c1_topic_tagged)
+      expect(discovery.topic_list).to have_topics(count: 2)
+
+      expect(discovery.tag_drop).to have_selected_name("all tags")
+      discovery.tag_drop.select_row_by_value(tag.name)
+
+      expect(discovery.topic_list).to have_topics(count: 1)
+      expect(discovery.topic_list).to have_topic(c1_topic_tagged)
+    end
+
+    it "maintains no-subcategories option" do
+      visit("/c/#{category3.slug}/#{category3.id}/none")
+      expect(discovery.topic_list).to have_topic(c3_topic)
+      expect(discovery.topic_list).to have_topic(c3_topic_tagged)
+      expect(discovery.topic_list).to have_topics(count: 2)
+
+      expect(discovery.subcategory_drop).to have_selected_name("none")
+      expect(discovery.tag_drop).to have_selected_name("all tags")
+      discovery.tag_drop.select_row_by_value(tag.name)
+
+      expect(page).to have_current_path(
+        "/tags/c/#{category3.slug}/#{category3.id}/none/#{tag.name}",
+      )
+      expect(discovery.topic_list).to have_topics(count: 1)
+      expect(discovery.topic_list).to have_topic(c3_topic_tagged)
+    end
+  end
+
+  describe "initial pageloads for nosubcategories" do
+    it "shows correct data for /c/" do
+      visit("/c/#{category3.id}")
+      expect(page).to have_current_path("/c/#{category3.slug}/#{category3.id}/none")
+      expect(discovery.topic_list).to have_topic(c3_topic)
+      expect(discovery.topic_list).to have_topics(count: 1)
+    end
+
+    it "shows correct data for /tags/c/" do
+      tag = Fabricate(:tag)
+      c3_topic.update!(tags: [tag])
+      c3_child_topic.update!(tags: [tag])
+
+      visit("/tags/c/#{category3.slug}/#{category3.id}/#{tag.name}")
+      expect(page).to have_current_path(
+        "/tags/c/#{category3.slug}/#{category3.id}/none/#{tag.name}",
+      )
+      expect(discovery.topic_list).to have_topic(c3_topic)
+      expect(discovery.topic_list).to have_topics(count: 1)
+    end
+  end
+end

spec/system/page_objects/components/select_kit.rb

@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+
+module PageObjects
+  module Components
+    class SelectKit < PageObjects::Components::Base
+      attr_reader :element
+
+      def initialize(element)
+        @element = element
+      end
+
+      def is_expanded?
+        element.has_css?(".is-expanded")
+      end
+
+      def is_collapsed?
+        element.has_css?(":not(.is-expanded)")
+      end
+
+      def has_selected_value?(value)
+        element.find(".select-kit-header[data-value='#{value}']")
+      end
+
+      def has_selected_name?(value)
+        element.find(".select-kit-header[data-name='#{value}']")
+      end
+
+      def expand
+        element.find(":not(.is-expanded) .select-kit-header").click
+      end
+
+      def collapse
+        element.find(".is-expanded .select-kit-header").click
+      end
+
+      def select_row_by_value(value)
+        expand
+        element.find(".select-kit-row[data-value='#{value}']").click
+      end
+    end
+  end
+end

spec/system/page_objects/pages/discovery.rb

@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+module PageObjects
+  module Pages
+    class Discovery < PageObjects::Pages::Base
+      def topic_list
+        Components::TopicList.new
+      end
+
+      def category_drop
+        element = page.find(".category-breadcrumb li:first-of-type .category-drop")
+        Components::SelectKit.new(element)
+      end
+
+      def subcategory_drop
+        element = page.find(".category-breadcrumb li:nth-of-type(2) .category-drop")
+        Components::SelectKit.new(element)
+      end
+
+      def tag_drop
+        element = page.find(".category-breadcrumb .tag-drop")
+        Components::SelectKit.new(element)
+      end
+    end
+  end
+end