Properly Handle Other Users' Personal Subcollections in CollectionPicker (#40822)

frontend/src/metabase/common/components/CollectionPicker/components/CollectionPicker.tsx

@@ -67,14 +67,23 @@ export const CollectionPickerInner = (
 
   const onFolderSelect = useCallback(
     ({ folder }: { folder: CollectionPickerItem }) => {
+      const isUserPersonalCollection = folder?.id === userPersonalCollectionId;
+      const isUserSubfolder =
+        path?.[1]?.query?.collection === "personal" &&
+        !isUserPersonalCollection;
+
       const newPath = getStateFromIdPath({
-        idPath: getCollectionIdPath(folder, userPersonalCollectionId),
+        idPath: getCollectionIdPath(
+          folder,
+          userPersonalCollectionId,
+          isUserSubfolder,
+        ),
         namespace: options.namespace,
       });
       setPath(newPath);
       onItemSelect(folder);
     },
-    [setPath, onItemSelect, options.namespace, userPersonalCollectionId],
+    [setPath, onItemSelect, options.namespace, userPersonalCollectionId, path],
   );
 
   // Exposing onFolderSelect so that parent can select newly created

frontend/src/metabase/common/components/CollectionPicker/utils.ts

@@ -16,6 +16,7 @@ export const getCollectionIdPath = (
     "id" | "location" | "is_personal" | "effective_location"
   >,
   userPersonalCollectionId?: CollectionId,
+  isPersonal?: boolean,
 ): CollectionId[] => {
   if (isRootCollection(collection)) {
     return ["root"];
@@ -39,10 +40,10 @@ export const getCollectionIdPath = (
     (collection.id === userPersonalCollectionId ||
       pathFromRoot.includes(userPersonalCollectionId));
 
-  if (isInUserPersonalCollection) {
-    return [...pathFromRoot, collection.id];
-  } else if (collection.is_personal) {
+  if (isPersonal) {
     return ["personal", ...pathFromRoot, collection.id];
+  } else if (isInUserPersonalCollection) {
+    return [...pathFromRoot, collection.id];
   } else {
     return ["root", ...pathFromRoot, collection.id];
   }

frontend/src/metabase/common/components/CollectionPicker/utils.unit.spec.ts

+import { getCollectionIdPath } from "./utils";
+
+describe("CollectionPicker > utils", () => {
+  describe("getCollectionIdPath", () => {
+    it("should handle the current user's personal collection", () => {
+      const path = getCollectionIdPath(
+        {
+          id: 1337,
+          location: "/",
+          effective_location: "/",
+          is_personal: true,
+        },
+        1337,
+        false,
+      );
+
+      expect(path).toEqual([1337]);
+    });
+
+    it("should handle subcollections of the current user's personal collection", () => {
+      const path = getCollectionIdPath(
+        {
+          id: 1339,
+          location: "/1337/",
+          effective_location: "/1337/",
+          is_personal: true,
+        },
+        1337,
+        false,
+      );
+
+      expect(path).toEqual([1337, 1339]);
+    });
+
+    it("should handle all users' personal collections", () => {
+      const path = getCollectionIdPath(
+        {
+          id: "personal",
+          location: "/",
+          effective_location: "/",
+        },
+        1337,
+        false,
+      );
+
+      expect(path).toEqual(["personal"]);
+    });
+
+    it("should handle subcollections of all users' personal collections", () => {
+      const path = getCollectionIdPath(
+        {
+          id: 8675309,
+          location: "/1400/",
+          effective_location: "/1400/",
+        },
+        1337,
+        true,
+      );
+
+      expect(path).toEqual(["personal", 1400, 8675309]);
+    });
+
+    it("should handle the current user's personal collection within all users' personal collections", () => {
+      const path = getCollectionIdPath(
+        {
+          id: 1337,
+          location: "/",
+          effective_location: "/",
+          is_personal: true,
+        },
+        1337,
+        true,
+      );
+
+      expect(path).toEqual(["personal", 1337]);
+    });
+
+    it("should handle subcollections of the current user's personal collection within all users' personal collections 🥴", () => {
+      const path = getCollectionIdPath(
+        {
+          id: 1339,
+          location: "/1337/",
+          effective_location: "/1337/",
+          is_personal: true,
+        },
+        1337,
+        true,
+      );
+
+      expect(path).toEqual(["personal", 1337, 1339]);
+    });
+
+    it("should handle root collection", () => {
+      const path = getCollectionIdPath(
+        {
+          id: "root",
+          location: "/",
+          effective_location: "/",
+        },
+        1337,
+        false,
+      );
+
+      expect(path).toEqual(["root"]);
+    });
+
+    it("should handle subcollections of the root collection", () => {
+      const path = getCollectionIdPath(
+        {
+          id: 9,
+          location: "/6/7/8/",
+          effective_location: "/6/7/8/",
+        },
+        1337,
+        false,
+      );
+
+      expect(path).toEqual(["root", 6, 7, 8, 9]);
+    });
+
+    it("should use effective location", () => {
+      const path = getCollectionIdPath(
+        {
+          id: 9,
+          location: "/4/5/6/7/8/",
+          effective_location: "/6/7/8/",
+        },
+        1337,
+        false,
+      );
+
+      expect(path).toEqual(["root", 6, 7, 8, 9]);
+    });
+  });
+});