diff --git a/frontend/src/metabase-types/api/collection.ts b/frontend/src/metabase-types/api/collection.ts index 64c216752a3067231842e8b2f4c33d25afba35cc..ea1930680fc52bb1715f121fd24bf089fae2fa68 100644 --- a/frontend/src/metabase-types/api/collection.ts +++ b/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"; diff --git a/frontend/src/metabase-types/store/entities.ts b/frontend/src/metabase-types/store/entities.ts index 3f1e2900137b3dc9da3cd4ee76384400843d5cc4..0a5347f0f1128b1a2eb3aecb9b11427b661c2a86 100644 --- a/frontend/src/metabase-types/store/entities.ts +++ b/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>; diff --git a/frontend/src/metabase/collections/containers/FormAuthorityLevelFieldContainer.tsx b/frontend/src/metabase/collections/containers/FormAuthorityLevelFieldContainer.tsx index c3108c4ac7c4ae7ff01e9aa6f84a20c1fb8c98f1..04b3fdf0ff6f80c8569904b4465a42670743fcc0 100644 --- a/frontend/src/metabase/collections/containers/FormAuthorityLevelFieldContainer.tsx +++ b/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"]; diff --git a/frontend/src/metabase/collections/utils.ts b/frontend/src/metabase/collections/utils.ts index 136fb9a5b2898a4daf996a18a522cd448b5dee1d..1ea913a7f8ae903c60032c650f2e22eda4badc8c 100644 --- a/frontend/src/metabase/collections/utils.ts +++ b/frontend/src/metabase/collections/utils.ts @@ -1,4 +1,5 @@ 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) diff --git a/frontend/src/metabase/containers/DataPicker/CardPicker/utils.ts b/frontend/src/metabase/containers/DataPicker/CardPicker/utils.ts index 6a5a14691eae7f9903c83df43aff9a2fb00c0d19..b552c54104f826d4c0c43a35dc966f3d2e4bb2b9 100644 --- a/frontend/src/metabase/containers/DataPicker/CardPicker/utils.ts +++ b/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)); diff --git a/frontend/src/metabase/entities/collections/constants.ts b/frontend/src/metabase/entities/collections/constants.ts index 31d0d07829e4654e982927dc8e27383bc9639ddd..7d8ab7c1bd682f4be5bfbc8a863f1625e93d7a83 100644 --- a/frontend/src/metabase/entities/collections/constants.ts +++ b/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], diff --git a/frontend/src/metabase/entities/collections/utils.ts b/frontend/src/metabase/entities/collections/utils.ts index e9b9f6a422b1a51abbca19772825c621630dbc60..4f5bfbd7462ba1e85d9bca94ac3fdd9d6d44cee0 100644 --- a/frontend/src/metabase/entities/collections/utils.ts +++ b/frontend/src/metabase/entities/collections/utils.ts @@ -1,5 +1,3 @@ -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, + }; }); } diff --git a/frontend/src/metabase/entities/collections/tests/buildCollectionTree.unit.spec.js b/frontend/src/metabase/entities/collections/utils.unit.spec.ts similarity index 57% rename from frontend/src/metabase/entities/collections/tests/buildCollectionTree.unit.spec.js rename to frontend/src/metabase/entities/collections/utils.unit.spec.ts index 6f5e7f6fa7b7b62416113f75cae356e2ea2a5cb9..cb8372d73611941220161843cdeee69dba7f69e3 100644 --- a/frontend/src/metabase/entities/collections/tests/buildCollectionTree.unit.spec.js +++ b/frontend/src/metabase/entities/collections/utils.unit.spec.ts @@ -1,19 +1,7 @@ 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), diff --git a/frontend/src/metabase/lib/collections.ts b/frontend/src/metabase/lib/collections.ts index dc5276f1dd5ecfb2ac03b7937a758b7360e61564..1ecdb78debc6e8c0314a3c1e75c66f14a6ebbec1 100644 --- a/frontend/src/metabase/lib/collections.ts +++ b/frontend/src/metabase/lib/collections.ts @@ -1,24 +1,28 @@ +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`], ]; } }; diff --git a/frontend/src/metabase/query_builder/components/DataSelector/saved-question-picker/SavedQuestionPicker.jsx b/frontend/src/metabase/query_builder/components/DataSelector/saved-question-picker/SavedQuestionPicker.jsx index f9f9880d81cb045672daad0a3f312957ca8c09a0..8e8fdbd230190f6c7612279f6678b90efca610d5 100644 --- a/frontend/src/metabase/query_builder/components/DataSelector/saved-question-picker/SavedQuestionPicker.jsx +++ b/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]); diff --git a/frontend/src/metabase/query_builder/components/DataSelector/saved-question-picker/SavedQuestionPicker.unit.spec.jsx b/frontend/src/metabase/query_builder/components/DataSelector/saved-question-picker/SavedQuestionPicker.unit.spec.jsx index 98602ca9b12afb1e7b18638ca1baed573544a7ba..830c3df0c124062f6de1551ce2c50e6ecc2d6e33 100644 --- a/frontend/src/metabase/query_builder/components/DataSelector/saved-question-picker/SavedQuestionPicker.unit.spec.jsx +++ b/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() { diff --git a/frontend/test/__support__/e2e/helpers/e2e-collection-helpers.js b/frontend/test/__support__/e2e/helpers/e2e-collection-helpers.js index 1dac50902a46c1868d1408abc87211e676813373..a8b1c791dd3244bc1ce69dbc8caa733b9b519bc0 100644 --- a/frontend/test/__support__/e2e/helpers/e2e-collection-helpers.js +++ b/frontend/test/__support__/e2e/helpers/e2e-collection-helpers.js @@ -1,4 +1,4 @@ -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`; +} diff --git a/frontend/test/__support__/e2e/helpers/e2e-misc-helpers.js b/frontend/test/__support__/e2e/helpers/e2e-misc-helpers.js index 26feff8077daf9ce67555e1e22e7199e109a3eea..07298563b82745cfba3d0117b477dfca21616b91 100644 --- a/frontend/test/__support__/e2e/helpers/e2e-misc-helpers.js +++ b/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(); }); diff --git a/frontend/test/metabase/scenarios/collections/collections.cy.spec.js b/frontend/test/metabase/scenarios/collections/collections.cy.spec.js index 936588248e59e8727d21ea1565cfe5b79fcb230f..8f9e7265154e7a055d1ebfa7ad059849932391a8 100644 --- a/frontend/test/metabase/scenarios/collections/collections.cy.spec.js +++ b/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") diff --git a/frontend/test/metabase/scenarios/question/new.cy.spec.js b/frontend/test/metabase/scenarios/question/new.cy.spec.js index e6048693e1b85d9cbabdd7b7e3e610a0bd437bc7..be4d70f3af444a530e358839ca21d2cb0c914454 100644 --- a/frontend/test/metabase/scenarios/question/new.cy.spec.js +++ b/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)", () => { diff --git a/frontend/test/metabase/scenarios/question/question-management.cy.spec.js b/frontend/test/metabase/scenarios/question/question-management.cy.spec.js index 1ffcabba617ccbbde7af25d4cbb9c1ddae72674f..cb28106325bf91734a8e28a8c64cd9658f312dd3 100644 --- a/frontend/test/metabase/scenarios/question/question-management.cy.spec.js +++ b/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();