Fix #17657: No recipients causes error resulting in blank screen (#17668)

* Invalidate the pulse if it contains no recipients

Switch statement was returning `true`, resulting in falsely "valid" pulse
in the case there were no recipients but the schedule was set to month.

This commit fixes that.

* Move all conditions before the switch statement

* Enable repro for #17657

* Guard against the error that causes the white screen

* Add another repro for #17657

* Fix typo in the test name

* Fix typo in the unrelated comment

frontend/src/metabase/lib/pulse.js

@@ -8,6 +8,28 @@ export function channelIsValid(channel, channelSpec) {
   if (!channelSpec) {
     return false;
   }
+
+  if (channelSpec.recipients) {
+    // default from formInput is an empty array, not a null array
+    // check for both
+    if (!channel.recipients || channel.recipients.length < 1) {
+      return false;
+    }
+  }
+
+  if (channelSpec.fields) {
+    for (const field of channelSpec.fields) {
+      if (
+        field.required &&
+        channel.details &&
+        (channel.details[field.name] == null ||
+          channel.details[field.name] === "")
+      ) {
+        return false;
+      }
+    }
+  }
+
   switch (channel.schedule_type) {
     case "monthly":
       if (channel.schedule_frame != null && channel.schedule_hour != null) {
@@ -30,25 +52,7 @@ export function channelIsValid(channel, channelSpec) {
     default:
       return false;
   }
-  if (channelSpec.recipients) {
-    // default from formInput is an empty array, not a null array
-    // check for both
-    if (!channel.recipients || channel.recipients.length < 1) {
-      return false;
-    }
-  }
-  if (channelSpec.fields) {
-    for (const field of channelSpec.fields) {
-      if (
-        field.required &&
-        channel.details &&
-        (channel.details[field.name] == null ||
-          channel.details[field.name] === "")
-      ) {
-        return false;
-      }
-    }
-  }
+
   return true;
 }
 

frontend/src/metabase/sharing/components/PulsesListSidebar.jsx

@@ -103,11 +103,14 @@ function buildRecipientText(pulse) {
   const {
     channels: [firstChannel],
   } = pulse;
-  if (firstChannel.channel_type !== "email") {
+
+  const { channel_type, recipients } = firstChannel;
+
+  if (channel_type !== "email" || _.isEmpty(recipients)) {
     return "";
   }
 
-  const [firstRecipient, ...otherRecipients] = firstChannel.recipients;
+  const [firstRecipient, ...otherRecipients] = recipients;
   const firstRecipientText = firstRecipient.common_name || firstRecipient.email;
   return _.isEmpty(otherRecipients)
     ? firstRecipientText

frontend/test/metabase/pulse/pulse.unit.spec.js

@@ -38,7 +38,7 @@ describe("recipient picker", () => {
           onRecipientsChange={() => alert("why?")}
         />,
       );
-      // Now only the recepient name should be visible
+      // Now only the recipient name should be visible
       screen.getByText("Barb");
       expect(screen.queryByText("Dustin")).toBeNull();
     });

frontend/test/metabase/scenarios/sharing/reproductions/17657.cy.spec.js

+import { restore, sidebar } from "__support__/e2e/cypress";
+import { USERS } from "__support__/e2e/cypress_data";
+
+const {
+  admin: { first_name, last_name },
+} = USERS;
+
+describe("issue 17657", () => {
+  beforeEach(() => {
+    restore();
+    cy.signInAsAdmin();
+
+    createSubscriptionWithoutRecipients();
+  });
+
+  it("frontend should gracefully handle the case of a subscription without a recipient (metabase#17657)", () => {
+    cy.visit("/dashboard/1");
+
+    cy.icon("share").click();
+    cy.findByText("Dashboard subscriptions").click();
+
+    cy.findByText(/^Emailed monthly/).click();
+
+    sidebar().within(() => {
+      cy.button("Done").should("be.disabled");
+    });
+
+    // Open the popover with all users
+    cy.findByPlaceholderText("Enter user names or email addresses").click();
+    // Pick admin as a recipient
+    cy.findByText(`${first_name} ${last_name}`).click();
+
+    sidebar().within(() => {
+      cy.button("Done").should("not.be.disabled");
+    });
+  });
+});
+
+function createSubscriptionWithoutRecipients() {
+  cy.request("POST", "/api/pulse", {
+    name: "Orders in a dashboard",
+    cards: [
+      {
+        id: 1,
+        collection_id: null,
+        description: null,
+        display: "table",
+        name: "Orders",
+        include_csv: false,
+        include_xls: false,
+        dashboard_card_id: 1,
+        dashboard_id: 1,
+        parameter_mappings: [],
+      },
+    ],
+    channels: [
+      {
+        channel_type: "email",
+        enabled: true,
+        // Since the fix (https://github.com/metabase/metabase/pull/17668), this is not even possible to do in the UI anymore.
+        // Backend still doesn't do this validation so we're making sure the FE handles the case of missing recipients gracefully.
+        recipients: [],
+        details: {},
+        schedule_type: "monthly",
+        schedule_day: "mon",
+        schedule_hour: 8,
+        schedule_frame: "first",
+      },
+    ],
+    skip_if_empty: false,
+    collection_id: null,
+    parameters: [],
+    dashboard_id: 1,
+  });
+}

frontend/test/metabase/scenarios/sharing/subscriptions.cy.spec.js

@@ -78,7 +78,7 @@ describe("scenarios > dashboard > subscriptions", () => {
     });
 
     describe("with no existing subscriptions", () => {
-      it.skip("should not enable subscriptions without the recepient (metabase#17657)", () => {
+      it("should not enable subscriptions without the recipient (metabase#17657)", () => {
         openDashboardSubscriptions();
 
         cy.findByText("Email it").click();