DEV: Remove experimental support for query string on /filter route

Why is this changed being made?

Prior to this change, we were relying on the client to ship a `q` query
param which represents a topics filtering query string that would be
parsed on the server to generate the corresponding topics scope.
However, we decided to revert this change because we lost the ability to
keep our URL "nice". By nice, we mean no encodings in the query params.

What does this commit do?

With this commit, we no longer ship a query string as the `q` query
params to the server. Instead, we parse the input given by the user on
the client side and converts it to the relevant query params. As an
example:

An input value of `status:closed tag:todo` will automatically set
`?status=closed&tag=todo` in the query params.

app/assets/javascripts/discourse/app/components/d-navigation.hbs

@@ -3,11 +3,11 @@
     <Input
       class="topic-query-filter__input"
       @value={{this.queryString}}
-      @enter={{route-action "changeQueryString" this.queryString}}
+      @enter={{action @updateTopicsListQueryParams this.queryString}}
     />
 
     <DButton
-      @action={{route-action "changeQueryString" this.queryString}}
+      @action={{action @updateTopicsListQueryParams this.queryString}}
       @icon="filter"
       @class="btn-primary topic-query-filter__button"
       @label="filters.filter.button.label"

app/assets/javascripts/discourse/app/components/d-navigation.js

@@ -6,12 +6,18 @@ import { NotificationLevels } from "discourse/lib/notification-levels";
 import { getOwner } from "discourse-common/lib/get-owner";
 import { htmlSafe } from "@ember/template";
 import { inject as service } from "@ember/service";
-import { alias, equal } from "@ember/object/computed";
+import { equal } from "@ember/object/computed";
 
 export default Component.extend(FilterModeMixin, {
   router: service(),
   dialog: service(),
   tagName: "",
+  queryString: "",
+
+  init() {
+    this._super(...arguments);
+    this.queryString = this.filterQueryString;
+  },
 
   // Should be a `readOnly` instead but some themes/plugins still pass
   // the `categories` property into this component
@@ -142,7 +148,6 @@ export default Component.extend(FilterModeMixin, {
   },
 
   isQueryFilterMode: equal("filterMode", "filter"),
-  queryString: alias("router.currentRoute.queryParams.q"),
 
   actions: {
     changeCategoryNotificationLevel(notificationLevel) {

app/assets/javascripts/discourse/app/controllers/discovery-sortable.js

@@ -1,4 +1,6 @@
 import Controller, { inject as controller } from "@ember/controller";
+import discourseComputed from "discourse-common/utils/decorators";
+import { action } from "@ember/object";
 
 // Just add query params here to have them automatically passed to topic list filters.
 export const queryParams = {
@@ -27,6 +29,31 @@ export const queryParams = {
 const controllerOpts = {
   discoveryTopics: controller("discovery/topics"),
   queryParams: Object.keys(queryParams),
+
+  @discourseComputed(...Object.keys(queryParams))
+  queryString() {
+    let paramStrings = [];
+
+    this.queryParams.forEach((key) => {
+      if (this[key]) {
+        paramStrings.push(`${key}:${this[key]}`);
+      }
+    });
+
+    return paramStrings.join(" ");
+  },
+
+  @action
+  updateTopicsListQueryParams(queryString) {
+    for (const match of queryString.matchAll(/(\w+):([^:\s]+)/g)) {
+      const key = match[1];
+      const value = match[2];
+
+      if (controllerOpts.queryParams.includes(key)) {
+        this.set(key, value);
+      }
+    }
+  },
 };
 
 // Default to `undefined`
@@ -34,10 +61,6 @@ controllerOpts.queryParams.forEach((p) => {
   controllerOpts[p] = queryParams[p].default;
 });
 
-export function changeQueryString(queryString) {
-  this.controller.set("q", queryString);
-}
-
 export function changeSort(sortBy) {
   let model = this.controllerFor("discovery.topics").model;
 

app/assets/javascripts/discourse/app/controllers/navigation/default.js

@@ -6,6 +6,7 @@ import { TRACKED_QUERY_PARAM_VALUE } from "discourse/lib/topic-list-tracked-filt
 
 export default Controller.extend(FilterModeMixin, {
   discovery: controller(),
+  discoveryFilter: controller("discovery.filter"),
   router: service(),
 
   @discourseComputed("router.currentRoute.queryParams.f")

app/assets/javascripts/discourse/app/pre-initializers/dynamic-route-builders.js

@@ -10,17 +10,13 @@ export default {
   after: "inject-discourse-objects",
   name: "dynamic-route-builders",
 
-  initialize(container, app) {
-    const siteSettings = container.lookup("service:site-settings");
+  initialize(_container, app) {
+    app.register(
+      "controller:discovery.filter",
+      DiscoverySortableController.extend()
+    );
 
-    if (siteSettings.experimental_topics_filter) {
-      app.register(
-        "controller:discovery.filter",
-        DiscoverySortableController.extend()
-      );
-
-      app.register("route:discovery.filter", buildTopicRoute("filter"));
-    }
+    app.register("route:discovery.filter", buildTopicRoute("filter"));
 
     app.register(
       "controller:discovery.category",

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

@@ -1,5 +1,4 @@
 import {
-  changeQueryString,
   changeSort,
   queryParams,
   resetParams,
@@ -168,11 +167,6 @@ export default function (filter, extras) {
         changeSort.call(this, sortBy);
       },
 
-      @action
-      changeQueryString(queryString) {
-        changeQueryString.call(this, queryString);
-      },
-
       @action
       resetParams(skipParams = []) {
         resetParams.call(this, skipParams);

app/assets/javascripts/discourse/app/templates/navigation/default.hbs

@@ -9,5 +9,7 @@
     @hasDraft={{this.currentUser.has_topic_draft}}
     @createTopic={{route-action "createTopic"}}
     @skipCategoriesNavItem={{this.skipCategoriesNavItem}}
+    @filterQueryString={{this.discoveryFilter.queryString}}
+    @updateTopicsListQueryParams={{this.discoveryFilter.updateTopicsListQueryParams}}
   />
 </DSection>

lib/topic_query.rb

@@ -838,17 +838,12 @@ class TopicQuery
     end
 
     if status = options[:status]
-      options[:q] ||= +""
-      options[:q] << " status:#{status}"
-    end
-
-    if options[:q].present?
       result =
         TopicsFilter.new(
           scope: result,
           guardian: @guardian,
           category_id: options[:category],
-        ).filter(options[:q])
+        ).filter(status: options[:status])
     end
 
     if (filter = (options[:filter] || options[:f])) && @user

lib/topics_filter.rb

@@ -1,51 +1,31 @@
 # frozen_string_literal: true
 
 class TopicsFilter
-  def self.register_filter(matcher, &block)
-    self.filters[matcher] = block
-  end
-
-  def self.filters
-    @@filters ||= {}
-  end
-
-  register_filter(/\Astatus:([a-zA-Z]+)\z/i) do |topics, match|
-    case match
-    when "open"
-      topics.where("NOT topics.closed AND NOT topics.archived")
-    when "closed"
-      topics.where("topics.closed")
-    when "archived"
-      topics.where("topics.archived")
-    when "deleted"
-      if @guardian.can_see_deleted_topics?(@category)
-        topics.unscope(where: :deleted_at).where("topics.deleted_at IS NOT NULL")
-      end
-    end
-  end
-
   def initialize(guardian:, scope: Topic, category_id: nil)
     @guardian = guardian
     @scope = scope
     @category = category_id.present? ? Category.find_by(id: category_id) : nil
   end
 
-  def filter(input)
-    input
-      .to_s
-      .scan(/(([^" \t\n\x0B\f\r]+)?(("[^"]+")?))/)
-      .to_a
-      .map do |(word, _)|
-        next if word.blank?
-
-        self.class.filters.each do |matcher, block|
-          cleaned = word.gsub(/["']/, "")
-
-          new_scope = instance_exec(@scope, $1, &block) if cleaned =~ matcher
-          @scope = new_scope if !new_scope.nil?
-        end
-      end
-
+  def filter(status: nil)
+    filter_status(@scope, status) if status
     @scope
   end
+
+  private
+
+  def filter_status(scope, status)
+    case status
+    when "open"
+      @scope = @scope.where("NOT topics.closed AND NOT topics.archived")
+    when "closed"
+      @scope = @scope.where("topics.closed")
+    when "archived"
+      @scope = @scope.where("topics.archived")
+    when "deleted"
+      if @guardian.can_see_deleted_topics?(@category)
+        @scope = @scope.unscope(where: :deleted_at).where("topics.deleted_at IS NOT NULL")
+      end
+    end
+  end
 end

spec/lib/topics_filter_spec.rb

@@ -9,41 +9,37 @@ RSpec.describe TopicsFilter do
 
   describe "#filter" do
     it "should return all topics when input is blank" do
-      expect(TopicsFilter.new(guardian: Guardian.new).filter("").pluck(:id)).to contain_exactly(
+      expect(TopicsFilter.new(guardian: Guardian.new).filter.pluck(:id)).to contain_exactly(
         topic.id,
         closed_topic.id,
         archived_topic.id,
       )
     end
 
-    it "should return all topics when input does not match any filters" do
-      expect(
-        TopicsFilter.new(guardian: Guardian.new).filter("randomstring").pluck(:id),
-      ).to contain_exactly(topic.id, closed_topic.id, archived_topic.id)
-    end
+    context "when filtering by topic's status" do
+      it "should only return topics that have not been closed or archived when input is `status:open`" do
+        expect(
+          TopicsFilter.new(guardian: Guardian.new).filter(status: "open").pluck(:id),
+        ).to contain_exactly(topic.id)
+      end
 
-    it "should only return topics that have not been closed or archived when input is `status:open`" do
-      expect(
-        TopicsFilter.new(guardian: Guardian.new).filter("status:open").pluck(:id),
-      ).to contain_exactly(topic.id)
-    end
+      it "should only return topics that have been deleted when input is `status:deleted` and user can see deleted topics" do
+        expect(
+          TopicsFilter.new(guardian: Guardian.new(admin)).filter(status: "deleted").pluck(:id),
+        ).to contain_exactly(deleted_topic_id)
+      end
 
-    it "should only return topics that have been deleted when input is `status:deleted` and user can see deleted topics" do
-      expect(
-        TopicsFilter.new(guardian: Guardian.new(admin)).filter("status:deleted").pluck(:id),
-      ).to contain_exactly(deleted_topic_id)
-    end
+      it "should status filter when input is `status:deleted` and user cannot see deleted topics" do
+        expect(
+          TopicsFilter.new(guardian: Guardian.new).filter(status: "deleted").pluck(:id),
+        ).to contain_exactly(topic.id, closed_topic.id, archived_topic.id)
+      end
 
-    it "should status filter when input is `status:deleted` and user cannot see deleted topics" do
-      expect(
-        TopicsFilter.new(guardian: Guardian.new).filter("status:deleted").pluck(:id),
-      ).to contain_exactly(topic.id, closed_topic.id, archived_topic.id)
-    end
-
-    it "should only return topics that have been archived when input is `status:archived`" do
-      expect(
-        TopicsFilter.new(guardian: Guardian.new).filter("status:archived").pluck(:id),
-      ).to contain_exactly(archived_topic.id)
+      it "should only return topics that have been archived when input is `status:archived`" do
+        expect(
+          TopicsFilter.new(guardian: Guardian.new).filter(status: "archived").pluck(:id),
+        ).to contain_exactly(archived_topic.id)
+      end
     end
   end
 end

spec/system/filtering_topics_spec.rb

@@ -8,7 +8,7 @@ describe "Filtering topics", type: :system, js: true do
 
   before { SiteSetting.experimental_topics_filter = true }
 
-  it "should allow users to enter a custom query string to filter through topics" do
+  it "should allow users to input a custom query string to filter through topics" do
     sign_in(user)
 
     visit("/filter")
@@ -21,19 +21,19 @@ describe "Filtering topics", type: :system, js: true do
 
     expect(topic_list).to have_topic(topic)
     expect(topic_list).to have_no_topic(closed_topic)
-    expect(page).to have_current_path("/filter?q=status%3Aopen")
+    expect(page).to have_current_path("/filter?status=open")
 
     topic_query_filter.fill_in("status:closed")
 
     expect(topic_list).to have_no_topic(topic)
     expect(topic_list).to have_topic(closed_topic)
-    expect(page).to have_current_path("/filter?q=status%3Aclosed")
+    expect(page).to have_current_path("/filter?status=closed")
   end
 
-  it "should filter topics when 'q' query params is present" do
+  it "should filter topics when 'status' query params is present" do
     sign_in(user)
 
-    visit("/filter?q=status:open")
+    visit("/filter?status=open")
 
     expect(topic_list).to have_topic(topic)
     expect(topic_list).to have_no_topic(closed_topic)