From 3afe4821a0b99a92bfea45ff50ae62e9e08f1c93 Mon Sep 17 00:00:00 2001
From: Alexander Polyankin <alexander.polyankin@metabase.com>
Date: Wed, 12 Jun 2024 23:52:09 -0400
Subject: [PATCH] Fix issue with root collection in QuestionPicker (#44081)

---
 .../components/QuestionPicker.tsx             | 53 +++++++++----------
 .../ValuesSourceModal.unit.spec.tsx           | 42 +++++++--------
 .../__support__/server-mocks/collection.ts    |  4 ++
 3 files changed, 46 insertions(+), 53 deletions(-)

diff --git a/frontend/src/metabase/common/components/QuestionPicker/components/QuestionPicker.tsx b/frontend/src/metabase/common/components/QuestionPicker/components/QuestionPicker.tsx
index f76d4e8f7f8..3cc62263cd6 100644
--- a/frontend/src/metabase/common/components/QuestionPicker/components/QuestionPicker.tsx
+++ b/frontend/src/metabase/common/components/QuestionPicker/components/QuestionPicker.tsx
@@ -7,7 +7,6 @@ import {
   useGetCollectionQuery,
 } from "metabase/api";
 import { isValidCollectionId } from "metabase/collections/utils";
-import LoadingAndErrorWrapper from "metabase/components/LoadingAndErrorWrapper";
 import { useSelector } from "metabase/lib/redux";
 import { getUserPersonalCollectionId } from "metabase/selectors/user";
 import type {
@@ -49,30 +48,34 @@ const useGetInitialCollection = (
 ) => {
   const isQuestion =
     initialValue && ["card", "dataset", "metric"].includes(initialValue.model);
-
+  const isCollection = initialValue?.model === "collection";
   const cardId = isQuestion ? Number(initialValue.id) : undefined;
+  const collectionId = isCollection
+    ? isValidCollectionId(initialValue.id)
+      ? initialValue.id
+      : "root"
+    : undefined;
 
-  const { data: currentQuestion, error: questionError } = useGetCardQuery(
-    cardId ? { id: cardId } : skipToken,
-  );
+  const { data: currentCollection, isLoading: isCollectionLoading } =
+    useGetCollectionQuery(collectionId ? collectionId : skipToken);
 
-  const collectionId =
-    isQuestion && currentQuestion
-      ? currentQuestion?.collection_id
-      : initialValue?.id;
+  const { data: currentQuestion, isLoading: isQuestionLoading } =
+    useGetCardQuery(cardId ? { id: cardId } : skipToken);
 
-  const { data: currentCollection, error: collectionError } =
-    useGetCollectionQuery(
-      !isQuestion || !!currentQuestion
-        ? (isValidCollectionId(collectionId) && collectionId) || "root"
-        : skipToken,
-    );
+  const {
+    data: currentQuestionCollection,
+    isLoading: isCurrentQuestionCollectionLoading,
+  } = useGetCollectionQuery(
+    currentQuestion ? currentQuestion.collection_id ?? "root" : skipToken,
+  );
 
   return {
     currentQuestion: currentQuestion,
-    currentCollection,
-    isLoading: !currentCollection,
-    error: questionError ?? collectionError,
+    currentCollection: currentQuestionCollection ?? currentCollection,
+    isLoading:
+      isCollectionLoading ||
+      isQuestionLoading ||
+      isCurrentQuestionCollectionLoading,
   };
 };
 
@@ -92,12 +95,8 @@ export const QuestionPicker = ({
     }),
   );
 
-  const {
-    currentCollection,
-    currentQuestion,
-    error,
-    isLoading: loadingCurrentCollection,
-  } = useGetInitialCollection(initialValue);
+  const { currentCollection, currentQuestion, isLoading } =
+    useGetInitialCollection(initialValue);
 
   const userPersonalCollectionId = useSelector(getUserPersonalCollectionId);
 
@@ -160,11 +159,7 @@ export const QuestionPicker = ({
     [currentCollection, userPersonalCollectionId],
   );
 
-  if (error) {
-    return <LoadingAndErrorWrapper error={error} />;
-  }
-
-  if (loadingCurrentCollection) {
+  if (isLoading) {
     return <DelayedLoadingSpinner />;
   }
 
diff --git a/frontend/src/metabase/parameters/components/ValuesSourceModal/ValuesSourceModal.unit.spec.tsx b/frontend/src/metabase/parameters/components/ValuesSourceModal/ValuesSourceModal.unit.spec.tsx
index 56c4ebdeabe..b558e784466 100644
--- a/frontend/src/metabase/parameters/components/ValuesSourceModal/ValuesSourceModal.unit.spec.tsx
+++ b/frontend/src/metabase/parameters/components/ValuesSourceModal/ValuesSourceModal.unit.spec.tsx
@@ -1,12 +1,10 @@
 import userEvent from "@testing-library/user-event";
-import fetchMock from "fetch-mock";
 
 import { createMockMetadata } from "__support__/metadata";
 import {
   setupCardsEndpoints,
   setupCollectionsEndpoints,
   setupDatabasesEndpoints,
-  setupDatabaseEndpoints,
   setupErrorParameterValuesEndpoints,
   setupParameterValuesEndpoints,
   setupSearchEndpoints,
@@ -14,6 +12,8 @@ import {
   setupUnauthorizedCollectionsEndpoints,
   setupRecentViewsAndSelectionsEndpoints,
   setupTableQueryMetadataEndpoint,
+  setupCollectionByIdEndpoint,
+  setupCollectionItemsEndpoint,
 } from "__support__/server-mocks";
 import {
   renderWithProviders,
@@ -32,6 +32,7 @@ import {
   createMockField,
   createMockParameterValues,
   createMockTable,
+  createMockUser,
 } from "metabase-types/api/mocks";
 
 import ValuesSourceModal from "./ValuesSourceModal";
@@ -281,25 +282,6 @@ describe("ValuesSourceModal", () => {
     });
 
     it("should allow searching for a card without access to the root collection (metabase#30355)", async () => {
-      fetchMock.get(
-        { url: "path:/api/collection", overwriteRoutes: false },
-        [],
-      );
-      fetchMock.get(
-        {
-          url: "path:/api/collection/tree",
-          query: { tree: true, "exclude-archived": true },
-          overwriteRoutes: false,
-        },
-        [],
-      );
-      setupDatabaseEndpoints(
-        createMockDatabase({
-          id: -1337,
-          tables: [createMockTable({ schema: "Everything%20else" })],
-        }),
-      );
-
       await setup({
         hasCollectionAccess: false,
       });
@@ -429,17 +411,28 @@ const setup = async ({
   hasCollectionAccess = true,
   hasParameterValuesError = false,
 }: SetupOpts = {}) => {
+  const currentUser = createMockUser();
   const databases = [createMockDatabase()];
-  const collections = [createMockCollection(ROOT_COLLECTION)];
+  const rootCollection = createMockCollection(ROOT_COLLECTION);
+  const personalCollection = createMockCollection({
+    id: currentUser.personal_collection_id,
+  });
   const onSubmit = jest.fn();
   const onClose = jest.fn();
 
   setupDatabasesEndpoints(databases);
   setupSearchEndpoints([]);
   setupRecentViewsAndSelectionsEndpoints([]);
+  setupCollectionByIdEndpoint({
+    collections: [personalCollection],
+  });
+  setupCollectionItemsEndpoint({
+    collection: personalCollection,
+    collectionItems: [],
+  });
 
   if (hasCollectionAccess) {
-    setupCollectionsEndpoints({ collections });
+    setupCollectionsEndpoints({ collections: [rootCollection] });
     setupCardsEndpoints(cards);
     cards.forEach(card =>
       setupTableQueryMetadataEndpoint(
@@ -450,7 +443,7 @@ const setup = async ({
       ),
     );
   } else {
-    setupUnauthorizedCollectionsEndpoints(collections);
+    setupUnauthorizedCollectionsEndpoints([rootCollection]);
     setupUnauthorizedCardsEndpoints(cards);
   }
 
@@ -466,6 +459,7 @@ const setup = async ({
       onSubmit={onSubmit}
       onClose={onClose}
     />,
+    { storeInitialState: { currentUser } },
   );
 
   await waitForLoaderToBeRemoved();
diff --git a/frontend/test/__support__/server-mocks/collection.ts b/frontend/test/__support__/server-mocks/collection.ts
index a10a463684e..9b6586bce07 100644
--- a/frontend/test/__support__/server-mocks/collection.ts
+++ b/frontend/test/__support__/server-mocks/collection.ts
@@ -136,6 +136,10 @@ export function setupUnauthorizedCollectionEndpoints(collection: Collection) {
     status: 403,
     body: PERMISSION_ERROR,
   });
+  fetchMock.get(`path:/api/collection/${collection.id}/items`, {
+    status: 403,
+    body: PERMISSION_ERROR,
+  });
 }
 
 export function setupCollectionByIdEndpoint({
-- 
GitLab