PERF: Paginate loading of tags in edit nav menu tags modal (#22380)
What is the problem? Before this change, we were relying on the `/tags` endpoint which returned all the tags that are visible to a give user on the site leading to potential performance problems. The attribute keys of the response also changes based on the `tags_listed_by_group` site setting. What is the fix? This commit fixes the problems listed above by creating a dedicate `#list` action in the `TagsController` to handle the listing of the tags in the edit navigation menu tags modal. This is because the `TagsController#index` action was created specifically for the `/tags` route and the response body does not really map well to what we need. The `TagsController#list` action added here is also much safer since the response is paginated and we avoid loading a whole bunch of tags upfront.
This commit is contained in:
Alan Guo Xiang Tan
GitHub
parent
6ae4d6cd4c
commit
82d6420e31
|
|
@ -0,0 +1,7 @@
|
|||
import RESTAdapter from "discourse/adapters/rest";
|
||||
|
||||
export default class extends RESTAdapter {
|
||||
pathFor(_store, _type, findArgs) {
|
||||
return this.appendQueryParams("/tags/list", findArgs);
|
||||
}
|
||||
}
|
||||
|
|
@ -1,4 +1,5 @@
|
|||
<Sidebar::EditNavigationMenu::Modal
|
||||
@class="sidebar__edit-navigation-menu__categories-modal"
|
||||
@title="sidebar.categories_form_modal.title"
|
||||
@disableSaveButton={{this.saving}}
|
||||
@save={{this.save}}
|
||||
|
|
|
|||
|
|
@ -1,5 +1,5 @@
|
|||
<DModal
|
||||
class="sidebar__edit-navigation-menu__modal"
|
||||
class={{concat-class "sidebar__edit-navigation-menu__modal" @class}}
|
||||
@title={{i18n @title}}
|
||||
@closeModal={{@closeModal}}
|
||||
>
|
||||
|
|
|
|||
|
|
@ -1,4 +1,5 @@
|
|||
<Sidebar::EditNavigationMenu::Modal
|
||||
@class="sidebar__edit-navigation-menu__tags-modal"
|
||||
@title="sidebar.tags_form_modal.title"
|
||||
@saving={{this.saving}}
|
||||
@save={{this.save}}
|
||||
|
|
@ -19,9 +20,13 @@
|
|||
{{#if this.tagsLoading}}
|
||||
<div class="spinner"></div>
|
||||
{{else}}
|
||||
{{#if (gt this.filteredTags.length 0)}}
|
||||
{{#each this.filteredTags as |tag|}}
|
||||
<div class="sidebar-tags-form__tag" data-tag-name={{tag.name}}>
|
||||
{{#if (gt this.tags.length 0)}}
|
||||
{{#each this.tags as |tag|}}
|
||||
<div
|
||||
class="sidebar-tags-form__tag"
|
||||
data-tag-name={{tag.name}}
|
||||
{{did-insert this.didInsertTag}}
|
||||
>
|
||||
<Input
|
||||
id={{concat "sidebar-tags-form__input--" tag.name}}
|
||||
class="sidebar-tags-form__input"
|
||||
|
|
@ -40,7 +45,7 @@
|
|||
</span>
|
||||
|
||||
<span class="sidebar-tags-form__tag-label-count">
|
||||
({{tag.count}})
|
||||
({{tag.topic_count}})
|
||||
</span>
|
||||
</p>
|
||||
</label>
|
||||
|
|
@ -53,4 +58,6 @@
|
|||
{{/if}}
|
||||
{{/if}}
|
||||
</form>
|
||||
|
||||
<ConditionalLoadingSpinner @condition={{this.tags.loadingMore}} />
|
||||
</Sidebar::EditNavigationMenu::Modal>
|
||||
|
|
@ -25,74 +25,80 @@ export default class extends Component {
|
|||
}
|
||||
|
||||
async #loadTags() {
|
||||
// This is loading all tags upfront and there is no pagination for it. However, this is what we are doing for the
|
||||
// `/tags` route as well so we have decided to kick this can of worms down the road for now.
|
||||
this.tagsLoading = true;
|
||||
|
||||
const findArgs = {};
|
||||
|
||||
if (this.filter !== "") {
|
||||
findArgs.filter = this.filter;
|
||||
}
|
||||
|
||||
if (this.onlySelected) {
|
||||
findArgs.only_tags = this.selectedTags.join(",");
|
||||
}
|
||||
|
||||
if (this.onlyUnselected) {
|
||||
findArgs.exclude_tags = this.selectedTags.join(",");
|
||||
}
|
||||
|
||||
await this.store
|
||||
.findAll("tag")
|
||||
.then((result) => {
|
||||
const tags = [...result.content];
|
||||
|
||||
if (result.extras) {
|
||||
if (result.extras.tag_groups) {
|
||||
result.extras.tag_groups.forEach((tagGroup) => {
|
||||
tagGroup.tags.forEach((tag) => {
|
||||
tags.push(tag);
|
||||
});
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
this.tags = tags.sort((a, b) => {
|
||||
return a.name.localeCompare(b.name);
|
||||
});
|
||||
|
||||
.findAll("listTag", findArgs)
|
||||
.then((tags) => {
|
||||
this.tagsLoading = false;
|
||||
this.tags = tags;
|
||||
})
|
||||
.catch((error) => {
|
||||
popupAjaxError(error);
|
||||
});
|
||||
}
|
||||
|
||||
get filteredTags() {
|
||||
return this.tags.reduce((acc, tag) => {
|
||||
if (this.onlySelected) {
|
||||
if (this.selectedTags.includes(tag.name) && this.#matchesFilter(tag)) {
|
||||
acc.push(tag);
|
||||
}
|
||||
} else if (this.onlyUnselected) {
|
||||
if (!this.selectedTags.includes(tag.name) && this.#matchesFilter(tag)) {
|
||||
acc.push(tag);
|
||||
}
|
||||
} else if (this.#matchesFilter(tag)) {
|
||||
acc.push(tag);
|
||||
@action
|
||||
didInsertTag(element) {
|
||||
const tagName = element.dataset.tagName;
|
||||
const lastTagName = this.tags.content[this.tags.content.length - 1].name;
|
||||
|
||||
if (tagName === lastTagName) {
|
||||
if (this.observer) {
|
||||
this.observer.disconnect();
|
||||
} else {
|
||||
this.observer = new IntersectionObserver(
|
||||
(entries) => {
|
||||
entries.forEach((entry) => {
|
||||
if (entry.isIntersecting) {
|
||||
this.tags.loadMore();
|
||||
}
|
||||
});
|
||||
},
|
||||
{
|
||||
root: document.querySelector(".modal-body"),
|
||||
threshold: 1.0,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
return acc;
|
||||
}, []);
|
||||
}
|
||||
|
||||
#matchesFilter(tag) {
|
||||
return (
|
||||
this.filter.length === 0 || tag.name.toLowerCase().includes(this.filter)
|
||||
);
|
||||
this.observer.observe(element);
|
||||
}
|
||||
}
|
||||
|
||||
@action
|
||||
resetFilter() {
|
||||
this.onlySelected = false;
|
||||
this.onlyUnselected = false;
|
||||
this.#loadTags();
|
||||
}
|
||||
|
||||
@action
|
||||
filterSelected() {
|
||||
this.onlySelected = true;
|
||||
this.onlyUnselected = false;
|
||||
this.#loadTags();
|
||||
}
|
||||
|
||||
@action
|
||||
filterUnselected() {
|
||||
this.onlySelected = false;
|
||||
this.onlyUnselected = true;
|
||||
this.#loadTags();
|
||||
}
|
||||
|
||||
@action
|
||||
|
|
@ -102,6 +108,7 @@ export default class extends Component {
|
|||
|
||||
#performFiltering(filter) {
|
||||
this.filter = filter.toLowerCase();
|
||||
this.#loadTags();
|
||||
}
|
||||
|
||||
@action
|
||||
|
|
|
|||
|
|
@ -104,16 +104,6 @@ acceptance("Sidebar - Logged on user - Tags section", function (needs) {
|
|||
);
|
||||
});
|
||||
|
||||
test("clicking on section header button", async function (assert) {
|
||||
await visit("/");
|
||||
|
||||
await click(
|
||||
".sidebar-section[data-section-name='tags'] .sidebar-section-header-button"
|
||||
);
|
||||
|
||||
assert.true(exists(".sidebar-tags-form"), "it shows the tags form modal");
|
||||
});
|
||||
|
||||
test("tags section is displayed with site's top tags when user has not added any tags and there are no default tags configured", async function (assert) {
|
||||
updateCurrentUser({
|
||||
sidebar_tags: [],
|
||||
|
|
|
|||
|
|
@ -1,6 +1,6 @@
|
|||
.sidebar__edit-navigation-menu__modal {
|
||||
.modal-body {
|
||||
min-height: 50vh;
|
||||
min-height: 25vh;
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
.sidebar-tags-form-modal {
|
||||
.sidebar__edit-navigation-menu__tags-modal {
|
||||
.modal-inner-container {
|
||||
min-width: var(--modal-max-width);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
.sidebar-tags-form-modal {
|
||||
.sidebar__edit-navigation-menu__tags-modal {
|
||||
.modal-inner-container {
|
||||
width: 35em;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -27,6 +27,7 @@ class TagsController < ::ApplicationController
|
|||
update_notifications
|
||||
personal_messages
|
||||
info
|
||||
list
|
||||
]
|
||||
|
||||
before_action :fetch_tag, only: %i[info create_synonyms destroy_synonym]
|
||||
|
|
@ -99,6 +100,46 @@ class TagsController < ::ApplicationController
|
|||
end
|
||||
end
|
||||
|
||||
LIST_LIMIT = 51
|
||||
|
||||
def list
|
||||
offset = params[:offset].to_i || 0
|
||||
tags = guardian.can_admin_tags? ? Tag.all : Tag.used_tags_in_regular_topics(guardian)
|
||||
|
||||
load_more_query_params = { offset: offset + 1 }
|
||||
|
||||
if filter = params[:filter]
|
||||
tags = tags.where("LOWER(tags.name) ILIKE ?", "%#{filter.downcase}%")
|
||||
load_more_query_params[:filter] = filter
|
||||
end
|
||||
|
||||
if only_tags = params[:only_tags]
|
||||
tags = tags.where("LOWER(tags.name) IN (?)", only_tags.split(",").map(&:downcase))
|
||||
load_more_query_params[:only_tags] = only_tags
|
||||
end
|
||||
|
||||
if exclude_tags = params[:exclude_tags]
|
||||
tags = tags.where("LOWER(tags.name) NOT IN (?)", exclude_tags.split(",").map(&:downcase))
|
||||
load_more_query_params[:exclude_tags] = exclude_tags
|
||||
end
|
||||
|
||||
tags_count = tags.count
|
||||
tags = tags.order("LOWER(tags.name) ASC").limit(LIST_LIMIT).offset(offset * LIST_LIMIT)
|
||||
|
||||
load_more_url = URI("/tags/list.json")
|
||||
load_more_url.query = URI.encode_www_form(load_more_query_params)
|
||||
|
||||
render_serialized(
|
||||
tags,
|
||||
TagSerializer,
|
||||
root: "list_tags",
|
||||
meta: {
|
||||
total_rows_list_tags: tags_count,
|
||||
load_more_list_tags: load_more_url.to_s,
|
||||
},
|
||||
)
|
||||
end
|
||||
|
||||
Discourse.filters.each do |filter|
|
||||
define_method("show_#{filter}") do
|
||||
@tag_id = params[:tag_id].force_encoding("UTF-8")
|
||||
|
|
|
|||
|
|
@ -1480,6 +1480,7 @@ Discourse::Application.routes.draw do
|
|||
get "/" => "tags#index"
|
||||
get "/filter/list" => "tags#index"
|
||||
get "/filter/search" => "tags#search"
|
||||
get "/list" => "tags#list"
|
||||
get "/personal_messages/:username" => "tags#personal_messages",
|
||||
:constraints => {
|
||||
username: RouteFormat.username,
|
||||
|
|
|
|||
|
|
@ -1441,4 +1441,148 @@ RSpec.describe TagsController do
|
|||
expect(tag_user.notification_level).to eq(NotificationLevels.all[:muted])
|
||||
end
|
||||
end
|
||||
|
||||
describe "#list" do
|
||||
fab!(:tag3) do
|
||||
Fabricate(:tag, name: "tag3").tap { |tag| Fabricate.times(1, :topic, tags: [tag]) }
|
||||
end
|
||||
|
||||
fab!(:tag2) do
|
||||
Fabricate(:tag, name: "tag2").tap { |tag| Fabricate.times(1, :topic, tags: [tag]) }
|
||||
end
|
||||
|
||||
fab!(:tag1) do
|
||||
Fabricate(:tag, name: "tag").tap { |tag| Fabricate.times(1, :topic, tags: [tag]) }
|
||||
end
|
||||
|
||||
fab!(:tag_not_used_in_topics) { Fabricate(:tag, name: "tag4") }
|
||||
|
||||
it "should return 403 for an anonymous user" do
|
||||
get "/tags/list.json"
|
||||
|
||||
expect(response.status).to eq(403)
|
||||
end
|
||||
|
||||
it "should return 404 when tagging is disabled" do
|
||||
SiteSetting.tagging_enabled = false
|
||||
|
||||
sign_in(user)
|
||||
|
||||
get "/tags/list.json"
|
||||
|
||||
expect(response.status).to eq(404)
|
||||
end
|
||||
|
||||
it "should only return tags used in topics for non admin users" do
|
||||
stub_const(TagsController, "LIST_LIMIT", 2) do
|
||||
sign_in(user)
|
||||
|
||||
get "/tags/list.json"
|
||||
|
||||
expect(response.status).to eq(200)
|
||||
|
||||
expect(response.parsed_body["list_tags"].map { |tag| tag["name"] }).to eq(
|
||||
[tag1.name, tag2.name],
|
||||
)
|
||||
|
||||
expect(response.parsed_body["meta"]["total_rows_list_tags"]).to eq(3)
|
||||
expect(response.parsed_body["meta"]["load_more_list_tags"]).to eq(
|
||||
"/tags/list.json?offset=1",
|
||||
)
|
||||
|
||||
get response.parsed_body["meta"]["load_more_list_tags"]
|
||||
|
||||
expect(response.status).to eq(200)
|
||||
expect(response.parsed_body["list_tags"].map { |tag| tag["name"] }).to eq([tag3.name])
|
||||
expect(response.parsed_body["meta"]["total_rows_list_tags"]).to eq(3)
|
||||
|
||||
expect(response.parsed_body["meta"]["load_more_list_tags"]).to eq(
|
||||
"/tags/list.json?offset=2",
|
||||
)
|
||||
end
|
||||
end
|
||||
|
||||
it "should return all tags for admin users" do
|
||||
stub_const(TagsController, "LIST_LIMIT", 2) do
|
||||
sign_in(admin)
|
||||
|
||||
get "/tags/list.json"
|
||||
|
||||
expect(response.status).to eq(200)
|
||||
|
||||
expect(response.parsed_body["list_tags"].map { |tag| tag["name"] }).to eq(
|
||||
[tag1.name, tag2.name],
|
||||
)
|
||||
|
||||
expect(response.parsed_body["meta"]["total_rows_list_tags"]).to eq(4)
|
||||
|
||||
expect(response.parsed_body["meta"]["load_more_list_tags"]).to eq(
|
||||
"/tags/list.json?offset=1",
|
||||
)
|
||||
|
||||
get response.parsed_body["meta"]["load_more_list_tags"]
|
||||
|
||||
expect(response.status).to eq(200)
|
||||
|
||||
expect(response.parsed_body["list_tags"].map { |tag| tag["name"] }).to eq(
|
||||
[tag3.name, tag_not_used_in_topics.name],
|
||||
)
|
||||
|
||||
expect(response.parsed_body["meta"]["total_rows_list_tags"]).to eq(4)
|
||||
|
||||
expect(response.parsed_body["meta"]["load_more_list_tags"]).to eq(
|
||||
"/tags/list.json?offset=2",
|
||||
)
|
||||
end
|
||||
end
|
||||
|
||||
it "accepts a `filter` param and filters the tags by tag name" do
|
||||
sign_in(user)
|
||||
|
||||
get "/tags/list.json", params: { filter: "3" }
|
||||
|
||||
expect(response.status).to eq(200)
|
||||
|
||||
expect(response.parsed_body["list_tags"].map { |tag| tag["name"] }).to eq([tag3.name])
|
||||
expect(response.parsed_body["meta"]["total_rows_list_tags"]).to eq(1)
|
||||
|
||||
expect(response.parsed_body["meta"]["load_more_list_tags"]).to eq(
|
||||
"/tags/list.json?offset=1&filter=3",
|
||||
)
|
||||
end
|
||||
|
||||
it "accepts a `only_tags` param and filters the tags by the given tags" do
|
||||
sign_in(user)
|
||||
|
||||
get "/tags/list.json", params: { only_tags: "#{tag1.name},#{tag3.name}" }
|
||||
|
||||
expect(response.status).to eq(200)
|
||||
|
||||
expect(response.parsed_body["list_tags"].map { |tag| tag["name"] }).to eq(
|
||||
[tag1.name, tag3.name],
|
||||
)
|
||||
|
||||
expect(response.parsed_body["meta"]["total_rows_list_tags"]).to eq(2)
|
||||
|
||||
expect(response.parsed_body["meta"]["load_more_list_tags"]).to eq(
|
||||
"/tags/list.json?offset=1&only_tags=#{tag1.name}%2C#{tag3.name}",
|
||||
)
|
||||
end
|
||||
|
||||
it "accepts a `exclude_tags` params and filters tags excluding the given tags" do
|
||||
sign_in(user)
|
||||
|
||||
get "/tags/list.json", params: { exclude_tags: "#{tag1.name},#{tag3.name}" }
|
||||
|
||||
expect(response.status).to eq(200)
|
||||
|
||||
expect(response.parsed_body["list_tags"].map { |tag| tag["name"] }).to eq([tag2.name])
|
||||
|
||||
expect(response.parsed_body["meta"]["total_rows_list_tags"]).to eq(1)
|
||||
|
||||
expect(response.parsed_body["meta"]["load_more_list_tags"]).to eq(
|
||||
"/tags/list.json?offset=1&exclude_tags=#{tag1.name}%2C#{tag3.name}",
|
||||
)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
|||
|
|
@ -88,16 +88,6 @@ RSpec.describe "Editing sidebar tags navigation", type: :system do
|
|||
include_examples "a user can edit the sidebar tags navigation", true
|
||||
end
|
||||
|
||||
it "displays the all tags in the modal when `tags_listed_by_group` site setting is true" do
|
||||
SiteSetting.tags_listed_by_group = true
|
||||
|
||||
visit "/latest"
|
||||
|
||||
modal = sidebar.click_edit_tags_button
|
||||
|
||||
expect(modal).to have_tag_checkboxes([tag1, tag2, tag3, tag4])
|
||||
end
|
||||
|
||||
it "allows a user to filter the tags in the modal by the tag's name" do
|
||||
visit "/latest"
|
||||
|
||||
|
|
@ -186,4 +176,16 @@ RSpec.describe "Editing sidebar tags navigation", type: :system do
|
|||
|
||||
expect(modal).to have_tag_checkboxes([tag1, tag2, tag3, tag4])
|
||||
end
|
||||
|
||||
it "loads more tags when the user scrolls views the last tag in the modal and there is more tags to load" do
|
||||
stub_const(TagsController, "LIST_LIMIT", 2) do
|
||||
visit "/latest"
|
||||
|
||||
expect(sidebar).to have_tags_section
|
||||
|
||||
modal = sidebar.click_edit_tags_button
|
||||
|
||||
expect(modal).to have_tag_checkboxes([tag1, tag2, tag3, tag4])
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
|||
Loading…
Reference in New Issue