From c78b46d62bbe9c35f43c715a37a5277adbf71858 Mon Sep 17 00:00:00 2001
From: Ryan Laurie <30528226+iethree@users.noreply.github.com>
Date: Mon, 19 Aug 2024 08:48:54 -0600
Subject: [PATCH] Remove unnecessary calls to `/api/collection` (#46752)
* use ee api to find custom reports collection
* remove deprecated hook
* fix race condition
* add unit tests
* fix circular dependency
* fix unit tests
* another plugin strategy
---
.../metabase-enterprise/collections/index.ts | 5 +-
.../use-get-default-collection-id/index.ts | 1 +
.../use-get-default-collection-id.ts | 39 ++++
...se-get-default-collection-id.unit.spec.tsx | 191 ++++++++++++++++++
.../metabase-enterprise/collections/utils.ts | 16 +-
.../CollectionContent/CollectionContent.tsx | 18 +-
frontend/src/metabase/collections/hooks.ts | 29 +++
.../metabase/collections/hooks.unit.spec.tsx | 135 +++++++++++++
frontend/src/metabase/collections/utils.ts | 6 -
.../common/hooks/entity-framework/index.ts | 1 -
.../use-collection-list-query/index.ts | 1 -
.../use-collection-list-query.ts | 24 ---
.../use-collection-list-query.unit.spec.tsx | 81 --------
.../components/SaveQuestionForm/context.tsx | 18 +-
.../components/SaveQuestionForm/util.ts | 26 +--
.../SaveQuestionModal.unit.spec.tsx | 33 +--
.../collections/getInitialCollectionId.ts | 2 +-
.../entities/containers/EntityCopyModal.tsx | 70 +++----
.../MainNavbarContainer.tsx | 20 +-
frontend/src/metabase/plugins/index.ts | 9 +-
.../components/QueryModals/QueryModals.tsx | 6 +-
.../questions/components/CopyQuestionForm.tsx | 1 +
22 files changed, 483 insertions(+), 249 deletions(-)
create mode 100644 enterprise/frontend/src/metabase-enterprise/collections/use-get-default-collection-id/index.ts
create mode 100644 enterprise/frontend/src/metabase-enterprise/collections/use-get-default-collection-id/use-get-default-collection-id.ts
create mode 100644 enterprise/frontend/src/metabase-enterprise/collections/use-get-default-collection-id/use-get-default-collection-id.unit.spec.tsx
create mode 100644 frontend/src/metabase/collections/hooks.ts
create mode 100644 frontend/src/metabase/collections/hooks.unit.spec.tsx
delete mode 100644 frontend/src/metabase/common/hooks/entity-framework/use-collection-list-query/index.ts
delete mode 100644 frontend/src/metabase/common/hooks/entity-framework/use-collection-list-query/use-collection-list-query.ts
delete mode 100644 frontend/src/metabase/common/hooks/entity-framework/use-collection-list-query/use-collection-list-query.unit.spec.tsx
diff --git a/enterprise/frontend/src/metabase-enterprise/collections/index.ts b/enterprise/frontend/src/metabase-enterprise/collections/index.ts
index 6047f8a3a17..f2bf384a9bb 100644
--- a/enterprise/frontend/src/metabase-enterprise/collections/index.ts
+++ b/enterprise/frontend/src/metabase-enterprise/collections/index.ts
@@ -16,10 +16,10 @@ import {
OFFICIAL_COLLECTION,
CUSTOM_INSTANCE_ANALYTICS_COLLECTION_ENTITY_ID,
} from "./constants";
+import { useGetDefaultCollectionId } from "./use-get-default-collection-id";
import {
getCollectionType,
isRegularCollection,
- getInstanceAnalyticsCustomCollection,
getIcon,
filterOutItemsFromInstanceAnalytics,
} from "./utils";
@@ -77,8 +77,7 @@ if (hasPremiumFeature("audit_app")) {
CollectionInstanceAnalyticsIcon;
PLUGIN_COLLECTIONS.getCollectionType = getCollectionType;
- PLUGIN_COLLECTIONS.getInstanceAnalyticsCustomCollection =
- getInstanceAnalyticsCustomCollection;
+ PLUGIN_COLLECTIONS.useGetDefaultCollectionId = useGetDefaultCollectionId;
PLUGIN_COLLECTIONS.CUSTOM_INSTANCE_ANALYTICS_COLLECTION_ENTITY_ID =
CUSTOM_INSTANCE_ANALYTICS_COLLECTION_ENTITY_ID;
diff --git a/enterprise/frontend/src/metabase-enterprise/collections/use-get-default-collection-id/index.ts b/enterprise/frontend/src/metabase-enterprise/collections/use-get-default-collection-id/index.ts
new file mode 100644
index 00000000000..2e9640a8b32
--- /dev/null
+++ b/enterprise/frontend/src/metabase-enterprise/collections/use-get-default-collection-id/index.ts
@@ -0,0 +1 @@
+export * from "./use-get-default-collection-id";
diff --git a/enterprise/frontend/src/metabase-enterprise/collections/use-get-default-collection-id/use-get-default-collection-id.ts b/enterprise/frontend/src/metabase-enterprise/collections/use-get-default-collection-id/use-get-default-collection-id.ts
new file mode 100644
index 00000000000..d9a8b950381
--- /dev/null
+++ b/enterprise/frontend/src/metabase-enterprise/collections/use-get-default-collection-id/use-get-default-collection-id.ts
@@ -0,0 +1,39 @@
+import { skipToken, useGetCollectionQuery } from "metabase/api";
+import { _useGetDefaultCollectionId as useOSSGetDefaultCollectionId } from "metabase/collections/hooks";
+import { useGetAuditInfoQuery } from "metabase-enterprise/api";
+import { isInstanceAnalyticsCollection } from "metabase-enterprise/collections/utils";
+import type { CollectionId } from "metabase-types/api";
+
+/**
+ * if the source collection is in the instance analytics collection, the default save location
+ * should be in the custom reports collection
+ */
+export const useGetDefaultCollectionId = (
+ sourceCollectionId?: CollectionId | null,
+): CollectionId | null => {
+ const { data: auditInfo } = useGetAuditInfoQuery(
+ sourceCollectionId ? undefined : skipToken,
+ );
+
+ const { data: collectionInfo } = useGetCollectionQuery(
+ sourceCollectionId ? { id: sourceCollectionId } : skipToken,
+ );
+
+ const { data: customReportsCollectionInfo } = useGetCollectionQuery(
+ auditInfo?.custom_reports ? { id: auditInfo?.custom_reports } : skipToken,
+ );
+
+ const isIAcollection = isInstanceAnalyticsCollection(collectionInfo);
+
+ const initialCollectionId = useOSSGetDefaultCollectionId(sourceCollectionId);
+
+ if (
+ isIAcollection &&
+ auditInfo?.custom_reports &&
+ customReportsCollectionInfo?.can_write
+ ) {
+ return auditInfo.custom_reports;
+ }
+
+ return initialCollectionId;
+};
diff --git a/enterprise/frontend/src/metabase-enterprise/collections/use-get-default-collection-id/use-get-default-collection-id.unit.spec.tsx b/enterprise/frontend/src/metabase-enterprise/collections/use-get-default-collection-id/use-get-default-collection-id.unit.spec.tsx
new file mode 100644
index 00000000000..34b41dc26aa
--- /dev/null
+++ b/enterprise/frontend/src/metabase-enterprise/collections/use-get-default-collection-id/use-get-default-collection-id.unit.spec.tsx
@@ -0,0 +1,191 @@
+import fetchMock from "fetch-mock";
+
+import { setupCollectionByIdEndpoint } from "__support__/server-mocks";
+import { createMockEntitiesState } from "__support__/store";
+import { renderWithProviders, screen } from "__support__/ui";
+import type { AuditInfo } from "metabase-enterprise/audit_app/types/state";
+import type { Collection, CollectionId } from "metabase-types/api";
+import { createMockCollection, createMockUser } from "metabase-types/api/mocks";
+import { createMockState } from "metabase-types/store/mocks";
+
+import { useGetDefaultCollectionId } from "./use-get-default-collection-id";
+
+const TestComponent = ({
+ collectionId,
+}: {
+ collectionId: CollectionId | null;
+}) => {
+ const defaultCollectionId = useGetDefaultCollectionId(collectionId);
+
+ return <div>id: {JSON.stringify(defaultCollectionId)}</div>;
+};
+
+const defaultAuditInfo: AuditInfo = {
+ dashboard_overview: 201,
+ question_overview: 202,
+ custom_reports: 203,
+};
+
+const user = createMockUser({
+ id: 801,
+ first_name: "Ash",
+ personal_collection_id: 301,
+});
+
+const defaultCollections: Collection[] = [
+ createMockCollection({ id: 101, name: "My Collection" }),
+ createMockCollection({ id: 102, name: "Other Collection" }),
+ createMockCollection({
+ id: 202,
+ name: "Instance Analytics",
+ type: "instance-analytics",
+ can_write: false,
+ }),
+ createMockCollection({ id: 203, name: "Custom Reports", can_write: true }),
+ createMockCollection({
+ id: 301,
+ name: "Ash's Personal Collection",
+ is_personal: true,
+ }),
+];
+
+const setup = ({
+ collectionId,
+ collections,
+ hasRootAccess = true,
+}: {
+ collectionId: CollectionId | null;
+ collections: Collection[];
+ hasRootAccess?: boolean;
+}) => {
+ setupCollectionByIdEndpoint({ collections });
+ fetchMock.get("path:/api/ee/audit-app/user/audit-info", defaultAuditInfo);
+
+ const entitiesState = createMockEntitiesState({
+ collections: [
+ createMockCollection({
+ id: "root",
+ name: "Our analytics",
+ can_write: hasRootAccess,
+ }),
+ ...collections,
+ ],
+ });
+ const state = createMockState({ currentUser: user, entities: entitiesState });
+
+ renderWithProviders(<TestComponent collectionId={collectionId} />, {
+ storeInitialState: state,
+ });
+};
+
+describe("enterprise > useGetDefaultCollectionId", () => {
+ describe("Regular Collection Source", () => {
+ it("should default to the root collection", async () => {
+ setup({
+ collectionId: null,
+ hasRootAccess: true,
+ collections: defaultCollections,
+ });
+
+ expect(await screen.findByText("id: null")).toBeInTheDocument();
+ });
+
+ it("should return the user's personal collection when the user lacks write access to our analytics", async () => {
+ setup({
+ collectionId: null,
+ hasRootAccess: false,
+ collections: defaultCollections,
+ });
+
+ expect(await screen.findByText("id: 301")).toBeInTheDocument();
+ });
+
+ it("should use the passed collection id if the user has access", async () => {
+ setup({
+ collectionId: 101,
+ hasRootAccess: false,
+ collections: defaultCollections,
+ });
+
+ expect(await screen.findByText("id: 101")).toBeInTheDocument();
+ });
+
+ it("should fall back to the root collection if the user doesn't have access to the passed collection", async () => {
+ setup({
+ collectionId: 101,
+ hasRootAccess: true,
+ collections: [
+ createMockCollection({
+ id: 101,
+ name: "My Collection",
+ can_write: false,
+ }),
+ ],
+ });
+
+ expect(await screen.findByText("id: null")).toBeInTheDocument();
+ });
+
+ it("should fall back to the personal collection if the user doesn't have access to the passed collection or the root collection", async () => {
+ setup({
+ collectionId: 101,
+ hasRootAccess: false,
+ collections: [
+ createMockCollection({
+ id: 101,
+ name: "My Collection",
+ can_write: false,
+ }),
+ ],
+ });
+
+ expect(await screen.findByText("id: 301")).toBeInTheDocument();
+ });
+ });
+
+ describe("Instance Analytics Source", () => {
+ it("should suggest the custom reports collection if the source id is the instance analytics collection", async () => {
+ setup({
+ collectionId: 202,
+ hasRootAccess: true,
+ collections: [
+ createMockCollection({
+ id: 202,
+ name: "Instance Analytics",
+ type: "instance-analytics",
+ can_write: false,
+ }),
+ createMockCollection({
+ id: 203,
+ name: "Custom Reports",
+ can_write: true,
+ }),
+ ],
+ });
+
+ expect(await screen.findByText("id: 203")).toBeInTheDocument();
+ });
+
+ it("should not return the custom reports collection if the user lacks write access to it", async () => {
+ setup({
+ collectionId: 202,
+ hasRootAccess: false,
+ collections: [
+ createMockCollection({
+ id: 202,
+ name: "Instance Analytics",
+ type: "instance-analytics",
+ can_write: false,
+ }),
+ createMockCollection({
+ id: 203,
+ name: "Custom Reports",
+ can_write: false,
+ }),
+ ],
+ });
+
+ expect(await screen.findByText("id: 301")).toBeInTheDocument();
+ });
+ });
+});
diff --git a/enterprise/frontend/src/metabase-enterprise/collections/utils.ts b/enterprise/frontend/src/metabase-enterprise/collections/utils.ts
index 12672891dbe..33551c7c2cd 100644
--- a/enterprise/frontend/src/metabase-enterprise/collections/utils.ts
+++ b/enterprise/frontend/src/metabase-enterprise/collections/utils.ts
@@ -1,4 +1,3 @@
-import { isInstanceAnalyticsCollection } from "metabase/collections/utils";
import type { IconData, ObjectWithModel } from "metabase/lib/icon";
import { getIconBase } from "metabase/lib/icon";
import type { ItemWithCollection } from "metabase/plugins";
@@ -12,7 +11,6 @@ import type {
import {
COLLECTION_TYPES,
- CUSTOM_INSTANCE_ANALYTICS_COLLECTION_ENTITY_ID,
INSTANCE_ANALYTICS_COLLECTION,
OFFICIAL_COLLECTION,
REGULAR_COLLECTION,
@@ -40,13 +38,13 @@ export function getCollectionType({
);
}
-export const getInstanceAnalyticsCustomCollection = (
- collections: Collection[],
-) =>
- collections?.find?.(
- collection =>
- collection.entity_id === CUSTOM_INSTANCE_ANALYTICS_COLLECTION_ENTITY_ID,
- ) ?? null;
+export function isInstanceAnalyticsCollection(
+ collection?: Pick<Collection, "type">,
+): boolean {
+ return (
+ !!collection && getCollectionType(collection).type === "instance-analytics"
+ );
+}
export const getIcon = (item: ObjectWithModel): IconData => {
if (getCollectionType({ type: item.type }).type === "instance-analytics") {
diff --git a/frontend/src/metabase/collections/components/CollectionContent/CollectionContent.tsx b/frontend/src/metabase/collections/components/CollectionContent/CollectionContent.tsx
index 3d1e5c1cbda..2b97deae3ba 100644
--- a/frontend/src/metabase/collections/components/CollectionContent/CollectionContent.tsx
+++ b/frontend/src/metabase/collections/components/CollectionContent/CollectionContent.tsx
@@ -1,8 +1,8 @@
import { useCallback } from "react";
+import { useListCollectionsTreeQuery } from "metabase/api";
import {
useBookmarkListQuery,
- useCollectionListQuery,
useCollectionQuery,
useDatabaseListQuery,
} from "metabase/common/hooks";
@@ -29,15 +29,13 @@ export function CollectionContent({
}) {
const { data: bookmarks, error: bookmarksError } = useBookmarkListQuery();
const { data: databases, error: databasesError } = useDatabaseListQuery();
- const { data: collections, error: collectionsError } = useCollectionListQuery(
- {
- query: {
- tree: true,
- "exclude-other-user-collections": true,
- "exclude-archived": true,
- },
- },
- );
+
+ const { data: collections, error: collectionsError } =
+ useListCollectionsTreeQuery({
+ "exclude-other-user-collections": true,
+ "exclude-archived": true,
+ });
+
const { data: collection, error: collectionError } = useCollectionQuery({
id: collectionId,
});
diff --git a/frontend/src/metabase/collections/hooks.ts b/frontend/src/metabase/collections/hooks.ts
new file mode 100644
index 00000000000..d5d9d817d46
--- /dev/null
+++ b/frontend/src/metabase/collections/hooks.ts
@@ -0,0 +1,29 @@
+import getInitialCollectionId from "metabase/entities/collections/getInitialCollectionId";
+import { useSelector } from "metabase/lib/redux";
+import { PLUGIN_COLLECTIONS } from "metabase/plugins";
+import type { CollectionId } from "metabase-types/api";
+
+export const _useGetDefaultCollectionId = (
+ sourceCollectionId?: CollectionId | null,
+): CollectionId | null => {
+ // TODO: refactor this selector to be this hook and fetch the necessary collections
+ // right now we assume that the root collection and any other relevant collections are already
+ // in the redux store
+ const initialCollectionId = useSelector(state =>
+ getInitialCollectionId(state, {
+ collectionId: sourceCollectionId ?? undefined,
+ }),
+ );
+
+ return initialCollectionId;
+};
+
+export const useGetDefaultCollectionId = (
+ sourceCollectionId?: CollectionId | null,
+): CollectionId | null => {
+ if (PLUGIN_COLLECTIONS.useGetDefaultCollectionId) {
+ // eslint-disable-next-line react-hooks/rules-of-hooks -- this won't change at runtime, so it's safe
+ return PLUGIN_COLLECTIONS.useGetDefaultCollectionId(sourceCollectionId);
+ }
+ return _useGetDefaultCollectionId(sourceCollectionId);
+};
diff --git a/frontend/src/metabase/collections/hooks.unit.spec.tsx b/frontend/src/metabase/collections/hooks.unit.spec.tsx
new file mode 100644
index 00000000000..de4c3e8f450
--- /dev/null
+++ b/frontend/src/metabase/collections/hooks.unit.spec.tsx
@@ -0,0 +1,135 @@
+import { setupCollectionByIdEndpoint } from "__support__/server-mocks";
+import { createMockEntitiesState } from "__support__/store";
+import { renderWithProviders, screen } from "__support__/ui";
+import type { Collection, CollectionId } from "metabase-types/api";
+import { createMockCollection, createMockUser } from "metabase-types/api/mocks";
+import { createMockState } from "metabase-types/store/mocks";
+
+import { useGetDefaultCollectionId } from "./hooks";
+
+const TestComponent = ({
+ collectionId,
+}: {
+ collectionId: CollectionId | null;
+}) => {
+ const defaultCollectionId = useGetDefaultCollectionId(collectionId);
+
+ return <div>id: {JSON.stringify(defaultCollectionId)}</div>;
+};
+
+const user = createMockUser({
+ id: 801,
+ first_name: "Ash",
+ personal_collection_id: 301,
+});
+
+const defaultCollections: Collection[] = [
+ createMockCollection({ id: 101, name: "My Collection" }),
+ createMockCollection({ id: 102, name: "Other Collection" }),
+ createMockCollection({
+ id: 202,
+ name: "Instance Analytics",
+ type: "instance-analytics",
+ can_write: false,
+ }),
+ createMockCollection({ id: 203, name: "Custom Reports", can_write: true }),
+ createMockCollection({
+ id: 301,
+ name: "Ash's Personal Collection",
+ is_personal: true,
+ }),
+];
+
+const setup = ({
+ collectionId,
+ collections,
+ hasRootAccess = true,
+}: {
+ collectionId: CollectionId | null;
+ collections: Collection[];
+ hasRootAccess?: boolean;
+}) => {
+ setupCollectionByIdEndpoint({ collections });
+
+ const entitiesState = createMockEntitiesState({
+ collections: [
+ createMockCollection({
+ id: "root",
+ name: "Our analytics",
+ can_write: hasRootAccess,
+ }),
+ ...collections,
+ ],
+ });
+ const state = createMockState({ currentUser: user, entities: entitiesState });
+
+ renderWithProviders(<TestComponent collectionId={collectionId} />, {
+ storeInitialState: state,
+ });
+};
+
+describe("enterprise > useGetDefaultCollectionId", () => {
+ describe("Regular Collection Source", () => {
+ it("should default to the root collection", async () => {
+ setup({
+ collectionId: null,
+ hasRootAccess: true,
+ collections: defaultCollections,
+ });
+
+ expect(await screen.findByText("id: null")).toBeInTheDocument();
+ });
+
+ it("should return the user's personal collection when the user lacks write access to our analytics", async () => {
+ setup({
+ collectionId: null,
+ hasRootAccess: false,
+ collections: defaultCollections,
+ });
+
+ expect(await screen.findByText("id: 301")).toBeInTheDocument();
+ });
+
+ it("should use the passed collection id if the user has access", async () => {
+ setup({
+ collectionId: 101,
+ hasRootAccess: false,
+ collections: defaultCollections,
+ });
+
+ expect(await screen.findByText("id: 101")).toBeInTheDocument();
+ });
+
+ it("should fall back to the root collection if the user doesn't have access to the passed collection", async () => {
+ setup({
+ collectionId: 101,
+ hasRootAccess: true,
+ collections: [
+ createMockCollection({
+ id: 101,
+ name: "My Collection",
+ can_write: false,
+ }),
+ ],
+ });
+
+ expect(await screen.findByText("id: null")).toBeInTheDocument();
+ });
+
+ it("should fall back to the personal collection if the user doesn't have access to the passed collection or the root collection", async () => {
+ setup({
+ collectionId: 101,
+ hasRootAccess: false,
+ collections: [
+ createMockCollection({
+ id: 101,
+ name: "My Collection",
+ can_write: false,
+ }),
+ ],
+ });
+
+ expect(await screen.findByText("id: 301")).toBeInTheDocument();
+ });
+ });
+});
diff --git a/frontend/src/metabase/collections/utils.ts b/frontend/src/metabase/collections/utils.ts
index 3c6db7b493d..0d30d91855b 100644
--- a/frontend/src/metabase/collections/utils.ts
+++ b/frontend/src/metabase/collections/utils.ts
@@ -63,12 +63,6 @@ export function isInstanceAnalyticsCollection(
);
}
-export function getInstanceAnalyticsCustomCollection(
- collections: Collection[],
-): Collection | null {
- return PLUGIN_COLLECTIONS.getInstanceAnalyticsCustomCollection(collections);
-}
-
export function isInstanceAnalyticsCustomCollection(
collection: Collection,
): boolean {
diff --git a/frontend/src/metabase/common/hooks/entity-framework/index.ts b/frontend/src/metabase/common/hooks/entity-framework/index.ts
index b0d5f3a246f..e508331ec7e 100644
--- a/frontend/src/metabase/common/hooks/entity-framework/index.ts
+++ b/frontend/src/metabase/common/hooks/entity-framework/index.ts
@@ -6,7 +6,6 @@
export * from "./use-action-list-query";
export * from "./use-action-query";
export * from "./use-bookmark-list-query";
-export * from "./use-collection-list-query";
export * from "./use-collection-query";
export * from "./use-dashboard-query";
export * from "./use-database-list-query";
diff --git a/frontend/src/metabase/common/hooks/entity-framework/use-collection-list-query/index.ts b/frontend/src/metabase/common/hooks/entity-framework/use-collection-list-query/index.ts
deleted file mode 100644
index 580bd133ad2..00000000000
--- a/frontend/src/metabase/common/hooks/entity-framework/use-collection-list-query/index.ts
+++ /dev/null
@@ -1 +0,0 @@
-export * from "./use-collection-list-query";
diff --git a/frontend/src/metabase/common/hooks/entity-framework/use-collection-list-query/use-collection-list-query.ts b/frontend/src/metabase/common/hooks/entity-framework/use-collection-list-query/use-collection-list-query.ts
deleted file mode 100644
index a221532d14c..00000000000
--- a/frontend/src/metabase/common/hooks/entity-framework/use-collection-list-query/use-collection-list-query.ts
+++ /dev/null
@@ -1,24 +0,0 @@
-import Collections from "metabase/entities/collections";
-import type { Collection, CollectionListQuery } from "metabase-types/api";
-
-import type {
- UseEntityListQueryProps,
- UseEntityListQueryResult,
-} from "../use-entity-list-query";
-import { useEntityListQuery } from "../use-entity-list-query";
-
-/**
- * @deprecated use "metabase/api" instead
- */
-export const useCollectionListQuery = (
- props: UseEntityListQueryProps<CollectionListQuery> = {},
-): UseEntityListQueryResult<Collection> => {
- return useEntityListQuery(props, {
- fetchList: Collections.actions.fetchList,
- getError: Collections.selectors.getError,
- getList: Collections.selectors.getList,
- getLoaded: Collections.selectors.getLoaded,
- getLoading: Collections.selectors.getLoading,
- getListMetadata: Collections.selectors.getListMetadata,
- });
-};
diff --git a/frontend/src/metabase/common/hooks/entity-framework/use-collection-list-query/use-collection-list-query.unit.spec.tsx b/frontend/src/metabase/common/hooks/entity-framework/use-collection-list-query/use-collection-list-query.unit.spec.tsx
deleted file mode 100644
index e76543ed72f..00000000000
--- a/frontend/src/metabase/common/hooks/entity-framework/use-collection-list-query/use-collection-list-query.unit.spec.tsx
+++ /dev/null
@@ -1,81 +0,0 @@
-import {
- setupCollectionsEndpoints,
- setupCollectionsWithError,
-} from "__support__/server-mocks";
-import {
- renderWithProviders,
- screen,
- waitForLoaderToBeRemoved,
- within,
-} from "__support__/ui";
-import LoadingAndErrorWrapper from "metabase/components/LoadingAndErrorWrapper";
-import { createMockCollection } from "metabase-types/api/mocks";
-
-import { useCollectionListQuery } from "./use-collection-list-query";
-
-const TEST_COLLECTION = createMockCollection();
-
-const TestComponent = () => {
- const { data = [], metadata, isLoading, error } = useCollectionListQuery();
-
- if (isLoading || error) {
- return <LoadingAndErrorWrapper loading={isLoading} error={error} />;
- }
-
- return (
- <div>
- {data.map(collection => (
- <div key={collection.id}>{collection.name}</div>
- ))}
- <div data-testid="metadata">
- {(!metadata || Object.keys(metadata).length === 0) && "No metadata"}
- </div>
- </div>
- );
-};
-
-const setup = ({ error }: { error?: string } = {}) => {
- if (error) {
- setupCollectionsWithError({ error });
- } else {
- setupCollectionsEndpoints({ collections: [TEST_COLLECTION] });
- }
-
- renderWithProviders(<TestComponent />);
-};
-
-describe("useCollectionListQuery", () => {
- it("should be initially loading", () => {
- setup();
-
- expect(screen.getByTestId("loading-indicator")).toBeInTheDocument();
- });
-
- it("should display error", async () => {
- const ERROR = "Server error";
-
- setup({ error: ERROR });
-
- await waitForLoaderToBeRemoved();
-
- expect(screen.getByText(ERROR)).toBeInTheDocument();
- });
-
- it("should show data from the response", async () => {
- setup();
-
- await waitForLoaderToBeRemoved();
-
- expect(screen.getByText(TEST_COLLECTION.name)).toBeInTheDocument();
- });
-
- it("should not have any metadata in the response", async () => {
- setup();
-
- await waitForLoaderToBeRemoved();
-
- expect(
- within(screen.getByTestId("metadata")).getByText("No metadata"),
- ).toBeInTheDocument();
- });
-});
diff --git a/frontend/src/metabase/components/SaveQuestionForm/context.tsx b/frontend/src/metabase/components/SaveQuestionForm/context.tsx
index 67ab261bef1..d99a5e5f9a4 100644
--- a/frontend/src/metabase/components/SaveQuestionForm/context.tsx
+++ b/frontend/src/metabase/components/SaveQuestionForm/context.tsx
@@ -7,7 +7,7 @@ import {
useState,
} from "react";
-import { useListCollectionsQuery } from "metabase/api";
+import { useGetDefaultCollectionId } from "metabase/collections/hooks";
import { FormProvider } from "metabase/forms";
import { useSelector } from "metabase/lib/redux";
import { getIsSavedQuestionChanged } from "metabase/query_builder/selectors";
@@ -37,21 +37,17 @@ export const SaveQuestionProvider = ({
onCreate,
onSave,
multiStep = false,
- initialCollectionId,
children,
}: PropsWithChildren<SaveQuestionProps>) => {
- const { data: collections = [] } = useListCollectionsQuery({});
const [originalQuestion] = useState(latestOriginalQuestion); // originalQuestion from props changes during saving
+ const defaultCollectionId = useGetDefaultCollectionId(
+ originalQuestion?.collectionId(),
+ );
+
const initialValues: FormValues = useMemo(
- () =>
- getInitialValues(
- collections,
- originalQuestion,
- question,
- initialCollectionId,
- ),
- [collections, initialCollectionId, originalQuestion, question],
+ () => getInitialValues(originalQuestion, question, defaultCollectionId),
+ [originalQuestion, defaultCollectionId, question],
);
const handleSubmit = useCallback(
diff --git a/frontend/src/metabase/components/SaveQuestionForm/util.ts b/frontend/src/metabase/components/SaveQuestionForm/util.ts
index aee1aa7f1c9..f059d99960d 100644
--- a/frontend/src/metabase/components/SaveQuestionForm/util.ts
+++ b/frontend/src/metabase/components/SaveQuestionForm/util.ts
@@ -1,13 +1,9 @@
import { match, P } from "ts-pattern";
import { t } from "ttag";
-import {
- canonicalCollectionId,
- getInstanceAnalyticsCustomCollection,
- isInstanceAnalyticsCollection,
-} from "metabase/collections/utils";
+import { canonicalCollectionId } from "metabase/collections/utils";
import type Question from "metabase-lib/v1/Question";
-import type { CardType, Collection } from "metabase-types/api";
+import type { CardType } from "metabase-types/api";
import type { FormValues } from "./types";
@@ -64,26 +60,12 @@ export async function submitQuestion(
}
export const getInitialValues = (
- collections: Collection[],
originalQuestion: Question | null,
question: Question,
initialCollectionId: FormValues["collection_id"],
): FormValues => {
const isReadonly = originalQuestion != null && !originalQuestion.canWrite();
- // we can't use null because that can be ID of the root collection
- const instanceAnalyticsCollectionId =
- collections?.find(isInstanceAnalyticsCollection)?.id ?? "not found";
- const isInInstanceAnalyticsQuestion =
- originalQuestion?.collectionId() === instanceAnalyticsCollectionId;
-
- if (collections && isInInstanceAnalyticsQuestion) {
- const customCollection = getInstanceAnalyticsCustomCollection(collections);
- if (customCollection) {
- initialCollectionId = customCollection.id;
- }
- }
-
const getOriginalNameModification = (originalQuestion: Question | null) =>
originalQuestion
? t`${originalQuestion.displayName()} - Modified`
@@ -99,9 +81,7 @@ export const getInitialValues = (
description:
originalQuestion?.description() || question.description() || "",
collection_id:
- question.collectionId() === undefined ||
- isReadonly ||
- isInInstanceAnalyticsQuestion
+ question.collectionId() === undefined || isReadonly
? initialCollectionId
: question.collectionId(),
saveType:
diff --git a/frontend/src/metabase/containers/SaveQuestionModal/SaveQuestionModal.unit.spec.tsx b/frontend/src/metabase/containers/SaveQuestionModal/SaveQuestionModal.unit.spec.tsx
index 21ba22884a4..a85ac86d62e 100644
--- a/frontend/src/metabase/containers/SaveQuestionModal/SaveQuestionModal.unit.spec.tsx
+++ b/frontend/src/metabase/containers/SaveQuestionModal/SaveQuestionModal.unit.spec.tsx
@@ -1,5 +1,4 @@
import userEvent from "@testing-library/user-event";
-import fetchMock from "fetch-mock";
import { setupEnterpriseTest } from "__support__/enterprise";
import { createMockMetadata } from "__support__/metadata";
@@ -77,17 +76,23 @@ const setup = async (
const onCloseMock = jest.fn();
if (collectionEndpoints) {
+ setupCollectionByIdEndpoint({
+ collections: collectionEndpoints.collections,
+ });
setupCollectionsEndpoints(collectionEndpoints);
} else {
- fetchMock.get("path:/api/collection", TEST_COLLECTIONS);
- fetchMock.get("path:/api/collection/root", ROOT_TEST_COLLECTION);
setupCollectionByIdEndpoint({ collections: [BOBBY_TEST_COLLECTION] });
- setupCollectionItemsEndpoint({
- collection: BOBBY_TEST_COLLECTION,
- collectionItems: [],
+ setupCollectionsEndpoints({
+ collections: TEST_COLLECTIONS,
+ rootCollection: ROOT_TEST_COLLECTION,
});
}
+ setupCollectionItemsEndpoint({
+ collection: BOBBY_TEST_COLLECTION,
+ collectionItems: [],
+ });
+
setupRecentViewsAndSelectionsEndpoints([]);
const settings = mockSettings();
@@ -253,7 +258,7 @@ describe("SaveQuestionModal", () => {
expect(newQuestion.id()).toBeUndefined();
expect(newQuestion.displayName()).toBe(EXPECTED_SUGGESTED_NAME);
expect(newQuestion.description()).toBe(null);
- expect(newQuestion.collectionId()).toBe(null);
+ expect(newQuestion.collectionId()).toBe(1);
});
it("should call onCreate correctly with edited form", async () => {
@@ -277,7 +282,7 @@ describe("SaveQuestionModal", () => {
expect(newQuestion.id()).toBeUndefined();
expect(newQuestion.displayName()).toBe("My favorite orders");
expect(newQuestion.description()).toBe("So many of them");
- expect(newQuestion.collectionId()).toBe(null);
+ expect(newQuestion.collectionId()).toBe(1);
});
it("should trim name and description", async () => {
@@ -301,7 +306,7 @@ describe("SaveQuestionModal", () => {
expect(newQuestion.id()).toBeUndefined();
expect(newQuestion.displayName()).toBe("My favorite orders");
expect(newQuestion.description()).toBe("So many of them");
- expect(newQuestion.collectionId()).toBe(null);
+ expect(newQuestion.collectionId()).toBe(1);
});
it('should correctly handle saving a question in the "root" collection', async () => {
@@ -324,7 +329,7 @@ describe("SaveQuestionModal", () => {
expect(newQuestion.id()).toBeUndefined();
expect(newQuestion.displayName()).toBe("foo");
expect(newQuestion.description()).toBe("bar");
- expect(newQuestion.collectionId()).toBe(null);
+ expect(newQuestion.collectionId()).toBe(1);
});
it("shouldn't call onSave when form is submitted", async () => {
@@ -723,6 +728,7 @@ describe("SaveQuestionModal", () => {
const cancelBtn = () => screen.getByRole("button", { name: /cancel/i });
const COLLECTION = {
+ USER: BOBBY_TEST_COLLECTION,
ROOT: createMockCollection({
...ROOT_COLLECTION,
can_write: true,
@@ -797,18 +803,13 @@ describe("SaveQuestionModal", () => {
rootCollection: COLLECTION.ROOT,
},
});
- setupCollectionByIdEndpoint({ collections: [COLLECTION.PARENT] });
- setupCollectionItemsEndpoint({
- collection: BOBBY_TEST_COLLECTION,
- collectionItems: [],
- });
});
it("should create collection inside nested folder", async () => {
await userEvent.click(collDropdown());
await waitFor(() => expect(newCollBtn()).toBeInTheDocument());
await userEvent.click(
await screen.findByRole("button", {
- name: new RegExp(COLLECTION.PARENT.name),
+ name: new RegExp(BOBBY_TEST_COLLECTION.name),
}),
);
await userEvent.click(newCollBtn());
diff --git a/frontend/src/metabase/entities/collections/getInitialCollectionId.ts b/frontend/src/metabase/entities/collections/getInitialCollectionId.ts
index e87a25574ba..f5a3db4390f 100644
--- a/frontend/src/metabase/entities/collections/getInitialCollectionId.ts
+++ b/frontend/src/metabase/entities/collections/getInitialCollectionId.ts
@@ -51,7 +51,7 @@ const getInitialCollectionId = createSelector(
byCollectionIdProp,
byCollectionIdNavParam,
byCollectionUrlId,
- byCollectionQueryParameter,
+ byCollectionQueryParameter, // used by new model flow
],
(collections, personalCollectionId, ...collectionIds) => {
const rootCollectionId = ROOT_COLLECTION.id as CollectionId;
diff --git a/frontend/src/metabase/entities/containers/EntityCopyModal.tsx b/frontend/src/metabase/entities/containers/EntityCopyModal.tsx
index 6bde838f143..8f0aedc06a6 100644
--- a/frontend/src/metabase/entities/containers/EntityCopyModal.tsx
+++ b/frontend/src/metabase/entities/containers/EntityCopyModal.tsx
@@ -1,15 +1,10 @@
import { dissoc } from "icepick";
import { t } from "ttag";
-import {
- getInstanceAnalyticsCustomCollection,
- isInstanceAnalyticsCollection,
-} from "metabase/collections/utils";
-import { useCollectionListQuery } from "metabase/common/hooks";
+import { useGetDefaultCollectionId } from "metabase/collections/hooks";
import ModalContent from "metabase/components/ModalContent";
import { CopyDashboardFormConnected } from "metabase/dashboard/containers/CopyDashboardForm";
import { CopyQuestionForm } from "metabase/questions/components/CopyQuestionForm";
-import { Flex, Loader } from "metabase/ui";
interface EntityCopyModalProps {
entityType: string;
@@ -17,7 +12,7 @@ interface EntityCopyModalProps {
copy: (data: any) => void;
title?: string;
onClose: () => void;
- onSaved: (newEntityObject: any) => void;
+ onSaved: (newEntityObject?: any) => void;
overwriteOnInitialValuesChange?: boolean;
onValuesChange?: (values: Record<string, unknown>) => void;
form?: any;
@@ -32,18 +27,17 @@ const EntityCopyModal = ({
onSaved,
...props
}: EntityCopyModalProps) => {
- const { data: collections = [] } = useCollectionListQuery();
-
const resolvedObject =
typeof entityObject?.getPlainObject === "function"
? entityObject.getPlainObject()
: entityObject;
- if (isInstanceAnalyticsCollection(resolvedObject?.collection)) {
- const customCollection = getInstanceAnalyticsCustomCollection(collections);
- if (customCollection) {
- resolvedObject.collection_id = customCollection.id;
- }
+ const defaultCollectionId = useGetDefaultCollectionId(
+ resolvedObject?.collection?.id,
+ );
+
+ if (defaultCollectionId) {
+ resolvedObject.collection_id = defaultCollectionId;
}
const initialValues = {
@@ -51,42 +45,28 @@ const EntityCopyModal = ({
name: resolvedObject.name + " - " + t`Duplicate`,
};
- const renderForm = (props: any) => {
- switch (entityType) {
- case "dashboards":
- return (
- <CopyDashboardFormConnected
- onSubmit={copy}
- onClose={onClose}
- onSaved={onSaved}
- collections={collections}
- {...props}
- />
- );
- case "questions":
- return (
- <CopyQuestionForm
- onSubmit={copy}
- onClose={onClose}
- onSaved={onSaved}
- collections={collections}
- {...props}
- />
- );
- }
- };
-
return (
<ModalContent
title={title || t`Duplicate "${resolvedObject.name}"`}
onClose={onClose}
>
- {!collections?.length ? (
- <Flex justify="center" p="lg">
- <Loader />
- </Flex>
- ) : (
- renderForm({ ...props, initialValues })
+ {entityType === "dashboards" && (
+ <CopyDashboardFormConnected
+ onSubmit={copy}
+ onClose={onClose}
+ onSaved={onSaved}
+ initialValues={initialValues}
+ {...props}
+ />
+ )}
+ {entityType === "questions" && (
+ <CopyQuestionForm
+ onSubmit={copy}
+ onCancel={onClose}
+ onSaved={onSaved}
+ initialValues={initialValues}
+ {...props}
+ />
)}
</ModalContent>
);
diff --git a/frontend/src/metabase/nav/containers/MainNavbar/MainNavbarContainer/MainNavbarContainer.tsx b/frontend/src/metabase/nav/containers/MainNavbar/MainNavbarContainer/MainNavbarContainer.tsx
index 5826b02857c..215f236fd67 100644
--- a/frontend/src/metabase/nav/containers/MainNavbar/MainNavbarContainer/MainNavbarContainer.tsx
+++ b/frontend/src/metabase/nav/containers/MainNavbar/MainNavbarContainer/MainNavbarContainer.tsx
@@ -3,7 +3,10 @@ import { useCallback, useMemo, useState, memo } from "react";
import { connect } from "react-redux";
import _ from "underscore";
-import { useGetCollectionQuery } from "metabase/api";
+import {
+ useGetCollectionQuery,
+ useListCollectionsTreeQuery,
+} from "metabase/api";
import { logout } from "metabase/auth/actions";
import CreateCollectionModal from "metabase/collections/containers/CreateCollectionModal";
import {
@@ -54,7 +57,6 @@ interface Props extends MainNavbarProps {
currentUser: User;
selectedItems: SelectedItem[];
bookmarks: Bookmark[];
- collections: Collection[];
rootCollection: Collection;
hasDataAccess: boolean;
hasOwnDatabase: boolean;
@@ -76,7 +78,6 @@ function MainNavbarContainer({
isOpen,
currentUser,
hasOwnDatabase,
- collections = [],
rootCollection,
hasDataAccess,
location,
@@ -96,6 +97,11 @@ function MainNavbarContainer({
error,
} = useGetCollectionQuery({ id: "trash" });
+ const { data: collections = [] } = useListCollectionsTreeQuery({
+ "exclude-other-user-collections": true,
+ "exclude-archived": true,
+ });
+
const collectionTree = useMemo<CollectionTreeItem[]>(() => {
const preparedCollections = [];
const userPersonalCollections = currentUserPersonalCollections(
@@ -209,14 +215,6 @@ export default _.compose(
entityAlias: "rootCollection",
loadingAndErrorWrapper: false,
}),
- Collections.loadList({
- query: () => ({
- tree: true,
- "exclude-other-user-collections": true,
- "exclude-archived": true,
- }),
- loadingAndErrorWrapper: false,
- }),
Databases.loadList({
loadingAndErrorWrapper: false,
}),
diff --git a/frontend/src/metabase/plugins/index.ts b/frontend/src/metabase/plugins/index.ts
index a1828a85740..0d8a51990e9 100644
--- a/frontend/src/metabase/plugins/index.ts
+++ b/frontend/src/metabase/plugins/index.ts
@@ -49,6 +49,7 @@ import type {
SearchResult,
User,
UserListResult,
+ CollectionId,
} from "metabase-types/api";
import type { AdminPathKey, State } from "metabase-types/store";
@@ -265,6 +266,10 @@ type CleanUpMenuItem = {
export type ItemWithCollection = { collection: CollectionEssentials };
+type GetCollectionIdType = (
+ sourceCollectionId?: CollectionId | null,
+) => CollectionId | null;
+
export const PLUGIN_COLLECTIONS = {
AUTHORITY_LEVEL: {
[JSON.stringify(AUTHORITY_LEVEL_REGULAR.type)]: AUTHORITY_LEVEL_REGULAR,
@@ -278,9 +283,7 @@ export const PLUGIN_COLLECTIONS = {
_: Partial<Collection>,
): CollectionAuthorityLevelConfig | CollectionInstanceAnaltyicsConfig =>
AUTHORITY_LEVEL_REGULAR,
- getInstanceAnalyticsCustomCollection: (
- _collections: Collection[],
- ): Collection | null => null,
+ useGetDefaultCollectionId: null as GetCollectionIdType | null,
CUSTOM_INSTANCE_ANALYTICS_COLLECTION_ENTITY_ID: "",
INSTANCE_ANALYTICS_ADMIN_READONLY_MESSAGE: UNABLE_TO_CHANGE_ADMIN_PERMISSIONS,
getAuthorityLevelMenuItems: (
diff --git a/frontend/src/metabase/query_builder/components/QueryModals/QueryModals.tsx b/frontend/src/metabase/query_builder/components/QueryModals/QueryModals.tsx
index 119e7e4adf1..93dd4a646f0 100644
--- a/frontend/src/metabase/query_builder/components/QueryModals/QueryModals.tsx
+++ b/frontend/src/metabase/query_builder/components/QueryModals/QueryModals.tsx
@@ -2,13 +2,13 @@ import { useCallback } from "react";
import { t } from "ttag";
import _ from "underscore";
+import { useGetDefaultCollectionId } from "metabase/collections/hooks";
import Modal from "metabase/components/Modal";
import QuestionSavedModal from "metabase/components/QuestionSavedModal";
import { AddToDashSelectDashModal } from "metabase/containers/AddToDashSelectDashModal";
import { MoveModal } from "metabase/containers/MoveModal";
import { SaveQuestionModal } from "metabase/containers/SaveQuestionModal";
import { ROOT_COLLECTION } from "metabase/entities/collections/constants";
-import getInitialCollectionId from "metabase/entities/collections/getInitialCollectionId";
import EntityCopyModal from "metabase/entities/containers/EntityCopyModal";
import Questions from "metabase/entities/questions";
import { useDispatch, useSelector } from "metabase/lib/redux";
@@ -74,9 +74,7 @@ export function QueryModals({
}: QueryModalsProps) {
const dispatch = useDispatch();
- const initialCollectionId = useSelector(state =>
- getInitialCollectionId(state, {}),
- );
+ const initialCollectionId = useGetDefaultCollectionId();
const questionWithParameters = useSelector(getQuestionWithParameters);
const showAlertsAfterQuestionSaved = useCallback(() => {
diff --git a/frontend/src/metabase/questions/components/CopyQuestionForm.tsx b/frontend/src/metabase/questions/components/CopyQuestionForm.tsx
index 12b0d33820b..673a97362a4 100644
--- a/frontend/src/metabase/questions/components/CopyQuestionForm.tsx
+++ b/frontend/src/metabase/questions/components/CopyQuestionForm.tsx
@@ -62,6 +62,7 @@ export const CopyQuestionForm = ({
initialValues={computedInitialValues}
validationSchema={QUESTION_SCHEMA}
onSubmit={handleDuplicate}
+ enableReinitialize
>
<Form>
<FormTextInput
--
GitLab