[Bug Fix] Use effective location when building initial path in entity…

… picker (#44327) * use effective location when building initial path in entity picker * unit test adjustment * more unit test adjustments
metabase · Jun 18, 2024 · 413efac · 413efac
1 parent bc78552
commit 413efac
Show file tree

Hide file tree

Showing 6 changed files with 97 additions and 7 deletions.
diff --git a/e2e/test/scenarios/collections/collections.cy.spec.js b/e2e/test/scenarios/collections/collections.cy.spec.js
@@ -1,14 +1,15 @@
 import { assocIn } from "icepick";
 import _ from "underscore";
 
-import { USERS, USER_GROUPS } from "e2e/support/cypress_data";
+import { SAMPLE_DB_ID, USERS, USER_GROUPS } from "e2e/support/cypress_data";
 import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
 import {
   ORDERS_QUESTION_ID,
   FIRST_COLLECTION_ID,
   SECOND_COLLECTION_ID,
   THIRD_COLLECTION_ID,
   ADMIN_PERSONAL_COLLECTION_ID,
+  ALL_USERS_GROUP_ID,
 } from "e2e/support/cypress_sample_instance_data";
 import {
   restore,
@@ -25,13 +26,15 @@ import {
   pickEntity,
   entityPickerModal,
   openCollectionMenu,
+  createQuestion,
+  entityPickerModalItem,
 } from "e2e/support/helpers";
 
 import { displaySidebarChildOf } from "./helpers/e2e-collections-sidebar.js";
 
 const { nocollection } = USERS;
 const { DATA_GROUP } = USER_GROUPS;
-const { ORDERS, ORDERS_ID } = SAMPLE_DATABASE;
+const { ORDERS, ORDERS_ID, FEEDBACK_ID } = SAMPLE_DATABASE;
 
 describe("scenarios > collection defaults", () => {
   beforeEach(() => {
@@ -280,6 +283,74 @@ describe("scenarios > collection defaults", () => {
       cy.signInAsAdmin();
     });
 
+    it("should handle moving a question when you don't have access to entier collection path (metabase#44316", () => {
+      cy.createCollection({
+        name: "Collection A",
+      }).then(({ body: collectionA }) => {
+        cy.createCollection({
+          name: "Collection B",
+          parent_id: collectionA.id,
+        }).then(({ body: collectionB }) => {
+          cy.createCollection({
+            name: "Collection C",
+            parent_id: collectionB.id,
+          }).then(({ body: collectionC }) => {
+            cy.createCollection({
+              name: "Collection D",
+              parent_id: collectionC.id,
+            }).then(({ body: collectionD }) => {
+              cy.createCollection({
+                name: "Collection E",
+                parent_id: collectionD.id,
+              }).then(({ body: collectionE }) => {
+                cy.updatePermissionsGraph({
+                  [ALL_USERS_GROUP_ID]: {
+                    [SAMPLE_DB_ID]: {
+                      "view-data": "unrestricted",
+                      "create-queries": "query-builder-and-native",
+                    },
+                  },
+                });
+                cy.updateCollectionGraph({
+                  [ALL_USERS_GROUP_ID]: {
+                    root: "none",
+                    [collectionA.id]: "none",
+                    [collectionB.id]: "write",
+                    [collectionC.id]: "none",
+                    [collectionD.id]: "none",
+                    [collectionE.id]: "write",
+                  },
+                });
+                cy.signIn("none");
+                createQuestion(
+                  {
+                    name: "Foo Question",
+                    query: {
+                      "source-table": FEEDBACK_ID,
+                    },
+                    collection_id: collectionE.id,
+                  },
+                  {
+                    visitQuestion: true,
+                  },
+                );
+              });
+            });
+          });
+        });
+      });
+
+      cy.findByTestId("qb-header").icon("ellipsis").click();
+      popover().findByText("Move").click();
+      entityPickerModalItem(1, "Collection B").should("exist");
+      entityPickerModalItem(2, "Collection E").should("exist");
+
+      entityPickerModal().should(
+        "not.contain.text",
+        "You don't have permissions to do that.",
+      );
+    });
+
     it("should show list of collection items even if one question has invalid parameters (metabase#25543)", () => {
       const questionDetails = {
         native: { query: "select 1 --[[]]", "template-tags": {} },

diff --git a/frontend/src/metabase/common/components/CollectionPicker/components/CollectionPicker.tsx b/frontend/src/metabase/common/components/CollectionPicker/components/CollectionPicker.tsx
@@ -158,7 +158,7 @@ export const CollectionPickerInner = (
           idPath: getCollectionIdPath(
             {
               id: currentCollection.id,
-              location: currentCollection.location,
+              location: currentCollection.effective_location,
               is_personal: currentCollection.is_personal,
             },
             userPersonalCollectionId,

diff --git a/...src/metabase/common/components/CollectionPicker/components/CollectionPicker.unit.spec.tsx b/...src/metabase/common/components/CollectionPicker/components/CollectionPicker.unit.spec.tsx
@@ -11,7 +11,7 @@ import {
   within,
   waitFor,
 } from "__support__/ui";
-import type { CollectionId } from "metabase-types/api";
+import type { Collection, CollectionId } from "metabase-types/api";
 import {
   createMockCollection,
   createMockCollectionItem,
@@ -25,6 +25,7 @@ type MockCollection = {
   id: CollectionId;
   name: string;
   location: string | null;
+  effective_location: string | null;
   is_personal: boolean;
   collections: MockCollection[];
 };
@@ -34,19 +35,22 @@ const collectionTree: MockCollection[] = [
     id: "root",
     name: "Our Analytics",
     location: null,
+    effective_location: null,
     is_personal: false,
     collections: [
       {
         id: 4,
         name: "Collection 4",
         location: "/",
+        effective_location: "/",
         is_personal: false,
         collections: [
           {
             id: 3,
             name: "Collection 3",
             collections: [],
             location: "/4/",
+            effective_location: "/4/",
             is_personal: false,
           },
         ],
@@ -56,6 +60,7 @@ const collectionTree: MockCollection[] = [
         is_personal: false,
         name: "Collection 2",
         location: "/",
+        effective_location: "/",
         collections: [],
       },
     ],
@@ -64,11 +69,13 @@ const collectionTree: MockCollection[] = [
     name: "My personal collection",
     id: 1,
     location: "/",
+    effective_location: "/",
     is_personal: true,
     collections: [
       {
         id: 5,
         location: "/1/",
+        effective_location: "/1/",
         name: "personal sub_collection",
         is_personal: true,
         collections: [],
@@ -86,6 +93,7 @@ const flattenCollectionTree = (
       id: n.id,
       is_personal: !!n.is_personal,
       location: n.location,
+      effective_location: n.effective_location,
     })),
   ].concat(...node.map(n => flattenCollectionTree(n.collections)));
 };
@@ -98,6 +106,7 @@ const setupCollectionTreeMocks = (node: MockCollection[]) => {
         name: c.name,
         model: "collection",
         location: c.location || "/",
+        effective_location: c.effective_location || "/",
       }),
     );
 
@@ -128,8 +137,9 @@ const setup = ({
   mockGetBoundingClientRect();
   mockScrollBy();
 
-  const allCollections =
-    flattenCollectionTree(collectionTree).map(createMockCollection);
+  const allCollections = flattenCollectionTree(collectionTree).map(c =>
+    createMockCollection(c as Collection),
+  );
 
   //Setup individual collection mocks
   allCollections.forEach(collection => {

diff --git a/frontend/src/metabase/common/components/DashboardPicker/components/DashboardPicker.tsx b/frontend/src/metabase/common/components/DashboardPicker/components/DashboardPicker.tsx
@@ -169,7 +169,7 @@ const DashboardPickerInner = (
         const idPath = getCollectionIdPath(
           {
             id: currentCollection.id,
-            location: currentCollection.location,
+            location: currentCollection.effective_location,
             is_personal: currentCollection.is_personal,
           },
           userPersonalCollectionId,

diff --git a/...d/src/metabase/common/components/DashboardPicker/components/DashboardPicker.unit.spec.tsx b/...d/src/metabase/common/components/DashboardPicker/components/DashboardPicker.unit.spec.tsx
@@ -60,6 +60,7 @@ const collectionTree: NestedCollectionItem[] = [
         name: "Collection 4",
         model: "collection",
         location: "/",
+        effective_location: "/",
         can_write: true,
         descendants: [
           {
@@ -79,6 +80,7 @@ const collectionTree: NestedCollectionItem[] = [
               },
             ],
             location: "/4/",
+            effective_location: "/4/",
             can_write: true,
             is_personal: false,
           },
@@ -90,6 +92,7 @@ const collectionTree: NestedCollectionItem[] = [
         is_personal: false,
         name: "Collection 2",
         location: "/",
+        effective_location: "/",
         can_write: true,
         descendants: [],
       },
@@ -100,13 +103,15 @@ const collectionTree: NestedCollectionItem[] = [
     id: 1,
     model: "collection",
     location: "/",
+    effective_location: "/",
     is_personal: true,
     can_write: true,
     descendants: [
       {
         id: 5,
         model: "collection",
         location: "/1/",
+        effective_location: "/1/",
         name: "personal sub_collection",
         is_personal: true,
         can_write: true,

diff --git a/...d/src/metabase/containers/AddToDashSelectDashModal/AddToDashSelectDashModal.unit.spec.tsx b/...d/src/metabase/containers/AddToDashSelectDashModal/AddToDashSelectDashModal.unit.spec.tsx
@@ -63,6 +63,7 @@ const COLLECTION = createMockCollection({
   can_write: true,
   is_personal: false,
   location: "/",
+  effective_location: "/",
 });
 
 const SUBCOLLECTION = createMockCollection({
@@ -71,6 +72,7 @@ const SUBCOLLECTION = createMockCollection({
   can_write: true,
   is_personal: false,
   location: `/${COLLECTION.id}/`,
+  effective_location: `/${COLLECTION.id}/`,
 });
 
 const PERSONAL_COLLECTION = createMockCollection({
@@ -80,6 +82,7 @@ const PERSONAL_COLLECTION = createMockCollection({
   can_write: true,
   is_personal: true,
   location: "/",
+  effective_location: "/",
 });
 
 const PERSONAL_SUBCOLLECTION = createMockCollection({
@@ -88,6 +91,7 @@ const PERSONAL_SUBCOLLECTION = createMockCollection({
   can_write: true,
   is_personal: true,
   location: `/${PERSONAL_COLLECTION.id}/`,
+  effective_location: `/${PERSONAL_COLLECTION.id}/`,
 });
 
 const ROOT_COLLECTION = createMockCollection({