Abstract e2e downloads helpers (#18746)

* Expose the e2e downloads helper function

* Apply the new e2e downloads helper to the #18382 repro

* Apply the new e2e downloads helper to the #18440 repro

* Update the helper so it applies both for saved and unsaved questions

* Add additional helper options

* Redirect to a non-existing page

* Reafactor `downloads` spec

* Extract repro for #10803

frontend/test/__support__/e2e/cypress.js

@@ -26,5 +26,6 @@ export * from "./helpers/e2e-email-helpers";
 export * from "./helpers/e2e-slack-helpers";
 export * from "./helpers/e2e-custom-column-helpers";
 export * from "./helpers/e2e-dimension-list-helpers";
+export * from "./helpers/e2e-downloads-helpers";
 
 Cypress.on("uncaught:exception", (err, runnable) => false);

frontend/test/__support__/e2e/helpers/e2e-downloads-helpers.js

+const xlsx = require("xlsx");
+
+/**
+ * Trigger the download of CSV or XLSX files and assert on the results in the related sheet.
+ * It applies to both unsaved questions (queries) and the saved ones.
+ *
+ * @param {Object} params
+ * @param {("csv"|"xlsx")} params.fileType - file type we're downloading
+ * @param {number} [params.questionId] - needed only for saved questions
+ * @param {boolean} [params.raw] - tell SheetJs not to parse values
+ * @param {boolean} [params.logResults] - preview the results in the console log
+ * @param {function} callback
+ */
+export function downloadAndAssert(
+  { fileType, questionId, raw, logResults } = {},
+  callback,
+) {
+  const downloadClassName = `.Icon-${fileType}`;
+  const endpoint = getEndpoint(fileType, questionId);
+
+  /**
+   * Please see the official Cypress example for more details:
+   * https://github.com/cypress-io/cypress-example-recipes/blob/master/examples/testing-dom__download/cypress/integration/form-submission-spec.js
+   */
+
+  cy.intercept("POST", endpoint, req => {
+    /**
+     * We must redirect in order to avoid Cypress being stuck on waiting for the new page to load.
+     * Intetionally redirecting to a non-existing page.
+     *
+     * Explanation:
+     * If we redirect to ANY of the existing pages, there's a lot of requests that need to complete for that page.
+     *  - This helper function is usually the last piece of code to execute in any given test.
+     *  - As soon as the assertions are complete, the new test starts
+     *  - Assertions are usually faster than all of the previously mentioned requests from the redirect
+     *  - This results in the next test being poluted with the requests that didn't finish from the last one.
+     *  - Those "spill-over" requests end up in the beforeEach hook of the next test and can have unexpected results.
+     */
+
+    req.redirect("/foo");
+  }).as("fileDownload");
+
+  cy.log(`Downloading ${fileType} file`);
+
+  cy.icon("download").click();
+  // Initiate the file download
+  cy.get(downloadClassName).click();
+
+  cy.wait("@fileDownload")
+    .its("request")
+    .then(req => {
+      // The payload for the xlsx is in the binary form
+      fileType === "xlsx" && Object.assign(req, { encoding: "binary" });
+
+      cy.request(req).then(({ body }) => {
+        const { SheetNames, Sheets } = xlsx.read(body, {
+          // See the full list of Parsing options: https://github.com/SheetJS/sheetjs#parsing-options
+          type: "binary",
+          raw,
+        });
+
+        const sheetName = SheetNames[0];
+        const sheet = Sheets[sheetName];
+
+        logResults && console.log(sheet);
+
+        callback(sheet);
+      });
+    });
+}
+
+function getEndpoint(fileType, questionId) {
+  const questionEndpoint = `/api/card/${questionId}/query/${fileType}`;
+  const queryEndpoint = `/api/dataset/${fileType}`;
+
+  return questionId ? questionEndpoint : queryEndpoint;
+}

frontend/test/metabase/scenarios/downloads/downloads.cy.spec.js

-import { restore } from "__support__/e2e/cypress";
+import { restore, downloadAndAssert } from "__support__/e2e/cypress";
 
-const xlsx = require("xlsx");
-
-// csv and Excel files have different sheet names, so define them here and we'll reuse it throughout
-const testCases = [
-  { type: "csv", firstSheetName: "Sheet1" },
-  { type: "xlsx", firstSheetName: "Query result" },
-];
-
-function testWorkbookDatetimes(workbook, download_type, sheetName) {
-  expect(workbook.SheetNames[0]).to.eq(sheetName);
-  expect(workbook.Sheets[sheetName]["A1"].v).to.eq("birth_date");
-  expect(workbook.Sheets[sheetName]["B1"].v).to.eq("created_at");
-
-  // Excel and CSV will have different formats
-  if (download_type === "csv") {
-    expect(workbook.Sheets[sheetName]["A2"].v).to.eq("2020-06-03");
-    expect(workbook.Sheets[sheetName]["B2"].v).to.eq("2020-06-03T23:41:23");
-  } else if (download_type === "xlsx") {
-    // We tell the xlsx library to read raw and not parse dates
-    // So for the _date_ format we expect an integer
-    // And for timestamp, we expect a float
-    expect(workbook.Sheets[sheetName]["A2"].v).to.eq(43985);
-    expect(workbook.Sheets[sheetName]["B2"].v).to.eq(43985.98707175926);
-  }
-}
+const testCases = ["csv", "xlsx"];
 
 describe("scenarios > question > download", () => {
   beforeEach(() => {
@@ -32,132 +8,18 @@ describe("scenarios > question > download", () => {
     cy.signInAsAdmin();
   });
 
-  it("downloads Excel and CSV files", () => {
-    // let's download a binary file
-
-    cy.visit("/question/new");
-    cy.findByText("Simple question").click();
-    cy.findByText("Saved Questions").click();
-    cy.findByText("Orders, Count").click();
-    cy.contains("18,760");
-    cy.icon("download").click();
-
-    // Programatically issue download requests for this query for both CSV and Excel
-
-    cy.wrap(testCases).each(testCase => {
-      const downloadClassName = `.Icon-${testCase.type}`;
-      const endpoint = `/api/dataset/${testCase.type}`;
-      const sheetName = testCase.firstSheetName;
-
-      cy.log(`downloading a ${testCase.type} file`);
-
-      cy.get(downloadClassName)
-        .parent()
-        .parent()
-        .get('input[name="query"]')
-        .invoke("val")
-        .then(download_query_params => {
-          cy.request({
-            url: endpoint,
-            method: "POST",
-            form: true,
-            body: { query: download_query_params },
-            encoding: "binary",
-          }).then(resp => {
-            const workbook = xlsx.read(resp.body, { type: "binary" });
-
-            expect(workbook.SheetNames[0]).to.eq(sheetName);
-            expect(workbook.Sheets[sheetName]["A1"].v).to.eq("Count");
-            expect(workbook.Sheets[sheetName]["A2"].v).to.eq(18760);
-          });
-        });
-    });
-  });
-
-  describe("metabase#10803", () => {
-    let questionId;
-
-    beforeEach(() => {
-      cy.createNativeQuestion({
-        name: "10803",
-        native: {
-          query:
-            "SELECT PARSEDATETIME('2020-06-03', 'yyyy-MM-dd') AS \"birth_date\", PARSEDATETIME('2020-06-03 23:41:23', 'yyyy-MM-dd hh:mm:ss') AS \"created_at\"",
-          "template-tags": {},
-        },
-      }).then(({ body }) => {
-        questionId = body.id;
-      });
-    });
-
-    describe("for saved questions", () => {
-      it("should format the date properly", () => {
-        cy.wrap(testCases).each(testCase => {
-          cy.log(`downloading a ${testCase.type} file`);
-          const endpoint = `/api/card/${questionId}/query/${testCase.type}`;
-
-          cy.request({
-            url: endpoint,
-            method: "POST",
-            encoding: "binary",
-          }).then(resp => {
-            const workbook = xlsx.read(resp.body, {
-              type: "binary",
-              raw: true,
-            });
-
-            testWorkbookDatetimes(
-              workbook,
-              testCase.type,
-              testCase.firstSheetName,
-            );
-          });
-        });
-      });
-    });
-
-    describe("for unsaved questions", () => {
-      it("should format the date properly", () => {
-        // Go to the existing question "10803"
-        cy.visit(`/question/${questionId}`);
-        cy.contains(/open editor/i).click();
-        cy.get(".ace_editor").type("{movetoend} "); // Adds a space at the end of the query to make it "dirty"
-        cy.icon("play")
-          .first()
-          .click();
-        cy.icon("download").click();
-
-        cy.wrap(testCases).each(testCase => {
-          cy.log(`downloading a ${testCase.type} file`);
-          const downloadClassName = `.Icon-${testCase.type}`;
-          const endpoint = `/api/dataset/${testCase.type}`;
+  testCases.forEach(fileType => {
+    it(`downloads ${fileType} file`, () => {
+      cy.visit("/question/new");
+      cy.findByText("Simple question").click();
+      cy.findByText("Saved Questions").click();
+      cy.findByText("Orders, Count").click();
 
-          cy.get(downloadClassName)
-            .parent()
-            .parent()
-            .get('input[name="query"]')
-            .invoke("val")
-            .then(download_query_params => {
-              cy.request({
-                url: endpoint,
-                method: "POST",
-                form: true,
-                body: { query: download_query_params },
-                encoding: "binary",
-              }).then(resp => {
-                const workbook = xlsx.read(resp.body, {
-                  type: "binary",
-                  raw: true,
-                });
+      cy.contains("18,760");
 
-                testWorkbookDatetimes(
-                  workbook,
-                  testCase.type,
-                  testCase.firstSheetName,
-                );
-              });
-            });
-        });
+      downloadAndAssert({ fileType }, sheet => {
+        expect(sheet["A1"].v).to.eq("Count");
+        expect(sheet["A2"].v).to.eq(18760);
       });
     });
   });

frontend/test/metabase/scenarios/downloads/reproductions/10803-timestamp-formatting.cy.spec.js

+import {
+  restore,
+  downloadAndAssert,
+  runNativeQuery,
+} from "__support__/e2e/cypress";
+
+let questionId;
+
+const testCases = ["csv", "xlsx"];
+
+describe("issue 10803", () => {
+  beforeEach(() => {
+    cy.intercept("POST", "/api/dataset").as("dataset");
+
+    restore();
+    cy.signInAsAdmin();
+
+    cy.createNativeQuestion({
+      name: "10803",
+      native: {
+        query:
+          "SELECT PARSEDATETIME('2020-06-03', 'yyyy-MM-dd') AS \"birth_date\", PARSEDATETIME('2020-06-03 23:41:23', 'yyyy-MM-dd hh:mm:ss') AS \"created_at\"",
+        "template-tags": {},
+      },
+    }).then(({ body }) => {
+      questionId = body.id;
+
+      cy.intercept("POST", `/api/card/${questionId}/query`).as("cardQuery");
+      cy.visit(`/question/${questionId}`);
+
+      cy.wait("@cardQuery");
+    });
+  });
+
+  testCases.forEach(fileType => {
+    it(`should format the date properly for ${fileType} in saved questions (metabase#10803)`, () => {
+      downloadAndAssert(
+        { fileType, questionId, logResults: true, raw: true },
+        testWorkbookDatetimes,
+      );
+    });
+
+    it(`should format the date properly for ${fileType} in unsaved questions`, () => {
+      // Add a space at the end of the query to make it "dirty"
+      cy.contains(/open editor/i).click();
+      cy.get(".ace_editor").type("{movetoend} ");
+
+      runNativeQuery();
+      downloadAndAssert({ fileType, raw: true }, testWorkbookDatetimes);
+    });
+
+    function testWorkbookDatetimes(sheet) {
+      expect(sheet["A1"].v).to.eq("birth_date");
+      expect(sheet["B1"].v).to.eq("created_at");
+
+      // Excel and CSV will have different formats
+      if (fileType === "csv") {
+        expect(sheet["A2"].v).to.eq("2020-06-03");
+        expect(sheet["B2"].v).to.eq("2020-06-03T23:41:23");
+      } else if (fileType === "xlsx") {
+        // We tell the xlsx library to read raw and not parse dates
+        // So for the _date_ format we expect an integer
+        // And for timestamp, we expect a float
+        expect(sheet["A2"].v).to.eq(43985);
+        expect(sheet["B2"].v).to.eq(43985.98707175926);
+      }
+    }
+  });
+});

frontend/test/metabase/scenarios/downloads/reproductions/18219-temporal-units-not-formatted.cy.spec.js

-import { restore } from "__support__/e2e/cypress";
+import { restore, downloadAndAssert } from "__support__/e2e/cypress";
 import { SAMPLE_DATASET } from "__support__/e2e/cypress_sample_dataset";
 
-const xlsx = require("xlsx");
 const { ORDERS, ORDERS_ID } = SAMPLE_DATASET;
 
 const questionDetails = {
@@ -13,10 +12,7 @@ const questionDetails = {
   },
 };
 
-const testCases = [
-  { type: "csv", sheetName: "Sheet1" },
-  { type: "xlsx", sheetName: "Query result" },
-];
+const testCases = ["csv", "xlsx"];
 
 describe.skip("issue 18219", () => {
   beforeEach(() => {
@@ -24,60 +20,47 @@ describe.skip("issue 18219", () => {
     cy.signInAsAdmin();
   });
 
-  it("should format temporal units on export (metabase#18219)", () => {
-    cy.createQuestion(questionDetails).then(({ body: { id } }) => {
-      cy.intercept("POST", `/api/card/${id}/query`).as("cardQuery");
-      cy.visit(`/question/${id}`);
+  testCases.forEach(fileType => {
+    it("should format temporal units on export (metabase#18219)", () => {
+      cy.createQuestion(questionDetails).then(
+        ({ body: { id: questionId } }) => {
+          cy.intercept("POST", `/api/card/${questionId}/query`).as("cardQuery");
+          cy.visit(`/question/${questionId}`);
 
-      // Wait for `result_metadata` to load
-      cy.wait("@cardQuery");
+          // Wait for `result_metadata` to load
+          cy.wait("@cardQuery");
 
-      cy.findByText("Created At: Year");
-      cy.findByText("2016");
-      cy.findByText("744");
+          cy.findByText("Created At: Year");
+          cy.findByText("2016");
+          cy.findByText("744");
 
-      cy.icon("download").click();
-
-      cy.wrap(testCases).each(({ type, sheetName }) => {
-        cy.log(`downloading a ${type} file`);
-        const endpoint = `/api/card/${id}/query/${type}`;
-
-        cy.request({
-          url: endpoint,
-          method: "POST",
-          encoding: "binary",
-        }).then(resp => {
-          const workbook = xlsx.read(resp.body, {
-            type: "binary",
-            raw: true,
-          });
-
-          const A1 = workbook.Sheets[sheetName]["A1"];
-          const A2 = workbook.Sheets[sheetName]["A2"];
+          downloadAndAssert({ fileType, questionId, raw: true }, assertion);
+        },
+      );
+    });
 
-          expect(A1.v).to.eq("Created At: Year");
+    function assertion(sheet) {
+      expect(sheet["A1"].v).to.eq("Created At: Year");
 
-          if (type === "csv") {
-            expect(A2.v).to.eq("2016");
-          }
+      if (fileType === "csv") {
+        expect(sheet["A2"].v).to.eq("2016");
+      }
 
-          if (type === "xlsx") {
-            /**
-             * Depending on how we end up solving this issue,
-             * the following assertion on the cell type might not be correct.
-             * It's very likely we'll format temporal breakouts as strings.
-             * I.e. we have to take into account Q1, Q2, etc.
-             */
-            // expect(A2.t).to.eq("n");
+      if (fileType === "xlsx") {
+        /**
+         * Depending on how we end up solving this issue,
+         * the following assertion on the cell type might not be correct.
+         * It's very likely we'll format temporal breakouts as strings.
+         * I.e. we have to take into account Q1, Q2, etc.
+         */
+        // expect(A2.t).to.eq("n");
 
-            /**
-             * Because of the excel date format, we cannot assert on the raw value `v`.
-             * Rather, we have to do it on the parsed value `w`.
-             */
-            expect(A2.w).to.eq("2016");
-          }
-        });
-      });
-    });
+        /**
+         * Because of the excel date format, we cannot assert on the raw value `v`.
+         * Rather, we have to do it on the parsed value `w`.
+         */
+        expect(sheet["A2"].w).to.eq("2016");
+      }
+    }
   });
 });

frontend/test/metabase/scenarios/downloads/reproductions/18382-old-syntax-missing-renamed-columns.cy.spec.js

-import { restore, visitQuestionAdhoc } from "__support__/e2e/cypress";
+import {
+  restore,
+  visitQuestionAdhoc,
+  downloadAndAssert,
+} from "__support__/e2e/cypress";
 import { SAMPLE_DATASET } from "__support__/e2e/cypress_sample_dataset";
 
-const xlsx = require("xlsx");
-
 const { REVIEWS, REVIEWS_ID, PRODUCTS, PRODUCTS_ID } = SAMPLE_DATASET;
 
 /**
@@ -39,7 +41,15 @@ const questionDetails = {
   },
   display: "table",
   visualization_settings: {
-    // Rename columns
+    /**
+     * Rename columns
+     *
+     * Please note: it is currently not possible to use the old syntax for columns rename.
+     * That results in `500` error, and backend doesn't handle it at all.
+     * Once some kind of mechanism is put in place to prevent the app from breaking in such cases,
+     * change the following syntax to the old style `["field-id", ${COLUMN_ID}]`
+     */
+
     column_settings: {
       [`["ref",["field",${REVIEWS.ID},null]]`]: {
         column_title: "MOD:ID",
@@ -74,14 +84,11 @@ const questionDetails = {
 
 const testCases = ["csv", "xlsx"];
 
-testCases.forEach(type => {
-  const downloadClassName = `.Icon-${type}`;
-  const endpoint = `/api/dataset/${type}`;
-
+testCases.forEach(fileType => {
   describe("issue 18382", () => {
     beforeEach(() => {
       // TODO: Please remove this line when issue gets fixed
-      cy.skipOn(type === "csv");
+      cy.skipOn(fileType === "csv");
 
       cy.intercept("POST", "/api/dataset").as("dataset");
 
@@ -92,42 +99,16 @@ testCases.forEach(type => {
       cy.wait("@dataset");
     });
 
-    it(`should handle the old syntax in downloads for ${type} (metabase#18382)`, () => {
-      cy.url().then(currentPage => {
-        cy.intercept("POST", endpoint, req => {
-          // We must redirect in order to avoid Cypress being stuck on waiting for the new page to load.
-          // But let's stay on the same page, instead of redirecting to `/` or something else.
-          req.redirect(currentPage);
-        }).as("fileDownload");
-      });
-
-      cy.log(`Downloading ${type} file`);
-
-      cy.icon("download").click();
-      // Initiate the file download
-      cy.get(downloadClassName).click();
-
-      cy.wait("@fileDownload")
-        .its("request")
-        .then(req => {
-          // The payload for the xlsx is in the binary form
-          type === "xlsx" && Object.assign(req, { encoding: "binary" });
-
-          cy.request(req).then(({ body }) => {
-            const { SheetNames, Sheets } = xlsx.read(body, {
-              type: "binary",
-            });
-
-            const sheetName = SheetNames[0];
-            const sheet = Sheets[sheetName];
-
-            expect(sheet["A1"].v).to.eq("MOD:Title");
-            expect(sheet["B1"].v).to.eq("MOD:ID");
-            expect(sheet["C1"].v).to.eq("MOD:Reviewer");
-
-            expect(sheet["A2"].v).to.eq("Aerodynamic Concrete Bench");
-          });
-        });
+    it(`should handle the old syntax in downloads for ${fileType} (metabase#18382)`, () => {
+      downloadAndAssert({ fileType }, assertion);
     });
   });
 });
+
+function assertion(sheet) {
+  expect(sheet["A1"].v).to.eq("MOD:Title");
+  expect(sheet["B1"].v).to.eq("MOD:ID");
+  expect(sheet["C1"].v).to.eq("MOD:Reviewer");
+
+  expect(sheet["A2"].v).to.eq("Aerodynamic Concrete Bench");
+}

frontend/test/metabase/scenarios/downloads/reproductions/18440-remapped-display-value-dropped.cy.spec.js

-import { restore, visitQuestionAdhoc } from "__support__/e2e/cypress";
+import {
+  restore,
+  visitQuestionAdhoc,
+  downloadAndAssert,
+} from "__support__/e2e/cypress";
 import { SAMPLE_DATASET } from "__support__/e2e/cypress_sample_dataset";
 
-const xlsx = require("xlsx");
-
 const { ORDERS, ORDERS_ID, PRODUCTS } = SAMPLE_DATASET;
 
+const query = { "source-table": ORDERS_ID, limit: 5 };
+
 const questionDetails = {
   dataset_query: {
     type: "query",
-    query: { "source-table": ORDERS_ID, limit: 5 },
+    query,
     database: 1,
   },
 };
 
-const testCases = [
-  { type: "csv", sheetName: "Sheet1" },
-  { type: "xlsx", sheetName: "Query result" },
-];
+const testCases = ["csv", "xlsx"];
 
 describe("issue 18440", () => {
   beforeEach(() => {
     cy.intercept("POST", "/api/card").as("saveQuestion");
+    cy.intercept("POST", "/api/dataset").as("dataset");
 
     restore();
     cy.signInAsAdmin();
@@ -31,76 +33,36 @@ describe("issue 18440", () => {
       type: "external",
       human_readable_field_id: PRODUCTS.TITLE,
     });
-
-    visitQuestionAdhoc(questionDetails);
   });
 
-  it("export should include a column with remapped values (metabase#18440)", () => {
-    cy.findByText("Product ID");
-    cy.findByText("Awesome Concrete Shoes");
-
-    cy.icon("download").click();
+  testCases.forEach(fileType => {
+    it(`export should include a column with remapped values for ${fileType} (metabase#18440-1)`, () => {
+      visitQuestionAdhoc(questionDetails);
+      cy.wait("@dataset");
 
-    cy.wrap(testCases).each(({ type, sheetName }) => {
-      cy.log(`downloading a ${type} file for an unsaved question`);
+      cy.findByText("Product ID");
+      cy.findByText("Awesome Concrete Shoes");
 
-      const downloadClassName = `.Icon-${type}`;
-      const endpoint = `/api/dataset/${type}`;
-
-      cy.get(downloadClassName)
-        .parent()
-        .parent()
-        .get('input[name="query"]')
-        .invoke("val")
-        .then(download_query_params => {
-          cy.request({
-            url: endpoint,
-            method: "POST",
-            form: true,
-            body: { query: download_query_params },
-            encoding: "binary",
-          }).then(resp => {
-            const workbook = xlsx.read(resp.body, {
-              type: "binary",
-              raw: true,
-            });
-
-            expect(workbook.Sheets[sheetName]["C1"].v).to.eq("Product ID");
-            expect(workbook.Sheets[sheetName]["C2"].v).to.eq(
-              "Awesome Concrete Shoes",
-            );
-          });
-        });
+      downloadAndAssert({ fileType }, assertion);
     });
 
-    // Save the question using UI
-    cy.findByText("Save").click();
-    cy.get(".Modal")
-      .button("Save")
-      .click();
+    it(`export should include a column with remapped values for ${fileType} for a saved question (metabase#18440-2)`, () => {
+      cy.createQuestion({ query }).then(({ body: { id } }) => {
+        cy.intercept("POST", `/api/card/${id}/query`).as("cardQuery");
 
-    cy.wait("@saveQuestion").then(({ response: { body: { id } } }) => {
-      cy.wrap(testCases).each(({ type, sheetName }) => {
-        cy.log(`downloading a ${type} file for a saved question`);
+        cy.visit(`/question/${id}`);
+        cy.wait("@cardQuery");
 
-        const endpoint = `/api/card/${id}/query/${type}`;
+        cy.findByText("Product ID");
+        cy.findByText("Awesome Concrete Shoes");
 
-        cy.request({
-          url: endpoint,
-          method: "POST",
-          encoding: "binary",
-        }).then(resp => {
-          const workbook = xlsx.read(resp.body, {
-            type: "binary",
-            raw: true,
-          });
-
-          expect(workbook.Sheets[sheetName]["C1"].v).to.eq("Product ID");
-          expect(workbook.Sheets[sheetName]["C2"].v).to.eq(
-            "Awesome Concrete Shoes",
-          );
-        });
+        downloadAndAssert({ fileType, questionId: id }, assertion);
       });
     });
   });
 });
+
+function assertion(sheet) {
+  expect(sheet["C1"].v).to.eq("Product ID");
+  expect(sheet["C2"].v).to.eq("Awesome Concrete Shoes");
+}