Remove collections without questions or models from SavedQuestionPicker (#28490)

frontend/src/metabase-types/api/collection.ts

@@ -3,7 +3,7 @@ import { CardDisplayType } from "./card";
 
 export type RegularCollectionId = number;
 
-export type CollectionId = RegularCollectionId | "root";
+export type CollectionId = RegularCollectionId | "root" | "personal";
 
 export type CollectionContentModel = "card" | "dataset";
 

frontend/src/metabase-types/store/entities.ts

@@ -19,7 +19,7 @@ import {
 
 export interface EntitiesState {
   actions?: Record<WritebackActionId, WritebackAction>;
-  collections?: Record<CollectionId, Collection>;
+  collections?: Partial<Record<CollectionId, Collection>>;
   dashboards?: Record<DashboardId, Dashboard>;
   databases?: Record<number, Database>;
   fields?: Record<FieldId, Field>;

frontend/src/metabase/collections/containers/FormAuthorityLevelFieldContainer.tsx

@@ -12,7 +12,7 @@ import { canManageCollectionAuthorityLevel } from "metabase/collections/utils";
 
 import { PLUGIN_COLLECTION_COMPONENTS } from "metabase/plugins";
 
-type CollectionsMap = Record<Collection["id"], Collection>;
+type CollectionsMap = Partial<Record<Collection["id"], Collection>>;
 
 interface OwnProps {
   collectionParentId: Collection["id"];

frontend/src/metabase/collections/utils.ts

 import { t } from "ttag";
+import { isNotNull } from "metabase/core/utils/types";
 import { Collection, CollectionId, CollectionItem } from "metabase-types/api";
 
 export function nonPersonalOrArchivedCollection(
@@ -147,14 +148,14 @@ function isPersonalOrPersonalChild(
 
 export function canManageCollectionAuthorityLevel(
   collection: Partial<Collection>,
-  collectionMap: Record<CollectionId, Collection>,
+  collectionMap: Partial<Record<CollectionId, Collection>>,
 ) {
   if (isPersonalCollection(collection)) {
     return false;
   }
   const parentId = coerceCollectionId(collection.parent_id);
   const parentCollection = collectionMap[parentId];
-  const collections = Object.values(collectionMap);
+  const collections = Object.values(collectionMap).filter(isNotNull);
   return (
     parentCollection &&
     !isPersonalOrPersonalChild(parentCollection, collections)

frontend/src/metabase/containers/DataPicker/CardPicker/utils.ts

@@ -64,9 +64,12 @@ export function buildCollectionTree({
     }
   }
 
-  const targetModels: CollectionContentModel[] =
-    targetModel === "model" ? ["dataset"] : ["card"];
-  const tree = _buildCollectionTree(preparedCollections, { targetModels });
+  const modelFilter =
+    targetModel === "model"
+      ? (model: CollectionContentModel) => model === "dataset"
+      : (model: CollectionContentModel) => model === "card";
+
+  const tree = _buildCollectionTree(preparedCollections, modelFilter);
 
   if (rootCollection) {
     tree.unshift(getOurAnalyticsCollection(rootCollection));

frontend/src/metabase/entities/collections/constants.ts

@@ -19,7 +19,7 @@ export const PERSONAL_COLLECTION = {
 
 // fake collection for admins that contains all other user's collections
 export const PERSONAL_COLLECTIONS = {
-  id: "personal", // placeholder id
+  id: "personal" as const, // placeholder id
   name: t`All personal collections`,
   location: "/",
   path: [ROOT_COLLECTION.id],

frontend/src/metabase/entities/collections/utils.ts

-import _ from "underscore";
-
 import { IconProps } from "metabase/components/Icon";
 
 import { color } from "metabase/lib/colors";
@@ -56,46 +54,41 @@ export function getCollectionType(
   return collectionId !== undefined ? "other" : null;
 }
 
-function hasIntersection(list1: unknown[], list2?: unknown[]) {
-  if (!list2) {
-    return false;
-  }
-  return _.intersection(list1, list2).length > 0;
-}
-
 export interface CollectionTreeItem extends Collection {
   icon: string | IconProps;
   children: CollectionTreeItem[];
+  schemaName?: string;
 }
 
 export function buildCollectionTree(
-  collections: Collection[],
-  { targetModels }: { targetModels?: CollectionContentModel[] } = {},
+  collections: Collection[] = [],
+  modelFilter?: (model: CollectionContentModel) => boolean,
 ): CollectionTreeItem[] {
-  if (collections == null) {
-    return [];
-  }
-
-  const shouldFilterCollections = Array.isArray(targetModels);
-
   return collections.flatMap(collection => {
-    const hasTargetModels =
-      !shouldFilterCollections ||
-      hasIntersection(targetModels, collection.below) ||
-      hasIntersection(targetModels, collection.here);
-
-    return hasTargetModels
-      ? {
-          ...collection,
-          schemaName: collection.originalName || collection.name,
-          icon: getCollectionIcon(collection),
-          children: buildCollectionTree(
-            collection.children?.filter(child => !child.archived) || [],
-            {
-              targetModels,
-            },
-          ),
-        }
-      : [];
+    const isPersonalRoot = collection.id === PERSONAL_COLLECTIONS.id;
+    const isMatchedByFilter =
+      !modelFilter ||
+      collection.here?.some(modelFilter) ||
+      collection.below?.some(modelFilter);
+
+    if (!isPersonalRoot && !isMatchedByFilter) {
+      return [];
+    }
+
+    const children = buildCollectionTree(
+      collection.children?.filter(child => !child.archived) || [],
+      modelFilter,
+    );
+
+    if (isPersonalRoot && children.length === 0) {
+      return [];
+    }
+
+    return {
+      ...collection,
+      schemaName: collection.originalName || collection.name,
+      icon: getCollectionIcon(collection),
+      children,
+    };
   });
 }

frontend/src/metabase/entities/collections/tests/buildCollectionTree.unit.spec.js → frontend/src/metabase/entities/collections/utils.unit.spec.ts

 import { setupEnterpriseTest } from "__support__/enterprise";
-import { buildCollectionTree } from "metabase/entities/collections";
-
-function getCollection({
-  id = 1,
-  name = "Collection",
-  children = [],
-  ...rest
-} = {}) {
-  return {
-    id,
-    name,
-    children,
-    ...rest,
-  };
-}
+import { createMockCollection } from "metabase-types/api/mocks";
+import { PERSONAL_COLLECTIONS } from "./constants";
+import { buildCollectionTree } from "./utils";
 
 describe("buildCollectionTree", () => {
   it("returns an empty array when collections are not passed", () => {
@@ -21,9 +9,9 @@ describe("buildCollectionTree", () => {
   });
 
   it("correctly transforms collections", () => {
-    const collection = getCollection({ children: [] });
+    const collection = createMockCollection({ children: [] });
     const [transformed] = buildCollectionTree([collection]);
-    expect(transformed).toEqual({
+    expect(transformed).toMatchObject({
       id: collection.id,
       name: collection.name,
       schemaName: collection.name,
@@ -33,19 +21,22 @@ describe("buildCollectionTree", () => {
   });
 
   it("prefers originalName over name for schema names", () => {
-    const collection = getCollection({ name: "bar", originalName: "foo" });
+    const collection = createMockCollection({
+      name: "bar",
+      originalName: "foo",
+    });
     const [transformed] = buildCollectionTree([collection]);
     expect(transformed.schemaName).toBe(collection.originalName);
   });
 
   it("recursively transforms collection children", () => {
-    const grandchild = getCollection({ id: 3, name: "C3" });
-    const child = getCollection({
+    const grandchild = createMockCollection({ id: 3, name: "C3" });
+    const child = createMockCollection({
       id: 2,
       name: "C2",
       children: [grandchild],
     });
-    const collection = getCollection({
+    const collection = createMockCollection({
       id: 1,
       name: "C1",
       children: [child],
@@ -53,7 +44,7 @@ describe("buildCollectionTree", () => {
 
     const [transformed] = buildCollectionTree([collection]);
 
-    expect(transformed).toEqual({
+    expect(transformed).toMatchObject({
       id: collection.id,
       name: collection.name,
       schemaName: collection.name,
@@ -79,41 +70,42 @@ describe("buildCollectionTree", () => {
   });
 
   it("returns regular icon for official collections in OSS", () => {
-    const collection = getCollection({ authority_level: "official" });
+    const collection = createMockCollection({ authority_level: "official" });
     const [transformed] = buildCollectionTree([collection]);
     expect(transformed.icon).toEqual({ name: "folder" });
   });
 
   describe("filtering by models", () => {
     it("only keeps collection branches containing target models", () => {
-      const grandchild1 = getCollection({
+      const grandchild1 = createMockCollection({
         id: 4,
         name: "Grandchild 1",
         here: ["dataset"],
       });
-      const grandchild2 = getCollection({
+      const grandchild2 = createMockCollection({
         id: 3,
         name: "Grandchild 2",
         here: ["card"],
       });
-      const child = getCollection({
+      const child = createMockCollection({
         id: 2,
         name: "Child",
         below: ["dataset", "card"],
         children: [grandchild1, grandchild2],
       });
-      const collection = getCollection({
+      const collection = createMockCollection({
         id: 1,
         name: "Top-level",
         below: ["dataset", "card"],
         children: [child],
       });
 
-      const transformed = buildCollectionTree([collection], {
-        targetModels: ["dataset"],
-      });
+      const transformed = buildCollectionTree(
+        [collection],
+        model => model === "dataset",
+      );
 
-      expect(transformed).toEqual([
+      expect(transformed).toMatchObject([
         {
           id: collection.id,
           name: collection.name,
@@ -144,13 +136,13 @@ describe("buildCollectionTree", () => {
     });
 
     it("filters top-level collections not containing target models", () => {
-      const collectionWithDatasets = getCollection({
+      const collectionWithDatasets = createMockCollection({
         id: 1,
         name: "Top-level",
         here: ["dataset"],
         children: [],
       });
-      const collectionWithCards = getCollection({
+      const collectionWithCards = createMockCollection({
         id: 5,
         name: "Top-level 2",
         below: ["card"],
@@ -158,12 +150,10 @@ describe("buildCollectionTree", () => {
 
       const transformed = buildCollectionTree(
         [collectionWithDatasets, collectionWithCards],
-        {
-          targetModels: ["dataset"],
-        },
+        model => model === "dataset",
       );
 
-      expect(transformed).toEqual([
+      expect(transformed).toMatchObject([
         {
           id: collectionWithDatasets.id,
           name: collectionWithDatasets.name,
@@ -175,15 +165,103 @@ describe("buildCollectionTree", () => {
       ]);
     });
 
-    it("doesn't filter collections if targetModels are not passed", () => {
-      const child = getCollection({ id: 2, name: "Child", here: ["dataset"] });
-      const collection = getCollection({
+    it("preserves personal collections root if there are other users personal collections with target models", () => {
+      const collection = createMockCollection({
+        ...PERSONAL_COLLECTIONS,
+        children: [
+          createMockCollection({
+            name: "A",
+            below: ["card"],
+            children: [
+              createMockCollection({
+                name: "A1",
+                here: ["card"],
+              }),
+            ],
+          }),
+          createMockCollection({
+            name: "B",
+            below: ["dataset"],
+            children: [
+              createMockCollection({
+                name: "B1",
+                here: ["dataset"],
+              }),
+            ],
+          }),
+          createMockCollection({
+            name: "C",
+            children: [
+              createMockCollection({
+                name: "C1",
+              }),
+            ],
+          }),
+        ],
+      });
+
+      const collectionTree = buildCollectionTree(
+        [collection],
+        model => model === "card",
+      );
+
+      expect(collectionTree).toMatchObject([
+        {
+          ...PERSONAL_COLLECTIONS,
+          children: [
+            {
+              name: "A",
+              children: [
+                {
+                  name: "A1",
+                },
+              ],
+            },
+          ],
+        },
+      ]);
+    });
+
+    it("does not preserve personal collections root if there are no other users personal collections with target models", () => {
+      const collection = createMockCollection({
+        ...PERSONAL_COLLECTIONS,
+        children: [
+          createMockCollection({
+            name: "A",
+            here: ["dataset"],
+            children: [
+              createMockCollection({
+                name: "A1",
+              }),
+            ],
+          }),
+          createMockCollection({
+            name: "B",
+          }),
+        ],
+      });
+
+      const collectionTree = buildCollectionTree(
+        [collection],
+        model => model === "card",
+      );
+
+      expect(collectionTree).toEqual([]);
+    });
+
+    it("doesn't filter collections if model filter is not passed", () => {
+      const child = createMockCollection({
+        id: 2,
+        name: "Child",
+        here: ["dataset"],
+      });
+      const collection = createMockCollection({
         id: 1,
         name: "Top-level",
         below: ["dataset"],
         children: [child],
       });
-      const collectionWithCards = getCollection({
+      const collectionWithCards = createMockCollection({
         id: 5,
         name: "Top-level 2",
         below: ["card"],
@@ -194,7 +272,7 @@ describe("buildCollectionTree", () => {
         collectionWithCards,
       ]);
 
-      expect(transformed).toEqual([
+      expect(transformed).toMatchObject([
         {
           id: collection.id,
           name: collection.name,
@@ -230,7 +308,7 @@ describe("buildCollectionTree", () => {
     });
 
     it("returns correct icon for official collections", () => {
-      const collection = getCollection({ authority_level: "official" });
+      const collection = createMockCollection({ authority_level: "official" });
       const [transformed] = buildCollectionTree([collection]);
       expect(transformed.icon).toEqual({
         color: expect.any(String),

frontend/src/metabase/lib/collections.ts

+import { t } from "ttag";
+import { isNotNull } from "metabase/core/utils/types";
 import { Collection, CollectionId } from "metabase-types/api";
 
 export const getCrumbs = (
   collection: Collection,
-  collectionsById: Record<CollectionId, Collection>,
+  collectionsById: Partial<Record<CollectionId, Collection>>,
   callback: (id: CollectionId) => void,
 ) => {
   if (collection && collection.path) {
     return [
       ...collection.path
-        .filter(id => collectionsById[id])
-        .map(id => [collectionsById[id].name, () => callback(id)]),
+        .map(id => collectionsById[id])
+        .filter(isNotNull)
+        .map(collection => [collection.name, () => callback(collection.id)]),
       [collection.name],
     ];
   } else {
+    const rootCollection = collectionsById.root;
+
     return [
-      [
-        collectionsById["root"].name,
-        () => callback(collectionsById["root"].id),
-      ],
-      ["Unknown"],
+      ...(rootCollection
+        ? [[rootCollection.name, () => callback(rootCollection.id)]]
+        : []),
+      [t`Unknown`],
     ];
   }
 };

frontend/src/metabase/query_builder/components/DataSelector/saved-question-picker/SavedQuestionPicker.jsx

@@ -61,7 +61,9 @@ function SavedQuestionPicker({
   rootCollection,
 }) {
   const collectionTree = useMemo(() => {
-    const targetModels = isDatasets ? ["dataset"] : null;
+    const modelFilter = isDatasets
+      ? model => model === "dataset"
+      : model => model === "card";
 
     const preparedCollections = [];
     const userPersonalCollections = currentUserPersonalCollections(
@@ -92,7 +94,7 @@ function SavedQuestionPicker({
 
     return [
       ...(rootCollection ? [getOurAnalyticsCollection(rootCollection)] : []),
-      ...buildCollectionTree(preparedCollections, { targetModels }),
+      ...buildCollectionTree(preparedCollections, modelFilter),
     ];
   }, [collections, rootCollection, currentUser, isDatasets]);
 

frontend/src/metabase/query_builder/components/DataSelector/saved-question-picker/SavedQuestionPicker.unit.spec.jsx

@@ -22,8 +22,13 @@ const COLLECTIONS = {
     id: CURRENT_USER.personal_collection_id,
     name: "My personal collection",
     personal_owner_id: CURRENT_USER.id,
+    here: ["card"],
+  }),
+  REGULAR: createMockCollection({
+    id: 1,
+    name: "Regular collection",
+    here: ["card"],
   }),
-  REGULAR: createMockCollection({ id: 1, name: "Regular collection" }),
 };
 
 function mockCollectionTreeEndpoint() {

frontend/test/__support__/e2e/helpers/e2e-collection-helpers.js

-import { popover } from "__support__/e2e/helpers";
+import { getFullName, popover } from "__support__/e2e/helpers";
 
 /**
  * Clicks the "+" icon on the collection page and selects one of the menu options
@@ -39,3 +39,7 @@ export function visitCollection(id) {
 
   cy.wait([`@${alias}`, `@${alias}`]);
 }
+
+export function getPersonalCollectionName(user) {
+  return `${getFullName(user)}'s Personal Collection`;
+}

frontend/test/__support__/e2e/helpers/e2e-misc-helpers.js

@@ -229,7 +229,7 @@ export function saveQuestion(
   cy.findByText("Save").click();
 
   modal().within(() => {
-    cy.findByLabelText("Name").type(name);
+    cy.findByLabelText("Name").clear().type(name);
     cy.button("Save").click();
   });
 

frontend/test/metabase/scenarios/collections/collections.cy.spec.js

@@ -10,7 +10,7 @@ import {
   closeNavigationSidebar,
   openCollectionMenu,
   visitCollection,
-  getFullName,
+  getPersonalCollectionName,
 } from "__support__/e2e/helpers";
 import { USERS, USER_GROUPS } from "__support__/e2e/cypress_data";
 import { displaySidebarChildOf } from "./helpers/e2e-collections-sidebar.js";
@@ -504,10 +504,6 @@ describe("scenarios > collection defaults", () => {
   });
 });
 
-function getPersonalCollectionName(user) {
-  return `${getFullName(USERS.admin)}'s Personal Collection`;
-}
-
 function openEllipsisMenuFor(item) {
   cy.findByText(item)
     .closest("tr")

frontend/test/metabase/scenarios/question/new.cy.spec.js

@@ -6,9 +6,11 @@ import {
   startNewQuestion,
   visitQuestionAdhoc,
   getCollectionIdFromSlug,
+  saveQuestion,
+  getPersonalCollectionName,
 } from "__support__/e2e/helpers";
 
-import { SAMPLE_DB_ID } from "__support__/e2e/cypress_data";
+import { SAMPLE_DB_ID, USERS } from "__support__/e2e/cypress_data";
 import { SAMPLE_DATABASE } from "__support__/e2e/cypress_sample_database";
 
 const { ORDERS, ORDERS_ID } = SAMPLE_DATABASE;
@@ -88,9 +90,7 @@ describe("scenarios > question > new", () => {
         .should("have.length", 1)
         .as("leftSide")
         // should display the collection tree on the left side
-        .should("contain", "Our analytics")
-        .and("contain", "Your personal collection")
-        .and("contain", "All personal collections");
+        .should("contain", "Our analytics");
 
       cy.findByText("Orders, Count").click();
       cy.findByText("Orders").should("not.exist");
@@ -149,6 +149,29 @@ describe("scenarios > question > new", () => {
         cy.findByText("Second collection").should("not.exist");
       });
     });
+
+    it("should be possible to create a question based on a question in another user personal collection", () => {
+      cy.signOut();
+      cy.signIn("nocollection");
+      startNewQuestion();
+      popover().findByText("Orders").click();
+      visualize();
+      saveQuestion("Personal question");
+
+      cy.signOut();
+      cy.signInAsAdmin();
+      startNewQuestion();
+      popover().within(() => {
+        cy.findByText("Saved Questions").click();
+        cy.findByText("All personal collections").click();
+        cy.findByText(getPersonalCollectionName(USERS.normal)).should(
+          "not.exist",
+        );
+        cy.findByText(getPersonalCollectionName(USERS.nocollection)).click();
+        cy.findByText("Personal question").click();
+      });
+      visualize();
+    });
   });
 
   it("should remove `/notebook` from URL when converting question to SQL/Native (metabase#12651)", () => {

frontend/test/metabase/scenarios/question/question-management.cy.spec.js

@@ -8,6 +8,7 @@ import {
   navigationSidebar,
   openQuestionActions,
   questionInfoButton,
+  getPersonalCollectionName,
 } from "__support__/e2e/helpers";
 
 import { USERS } from "__support__/e2e/cypress_data";
@@ -249,12 +250,6 @@ function assertOnRequest(xhr_alias) {
   );
 }
 
-function getPersonalCollectionName(user) {
-  const name = [user.first_name, user.last_name].join(" ");
-
-  return `${name}'s Personal Collection`;
-}
-
 function turnIntoModel() {
   openQuestionActions();
   cy.findByText("Turn into a model").click();