From 14fd9cb23fac543da4924bbb2b81ebc8b6e208b7 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Fri, 10 Mar 2023 12:48:49 +0800 Subject: [PATCH] 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. --- .../discourse/app/components/d-navigation.hbs | 4 +- .../discourse/app/components/d-navigation.js | 9 ++- .../app/controllers/discovery-sortable.js | 31 ++++++++-- .../app/controllers/navigation/default.js | 1 + .../dynamic-route-builders.js | 16 ++--- .../discourse/app/routes/build-topic-route.js | 6 -- .../app/templates/navigation/default.hbs | 2 + lib/topic_query.rb | 7 +-- lib/topics_filter.rb | 58 ++++++------------- spec/lib/topics_filter_spec.rb | 48 +++++++-------- spec/system/filtering_topics_spec.rb | 10 ++-- 11 files changed, 92 insertions(+), 100 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/d-navigation.hbs b/app/assets/javascripts/discourse/app/components/d-navigation.hbs index 04092dc19e..2b7badf209 100644 --- a/app/assets/javascripts/discourse/app/components/d-navigation.hbs +++ b/app/assets/javascripts/discourse/app/components/d-navigation.hbs @@ -3,11 +3,11 @@ { + 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; diff --git a/app/assets/javascripts/discourse/app/controllers/navigation/default.js b/app/assets/javascripts/discourse/app/controllers/navigation/default.js index 2e46e7abb1..18e62a9370 100644 --- a/app/assets/javascripts/discourse/app/controllers/navigation/default.js +++ b/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") diff --git a/app/assets/javascripts/discourse/app/pre-initializers/dynamic-route-builders.js b/app/assets/javascripts/discourse/app/pre-initializers/dynamic-route-builders.js index 9c11c4c424..99ea7796dd 100644 --- a/app/assets/javascripts/discourse/app/pre-initializers/dynamic-route-builders.js +++ b/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", diff --git a/app/assets/javascripts/discourse/app/routes/build-topic-route.js b/app/assets/javascripts/discourse/app/routes/build-topic-route.js index 0e3b344023..feea4a2a09 100644 --- a/app/assets/javascripts/discourse/app/routes/build-topic-route.js +++ b/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); diff --git a/app/assets/javascripts/discourse/app/templates/navigation/default.hbs b/app/assets/javascripts/discourse/app/templates/navigation/default.hbs index 9848aadf5f..cfa4ee3536 100644 --- a/app/assets/javascripts/discourse/app/templates/navigation/default.hbs +++ b/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}} /> \ No newline at end of file diff --git a/lib/topic_query.rb b/lib/topic_query.rb index fe5dc2ff50..479868df69 100644 --- a/lib/topic_query.rb +++ b/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 diff --git a/lib/topics_filter.rb b/lib/topics_filter.rb index af80382e12..e15d707e15 100644 --- a/lib/topics_filter.rb +++ b/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 diff --git a/spec/lib/topics_filter_spec.rb b/spec/lib/topics_filter_spec.rb index 778bbbb9fc..13b3cfd666 100644 --- a/spec/lib/topics_filter_spec.rb +++ b/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 diff --git a/spec/system/filtering_topics_spec.rb b/spec/system/filtering_topics_spec.rb index fcc8513a41..81f305fd32 100644 --- a/spec/system/filtering_topics_spec.rb +++ b/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)