[FIX: #39152] Public links refer to null when question is new (#39154)

metabase · Mar 2, 2024 · ead86bf · ead86bf
1 parent ee5e9e1
commit ead86bf
Show file tree

Hide file tree

Showing 12 changed files with 175 additions and 113 deletions.
diff --git a/e2e/test/scenarios/sharing/public-sharing-embed-button-behavior.cy.spec.js b/e2e/test/scenarios/sharing/public-sharing-embed-button-behavior.cy.spec.js
@@ -10,14 +10,17 @@ import {
   mantinePopover,
   modal,
   openEmbedModalFromMenu,
+  openNewPublicLinkDropdown,
   openPublicLinkPopoverFromMenu,
   openStaticEmbeddingModal,
   popover,
   resetSnowplow,
   restore,
   setTokenFeatures,
+  startNewQuestion,
   visitDashboard,
   visitQuestion,
+  visualize,
 } from "e2e/support/helpers";
 
 ["dashboard", "question"].forEach(resource => {
@@ -42,7 +45,7 @@ import {
             visitResource(resource, id);
           });
 
-          cy.findByTestId("dashboard-embed-button").click();
+          cy.findByTestId("resource-embed-button").click();
           cy.findByTestId("embed-header-menu").within(() => {
             cy.findByTestId("embed-menu-embed-modal-item").should(
               "be.disabled",
@@ -95,11 +98,7 @@ import {
 
             openPublicLinkPopoverFromMenu();
 
-            cy.findByTestId("public-link-popover-content").within(() => {
-              cy.findByText("Public link").should("be.visible");
-              cy.findByTestId("public-link-input").should("be.visible");
-              cy.findByText("Remove public link").should("be.visible");
-            });
+            assertValidPublicLink({ resource, shouldHaveRemoveLink: true });
           });
         });
 
@@ -124,11 +123,7 @@ import {
 
             openPublicLinkPopoverFromMenu();
 
-            cy.findByTestId("public-link-popover-content").within(() => {
-              cy.findByText("Public link").should("be.visible");
-              cy.findByTestId("public-link-input").should("be.visible");
-              cy.findByText("Remove public link").should("be.visible");
-            });
+            assertValidPublicLink({ resource, shouldHaveRemoveLink: true });
 
             cy.signInAsNormalUser();
 
@@ -138,10 +133,9 @@ import {
 
             cy.icon("share").click();
 
-            cy.findByTestId("public-link-popover-content").within(() => {
-              cy.findByText("Public link").should("be.visible");
-              cy.findByTestId("public-link-input").should("be.visible");
-              cy.findByText("Remove public link").should("not.exist");
+            assertValidPublicLink({
+              resource,
+              shouldHaveRemoveLink: false,
             });
           });
         });
@@ -256,6 +250,34 @@ describe("embed modal display", () => {
   });
 });
 
+describe("#39152 sharing an unsaved question", () => {
+  beforeEach(() => {
+    restore();
+    cy.signInAsAdmin();
+    cy.request("PUT", "/api/setting/enable-public-sharing", { value: true });
+  });
+
+  it("should ask the user to save the question before creating a public link", () => {
+    startNewQuestion();
+    popover().within(() => {
+      cy.findByText("Raw Data").click();
+      cy.findByText("People").click();
+    });
+    visualize();
+
+    cy.findByTestId("resource-embed-button").click();
+
+    modal().within(() => {
+      cy.findByText("First, save your question").should("be.visible");
+      cy.findByText("Save").click();
+    });
+
+    openNewPublicLinkDropdown("card");
+
+    assertValidPublicLink({ resource: "question", shouldHaveRemoveLink: true });
+  });
+});
+
 ["dashboard", "question"].forEach(resource => {
   describeWithSnowplow(`public ${resource} sharing snowplow events`, () => {
     beforeEach(() => {
@@ -719,9 +741,10 @@ describe("embed modal display", () => {
 function toSecond(milliseconds) {
   return Math.round(milliseconds / 1000);
 }
+
 function expectDisabledButtonWithTooltipLabel(tooltipLabel) {
-  cy.findByTestId("dashboard-embed-button").should("be.disabled");
-  cy.findByTestId("dashboard-embed-button").realHover();
+  cy.findByTestId("resource-embed-button").should("be.disabled");
+  cy.findByTestId("resource-embed-button").realHover();
   cy.findByRole("tooltip").findByText(tooltipLabel).should("be.visible");
 }
 
@@ -817,6 +840,27 @@ function enableEmbeddingForResource({ resource, id }) {
   });
 }
 
+function assertValidPublicLink({ resource, shouldHaveRemoveLink }) {
+  const regex = new RegExp(
+    `https?:\\/\\/[^\\/]+\\/public\\/${resource}\\/[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}(\\.csv|\\.json|\\.xlsx)?`,
+  );
+
+  cy.findByTestId("public-link-popover-content").within(() => {
+    cy.findByText("Public link").should("be.visible");
+
+    cy.findByTestId("public-link-input")
+      .should("be.visible")
+      .invoke("val")
+      .then(value => {
+        expect(value).to.match(regex);
+      });
+
+    cy.findByText("Remove public link").should(
+      shouldHaveRemoveLink ? "be.visible" : "not.exist",
+    );
+  });
+}
+
 function closeTo(value, offset) {
   return comparedValue => {
     return Math.abs(comparedValue - value) <= offset;

diff --git a/e2e/test/scenarios/visualizations-tabular/pivot_tables.cy.spec.js b/e2e/test/scenarios/visualizations-tabular/pivot_tables.cy.spec.js
@@ -14,6 +14,7 @@ import {
   getIframeBody,
   openPublicLinkPopoverFromMenu,
   openStaticEmbeddingModal,
+  modal,
 } from "e2e/support/helpers";
 
 const {
@@ -30,7 +31,7 @@ const QUESTION_NAME = "Cypress Pivot Table";
 const DASHBOARD_NAME = "Pivot Table Dashboard";
 
 const TEST_CASES = [
-  { case: "question", subject: QUESTION_NAME, confirmSave: true },
+  { case: "question", subject: QUESTION_NAME, confirmSave: false },
   { case: "dashboard", subject: DASHBOARD_NAME, confirmSave: false },
 ];
 
@@ -655,6 +656,12 @@ describe("scenarios > visualizations > pivot tables", { tags: "@slow" }, () => {
         });
 
         it("should display pivot table in a public link", () => {
+          if (test.case === "question") {
+            cy.icon("share").click();
+            modal().within(() => {
+              cy.findByText("Save").click();
+            });
+          }
           openPublicLinkPopoverFromMenu();
           cy.findByTestId("public-link-popover-content")
             .findByTestId("public-link-input")
@@ -677,6 +684,13 @@ describe("scenarios > visualizations > pivot tables", { tags: "@slow" }, () => {
         });
 
         it("should display pivot table in an embed URL", () => {
+          if (test.case === "question") {
+            cy.icon("share").click();
+            modal().within(() => {
+              cy.findByText("Save").click();
+            });
+          }
+
           openStaticEmbeddingModal({
             activeTab: "parameters",
             confirmSave: test.confirmSave,

diff --git a/...c/metabase/dashboard/components/DashboardEmbedHeaderButton/DashboardEmbedHeaderButton.tsx b/...c/metabase/dashboard/components/DashboardEmbedHeaderButton/DashboardEmbedHeaderButton.tsx
diff --git a/frontend/src/metabase/dashboard/components/DashboardEmbedHeaderButton/index.ts b/frontend/src/metabase/dashboard/components/DashboardEmbedHeaderButton/index.ts
diff --git a/frontend/src/metabase/dashboard/components/EmbedMenu/AdminEmbedMenu.tsx b/frontend/src/metabase/dashboard/components/EmbedMenu/AdminEmbedMenu.tsx
@@ -1,7 +1,6 @@
 import { useState } from "react";
 import { t } from "ttag";
 
-import { DashboardEmbedHeaderButton } from "metabase/dashboard/components/DashboardEmbedHeaderButton";
 import type {
   EmbedMenuModes,
   EmbedMenuProps,
@@ -11,6 +10,7 @@ import {
   QuestionPublicLinkPopover,
 } from "metabase/dashboard/components/PublicLinkPopover";
 import { useSelector } from "metabase/lib/redux";
+import { ResourceEmbedButton } from "metabase/public/components/ResourceEmbedButton";
 import { getSetting } from "metabase/selectors/settings";
 import { Menu, Title, Text, Stack, Center, Icon } from "metabase/ui";
 
@@ -32,7 +32,7 @@ export const AdminEmbedMenu = ({
   );
 
   const target = (
-    <DashboardEmbedHeaderButton hasBackground={resourceType === "dashboard"} />
+    <ResourceEmbedButton hasBackground={resourceType === "dashboard"} />
   );
 
   if (menuMode === "public-link-popover") {

diff --git a/frontend/src/metabase/dashboard/components/EmbedMenu/AdminEmbedMenu.unit.spec.tsx b/frontend/src/metabase/dashboard/components/EmbedMenu/AdminEmbedMenu.unit.spec.tsx
@@ -51,13 +51,13 @@ describe("AdminEmbedMenu", () => {
   describe("when public sharing enabled, public link exists, embedding enabled", () => {
     it("should have a `Sharing` tooltip", () => {
       setup();
-      userEvent.hover(screen.getByTestId("dashboard-embed-button"));
+      userEvent.hover(screen.getByTestId("resource-embed-button"));
       expect(screen.getByRole("tooltip")).toHaveTextContent("Sharing");
     });
 
     it("should show `Public link` and `Embed` options", async () => {
       setup();
-      userEvent.click(screen.getByTestId("dashboard-embed-button"));
+      userEvent.click(screen.getByTestId("resource-embed-button"));
 
       expect(
         await screen.findByTestId("embed-header-menu"),
@@ -72,7 +72,7 @@ describe("AdminEmbedMenu", () => {
 
     it("should open the public link popover when `Public link` is clicked", async () => {
       setup();
-      userEvent.click(screen.getByTestId("dashboard-embed-button"));
+      userEvent.click(screen.getByTestId("resource-embed-button"));
 
       expect(
         await screen.findByTestId("embed-header-menu"),
@@ -101,7 +101,7 @@ describe("AdminEmbedMenu", () => {
 
     it("should open the embed modal when `Embed` is clicked", async () => {
       const { onModalOpen } = setup();
-      userEvent.click(screen.getByTestId("dashboard-embed-button"));
+      userEvent.click(screen.getByTestId("resource-embed-button"));
 
       expect(
         await screen.findByTestId("embed-header-menu"),
@@ -119,12 +119,12 @@ describe("AdminEmbedMenu", () => {
     });
 
     it("should have a `Sharing` tooltip", () => {
-      userEvent.hover(screen.getByTestId("dashboard-embed-button"));
+      userEvent.hover(screen.getByTestId("resource-embed-button"));
       expect(screen.getByRole("tooltip")).toHaveTextContent("Sharing");
     });
 
     it("should show `Create a public link` and `Embed` options", async () => {
-      userEvent.click(screen.getByTestId("dashboard-embed-button"));
+      userEvent.click(screen.getByTestId("resource-embed-button"));
 
       expect(
         await screen.findByTestId("embed-header-menu"),
@@ -144,12 +144,12 @@ describe("AdminEmbedMenu", () => {
     });
 
     it("should have a `Sharing` tooltip", () => {
-      userEvent.hover(screen.getByTestId("dashboard-embed-button"));
+      userEvent.hover(screen.getByTestId("resource-embed-button"));
       expect(screen.getByRole("tooltip")).toHaveTextContent("Sharing");
     });
 
     it("should show `Public link` and `Embed` options", async () => {
-      userEvent.click(screen.getByTestId("dashboard-embed-button"));
+      userEvent.click(screen.getByTestId("resource-embed-button"));
 
       expect(
         await screen.findByTestId("embed-header-menu"),
@@ -170,12 +170,12 @@ describe("AdminEmbedMenu", () => {
     });
 
     it("should have a `Sharing` tooltip", () => {
-      userEvent.hover(screen.getByTestId("dashboard-embed-button"));
+      userEvent.hover(screen.getByTestId("resource-embed-button"));
       expect(screen.getByRole("tooltip")).toHaveTextContent("Sharing");
     });
 
     it("should show `Public link` and `Embed` options", async () => {
-      userEvent.click(screen.getByTestId("dashboard-embed-button"));
+      userEvent.click(screen.getByTestId("resource-embed-button"));
 
       expect(
         await screen.findByTestId("embed-header-menu"),
@@ -196,12 +196,12 @@ describe("AdminEmbedMenu", () => {
     });
 
     it("should have an `Embedding` tooltip", () => {
-      userEvent.hover(screen.getByTestId("dashboard-embed-button"));
+      userEvent.hover(screen.getByTestId("resource-embed-button"));
       expect(screen.getByRole("tooltip")).toHaveTextContent("Embedding");
     });
 
     it("should show `Public link` and `Embed` options", async () => {
-      userEvent.click(screen.getByTestId("dashboard-embed-button"));
+      userEvent.click(screen.getByTestId("resource-embed-button"));
 
       expect(
         await screen.findByTestId("embed-header-menu"),
@@ -222,12 +222,12 @@ describe("AdminEmbedMenu", () => {
     });
 
     it("should have an `Embedding` tooltip", () => {
-      userEvent.hover(screen.getByTestId("dashboard-embed-button"));
+      userEvent.hover(screen.getByTestId("resource-embed-button"));
       expect(screen.getByRole("tooltip")).toHaveTextContent("Embedding");
     });
 
     it("should show `Public link` and `Embed` options", async () => {
-      userEvent.click(screen.getByTestId("dashboard-embed-button"));
+      userEvent.click(screen.getByTestId("resource-embed-button"));
 
       expect(
         await screen.findByTestId("embed-header-menu"),