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

@@ -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])

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 },

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 <span> 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 <span> to track the size.
+    inputSizerRef.current.innerText = value;
+
+    if (inputPosition !== null) {
+      resizeInputToContentSize();
+    }
+    await updateSuggestion(value)
   };
 
   const clearInput = () => {

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('<MultiSelectAutocomplete />', () => {
   it('renders default UI', () => {
     const { container } = render(

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(
-      <UsernameInput
-        labelText="Enter a username"
-        placeholder="Enter a username"
-        maxSelections={1}
-        inputId={inputId}
-        defaultValue={value}
-        fetchSuggestions={fetchSuggestions}
-        handleSelectionsChanged={handleSelectionsChanged}
-      />,
-      newDiv,
-    );
-  });
+  render(
+    <UsernameInput
+      labelText="Enter a username"
+      placeholder="Enter a username"
+      maxSelections={1}
+      inputId={inputId}
+      defaultValue={value}
+      fetchSuggestions={fetchSuggestions}
+      handleSelectionsChanged={handleSelectionsChanged}
+    />,
+    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(
-      <UsernameInput
-        labelText="Add up to 4"
-        placeholder="Add up to 4..."
-        maxSelections={4}
-        inputId={inputId}
-        defaultValue={value}
-        fetchSuggestions={fetchSuggestions}
-        handleSelectionsChanged={handleSelectionsChanged}
-      />,
-      newDiv,
-    );
-  });
+  render(
+    <UsernameInput
+      labelText="Add up to 4"
+      placeholder="Add up to 4..."
+      maxSelections={4}
+      inputId={inputId}
+      defaultValue={value}
+      fetchSuggestions={(term) => 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);
 });

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(
-        <UsernameInput
-          labelText="Add up to 4"
-          placeholder="Add up to 4..."
-          maxSelections={4}
-          inputId={inputId}
-          defaultValue={value}
-          fetchSuggestions={fetchSuggestions}
-          handleSelectionsChanged={handleSelectionsChanged}
-        />,
-        row,
-      );
-    });
+    render(
+      <UsernameInput
+        labelText="Add up to 4"
+        placeholder="Add up to 4..."
+        maxSelections={4}
+        inputId={inputId}
+        defaultValue={value}
+        fetchSuggestions={fetchSuggestions}
+        handleSelectionsChanged={handleSelectionsChanged}
+      />,
+      row,
+    );
   }
 }
 

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) {

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 },

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 },

app/queries/admin/users_query.rb

@@ -21,19 +21,23 @@ module Admin
     # @param search [String, nil]
     # @param roles [Array<String>, nil]
     # @param statuses [Array<String>, nil]
+    # @param ids [Array<Integer>, nil]
     # @param organizations [Array<String>, 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",

app/views/admin/articles/_article_item.html.erb

@@ -153,13 +153,13 @@
       <div class="flex gap-4">
         <div class="crayons-field flex-1">
           <label for="author_id_<%= article.id %>" class="crayons-field__label"><%= t("views.moderations.article.author_id") %></label>
-          <input id="author_id_<%= article.id %>" class=" crayons-textfield" size="6" name="article[user_id]"
+          <input id="author_id_<%= article.id %>" class="js-username_id_input crayons-textfield" size="6" name="article[user_id]"
             value="<%= article.user_id %>">
         </div>
 
         <div class="crayons-field flex-1">
           <label for="co_author_ids_list_<%= article.id %>" class="crayons-field__label"><%= t("views.moderations.article.co_author_ids") %></label>
-          <input id="co_author_ids_list_<%= article.id %>" class=" crayons-textfield" size="6" name="article[co_author_ids_list]" placeholder="Comma separated"
+          <input id="co_author_ids_list_<%= article.id %>" class="js-coauthor_username_id_input crayons-textfield" size="6" name="article[co_author_ids_list]" placeholder="Comma separated"
             value="<%= article.co_author_ids&.join(", ") %>">
         </div>
       </div>

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] %>
         </div>

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] %>

app/views/admin/tags/edit.html.erb

@@ -17,7 +17,7 @@
         <div class="crayons-field w-50 my-4">
           <%= f.label :user_id, "Add Moderator:", class: "crayons-field__label" %>
           <div>
-            <%= 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" %>
           </div>
           <%= f.submit "Add Moderator", class: "crayons-btn" %>
         </div>

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