Fix notification management FE issues (#17618)

frontend/src/metabase/account/notifications/components/NotificationCard/NotificationCard.jsx

@@ -19,13 +19,13 @@ import {
 const propTypes = {
   item: PropTypes.object.isRequired,
   type: PropTypes.oneOf(["pulse", "alert"]).isRequired,
-  user: PropTypes.object,
+  user: PropTypes.object.isRequired,
   onUnsubscribe: PropTypes.func,
   onArchive: PropTypes.func,
 };
 
 const NotificationCard = ({ item, type, user, onUnsubscribe, onArchive }) => {
-  const hasSubscribed = isSubscribed(item, user);
+  const hasArchive = canArchive(item, user);
 
   const onUnsubscribeClick = useCallback(() => {
     onUnsubscribe(item, type);
@@ -45,14 +45,14 @@ const NotificationCard = ({ item, type, user, onUnsubscribe, onArchive }) => {
           {formatDescription(item, user)}
         </NotificationDescription>
       </NotificationContent>
-      {hasSubscribed && (
+      {!hasArchive && (
         <NotificationIcon
           name="close"
           tooltip={t`Unsubscribe`}
           onClick={onUnsubscribeClick}
         />
       )}
-      {!hasSubscribed && (
+      {hasArchive && (
         <NotificationIcon
           name="close"
           tooltip={t`Delete`}
@@ -65,10 +65,16 @@ const NotificationCard = ({ item, type, user, onUnsubscribe, onArchive }) => {
 
 NotificationCard.propTypes = propTypes;
 
-const isSubscribed = (item, user) => {
-  return item.channels.some(channel =>
-    channel.recipients.some(recipient => recipient.id === user.id),
+const canArchive = (item, user) => {
+  const recipients = item.channels.flatMap(channel =>
+    channel.recipients.map(recipient => recipient.id),
   );
+
+  const isCreator = item.creator?.id === user.id;
+  const isSubscribed = recipients.includes(user.id);
+  const isOnlyRecipient = recipients.length === 1;
+
+  return isCreator && (!isSubscribed || isOnlyRecipient);
 };
 
 const formatTitle = (item, type) => {
@@ -156,7 +162,7 @@ const formatCreator = (item, user) => {
   let creatorString = "";
   const options = Settings.formattingOptions();
 
-  if (user?.id === item.creator?.id) {
+  if (user.id === item.creator?.id) {
     creatorString += t`Created by you`;
   } else if (item.creator?.common_name) {
     creatorString += t`Created by ${item.creator.common_name}`;

frontend/src/metabase/account/notifications/components/NotificationCard/NotificationCard.unit.spec.js

@@ -116,9 +116,13 @@ describe("NotificationCard", () => {
     screen.getByText("Created by John Doe", { exact: false });
   });
 
-  it("should unsubscribe when subscribed and clicked on the close icon", () => {
-    const user = getUser();
-    const alert = getAlert({ channels: [getChannel({ recipients: [user] })] });
+  it("should unsubscribe when the user is not the creator and subscribed", () => {
+    const creator = getUser({ id: 1 });
+    const user = getUser({ id: 2 });
+    const alert = getAlert({
+      creator,
+      channels: [getChannel({ recipients: [user] })],
+    });
     const onUnsubscribe = jest.fn();
     const onArchive = jest.fn();
 
@@ -137,9 +141,13 @@ describe("NotificationCard", () => {
     expect(onArchive).not.toHaveBeenCalled();
   });
 
-  it("should archive when not subscribed and clicked on the close icon", () => {
-    const alert = getAlert();
-    const user = getUser();
+  it("should unsubscribe when user user is the creator and subscribed with another user", () => {
+    const creator = getUser({ id: 1 });
+    const recipient = getUser({ id: 2 });
+    const alert = getAlert({
+      creator,
+      channels: [getChannel({ recipients: [creator, recipient] })],
+    });
     const onUnsubscribe = jest.fn();
     const onArchive = jest.fn();
 
@@ -147,7 +155,52 @@ describe("NotificationCard", () => {
       <NotificationCard
         item={alert}
         type="alert"
-        user={user}
+        user={creator}
+        onUnsubscribe={onUnsubscribe}
+        onArchive={onArchive}
+      />,
+    );
+
+    fireEvent.click(screen.getByLabelText("close icon"));
+    expect(onUnsubscribe).toHaveBeenCalledWith(alert, "alert");
+    expect(onArchive).not.toHaveBeenCalled();
+  });
+
+  it("should archive when the user is the creator and not subscribed", () => {
+    const creator = getUser();
+    const alert = getAlert({ creator });
+    const onUnsubscribe = jest.fn();
+    const onArchive = jest.fn();
+
+    render(
+      <NotificationCard
+        item={alert}
+        type="alert"
+        user={creator}
+        onUnsubscribe={onUnsubscribe}
+        onArchive={onArchive}
+      />,
+    );
+
+    fireEvent.click(screen.getByLabelText("close icon"));
+    expect(onUnsubscribe).not.toHaveBeenCalled();
+    expect(onArchive).toHaveBeenCalledWith(alert, "alert");
+  });
+
+  it("should archive when the user is the creator and is the only one subscribed", () => {
+    const creator = getUser();
+    const alert = getAlert({
+      creator,
+      channels: [getChannel({ recipients: [creator] })],
+    });
+    const onUnsubscribe = jest.fn();
+    const onArchive = jest.fn();
+
+    render(
+      <NotificationCard
+        item={alert}
+        type="alert"
+        user={creator}
         onUnsubscribe={onUnsubscribe}
         onArchive={onArchive}
       />,

frontend/src/metabase/account/notifications/components/NotificationList/NotificationList.jsx

@@ -13,7 +13,7 @@ import {
 
 const propTypes = {
   items: PropTypes.array.isRequired,
-  user: PropTypes.object,
+  user: PropTypes.object.isRequired,
   children: PropTypes.node,
   onHelp: PropTypes.func,
   onUnsubscribe: PropTypes.func,

frontend/src/metabase/account/notifications/components/NotificationList/NotificationList.unit.spec.js

@@ -8,17 +8,27 @@ const getPulse = () => ({
   created_at: "2021-05-08T02:02:07.441Z",
 });
 
+const getUser = () => ({
+  id: 1,
+  common_name: "John Doe",
+});
+
 describe("NotificationList", () => {
   it("should render items", () => {
     const pulse = getPulse();
+    const user = getUser();
 
-    render(<NotificationList items={[{ item: pulse, type: "pulse" }]} />);
+    render(
+      <NotificationList items={[{ item: pulse, type: "pulse" }]} user={user} />,
+    );
 
     screen.getByText("Pulse");
   });
 
   it("should render empty state when there are no items", () => {
-    render(<NotificationList items={[]} />);
+    const user = getUser();
+
+    render(<NotificationList items={[]} user={user} />);
 
     screen.getByText("you’ll be able to manage those here", { exact: false });
   });

frontend/src/metabase/account/notifications/selectors.js

 import { createSelector } from "reselect";
+import { parseTimestamp } from "metabase/lib/time";
 
 export const getAlertId = ({ params: { alertId } }) => {
   return parseInt(alertId);
@@ -22,6 +23,10 @@ export const getNotifications = createSelector(
       })),
     ];
 
-    return items.sort((a, b) => b.item.created_at - a.item.created_at);
+    return items.sort(
+      (a, b) =>
+        parseTimestamp(b.item.created_at).unix() -
+        parseTimestamp(a.item.created_at).unix(),
+    );
   },
 );

frontend/src/metabase/entities/alerts.js

@@ -26,7 +26,7 @@ const Alerts = createEntity({
       return Alerts.actions.update(
         { id },
         { channels: newChannels },
-        undo(opts, "", t`Successfully unsubscribed`),
+        undo(opts, "", t`unsubscribed`),
       );
     },
   },

frontend/src/metabase/entities/pulses.js

@@ -52,7 +52,7 @@ const Pulses = createEntity({
       return Pulses.actions.update(
         { id },
         { channels: newChannels },
-        undo(opts, "", t`Successfully unsubscribed`),
+        undo(opts, "", t`unsubscribed`),
       );
     },
   },

frontend/test/metabase/scenarios/account/notifications/notifications.cy.spec.js

 import { restore } from "__support__/e2e/helpers/e2e-setup-helpers";
 import { SAMPLE_DATASET } from "__support__/e2e/cypress_sample_dataset";
+import { modal } from "__support__/e2e/helpers/e2e-ui-elements-helpers";
 
 const { ORDERS_ID } = SAMPLE_DATASET;
 
@@ -10,7 +11,7 @@ const getQuestionDetails = () => ({
   },
 });
 
-const getAlertDetails = ({ user_id, card_id }) => ({
+const getAlertDetails = ({ card_id, user_id, admin_id }) => ({
   card: {
     id: card_id,
     include_csv: false,
@@ -25,6 +26,9 @@ const getAlertDetails = ({ user_id, card_id }) => ({
         {
           id: user_id,
         },
+        {
+          id: admin_id,
+        },
       ],
     },
   ],
@@ -52,17 +56,24 @@ const getPulseDetails = ({ card_id, dashboard_id }) => ({
 describe("scenarios > account > notifications", () => {
   beforeEach(() => {
     restore();
-    cy.signInAsNormalUser();
   });
 
   describe("alerts", () => {
     beforeEach(() => {
-      cy.getCurrentUser().then(({ body: { id: user_id } }) => {
-        cy.createQuestion(getQuestionDetails()).then(
-          ({ body: { id: card_id } }) => {
-            cy.createAlert(getAlertDetails({ user_id, card_id }));
-          },
-        );
+      cy.signInAsAdmin().then(() => {
+        cy.getCurrentUser().then(({ body: { id: admin_id } }) => {
+          cy.signInAsNormalUser().then(() => {
+            cy.getCurrentUser().then(({ body: { id: user_id } }) => {
+              cy.createQuestion(getQuestionDetails()).then(
+                ({ body: { id: card_id } }) => {
+                  cy.createAlert(
+                    getAlertDetails({ card_id, user_id, admin_id }),
+                  );
+                },
+              );
+            });
+          });
+        });
       });
     });
 
@@ -70,9 +81,13 @@ describe("scenarios > account > notifications", () => {
       cy.visit("/account/notifications");
 
       cy.findByText("Not seeing one here?").click();
-      cy.findByText("Not seeing something listed here?");
-      cy.findByText("Got it").click();
-      cy.findByText("Not seeing something listed here?").should("not.exist");
+
+      modal().within(() => {
+        cy.findByText("Not seeing something listed here?");
+        cy.findByText("Got it").click();
+      });
+
+      modal().should("not.exist");
     });
 
     it("should be able to see alerts notifications", () => {
@@ -89,11 +104,15 @@ describe("scenarios > account > notifications", () => {
       cy.findByText("Question");
       cy.findByLabelText("close icon").click();
 
-      cy.findByText("Confirm you want to unsubscribe");
-      cy.findByText("Unsubscribe").click();
+      modal().within(() => {
+        cy.findByText("Confirm you want to unsubscribe");
+        cy.findByText("Unsubscribe").click();
+      });
 
-      cy.findByText("You’re unsubscribed. Delete this alert as well?");
-      cy.findByText("Delete this alert").click();
+      modal().within(() => {
+        cy.findByText("You’re unsubscribed. Delete this alert as well?");
+        cy.findByText("Delete this alert").click();
+      });
 
       cy.findByText("Question").should("not.exist");
     });
@@ -101,7 +120,7 @@ describe("scenarios > account > notifications", () => {
 
   describe("pulses", () => {
     beforeEach(() => {
-      cy.getCurrentUser().then(({ body: { id: user_id } }) => {
+      cy.signInAsNormalUser().then(() => {
         cy.createQuestionAndDashboard({
           questionDetails: getQuestionDetails(),
         }).then(({ body: { card_id, dashboard_id } }) => {
@@ -114,9 +133,13 @@ describe("scenarios > account > notifications", () => {
       cy.visit("/account/notifications");
 
       cy.findByText("Not seeing one here?").click();
-      cy.findByText("Not seeing something listed here?");
-      cy.findByText("Got it").click();
-      cy.findByText("Not seeing something listed here?").should("not.exist");
+
+      modal().within(() => {
+        cy.findByText("Not seeing something listed here?");
+        cy.findByText("Got it").click();
+      });
+
+      modal().should("not.exist");
     });
 
     it("should be able to see pulses notifications", () => {
@@ -133,8 +156,10 @@ describe("scenarios > account > notifications", () => {
       cy.findByText("Subscription");
       cy.findByLabelText("close icon").click();
 
-      cy.findByText("Delete this subscription?");
-      cy.findByText("Yes, delete this subscription").click();
+      modal().within(() => {
+        cy.findByText("Delete this subscription?");
+        cy.findByText("Yes, delete this subscription").click();
+      });
 
       cy.findByText("Subscription").should("not.exist");
     });