From 7b80be77716b0e4e9cfa85bba0523fbfae87bec3 Mon Sep 17 00:00:00 2001
From: Oisin Coveney <oisin@metabase.com>
Date: Tue, 14 Nov 2023 20:13:22 +0200
Subject: [PATCH] Remove redundant /user/recipient API calls (#35644)
---
.../admin-2/people/people.cy.spec.js | 7 +++-
frontend/src/metabase-types/api/user.ts | 4 ++
.../admin/people/components/PeopleList.jsx | 5 ++-
.../use-user-list-query.ts | 4 +-
.../use-user-list-query.unit.spec.tsx | 42 ++++++++++++++++---
frontend/src/metabase/entities/users.js | 9 ++++
.../SearchResults/SearchResults.unit.spec.tsx | 8 +++-
.../InfoText/InfoTextEditedInfo.tsx | 14 +++----
.../SearchUserPicker/SearchUserPicker.tsx | 11 +++--
.../test/__support__/server-mocks/user.ts | 2 +-
10 files changed, 80 insertions(+), 26 deletions(-)
diff --git a/e2e/test/scenarios/admin-2/people/people.cy.spec.js b/e2e/test/scenarios/admin-2/people/people.cy.spec.js
index 2e28104ce25..c6c62f98376 100644
--- a/e2e/test/scenarios/admin-2/people/people.cy.spec.js
+++ b/e2e/test/scenarios/admin-2/people/people.cy.spec.js
@@ -396,13 +396,16 @@ describe("scenarios > admin > people", () => {
assertTableRowsCount(PAGE_SIZE);
cy.findByLabelText("Previous page").should("be.disabled");
- cy.findByLabelText("Next page").click();
+ // cy.findByLabelText("Next page").click();
+ cy.findByTestId("next-page-btn").click();
waitForUserRequests();
// eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
cy.findByText("Loading...").should("not.exist");
// Page 2
- cy.findByTextEnsureVisible(`${PAGE_SIZE + 1} - ${NEW_TOTAL_USERS}`);
+ cy.findByTestId("people-list-footer")
+ .findByText(`${PAGE_SIZE + 1} - ${NEW_TOTAL_USERS}`)
+ .should("be.visible");
assertTableRowsCount(NEW_TOTAL_USERS % PAGE_SIZE);
cy.findByLabelText("Next page").should("be.disabled");
diff --git a/frontend/src/metabase-types/api/user.ts b/frontend/src/metabase-types/api/user.ts
index 760a8faf8ff..61f7640efbc 100644
--- a/frontend/src/metabase-types/api/user.ts
+++ b/frontend/src/metabase-types/api/user.ts
@@ -54,3 +54,7 @@ export type UserInfo = Pick<
| "is_superuser"
| "is_qbnewb"
>;
+
+export type UserListQuery = {
+ recipients: boolean;
+};
diff --git a/frontend/src/metabase/admin/people/components/PeopleList.jsx b/frontend/src/metabase/admin/people/components/PeopleList.jsx
index 5c597978c4c..e14fa879608 100644
--- a/frontend/src/metabase/admin/people/components/PeopleList.jsx
+++ b/frontend/src/metabase/admin/people/components/PeopleList.jsx
@@ -219,7 +219,10 @@ const PeopleList = ({
</table>
{hasUsers && (
- <div className="flex align-center justify-between p2">
+ <div
+ className="flex align-center justify-between p2"
+ data-testid="people-list-footer"
+ >
<div className="text-medium text-bold">
{ngettext(
msgid`${total} person found`,
diff --git a/frontend/src/metabase/common/hooks/use-user-list-query/use-user-list-query.ts b/frontend/src/metabase/common/hooks/use-user-list-query/use-user-list-query.ts
index ba5d526e4f9..80f455373db 100644
--- a/frontend/src/metabase/common/hooks/use-user-list-query/use-user-list-query.ts
+++ b/frontend/src/metabase/common/hooks/use-user-list-query/use-user-list-query.ts
@@ -4,10 +4,10 @@ import type {
UseEntityListQueryResult,
} from "metabase/common/hooks/use-entity-list-query";
import { useEntityListQuery } from "metabase/common/hooks/use-entity-list-query";
-import type { UserListResult } from "metabase-types/api";
+import type { UserListQuery, UserListResult } from "metabase-types/api";
export const useUserListQuery = (
- props: UseEntityListQueryProps<Record<string, never>> = {},
+ props: UseEntityListQueryProps<UserListQuery> = {},
): UseEntityListQueryResult<UserListResult> => {
return useEntityListQuery(props, {
fetchList: Users.actions.fetchList,
diff --git a/frontend/src/metabase/common/hooks/use-user-list-query/use-user-list-query.unit.spec.tsx b/frontend/src/metabase/common/hooks/use-user-list-query/use-user-list-query.unit.spec.tsx
index 8c15abfa090..a7a785d7718 100644
--- a/frontend/src/metabase/common/hooks/use-user-list-query/use-user-list-query.unit.spec.tsx
+++ b/frontend/src/metabase/common/hooks/use-user-list-query/use-user-list-query.unit.spec.tsx
@@ -1,4 +1,8 @@
-import { setupUsersEndpoints } from "__support__/server-mocks";
+import fetchMock from "fetch-mock";
+import {
+ setupUserRecipientsEndpoint,
+ setupUsersEndpoints,
+} from "__support__/server-mocks";
import {
renderWithProviders,
screen,
@@ -12,8 +16,19 @@ import { useUserListQuery } from "./use-user-list-query";
const TEST_USER = createMockUserInfo();
-function TestComponent() {
- const { data = [], metadata, isLoading, error } = useUserListQuery();
+type TestComponentProps = { getRecipients?: boolean };
+
+function TestComponent({ getRecipients = false }: TestComponentProps) {
+ const {
+ data = [],
+ metadata,
+ isLoading,
+ error,
+ } = useUserListQuery({
+ query: {
+ recipients: getRecipients,
+ },
+ });
if (isLoading || error) {
return <LoadingAndErrorWrapper loading={isLoading} error={error} />;
@@ -32,9 +47,12 @@ function TestComponent() {
);
}
-function setup() {
+function setup({ getRecipients = false }: TestComponentProps = {}) {
setupUsersEndpoints([TEST_USER]);
- renderWithProviders(<TestComponent />);
+ setupUserRecipientsEndpoint({
+ users: [TEST_USER],
+ });
+ renderWithProviders(<TestComponent getRecipients={getRecipients} />);
}
describe("useUserListQuery", () => {
@@ -56,4 +74,18 @@ describe("useUserListQuery", () => {
within(screen.getByTestId("metadata")).getByText("No metadata"),
).toBeInTheDocument();
});
+
+ it("should call /api/user when recipient isn't passed or is false", () => {
+ setup();
+
+ expect(fetchMock.calls("path:/api/user")).toHaveLength(1);
+ expect(fetchMock.calls("path:/api/user/recipients")).toHaveLength(0);
+ });
+
+ it("should call /api/user/recipients when the `recipient` is passed", () => {
+ setup({ getRecipients: true });
+
+ expect(fetchMock.calls("path:/api/user")).toHaveLength(0);
+ expect(fetchMock.calls("path:/api/user/recipients")).toHaveLength(1);
+ });
});
diff --git a/frontend/src/metabase/entities/users.js b/frontend/src/metabase/entities/users.js
index 5b9619057cf..39e1ca86ca3 100644
--- a/frontend/src/metabase/entities/users.js
+++ b/frontend/src/metabase/entities/users.js
@@ -9,6 +9,7 @@ import { createEntity } from "metabase/lib/entities";
import { UserApi, SessionApi } from "metabase/services";
import { generatePassword } from "metabase/lib/security";
+import { GET } from "metabase/lib/api";
import forms from "./users/forms";
export const DEACTIVATE = "metabase/entities/users/DEACTIVATE";
@@ -24,6 +25,9 @@ function loadMemberships() {
return require("metabase/admin/people/people").loadMemberships();
}
+const getUserList = GET("/api/user");
+const getRecipientsList = GET("/api/user/recipients");
+
const Users = createEntity({
name: "users",
nameOne: "user",
@@ -31,6 +35,11 @@ const Users = createEntity({
path: "/api/user",
+ api: {
+ list: ({ recipients = false, ...args }) =>
+ recipients ? getRecipientsList() : getUserList(args),
+ },
+
objectSelectors: {
getName: user => user.common_name,
},
diff --git a/frontend/src/metabase/nav/components/search/SearchResults/SearchResults.unit.spec.tsx b/frontend/src/metabase/nav/components/search/SearchResults/SearchResults.unit.spec.tsx
index 3de190d89d6..f21f0d885b4 100644
--- a/frontend/src/metabase/nav/components/search/SearchResults/SearchResults.unit.spec.tsx
+++ b/frontend/src/metabase/nav/components/search/SearchResults/SearchResults.unit.spec.tsx
@@ -1,5 +1,6 @@
/* eslint-disable react/prop-types */
import userEvent from "@testing-library/user-event";
+import fetchMock from "fetch-mock";
import { Route } from "react-router";
import {
renderWithProviders,
@@ -47,8 +48,8 @@ const setup = async ({
searchText = "test",
footer = null,
}: SearchResultsSetupProps = {}) => {
- setupSearchEndpoints(searchResults);
setupUserRecipientsEndpoint({ users: [createMockUser()] });
+ setupSearchEndpoints(searchResults);
setupCollectionByIdEndpoint({
collections: [createMockCollection()],
});
@@ -177,4 +178,9 @@ describe("SearchResults", () => {
TEST_SEARCH_RESULTS.length.toString(),
);
});
+
+ it("should only call the /api/user/recipients endpoint once even if there are multiple search results", async () => {
+ await setup();
+ expect(fetchMock.calls("path:/api/user/recipients").length).toBe(1);
+ });
});
diff --git a/frontend/src/metabase/search/components/InfoText/InfoTextEditedInfo.tsx b/frontend/src/metabase/search/components/InfoText/InfoTextEditedInfo.tsx
index c4f373dea75..ac0733a7c22 100644
--- a/frontend/src/metabase/search/components/InfoText/InfoTextEditedInfo.tsx
+++ b/frontend/src/metabase/search/components/InfoText/InfoTextEditedInfo.tsx
@@ -1,8 +1,7 @@
import dayjs from "dayjs";
-import { useAsync } from "react-use";
import { t } from "ttag";
import { isNull } from "underscore";
-import { UserApi } from "metabase/services";
+import { useUserListQuery } from "metabase/common/hooks/use-user-list-query";
import type { UserListResult } from "metabase-types/api";
import Tooltip from "metabase/core/components/Tooltip";
import { isNotNull } from "metabase/lib/types";
@@ -37,12 +36,11 @@ export const InfoTextEditedInfo = ({
result: WrappedResult;
isCompact?: boolean;
}) => {
- const {
- loading: isLoading,
- value,
- error,
- } = useAsync<() => Promise<{ data: UserListResult[] }>>(UserApi.list);
- const users = value?.data ?? [];
+ const { isLoading, data, error } = useUserListQuery({
+ query: { recipients: true },
+ });
+
+ const users = data ?? [];
if (isLoading) {
return (
diff --git a/frontend/src/metabase/search/components/SearchUserPicker/SearchUserPicker.tsx b/frontend/src/metabase/search/components/SearchUserPicker/SearchUserPicker.tsx
index db9b56ade90..e9812d1ac08 100644
--- a/frontend/src/metabase/search/components/SearchUserPicker/SearchUserPicker.tsx
+++ b/frontend/src/metabase/search/components/SearchUserPicker/SearchUserPicker.tsx
@@ -1,8 +1,7 @@
-import { useAsync } from "react-use";
import { without } from "underscore";
import { useState } from "react";
import { t } from "ttag";
-import { UserApi } from "metabase/services";
+import { useUserListQuery } from "metabase/common/hooks/use-user-list-query";
import type { UserId, UserListResult } from "metabase-types/api";
import { Center, Text } from "metabase/ui";
import { SearchFilterPopoverWrapper } from "metabase/search/components/SearchFilterPopoverWrapper";
@@ -24,11 +23,11 @@ export const SearchUserPicker = ({
value: UserId[];
onChange: (value: UserId[]) => void;
}) => {
- const { loading: isLoading, value: userApiReturnValue } = useAsync<
- () => Promise<{ data: UserListResult[] }>
- >(UserApi.list);
+ const { isLoading, data } = useUserListQuery({
+ query: { recipients: true },
+ });
- const users = userApiReturnValue?.data ?? [];
+ const users = data ?? [];
const [userFilter, setUserFilter] = useState("");
const [selectedUserIds, setSelectedUserIds] = useState(value);
diff --git a/frontend/test/__support__/server-mocks/user.ts b/frontend/test/__support__/server-mocks/user.ts
index 68cfdc52bc1..ce083382aa2 100644
--- a/frontend/test/__support__/server-mocks/user.ts
+++ b/frontend/test/__support__/server-mocks/user.ts
@@ -6,8 +6,8 @@ export function setupUserEndpoints(user: UserListResult) {
}
export function setupUsersEndpoints(users: UserListResult[]) {
- fetchMock.get("path:/api/user", users);
users.forEach(user => setupUserEndpoints(user));
+ return fetchMock.get("path:/api/user", users);
}
export function setupCurrentUserEndpoint(user: User) {
--
GitLab