From 8963e185f85783f162c8fd2e187e0e7d316de149 Mon Sep 17 00:00:00 2001 From: ktmouk Date: Thu, 26 Oct 2023 04:53:53 +0900 Subject: [PATCH] Fix username combobox tries to load all users (#20227) * Fix username combobox tries to load all users * Add the limit param to the users_query * Add a comment --- app/controllers/admin/users_controller.rb | 2 + ...convertCoauthorIdsToUsernameInputs.test.js | 4 + .../MultiSelectAutocomplete.jsx | 22 ++- .../MultiSelectAutocomplete.test.jsx | 4 + .../admin/convertUserIdsToUsernameInputs.js | 166 ++++++++++-------- .../convertCoauthorIdsToUsernameInputs.js | 68 ++++--- app/javascript/shared/components/UserStore.js | 35 +--- .../components/__tests__/UserStore.test.js | 29 +-- .../__tests__/UsernameInput.test.js | 4 + app/queries/admin/users_query.rb | 13 +- .../admin/articles/_article_item.html.erb | 4 +- .../admin/settings/forms/_community.html.erb | 2 +- .../admin/settings/forms/_mascot.html.erb | 2 +- app/views/admin/tags/edit.html.erb | 2 +- spec/queries/admin/users_query_spec.rb | 43 ++++- 15 files changed, 239 insertions(+), 161 deletions(-) diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index e08759037..df317fedf 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -344,6 +344,8 @@ module Admin @users = Admin::UsersQuery.call( relation: User.registered, search: params[:search], + ids: params[:ids], + limit: params[:limit], ) render json: @users.to_json(only: %i[id name username]) diff --git a/app/javascript/__tests__/convertCoauthorIdsToUsernameInputs.test.js b/app/javascript/__tests__/convertCoauthorIdsToUsernameInputs.test.js index f3545a367..63f9dab8a 100644 --- a/app/javascript/__tests__/convertCoauthorIdsToUsernameInputs.test.js +++ b/app/javascript/__tests__/convertCoauthorIdsToUsernameInputs.test.js @@ -6,6 +6,10 @@ import { convertCoauthorIdsToUsernameInputs } from '../packs/dashboards/convertC global.fetch = fetch; +jest.mock('@utilities/debounceAction', () => ({ + debounceAction: fn => fn +})); + function fakeFetchResponseJSON() { return JSON.stringify([ { name: 'Alice', username: 'alice', id: 1 }, diff --git a/app/javascript/crayons/MultiSelectAutocomplete/MultiSelectAutocomplete.jsx b/app/javascript/crayons/MultiSelectAutocomplete/MultiSelectAutocomplete.jsx index 959b813f2..7a9db2f5b 100644 --- a/app/javascript/crayons/MultiSelectAutocomplete/MultiSelectAutocomplete.jsx +++ b/app/javascript/crayons/MultiSelectAutocomplete/MultiSelectAutocomplete.jsx @@ -4,6 +4,7 @@ import { h, Fragment } from 'preact'; import PropTypes from 'prop-types'; import { useEffect, useRef, useReducer } from 'preact/hooks'; import { DefaultSelectionTemplate } from '../../shared/components/defaultSelectionTemplate'; +import { debounceAction } from '@utilities/debounceAction'; const KEYS = { UP: 'ArrowUp', @@ -295,15 +296,7 @@ export const MultiSelectAutocomplete = ({ } }; - const handleInputChange = async ({ target: { value } }) => { - // When the input appears inline in "edit" mode, we need to dynamically calculate the width to ensure it occupies the right space - // (an input cannot resize based on its text content). We use a hidden to track the size. - inputSizerRef.current.innerText = value; - - if (inputPosition !== null) { - resizeInputToContentSize(); - } - + const updateSuggestion = debounceAction(async (value) => { // If max selections have already been reached, no need to fetch further suggestions if (!allowSelections) { return; @@ -343,6 +336,17 @@ export const MultiSelectAutocomplete = ({ ), ), }); + }) + + const handleInputChange = async ({ target: { value } }) => { + // When the input appears inline in "edit" mode, we need to dynamically calculate the width to ensure it occupies the right space + // (an input cannot resize based on its text content). We use a hidden to track the size. + inputSizerRef.current.innerText = value; + + if (inputPosition !== null) { + resizeInputToContentSize(); + } + await updateSuggestion(value) }; const clearInput = () => { diff --git a/app/javascript/crayons/MultiSelectAutocomplete/__tests__/MultiSelectAutocomplete.test.jsx b/app/javascript/crayons/MultiSelectAutocomplete/__tests__/MultiSelectAutocomplete.test.jsx index 7f74e90db..e3c5a3c9a 100644 --- a/app/javascript/crayons/MultiSelectAutocomplete/__tests__/MultiSelectAutocomplete.test.jsx +++ b/app/javascript/crayons/MultiSelectAutocomplete/__tests__/MultiSelectAutocomplete.test.jsx @@ -6,6 +6,10 @@ import '@testing-library/jest-dom'; import { MultiSelectAutocomplete } from '../MultiSelectAutocomplete'; +jest.mock('@utilities/debounceAction', () => ({ + debounceAction: (fn) => fn, +})); + describe('', () => { it('renders default UI', () => { const { container } = render( diff --git a/app/javascript/packs/admin/convertUserIdsToUsernameInputs.js b/app/javascript/packs/admin/convertUserIdsToUsernameInputs.js index 9003d7cc4..83c34d58f 100644 --- a/app/javascript/packs/admin/convertUserIdsToUsernameInputs.js +++ b/app/javascript/packs/admin/convertUserIdsToUsernameInputs.js @@ -1,108 +1,128 @@ import { h, render } from 'preact'; -import { UsernameInput } from '../../shared/components/UsernameInput'; -import { UserStore } from '../../shared/components/UserStore'; +import { UsernameInput } from '@components/UsernameInput'; +import { UserStore } from '@components/UserStore'; import '@utilities/document_ready'; -async function convertUserIdFieldToUsernameField(targetField, users, fetchUrl) { +const CO_AUTHOR_FIELD_CLASS_NAME = '.js-coauthor_username_id_input'; +const USERNAME_FIELD_CLASS_NAME = '.js-username_id_input'; + +function generateFetchUrl(ids) { + const searchParams = new URLSearchParams(); + ids.forEach((id) => searchParams.append('ids[]', Number(id))); + return `/admin/member_manager/users.json?${searchParams}'`; +} + +function extractUserIds(value) { + return value + .split(',') + .map((id) => id.replace(' ', '')) + .filter((id) => id !== ''); +} + +function getAllUserIdsFromTextFields(fieldClassNames) { + const uniqueIds = new Set(); + + fieldClassNames.forEach((className) => { + document.querySelectorAll(className).forEach((field) => { + extractUserIds(field.value).forEach((id) => uniqueIds.add(id)); + }); + }); + return [...uniqueIds]; +} + +async function fetchUsers(ids) { + if (ids.length <= 0) { + return new UserStore(); + } + return await UserStore.fetch(generateFetchUrl(ids)); +} + +const fetchSuggestions = async (term, searchOptions = {}) => { + const searchParams = new URLSearchParams(); + searchParams.append('limit', 10); + searchParams.append('search', term); + const userStore = await UserStore.fetch( + `/admin/member_manager/users.json?${searchParams}`, + ); + return userStore.search(term, searchOptions); +}; + +async function convertUserIdFieldToUsernameField(targetField, users) { targetField.type = 'hidden'; const inputId = `auto${targetField.id}`; const newDiv = document.createElement('div'); targetField.parentElement.append(newDiv); - await users.fetch(fetchUrl).then(() => { - const value = users.matchingIds(targetField.value.split(',')); + const value = users.matchingIds(extractUserIds(targetField.value)); - const fetchSuggestions = function (term) { - return users.search(term); - }; + const handleSelectionsChanged = function (ids) { + targetField.value = ids; + }; - const handleSelectionsChanged = function (ids) { - targetField.value = ids; - }; - - render( - , - newDiv, - ); - }); + render( + , + newDiv, + ); } -async function convertCoAuthorIdsToUsernameInputs( - targetField, - users, - fetchUrl, -) { +async function convertCoAuthorIdsToUsernameInputs(targetField, users) { targetField.type = 'hidden'; const exceptAuthorId = targetField.form.querySelector( 'input[name="article[user_id]"]', )?.value; - let searchOptions = {}; - if (exceptAuthorId) { - searchOptions = { except: exceptAuthorId }; - } - + const searchOptions = exceptAuthorId ? { except: exceptAuthorId } : {}; const inputId = `auto${targetField.id}`; const newDiv = document.createElement('div'); targetField.parentElement.append(newDiv); - await users.fetch(fetchUrl).then(() => { - const value = users.matchingIds(targetField.value.split(',')); + const value = users.matchingIds(extractUserIds(targetField.value)); - const fetchSuggestions = function (term) { - return users.search(term, searchOptions); - }; + const handleSelectionsChanged = function (ids) { + targetField.value = ids; + }; - const handleSelectionsChanged = function (ids) { - targetField.value = ids; - }; - - render( - , - newDiv, - ); - }); + render( + fetchSuggestions(term, searchOptions)} + handleSelectionsChanged={handleSelectionsChanged} + />, + newDiv, + ); } -export async function convertCoauthorIdsToUsernameInputs() { - const users = new UserStore(); - const fetchUrl = '/admin/member_manager/users.json'; - - const usernameFields = document.getElementsByClassName( - 'js-username_id_input', - ); - +export async function convertCoauthorIdsToUsernameInputs(users) { + const usernameFields = document.querySelectorAll(USERNAME_FIELD_CLASS_NAME); for (const targetField of usernameFields) { - convertUserIdFieldToUsernameField(targetField, users, fetchUrl); + convertUserIdFieldToUsernameField(targetField, users); } - const coAuthorFields = document.getElementsByClassName( - 'js-coauthor_username_id_input', - ); - + const coAuthorFields = document.querySelectorAll(CO_AUTHOR_FIELD_CLASS_NAME); for (const coAuthorField of coAuthorFields) { - convertCoAuthorIdsToUsernameInputs(coAuthorField, users, fetchUrl); + convertCoAuthorIdsToUsernameInputs(coAuthorField, users); } } -document.ready.then(() => { - convertCoauthorIdsToUsernameInputs(); +document.ready.then(async () => { + const ids = getAllUserIdsFromTextFields([ + CO_AUTHOR_FIELD_CLASS_NAME, + USERNAME_FIELD_CLASS_NAME, + ]); + const users = await fetchUsers(ids); + convertCoauthorIdsToUsernameInputs(users); }); diff --git a/app/javascript/packs/dashboards/convertCoauthorIdsToUsernameInputs.js b/app/javascript/packs/dashboards/convertCoauthorIdsToUsernameInputs.js index 173725d4c..22bab7418 100644 --- a/app/javascript/packs/dashboards/convertCoauthorIdsToUsernameInputs.js +++ b/app/javascript/packs/dashboards/convertCoauthorIdsToUsernameInputs.js @@ -1,46 +1,64 @@ import { h, render } from 'preact'; -import { UsernameInput } from '../../shared/components/UsernameInput'; -import { UserStore } from '../../shared/components/UserStore'; +import { UsernameInput } from '@components/UsernameInput'; +import { UserStore } from '@components/UserStore'; import '@utilities/document_ready'; +async function fetchAllUsers(fetchUrls) { + const users = await Promise.all( + fetchUrls.map((fetchUrl) => + UserStore.fetch(fetchUrl).then((data) => [fetchUrl, data]), + ), + ); + return new Map(users); +} + +function extractFetchUrl(field) { + return field.dataset.fetchUsers; +} + +function extractFetchUrls(fields) { + const urls = [...fields].map((field) => extractFetchUrl(field)); + return [...new Set(urls)]; +} + export async function convertCoauthorIdsToUsernameInputs() { const usernameFields = document.getElementsByClassName( 'article_org_co_author_ids_list', ); - const users = new UserStore(); + const fetchUrls = extractFetchUrls(usernameFields); + const usersMap = await fetchAllUsers(fetchUrls); for (const targetField of usernameFields) { targetField.type = 'hidden'; const exceptAuthorId = targetField.form.querySelector('#article_user_id').value; const inputId = `auto${targetField.id}`; - const fetchUrl = targetField.dataset.fetchUsers; + + const users = usersMap.get(extractFetchUrl(targetField)); const row = targetField.parentElement; - await users.fetch(fetchUrl).then(() => { - const value = users.matchingIds(targetField.value.split(',')); - const fetchSuggestions = function (term) { - return users.search(term, { except: exceptAuthorId }); - }; + const value = users.matchingIds(targetField.value.split(',')); + const fetchSuggestions = function (term) { + return users.search(term, { except: exceptAuthorId }); + }; - const handleSelectionsChanged = function (ids) { - targetField.value = ids; - }; + const handleSelectionsChanged = function (ids) { + targetField.value = ids; + }; - render( - , - row, - ); - }); + render( + , + row, + ); } } diff --git a/app/javascript/shared/components/UserStore.js b/app/javascript/shared/components/UserStore.js index 088fe30a3..aa2cd6fa2 100644 --- a/app/javascript/shared/components/UserStore.js +++ b/app/javascript/shared/components/UserStore.js @@ -1,32 +1,15 @@ export class UserStore { - constructor() { - this.users = []; - this.wasFetched = false; + constructor(users = []) { + this.users = users; } - fetch(url) { - const myStore = this; - return new Promise((resolve, _reject) => { - if (myStore.wasFetched) { - resolve(); - } else { - window - .fetch(url) - .then((res) => res.json()) - .then((data) => { - myStore.users = data.reduce((array, aUser) => { - array.push(aUser); - return array; - }, []); - myStore.wasFetched = true; - resolve(); - }) - .catch((error) => { - Honeybadger.notify(error); - resolve(); - }); - } - }); + static async fetch(url) { + try { + const res = await window.fetch(url); + return new UserStore(await res.json()); + } catch (error) { + Honeybadger.notify(error); + } } matchingIds(arrayOfIds) { diff --git a/app/javascript/shared/components/__tests__/UserStore.test.js b/app/javascript/shared/components/__tests__/UserStore.test.js index a71cada7c..29f9dff46 100644 --- a/app/javascript/shared/components/__tests__/UserStore.test.js +++ b/app/javascript/shared/components/__tests__/UserStore.test.js @@ -23,28 +23,19 @@ describe('UserStore', () => { expect(subject.users).toStrictEqual([]); }); - test('initializes unfetched', () => { - const subject = new UserStore(); - expect(subject.wasFetched).toBeFalsy(); + test('initializes with a user list', () => { + const users = [{ name: 'Bob', username: 'bob', id: 2 }]; + const subject = new UserStore(users); + expect(subject.users).toStrictEqual(users); }); - test('fetch from a given url', () => { - new UserStore().fetch('/path/to/the/users'); + test('fetch from a given url', async () => { + await UserStore.fetch('/path/to/the/users'); expect(fetch).toHaveBeenCalledWith('/path/to/the/users'); }); - test('only fetches once', async () => { - const subject = new UserStore(); - await subject.fetch('/path/to/the/users'); - expect(subject.wasFetched).toBeTruthy(); - await subject.fetch('/path/to/the/users'); - await subject.fetch('/path/to/the/users'); - expect(fetch).toHaveBeenCalledTimes(1); - }); - test('return a sub-set of users matching given IDs', async () => { - const subject = new UserStore(); - await subject.fetch('/path/to/the/users'); + const subject = await UserStore.fetch('/path/to/the/users'); expect(subject.matchingIds(['1', '4'])).toStrictEqual([ { name: 'Alice', username: 'alice', id: 1 }, { name: 'Almost Alice', username: 'almostalice', id: 4 }, @@ -56,8 +47,7 @@ describe('UserStore', () => { }); test('return a sub-set of users matching search term', async () => { - const subject = new UserStore(); - await subject.fetch('/path/to/the/users'); + const subject = await UserStore.fetch('/path/to/the/users'); expect(subject.search('alice')).toStrictEqual([ { name: 'Alice', username: 'alice', id: 1 }, { name: 'Almost Alice', username: 'almostalice', id: 4 }, @@ -69,8 +59,7 @@ describe('UserStore', () => { }); test('search with an exception', async () => { - const subject = new UserStore(); - await subject.fetch('/path/to/the/users'); + const subject = await UserStore.fetch('/path/to/the/users'); expect(subject.search('a', { except: '3' })).toStrictEqual([ { name: 'Alice', username: 'alice', id: 1 }, { name: 'Almost Alice', username: 'almostalice', id: 4 }, diff --git a/app/javascript/shared/components/__tests__/UsernameInput.test.js b/app/javascript/shared/components/__tests__/UsernameInput.test.js index 7bb5c5e09..122a94338 100644 --- a/app/javascript/shared/components/__tests__/UsernameInput.test.js +++ b/app/javascript/shared/components/__tests__/UsernameInput.test.js @@ -6,6 +6,10 @@ import '@testing-library/jest-dom'; import { UsernameInput } from '../UsernameInput'; +jest.mock('@utilities/debounceAction', () => ({ + debounceAction: fn => fn +})); + function fakeUsers() { return [ { name: 'Alice', username: 'alice', id: 1 }, diff --git a/app/queries/admin/users_query.rb b/app/queries/admin/users_query.rb index 063b987d2..92d2e7452 100644 --- a/app/queries/admin/users_query.rb +++ b/app/queries/admin/users_query.rb @@ -21,19 +21,23 @@ module Admin # @param search [String, nil] # @param roles [Array, nil] # @param statuses [Array, nil] + # @param ids [Array, nil] # @param organizations [Array, nil] # @param joining_start [String, nil] # @param joining_end [String, nil] # @param date_format [String] + # @param limit [Integer, nil] def self.call(relation: User.registered, role: nil, search: nil, roles: [], organizations: [], statuses: [], + ids: [], joining_start: nil, joining_end: nil, - date_format: "DD/MM/YYYY") + date_format: "DD/MM/YYYY", + limit: nil) # We are at an interstitial moment where we are exposing both the role and roles param. We # need to favor one or the other. if role.presence @@ -53,6 +57,9 @@ module Admin end relation = search_relation(relation, search) if search.presence + relation = filter_ids(relation, ids) if ids.presence + relation = relation.limit(limit.to_i) if limit.to_i.positive? + relation.distinct.order(created_at: :desc) end @@ -60,6 +67,10 @@ module Admin relation.where(SEARCH_CLAUSE, search: "%#{search.strip}%") end + def self.filter_ids(relation, ids) + relation.where(id: ids) + end + def self.filter_joining_date(relation:, joining_start:, joining_end:, date_format:) ui_formats_to_parse_format = { "DD/MM/YYYY" => "%d/%m/%Y", diff --git a/app/views/admin/articles/_article_item.html.erb b/app/views/admin/articles/_article_item.html.erb index 7c10dd9e6..43510a308 100644 --- a/app/views/admin/articles/_article_item.html.erb +++ b/app/views/admin/articles/_article_item.html.erb @@ -153,13 +153,13 @@
-
- ">
diff --git a/app/views/admin/settings/forms/_community.html.erb b/app/views/admin/settings/forms/_community.html.erb index 7d6913d98..c796ab7e5 100644 --- a/app/views/admin/settings/forms/_community.html.erb +++ b/app/views/admin/settings/forms/_community.html.erb @@ -54,7 +54,7 @@ <%= admin_config_label :staff_user_id, model: Settings::Community %> <%= admin_config_description Constants::Settings::Community.details[:staff_user_id][:description] %> <%= f.text_field :staff_user_id, - class: "crayons-textfield ", + class: "crayons-textfield js-username_id_input", value: Settings::Community.staff_user_id, placeholder: Constants::Settings::Community.details[:staff_user_id][:placeholder] %> diff --git a/app/views/admin/settings/forms/_mascot.html.erb b/app/views/admin/settings/forms/_mascot.html.erb index bbd7ae977..95afaea9b 100644 --- a/app/views/admin/settings/forms/_mascot.html.erb +++ b/app/views/admin/settings/forms/_mascot.html.erb @@ -12,7 +12,7 @@ <%= admin_config_label :mascot_user_id, "Mascot user" %> <%= admin_config_description Constants::Settings::General.details[:mascot_user_id][:description] %> <%= f.text_field :mascot_user_id, - class: "crayons-textfield ", + class: "crayons-textfield js-username_id_input", value: Settings::General.mascot_user_id, min: 1, placeholder: Constants::Settings::General.details[:mascot_user_id][:placeholder] %> diff --git a/app/views/admin/tags/edit.html.erb b/app/views/admin/tags/edit.html.erb index ced9eaa5f..4f9230e61 100644 --- a/app/views/admin/tags/edit.html.erb +++ b/app/views/admin/tags/edit.html.erb @@ -17,7 +17,7 @@
<%= f.label :user_id, "Add Moderator:", class: "crayons-field__label" %>
- <%= f.text_field :user_id, class: "crayons-textfield ", placeholder: "User ID (i.e. 789)" %> + <%= f.text_field :user_id, class: "crayons-textfield js-username_id_input" %>
<%= f.submit "Add Moderator", class: "crayons-btn" %>
diff --git a/spec/queries/admin/users_query_spec.rb b/spec/queries/admin/users_query_spec.rb index 5ac3be5c5..a92ed7463 100644 --- a/spec/queries/admin/users_query_spec.rb +++ b/spec/queries/admin/users_query_spec.rb @@ -8,6 +8,8 @@ RSpec.describe Admin::UsersQuery, type: :query do let(:search) { [] } let(:joining_start) { nil } let(:joining_end) { nil } + let(:ids) { [] } + let(:limit) { nil } describe ".find" do let!(:user1) { create(:user, username: "user1") } @@ -56,7 +58,7 @@ RSpec.describe Admin::UsersQuery, type: :query do subject do described_class.call(search: search, role: role, roles: roles, organizations: organizations, joining_start: joining_start, joining_end: joining_end, date_format: date_format, - statuses: statuses) + statuses: statuses, ids: ids, limit: limit) end let(:date_format) { "DD/MM/YYYY" } @@ -78,10 +80,11 @@ RSpec.describe Admin::UsersQuery, type: :query do let!(:user8) { create(:user, :comment_suspended, name: "Bob", registered_at: "2020-10-08T13:09:47+0000") } let!(:user9) { create(:user, name: "Lucia", registered_at: "2020-10-08T13:09:47+0000") } let!(:user10) { create(:user, :warned, name: "Billie", registered_at: "2020-10-08T13:09:47+0000") } + let!(:all_users) { [user10, user9, user8, user7, user6, user5, user4, user3, user2, user] } context "when no arguments are given" do it "returns all users" do - expect(described_class.call).to eq([user10, user9, user8, user7, user6, user5, user4, user3, user2, user]) + expect(described_class.call).to eq(all_users) end end @@ -198,5 +201,41 @@ RSpec.describe Admin::UsersQuery, type: :query do it { is_expected.to eq([user3, user2, user]) } end + + context "when given ids" do + let(:ids) { [user2.id, user3.id, user4.id] } + + it { is_expected.to eq([user4, user3, user2]) } + end + + context "when ids contains an invalid user_id" do + let(:ids) { [0, "foo"] } + + it { is_expected.to eq([]) } + end + + context "when limit is grater than zero" do + let(:limit) { 2 } + + it { is_expected.to eq([user10, user9]) } + end + + context "when limit is zero" do + let(:limit) { 0 } + + it { expect(described_class.call).to eq(all_users) } + end + + context "when limit is not a number" do + let(:limit) { "foo" } + + it { expect(described_class.call).to eq(all_users) } + end + + context "when limit is nil" do + let(:limit) { nil } + + it { expect(described_class.call).to eq(all_users) } + end end end