Feedback for the Embedding Homepage (#40746)

* FeedbackModal for the Embedding Homepage

* add source and change to correct endpoint

* fix tests, adding await and fixing endpoint

* use toEqual to also check for the source

* addresses PR feedback

* button becomes Send when email is written

* make 'comments' optional, trim inputs and refactor tests

* product-feedback endpoint (#40974)

* mvp of the proxy endpoint for the product feedback

* cleanup and fixes

* email :string -> ms/NonBlankString

* revert my solution

* Dan diff file

* makes fields other than source optional

* don't use default value in dev, to not spam the prod endpoint

* change from setting to function for product-feedback-url, as it's not meant to be changed by end users

* changed endpoint as per https://metaboat.slack.com/archives/C010ZSXQY87/p1712918951362979?thread_ts=1704910781.153589&cid=C010ZSXQY87



* fix wrong dismiss reason sent for 'I'm not interested right now'

* Update frontend/src/metabase/home/components/EmbedHomepage/EmbedHomepage.tsx

Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev>

* renamed field: feedback -> comment

---------

Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev>

frontend/src/metabase/api/product-feedback.ts

+import { Api } from "./api";
+
+const productFeedbackApi = Api.injectEndpoints({
+  endpoints: builder => ({
+    sendProductFeedback: builder.mutation<
+      void,
+      { comment?: string; email?: string; source: string }
+    >({
+      query: ({ comment, email, source }) => ({
+        method: "POST",
+        url: `/api/util/product-feedback`,
+        body: {
+          // harbormaster expects the field `comments`, we use `comment` because it
+          // reflects better what it's shown in the UI
+          comments: comment,
+          email,
+          source,
+        },
+      }),
+    }),
+  }),
+});
+
+export const { useSendProductFeedbackMutation } = productFeedbackApi;

frontend/src/metabase/home/components/EmbedHomepage/EmbedHomepage.tsx

-import { useMemo } from "react";
+import { useMemo, useState } from "react";
 
 import { updateSetting } from "metabase/admin/settings/settings";
+import { useSendProductFeedbackMutation } from "metabase/api/product-feedback";
 import { useSetting } from "metabase/common/hooks";
 import { getPlan } from "metabase/common/utils/plan";
 import { useDispatch, useSelector } from "metabase/lib/redux";
@@ -8,13 +9,16 @@ import { isEEBuild } from "metabase/lib/utils";
 import { getDocsUrl, getSetting } from "metabase/selectors/settings";
 
 import { EmbedHomepageView } from "./EmbedHomepageView";
+import { FeedbackModal } from "./FeedbackModal";
 import type { EmbedHomepageDismissReason } from "./types";
 
 export const EmbedHomepage = () => {
+  const [feedbackModalOpened, setFeedbackModalOpened] = useState(false);
   const dispatch = useDispatch();
   const embeddingAutoEnabled = useSetting("setup-embedding-autoenabled");
   const licenseActiveAtSetup = useSetting("setup-license-active-at-setup");
   const exampleDashboardId = useSetting("example-dashboard-id");
+  const [sendProductFeedback] = useSendProductFeedbackMutation();
 
   const interactiveEmbeddingQuickStartUrl = useSelector(state =>
     // eslint-disable-next-line no-unconditional-metabase-links-render -- only visible to admins
@@ -51,22 +55,55 @@ export const EmbedHomepage = () => {
   }, [plan]);
 
   const onDismiss = (reason: EmbedHomepageDismissReason) => {
-    dispatch(updateSetting({ key: "embedding-homepage", value: reason }));
+    if (reason === "dismissed-run-into-issues") {
+      setFeedbackModalOpened(true);
+    } else {
+      dispatch(updateSetting({ key: "embedding-homepage", value: reason }));
+    }
+  };
+
+  const onFeedbackSubmit = ({
+    comment,
+    email,
+  }: {
+    comment?: string;
+    email?: string;
+  }) => {
+    dispatch(
+      updateSetting({
+        key: "embedding-homepage",
+        value: "dismiss-run-into-issues",
+      }),
+    );
+
+    setFeedbackModalOpened(false);
+    sendProductFeedback({
+      comment,
+      email: email,
+      source: "embedding-homepage-dismiss",
+    });
   };
 
   return (
-    <EmbedHomepageView
-      onDismiss={onDismiss}
-      exampleDashboardId={exampleDashboardId}
-      embeddingAutoEnabled={embeddingAutoEnabled}
-      licenseActiveAtSetup={licenseActiveAtSetup}
-      defaultTab={defaultTab}
-      interactiveEmbeddingQuickstartUrl={interactiveEmbeddingQuickStartUrl}
-      embeddingDocsUrl={embeddingDocsUrl}
-      // eslint-disable-next-line no-unconditional-metabase-links-render -- only visible to admins
-      analyticsDocsUrl="https://www.metabase.com/learn/customer-facing-analytics/"
-      learnMoreInteractiveEmbedUrl={learnMoreInteractiveEmbedding}
-      learnMoreStaticEmbedUrl={learnMoreStaticEmbedding}
-    />
+    <>
+      <EmbedHomepageView
+        onDismiss={onDismiss}
+        exampleDashboardId={exampleDashboardId}
+        embeddingAutoEnabled={embeddingAutoEnabled}
+        licenseActiveAtSetup={licenseActiveAtSetup}
+        defaultTab={defaultTab}
+        interactiveEmbeddingQuickstartUrl={interactiveEmbeddingQuickStartUrl}
+        embeddingDocsUrl={embeddingDocsUrl}
+        // eslint-disable-next-line no-unconditional-metabase-links-render -- only visible to admins
+        analyticsDocsUrl="https://www.metabase.com/learn/customer-facing-analytics/"
+        learnMoreInteractiveEmbedUrl={learnMoreInteractiveEmbedding}
+        learnMoreStaticEmbedUrl={learnMoreStaticEmbedding}
+      />
+      <FeedbackModal
+        opened={feedbackModalOpened}
+        onClose={() => setFeedbackModalOpened(false)}
+        onSubmit={onFeedbackSubmit}
+      />
+    </>
   );
 };

frontend/src/metabase/home/components/EmbedHomepage/EmbedHomepageView.tsx

@@ -44,7 +44,7 @@ export const EmbedHomepageView = (props: EmbedHomepageViewProps) => {
       <Group position="apart">
         {/*  eslint-disable-next-line no-literal-metabase-strings -- only visible to admins */}
         <Text fw="bold">{t`Get started with Embedding Metabase in your app`}</Text>
-        <Menu trigger="hover">
+        <Menu trigger="hover" closeDelay={200}>
           <Menu.Target>
             <Text
               fw="bold"
@@ -60,7 +60,7 @@ export const EmbedHomepageView = (props: EmbedHomepageViewProps) => {
               onClick={() => onDismiss("dismissed-run-into-issues")}
             >{t`I ran into issues`}</Menu.Item>
             <Menu.Item
-              onClick={() => onDismiss("dismissed-run-into-issues")}
+              onClick={() => onDismiss("dismissed-not-interested-now")}
             >{t`I'm not interested right now`}</Menu.Item>
           </Menu.Dropdown>
         </Menu>

frontend/src/metabase/home/components/EmbedHomepage/FeedbackModal.tsx

+import { useState } from "react";
+import { t } from "ttag";
+
+import {
+  Button,
+  Group,
+  Modal,
+  rem,
+  Stack,
+  Text,
+  Textarea,
+  TextInput,
+  Title,
+} from "metabase/ui";
+
+type FeedbackModalProps = {
+  opened: boolean;
+  onClose: () => void;
+  onSubmit: (feedback: { comment?: string; email?: string }) => void;
+};
+
+export const FeedbackModal = ({
+  opened,
+  onClose,
+  onSubmit,
+}: FeedbackModalProps) => {
+  const [comment, setComment] = useState("");
+  const [email, setEmail] = useState("");
+  const handleSubmit = () =>
+    onSubmit({
+      comment: comment.trim() || undefined,
+      email: email.trim() || undefined,
+    });
+
+  return (
+    <Modal
+      size={rem(530)}
+      padding="xl"
+      opened={opened}
+      withCloseButton={false}
+      onClose={onClose}
+    >
+      <Title pb="sm">{t`How can we improve embedding?`}</Title>
+      <Stack spacing="lg">
+        {/* eslint-disable-next-line no-literal-metabase-strings -- only admins can see this component */}
+        <Text>{t`Please let us know what happened. We’re always looking for ways to improve Metabase.`}</Text>
+
+        <Textarea
+          label={t`Feedback`}
+          name="comment"
+          placeholder={t`Tell us what happened`}
+          onChange={e => setComment(e.currentTarget.value)}
+          minRows={3}
+        />
+
+        <TextInput
+          label={t`Email`}
+          type="email"
+          name="email"
+          placeholder={t`Leave your email if you want us to follow up with you`}
+          onChange={e => setEmail(e.currentTarget.value)}
+        />
+
+        <Group position="right">
+          <Button onClick={onClose}>{t`Cancel`}</Button>
+          <Button variant="filled" onClick={handleSubmit}>
+            {comment.trim().length + email.trim().length > 0
+              ? t`Send`
+              : t`Skip`}
+          </Button>
+        </Group>
+      </Stack>
+    </Modal>
+  );
+};

frontend/src/metabase/home/components/EmbedHomepage/tests/common.unit.spec.tsx

 import userEvent from "@testing-library/user-event";
-import fetchMock from "fetch-mock";
 
-import { screen } from "__support__/ui";
+import { screen, waitForElementToBeRemoved } from "__support__/ui";
 
-import { setup } from "./setup";
+import {
+  queryFeedbackModal,
+  getLastHomepageSettingSettingCall,
+  setup,
+  getLastFeedbackCall,
+} from "./setup";
 
 describe("EmbedHomepage (OSS)", () => {
   it("should default to the static tab for OSS builds", () => {
@@ -13,6 +17,7 @@ describe("EmbedHomepage (OSS)", () => {
     ).toBeInTheDocument();
 
     // making sure Tabs isn't just rendering both tabs, making the test always pass
+
     expect(
       screen.queryByText("Use interactive embedding", { exact: false }),
     ).not.toBeInTheDocument();
@@ -84,14 +89,120 @@ describe("EmbedHomepage (OSS)", () => {
 
     await userEvent.click(screen.getByText("Embedding done, all good"));
 
-    const lastCall = fetchMock.lastCall(
-      "path:/api/setting/embedding-homepage",
-      {
-        method: "PUT",
-      },
-    );
+    const lastCall = getLastHomepageSettingSettingCall();
 
     const body = await lastCall?.request?.json();
     expect(body).toEqual({ value: "dismissed-done" });
   });
+
+  it("should set 'embedding-homepage' to 'dismissed-not-interested-now' when dismissing because not interested", async () => {
+    setup();
+    await userEvent.hover(screen.getByText("Hide these"));
+
+    await userEvent.click(screen.getByText("I'm not interested right now"));
+
+    const lastCall = getLastHomepageSettingSettingCall();
+
+    const body = await lastCall?.request?.json();
+    expect(body).toEqual({ value: "dismissed-not-interested-now" });
+  });
+
+  describe("Feedback modal", () => {
+    const setupForFeedbackModal = async () => {
+      setup();
+      await userEvent.hover(screen.getByText("Hide these"));
+
+      await userEvent.click(screen.getByText("I ran into issues"));
+    };
+
+    it("should ask for feedback when dismissing because of issues", async () => {
+      await setupForFeedbackModal();
+
+      expect(
+        screen.getByText("How can we improve embedding?"),
+      ).toBeInTheDocument();
+    });
+
+    it("should display 'Skip' in the button when inputs are empty, 'Send' if any input has content", async () => {
+      await setupForFeedbackModal();
+
+      expect(screen.getByText("Skip")).toBeInTheDocument();
+
+      await userEvent.type(
+        screen.getByLabelText("Feedback"),
+        "I had an issue with X",
+      );
+
+      expect(screen.queryByText("Skip")).not.toBeInTheDocument();
+      expect(screen.getByText("Send")).toBeInTheDocument();
+
+      await userEvent.clear(screen.getByLabelText("Feedback"));
+      await userEvent.type(screen.getByLabelText("Feedback"), "   ");
+      expect(screen.getByText("Skip")).toBeInTheDocument();
+
+      await userEvent.type(
+        screen.getByLabelText("Email"),
+        "example@example.org",
+      );
+
+      expect(screen.queryByText("Skip")).not.toBeInTheDocument();
+      expect(screen.getByText("Send")).toBeInTheDocument();
+    });
+
+    it("should not dismiss the homepage when the user cancels the feedback modal", async () => {
+      await setupForFeedbackModal();
+
+      await userEvent.click(screen.getByText("Cancel"));
+
+      expect(getLastHomepageSettingSettingCall()).toBeUndefined();
+
+      await waitForElementToBeRemoved(() => queryFeedbackModal());
+    });
+
+    it("should dismiss when submitting feedback - even if empty", async () => {
+      await setupForFeedbackModal();
+
+      await userEvent.click(screen.getByText("Skip"));
+
+      const lastCall = getLastHomepageSettingSettingCall();
+
+      const body = await lastCall?.request?.json();
+      expect(body).toEqual({ value: "dismiss-run-into-issues" });
+
+      const feedbackBody = await getLastFeedbackCall()?.request?.json();
+
+      expect(feedbackBody).toEqual({
+        source: "embedding-homepage-dismiss",
+      });
+
+      await waitForElementToBeRemoved(() => queryFeedbackModal());
+    });
+
+    it("should send feedback when submitting the modal", async () => {
+      await setupForFeedbackModal();
+
+      await userEvent.type(
+        screen.getByLabelText("Feedback"),
+        "I had an issue with X",
+      );
+
+      await userEvent.type(screen.getByLabelText("Email"), "user@example.org");
+
+      await userEvent.click(screen.getByText("Send"));
+
+      const lastCall = getLastHomepageSettingSettingCall();
+      const body = await lastCall?.request?.json();
+      expect(body).toEqual({ value: "dismiss-run-into-issues" });
+
+      const feedbackBody = await getLastFeedbackCall()?.request?.json();
+
+      expect(feedbackBody).toEqual({
+        comments: "I had an issue with X",
+        email: "user@example.org",
+        source: "embedding-homepage-dismiss",
+      });
+
+      await waitForElementToBeRemoved(() => queryFeedbackModal());
+    });
+  });
 });

frontend/src/metabase/home/components/EmbedHomepage/tests/setup.tsx

@@ -6,7 +6,7 @@ import {
   setupPropertiesEndpoints,
   setupSettingsEndpoints,
 } from "__support__/server-mocks";
-import { renderWithProviders } from "__support__/ui";
+import { screen, renderWithProviders } from "__support__/ui";
 import type { Settings, TokenFeatures } from "metabase-types/api";
 import {
   createMockSettings,
@@ -34,6 +34,7 @@ export async function setup({
   jest.clearAllMocks();
 
   fetchMock.put("path:/api/setting/embedding-homepage", 200);
+  fetchMock.post("path:/api/util/product-feedback", 200);
   setupSettingsEndpoints([createMockSettingDefinition()]);
   setupPropertiesEndpoints(createMockSettings());
 
@@ -53,3 +54,16 @@ export async function setup({
     withRouter: true,
   });
 }
+
+export const getLastHomepageSettingSettingCall = () =>
+  fetchMock.lastCall("path:/api/setting/embedding-homepage", {
+    method: "PUT",
+  });
+
+export const getLastFeedbackCall = () =>
+  fetchMock.lastCall("path:/api/util/product-feedback", {
+    method: "POST",
+  });
+
+export const queryFeedbackModal = () =>
+  screen.queryByText("How can we improve embedding?");

src/metabase/api/util.clj

@@ -2,14 +2,20 @@
   "Random utilty endpoints for things that don't belong anywhere else in particular, e.g. endpoints for certain admin
   page tasks."
   (:require
+   [cheshire.core :as json]
+   [clj-http.client :as http]
    [compojure.core :refer [GET POST]]
    [crypto.random :as crypto-random]
+   [environ.core :refer [env]]
    [metabase.analytics.prometheus :as prometheus]
    [metabase.analytics.stats :as stats]
    [metabase.api.common :as api]
    [metabase.api.common.validation :as validation]
+   [metabase.config :as config]
    [metabase.logger :as logger]
    [metabase.troubleshooting :as troubleshooting]
+   [metabase.util.log :as log]
+   [metabase.util.malli :as mu]
    [metabase.util.malli.schema :as ms]
    [ring.util.response :as response]))
 
@@ -38,6 +44,37 @@
   []
   {:token (crypto-random/hex 32)})
 
+(defn- product-feedback-url
+  "Product feedback url. When not prod, reads `MB_PRODUCT_FEEDBACK_URL` from the environment to prevent development
+  feedback from hitting the endpoint."
+  []
+  (if config/is-prod?
+    "https://prod-feedback.metabase.com/api/v1/crm/product-feedback"
+    (env :mb-product-feedback-url)))
+
+(mu/defn send-feedback!
+  "Sends the feedback to the api endpoint"
+  [comments :- [:maybe ms/NonBlankString]
+   source :- ms/NonBlankString
+   email :- [:maybe ms/NonBlankString]]
+  (try (http/post (product-feedback-url)
+                  {:content-type :json
+                   :body         (json/generate-string {:comments comments
+                                                        :source   source
+                                                        :email    email})})
+       (catch Exception e
+         (log/warn e)
+         (throw e))))
+
+(api/defendpoint POST "/product-feedback"
+  "Endpoint to provide feedback from the product"
+  [:as {{:keys [comments source email]} :body}]
+  {comments [:maybe ms/NonBlankString]
+   source ms/NonBlankString
+   email [:maybe ms/NonBlankString]}
+  (future (send-feedback! comments source email))
+  api/generic-204-no-content)
+
 (api/defendpoint GET "/bug_report_details"
   "Returns version and system information relevant to filing a bug report against Metabase."
   []

test/metabase/api/util_test.clj

 (ns metabase.api.util-test
   "Tests for /api/util"
   (:require
+   [clojure.string :as str]
    [clojure.test :refer :all]
+   [metabase.api.util :as api.util]
    [metabase.test :as mt]
    [metabase.util.log :as log]))
 
@@ -57,3 +59,24 @@
              (mt/user-http-request :rasta :get 403 "util/diagnostic_info/connection_pool_info"))))
     (testing "Call successful for superusers"
       (is (map? (mt/user-http-request :crowberto :get 200 "util/diagnostic_info/connection_pool_info"))))))
+
+(deftest product-feedback-tests
+  (testing "requires non-blank source"
+    (let [payload  {:comments "foo"
+                    :email    "foo"}
+          response (mt/user-http-request :crowberto :post 400 "util/product-feedback" payload)]
+      (testing (str "without " :source)
+        (is (= {:errors          {:source "value must be a non-blank string."},
+                :specific-errors {:source ["should be a string, received: nil" "non-blank string, received: nil"]}}
+               response)))))
+  (testing "fires the proxy in background"
+    (let [sent? (promise)]
+      (with-redefs [api.util/send-feedback! (fn [comments source email]
+                                              (doseq [prop [comments source email]]
+                                                (is (not (str/blank? prop)) "got a blank property to send-feedback!"))
+                                              (deliver sent? true))]
+        (mt/user-http-request :crowberto :post 204 "util/product-feedback"
+                              {:comments "I like Metabase"
+                               :email    "happy_user@test.com"
+                               :source   "Analytics Inc"})
+        (is (true? (deref sent? 2000 ::timedout)))))))