From 13083d03ae0eda2edc24b210f1c80fc79302d5e7 Mon Sep 17 00:00:00 2001 From: Daniel Waterworth Date: Tue, 20 Feb 2024 11:24:30 -0600 Subject: [PATCH] DEV: Async category search for sidebar modal (#25686) --- .../edit-navigation-menu/categories-modal.gjs | 186 ++++++++++-------- .../discourse/app/models/category.js | 18 +- .../sidebar-user-categories-section-test.js | 4 + app/controllers/categories_controller.rb | 19 +- app/jobs/base.rb | 8 + app/models/category.rb | 17 ++ app/services/search_indexer.rb | 8 + spec/models/category_spec.rb | 27 +++ spec/requests/categories_controller_spec.rb | 24 +++ ...ting_sidebar_categories_navigation_spec.rb | 47 ++++- 10 files changed, 266 insertions(+), 92 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/sidebar/edit-navigation-menu/categories-modal.gjs b/app/assets/javascripts/discourse/app/components/sidebar/edit-navigation-menu/categories-modal.gjs index 44e4f2cf706..d582b0b62e3 100644 --- a/app/assets/javascripts/discourse/app/components/sidebar/edit-navigation-menu/categories-modal.gjs +++ b/app/assets/javascripts/discourse/app/components/sidebar/edit-navigation-menu/categories-modal.gjs @@ -9,6 +9,7 @@ import EditNavigationMenuModal from "discourse/components/sidebar/edit-navigatio import borderColor from "discourse/helpers/border-color"; import categoryBadge from "discourse/helpers/category-badge"; import dirSpan from "discourse/helpers/dir-span"; +import loadingSpinner from "discourse/helpers/loading-spinner"; import { popupAjaxError } from "discourse/lib/ajax-error"; import Category from "discourse/models/category"; import { INPUT_DELAY } from "discourse-common/config/environment"; @@ -18,126 +19,145 @@ import gt from "truth-helpers/helpers/gt"; import includes from "truth-helpers/helpers/includes"; import not from "truth-helpers/helpers/not"; +// Given a list, break into chunks starting a new chunk whenever the predicate +// is true for an element. +function splitWhere(elements, f) { + return elements.reduce((acc, el, i) => { + if (i === 0 || f(el)) { + acc.push([]); + } + acc[acc.length - 1].push(el); + return acc; + }, []); +} + +function findAncestors(categories) { + let categoriesToCheck = categories; + const ancestors = []; + + for (let i = 0; i < 3; i++) { + categoriesToCheck = categoriesToCheck + .map((c) => Category.findById(c.parent_category_id)) + .filter(Boolean) + .uniqBy((c) => c.id); + + ancestors.push(...categoriesToCheck); + } + + return ancestors; +} + export default class extends Component { @service currentUser; @service site; @service siteSettings; - @tracked filter = ""; - @tracked filteredCategoryIds; - @tracked onlySelected = false; - @tracked onlyUnselected = false; + @tracked initialLoad = true; + @tracked filteredCategoriesGroupings = []; + @tracked filteredCategoryIds = []; @tracked selectedSidebarCategoryIds = [...this.currentUser.sidebar_category_ids]; - categoryGroupings = []; - constructor() { super(...arguments); - let categories = [...this.site.categories]; - - if (!this.siteSettings.fixed_category_positions) { - categories.sort((a, b) => a.name.localeCompare(b.name)); - } - - Category.sortCategories(categories).reduce( - (categoryGrouping, category, index, arr) => { - if (category.isUncategorizedCategory) { - return categoryGrouping; - } - - categoryGrouping.push(category); - - const nextCategory = arr[index + 1]; - - if (!nextCategory || nextCategory.level === 0) { - this.categoryGroupings.push(categoryGrouping); - return []; - } - - return categoryGrouping; - }, - [] - ); + this.processing = false; + this.setFilterAndMode("", "everything"); } - get filteredCategoriesGroupings() { - const filteredCategoryIds = new Set(); + setFilteredCategories(categories) { + const ancestors = findAncestors(categories); + const allCategories = categories.concat(ancestors).uniqBy((c) => c.id); - const groupings = this.categoryGroupings.reduce((acc, categoryGrouping) => { - const filteredCategories = new Set(); + if (this.siteSettings.fixed_category_positions) { + allCategories.sort((a, b) => a.position - b.position); + } else { + allCategories.sort((a, b) => a.name.localeCompare(b.name)); + } - const addCategory = (category) => { - if (this.#matchesFilter(category)) { - if (category.parentCategory?.parentCategory) { - filteredCategories.add(category.parentCategory.parentCategory); - } + this.filteredCategoriesGroupings = splitWhere( + Category.sortCategories(allCategories), + (category) => category.parent_category_id === undefined + ); - if (category.parentCategory) { - filteredCategories.add(category.parentCategory); - } + this.filteredCategoryIds = categories.map((c) => c.id); + } - filteredCategoryIds.add(category.id); - filteredCategories.add(category); - } - }; + async searchCategories(filter, mode) { + if (filter === "" && mode === "only-selected") { + this.setFilteredCategories( + await Category.asyncFindByIds(this.selectedSidebarCategoryIds) + ); + } else { + const { categories } = await Category.asyncSearch(filter, { + includeAncestors: true, + includeUncategorized: false, + }); - categoryGrouping.forEach((category) => { - if (this.onlySelected) { - if (this.selectedSidebarCategoryIds.includes(category.id)) { - addCategory(category); - } - } else if (this.onlyUnselected) { - if (!this.selectedSidebarCategoryIds.includes(category.id)) { - addCategory(category); - } - } else { - addCategory(category); + const filteredFetchedCategories = categories.filter((c) => { + switch (mode) { + case "everything": + return true; + case "only-selected": + return this.selectedSidebarCategoryIds.includes(c.id); + case "only-unselected": + return !this.selectedSidebarCategoryIds.includes(c.id); } }); - if (filteredCategories.size > 0) { - acc.push(Array.from(filteredCategories)); - } - - return acc; - }, []); - - this.filteredCategoryIds = Array.from(filteredCategoryIds); - return groupings; + this.setFilteredCategories(filteredFetchedCategories); + } } - #matchesFilter(category) { - return this.filter.length === 0 || category.nameLower.includes(this.filter); + async setFilterAndMode(newFilter, newMode) { + this.filter = newFilter; + this.mode = newMode; + + if (!this.processing) { + this.processing = true; + + try { + while (true) { + const filter = this.filter; + const mode = this.mode; + + await this.searchCategories(filter, mode); + + this.initialLoad = false; + + if (filter === this.filter && mode === this.mode) { + break; + } + } + } finally { + this.processing = false; + } + } + } + + debouncedSetFilterAndMode(filter, mode) { + discourseDebounce(this, this.setFilterAndMode, filter, mode, INPUT_DELAY); } @action resetFilter() { - this.onlySelected = false; - this.onlyUnselected = false; + this.debouncedSetFilterAndMode(this.filter, "everything"); } @action filterSelected() { - this.onlySelected = true; - this.onlyUnselected = false; + this.debouncedSetFilterAndMode(this.filter, "only-selected"); } @action filterUnselected() { - this.onlySelected = false; - this.onlyUnselected = true; + this.debouncedSetFilterAndMode(this.filter, "only-unselected"); } @action onFilterInput(filter) { - discourseDebounce(this, this.#performFiltering, filter, INPUT_DELAY); - } - - #performFiltering(filter) { - this.filter = filter.toLowerCase(); + this.debouncedSetFilterAndMode(filter.toLowerCase().trim(), this.mode); } @action @@ -209,7 +229,11 @@ export default class extends Component { class="sidebar__edit-navigation-menu__categories-modal" >
- {{#if (gt this.filteredCategoriesGroupings.length 0)}} + {{#if this.initialLoad}} +
+ {{loadingSpinner size="small"}} +
+ {{else if (gt this.filteredCategoriesGroupings.length 0)}} {{#each this.filteredCategoriesGroupings as |categories|}}
- Site.current().updateCategory(category) - ); + if (opts.includeAncestors) { + return { + ancestors: result["ancestors"].map((category) => + Site.current().updateCategory(category) + ), + categories: result["categories"].map((category) => + Site.current().updateCategory(category) + ), + }; + } else { + return result["categories"].map((category) => + Site.current().updateCategory(category) + ); + } } permissions = null; diff --git a/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-categories-section-test.js b/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-categories-section-test.js index 3c0acdef4c5..13b734a5dbd 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-categories-section-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-categories-section-test.js @@ -90,6 +90,10 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) { server.get("/c/:categorySlug/:categoryId/find_by_slug.json", () => { return helper.response(cloneJSON(categoryFixture["/c/1/show.json"])); }); + + server.get("/categories/search", () => { + return helper.response({ categories: [], ancestors: [] }); + }); }); const setupUserSidebarCategories = function () { diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb index d143ff8e6d9..bada2b9bd7c 100644 --- a/app/controllers/categories_controller.rb +++ b/app/controllers/categories_controller.rb @@ -341,6 +341,12 @@ class CategoriesController < ApplicationController else true end + include_ancestors = + if params[:include_ancestors].present? + ActiveModel::Type::Boolean.new.cast(params[:include_ancestors]) + else + false + end prioritized_category_id = params[:prioritized_category_id].to_i if params[ :prioritized_category_id ].present? @@ -388,7 +394,18 @@ class CategoriesController < ApplicationController Category.preload_user_fields!(guardian, categories) - render_serialized(categories, SiteCategorySerializer, root: :categories, scope: guardian) + if include_ancestors + ancestors = Category.secured(guardian).ancestors_of(categories.map(&:id)) + + render_json_dump( + { + categories: serialize_data(categories, SiteCategorySerializer, scope: guardian), + ancestors: serialize_data(ancestors, SiteCategorySerializer, scope: guardian), + }, + ) + else + render_serialized(categories, SiteCategorySerializer, root: :categories, scope: guardian) + end end private diff --git a/app/jobs/base.rb b/app/jobs/base.rb index 64add443a22..730b2210000 100644 --- a/app/jobs/base.rb +++ b/app/jobs/base.rb @@ -21,6 +21,14 @@ module Jobs @run_immediately = false end + def self.with_immediate_jobs + prior = @run_immediately + run_immediately! + yield + ensure + @run_immediately = prior + end + def self.last_job_performed_at Sidekiq.redis do |r| int = r.get("last_job_perform_at") diff --git a/app/models/category.rb b/app/models/category.rb index 8e58c11954d..b7e71fc0834 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -257,6 +257,23 @@ class Category < ActiveRecord::Base end end + def self.ancestors_of(category_ids) + ancestor_ids = [] + + SiteSetting.max_category_nesting.times do + category_ids = + where(id: category_ids) + .where.not(parent_category_id: nil) + .pluck("DISTINCT parent_category_id") + + ancestor_ids.concat(category_ids) + + break if category_ids.empty? + end + + where(id: ancestor_ids) + end + def self.topic_id_cache @topic_id_cache ||= DistributedCache.new("category_topic_ids") end diff --git a/app/services/search_indexer.rb b/app/services/search_indexer.rb index 75ae8e99821..1b0aa62e749 100644 --- a/app/services/search_indexer.rb +++ b/app/services/search_indexer.rb @@ -21,6 +21,14 @@ class SearchIndexer @disabled = false end + def self.with_indexing + prior = @disabled + enable + yield + ensure + @disabled = prior + end + def self.update_index(table:, id:, a_weight: nil, b_weight: nil, c_weight: nil, d_weight: nil) raw_data = { a: a_weight, b: b_weight, c: c_weight, d: d_weight } diff --git a/spec/models/category_spec.rb b/spec/models/category_spec.rb index f5618987473..f32ea81afa9 100644 --- a/spec/models/category_spec.rb +++ b/spec/models/category_spec.rb @@ -1483,4 +1483,31 @@ RSpec.describe Category do end end end + + describe ".ancestors_of" do + fab!(:category) + fab!(:subcategory) { Fabricate(:category, parent_category: category) } + + fab!(:sub_subcategory) do + SiteSetting.max_category_nesting = 3 + Fabricate(:category, parent_category: subcategory) + end + + it "finds the parent" do + expect(Category.ancestors_of([subcategory.id]).to_a).to eq([category]) + end + + it "finds the grandparent" do + expect(Category.ancestors_of([sub_subcategory.id]).to_a).to contain_exactly( + category, + subcategory, + ) + end + + it "respects the relation it's called on" do + expect(Category.where.not(id: category.id).ancestors_of([sub_subcategory.id]).to_a).to eq( + [subcategory], + ) + end + end end diff --git a/spec/requests/categories_controller_spec.rb b/spec/requests/categories_controller_spec.rb index 4ddfa8a4c5b..52d3091effa 100644 --- a/spec/requests/categories_controller_spec.rb +++ b/spec/requests/categories_controller_spec.rb @@ -1127,6 +1127,30 @@ RSpec.describe CategoriesController do [category, category2, subcategory].each { |c| SearchIndexer.index(c, force: true) } end + context "without include_ancestors" do + it "doesn't return ancestors" do + get "/categories/search.json", params: { term: "Notfoo" } + + expect(response.parsed_body).not_to have_key("ancestors") + end + end + + context "with include_ancestors=false" do + it "returns ancestors" do + get "/categories/search.json", params: { term: "Notfoo", include_ancestors: false } + + expect(response.parsed_body).not_to have_key("ancestors") + end + end + + context "with include_ancestors=true" do + it "returns ancestors" do + get "/categories/search.json", params: { term: "Notfoo", include_ancestors: true } + + expect(response.parsed_body).to have_key("ancestors") + end + end + context "with term" do it "returns categories" do get "/categories/search.json", params: { term: "Foo" } diff --git a/spec/system/editing_sidebar_categories_navigation_spec.rb b/spec/system/editing_sidebar_categories_navigation_spec.rb index b7ae3bd0b98..2c0d46f6322 100644 --- a/spec/system/editing_sidebar_categories_navigation_spec.rb +++ b/spec/system/editing_sidebar_categories_navigation_spec.rb @@ -3,10 +3,10 @@ RSpec.describe "Editing sidebar categories navigation", type: :system do fab!(:user) - fab!(:category2) { Fabricate(:category, name: "category2") } + fab!(:category2) { Fabricate(:category, name: "category 2") } fab!(:category2_subcategory) do - Fabricate(:category, parent_category_id: category2.id, name: "category2 subcategory") + Fabricate(:category, parent_category_id: category2.id, name: "category 2 subcategory") end fab!(:category) { Fabricate(:category, name: "category") } @@ -21,6 +21,19 @@ RSpec.describe "Editing sidebar categories navigation", type: :system do let(:sidebar) { PageObjects::Components::NavigationMenu::Sidebar.new } + before_all do + Jobs.with_immediate_jobs do + SearchIndexer.with_indexing do + category2.index_search + category2_subcategory.index_search + + category.index_search + category_subcategory2.index_search + category_subcategory.index_search + end + end + end + before { sign_in(user) } shared_examples "a user can edit the sidebar categories navigation" do |mobile| @@ -149,9 +162,11 @@ RSpec.describe "Editing sidebar categories navigation", type: :system do modal = sidebar.click_edit_categories_button - modal.filter("category subcategory 2") + modal.filter("subcategory") - expect(modal).to have_categories([category, category_subcategory2]) + expect(modal).to have_categories( + [category, category_subcategory, category_subcategory2, category2, category2_subcategory], + ) modal.filter("2") @@ -233,10 +248,20 @@ RSpec.describe "Editing sidebar categories navigation", type: :system do Fabricate( :category, parent_category_id: category2_subcategory.id, - name: "category2 subcategory subcategory", + name: "category 2 subcategory subcategory", ) end + before_all do + Jobs.with_immediate_jobs do + SearchIndexer.with_indexing do + category_subcategory_subcategory.index_search + category_subcategory_subcategory2.index_search + category2_subcategory_subcategory.index_search + end + end + end + it "allows a user to edit sub-subcategories to be included in the sidebar categories section" do visit "/latest" @@ -265,10 +290,18 @@ RSpec.describe "Editing sidebar categories navigation", type: :system do expect(sidebar).to have_categories_section modal = sidebar.click_edit_categories_button - modal.filter("category2 subcategory subcategory") + modal.filter("category 2 subcategory subcategory") expect(modal).to have_categories( - [category2, category2_subcategory, category2_subcategory_subcategory], + [ + category, + category_subcategory, + category_subcategory_subcategory2, + category_subcategory2, + category2, + category2_subcategory, + category2_subcategory_subcategory, + ], ) end end