CSV appends: Milestone 0, Merge 2 of 2: upload through model page (#3…

…7220)

Co-authored-by: Ryan Laurie <30528226+iethree@users.noreply.github.com>

e2e/test/scenarios/collections/uploads.cy.spec.js

@@ -8,6 +8,7 @@ import {
   resetSnowplow,
   enableTracking,
   setTokenFeatures,
+  modal,
 } from "e2e/support/helpers";
 
 import { WRITABLE_DB_ID, USER_GROUPS } from "e2e/support/cypress_data";
@@ -41,6 +42,12 @@ describeWithSnowplow(
   "CSV Uploading",
   { tags: ["@external", "@actions"] },
   () => {
+    beforeEach(() => {
+      cy.intercept("POST", "/api/dataset").as("dataset");
+      cy.intercept("POST", "/api/card/from-csv").as("uploadCSV");
+      cy.intercept("POST", "/api/table/*/append-csv").as("appendCSV");
+    });
+
     it("Can upload a CSV file to an empty postgres schema", () => {
       const testFile = validTestFiles[0];
       const EMPTY_SCHEMA_NAME = "empty_uploads";
@@ -97,7 +104,7 @@ describeWithSnowplow(
       });
     });
 
-    ["postgres"].forEach(dialect => {
+    ["postgres", "mysql"].forEach(dialect => {
       describe(`CSV Uploading (${dialect})`, () => {
         beforeEach(() => {
           restore(`${dialect}-writable`);
@@ -130,9 +137,10 @@ describeWithSnowplow(
 
             queryWritableDB(tableQuery, dialect).then(result => {
               expect(result.rows.length).to.equal(1);
-              const tableName = result.rows[0].table_name;
+              const tableName =
+                result.rows[0].table_name ?? result.rows[0].TABLE_NAME;
               queryWritableDB(
-                `SELECT count(*) FROM ${tableName};`,
+                `SELECT count(*) as count FROM ${tableName};`,
                 dialect,
               ).then(result => {
                 expect(Number(result.rows[0].count)).to.equal(
@@ -158,6 +166,32 @@ describeWithSnowplow(
             });
           });
         });
+
+        describe("CSV appends", () => {
+          it("Can append a CSV file to an existing table", () => {
+            uploadFile(validTestFiles[0]);
+            cy.findByTestId("view-footer").findByText(
+              `Showing ${validTestFiles[0].rowCount} rows`,
+            );
+
+            appendFile(validTestFiles[0]);
+            cy.findByTestId("view-footer").findByText(
+              `Showing ${validTestFiles[0].rowCount * 2} rows`,
+            );
+          });
+
+          it("Cannot append a CSV file to a table with a different schema", () => {
+            uploadFile(validTestFiles[0]);
+            cy.findByTestId("view-footer").findByText(
+              `Showing ${validTestFiles[0].rowCount} rows`,
+            );
+
+            appendFile(validTestFiles[1], false);
+            cy.findByTestId("view-footer").findByText(
+              `Showing ${validTestFiles[0].rowCount} rows`,
+            );
+          });
+        });
       });
     });
   },
@@ -265,14 +299,12 @@ function uploadFile(testFile, valid = true) {
   });
 
   if (valid) {
-    // After #35498 has been merged, we now sometimes encounter two elements with the "status" role in UI.
-    // The first (older) one is related to the sync that didn't finish, and the second one is related to CSV upload.
-    // This is the reason we have to start using `findAllByRole` rather than `findByRole`.
-    // Since CSV status element is newer, we can and must use `.last()` to yield only one element within we perform the search.
-    cy.findAllByRole("status")
-      .last()
+    cy.findByTestId("status-root-container")
       .should("contain", "Uploading data to")
       .and("contain", testFile.fileName);
+
+    cy.wait("@uploadCSV");
+
     cy.findAllByRole("status")
       .last()
       .findByText("Data added to Uploads Collection", {
@@ -285,12 +317,55 @@ function uploadFile(testFile, valid = true) {
       cy.findByText(testFile.humanName);
     });
 
-    cy.findAllByRole("status").last().findByText("Start exploring").click();
+    cy.findByTestId("status-root-container")
+      .findByText("Start exploring")
+      .click();
+    cy.wait("@dataset");
 
     cy.url().should("include", `/model/`);
     cy.findByTestId("TableInteractive-root");
   } else {
-    cy.findAllByRole("status").last().findByText("Error uploading your File");
+    cy.wait("@uploadCSV");
+
+    cy.findByTestId("status-root-container").findByText(
+      "Error uploading your file",
+    );
+  }
+}
+
+function appendFile(testFile, valid = true) {
+  // assumes we're already looking at an uploadable model page
+  cy.findByTestId("qb-header").icon("upload");
+
+  cy.fixture(`${FIXTURE_PATH}/${testFile.fileName}`).then(file => {
+    cy.get("#append-file-input").selectFile(
+      {
+        contents: Cypress.Buffer.from(file),
+        fileName: testFile.fileName,
+        mimeType: "text/csv",
+      },
+      { force: true },
+    );
+  });
+
+  if (valid) {
+    cy.findByTestId("status-root-container")
+      .should("contain", "Uploading data to")
+      .and("contain", testFile.fileName);
+
+    cy.wait("@appendCSV");
+
+    cy.findByTestId("status-root-container").findByText(/Data added to/i, {
+      timeout: 10 * 1000,
+    });
+  } else {
+    cy.wait("@appendCSV");
+
+    cy.findByTestId("status-root-container").findByText(
+      "Error uploading your file",
+    );
+
+    modal().findByText("Upload error details");
   }
 }
 

enterprise/backend/test/metabase_enterprise/upload_test.clj

@@ -1,4 +1,4 @@
-(ns metabase-enterprise.upload-test
+(ns ^:once metabase-enterprise.upload-test
   (:require
    [clojure.test :refer :all]
    [metabase-enterprise.test :as met]
@@ -23,3 +23,24 @@
       (met/with-gtaps-for-user :rasta {:gtaps {:venues {}}}
           (is (thrown-with-msg? Exception #"Uploads are not permitted for sandboxed users\."
                 (upload-test/append-csv-with-defaults! :user-id (mt/user->id :rasta))))))))
+
+(deftest based-on-upload-for-sandboxed-user-test
+  (mt/with-temporary-setting-values [uploads-enabled true]
+    (mt/test-drivers (mt/normal-drivers-with-feature :uploads)
+      (mt/dataset (mt/dataset-definition
+                   (mt/random-name)
+                   ["venues"
+                    [{:field-name "name" :base-type :type/Text}]
+                    [["something"]]])
+        (mt/with-temp [:model/Database {db-id :id}    {:engine "h2"}
+                       :model/Table    {table-id :id} {:db_id     db-id
+                                                       :is_upload true}
+                       :model/Card     {card-id :id}  {:dataset       true
+                                                       :dataset_query {:type     :query
+                                                                       :database db-id
+                                                                       :query    {:source-table table-id}}}]
+          (testing "Sanity check: if the user is not sandboxed, based_on_upload is non-nil"
+            (is (= table-id (:based_on_upload (mt/user-http-request :crowberto :get 200 (str "card/" card-id))))))
+          (testing "If the user is sandboxed, based_on_upload is nil"
+           (met/with-gtaps-for-user :rasta {:gtaps {:venues {}}}
+             (is (=? {:based_on_upload nil} (mt/user-http-request :rasta :get 200 (str "card/" card-id)))))))))))

frontend/src/metabase-types/api/card.ts

@@ -30,6 +30,7 @@ export interface Card<Q extends DatasetQuery = DatasetQuery>
   last_query_start: string | null;
   average_query_time: number | null;
   cache_ttl: number | null;
+  based_on_upload?: number | null; // table id of upload table, if any
 
   archived: boolean;
 

frontend/src/metabase-types/api/mocks/card.ts

@@ -31,6 +31,7 @@ export const createMockCard = (opts?: Partial<Card>): Card => ({
   collection_id: null,
   last_query_start: null,
   average_query_time: null,
+  based_on_upload: null,
   archived: false,
   enable_embedding: false,
   ...opts,

frontend/src/metabase-types/store/upload.ts

@@ -1,10 +1,11 @@
-import type { CollectionId } from "metabase-types/api";
+import type { CollectionId, TableId } from "metabase-types/api";
 
 export type FileUpload = {
   status: "complete" | "in-progress" | "error";
   name: string;
-  collectionId: CollectionId;
+  collectionId?: CollectionId;
   modelId?: string;
+  tableId?: TableId;
   message?: string;
   error?: string;
   id: number;

frontend/src/metabase/collections/components/CollectionHeader/CollectionHeader.tsx

@@ -20,7 +20,13 @@ export interface CollectionHeaderProps {
   onUpdateCollection: (entity: Collection, values: Partial<Collection>) => void;
   onCreateBookmark: (collection: Collection) => void;
   onDeleteBookmark: (collection: Collection) => void;
-  onUpload: (file: File, collectionId: CollectionId) => void;
+  onUpload: ({
+    file,
+    collectionId,
+  }: {
+    file: File;
+    collectionId: CollectionId;
+  }) => void;
   canUpload: boolean;
   uploadsEnabled: boolean;
 }

frontend/src/metabase/collections/components/CollectionHeader/CollectionUpload.tsx

@@ -27,7 +27,13 @@ export function CollectionUpload({
   collection: Collection;
   uploadsEnabled: boolean;
   isAdmin: boolean;
-  onUpload: (file: File, collectionId: CollectionId) => void;
+  onUpload: ({
+    file,
+    collectionId,
+  }: {
+    file: File;
+    collectionId: CollectionId;
+  }) => void;
 }) {
   const [showInfoModal, setShowInfoModal] = useState(false);
 
@@ -55,7 +61,7 @@ export function CollectionUpload({
   const handleFileUpload = (event: ChangeEvent<HTMLInputElement>) => {
     const file = event.target.files?.[0];
     if (file !== undefined) {
-      onUpload(file, collection.id);
+      onUpload({ file, collectionId: collection.id });
     }
   };
 

frontend/src/metabase/collections/containers/CollectionContent.jsx

@@ -124,7 +124,7 @@ function CollectionContent({
 
   const onDrop = useCallback(
     acceptedFiles => {
-      uploadFile(acceptedFiles[0], collectionId);
+      uploadFile({ file: acceptedFiles[0], collectionId });
     },
     [collectionId, uploadFile],
   );

frontend/src/metabase/lib/api.js

@@ -100,7 +100,7 @@ export class Api extends EventEmitter {
         let body;
         if (options.hasBody) {
           body = options.formData
-            ? rawData
+            ? rawData.formData
             : JSON.stringify(
                 options.bodyParamName != null
                   ? data[options.bodyParamName]

frontend/src/metabase/query_builder/components/QuestionActions.tsx

@@ -1,4 +1,5 @@
-import { useCallback } from "react";
+import type { ChangeEvent } from "react";
+import { useCallback, useRef } from "react";
 import { t } from "ttag";
 
 import * as Urls from "metabase/lib/urls";
@@ -12,6 +13,7 @@ import { MODAL_TYPES } from "metabase/query_builder/constants";
 
 import { softReloadCard } from "metabase/query_builder/actions";
 import { getUserIsAdmin } from "metabase/selectors/user";
+import { uploadFile } from "metabase/redux/uploads";
 
 import { color } from "metabase/lib/colors";
 
@@ -27,6 +29,7 @@ import {
   checkCanBeModel,
   checkDatabaseCanPersistDatasets,
 } from "metabase-lib/metadata/utils/models";
+import { canUploadToQuestion } from "../selectors";
 import {
   QuestionActionsDivider,
   StrengthIndicator,
@@ -71,6 +74,9 @@ const QuestionActions = ({
   const isMetabotEnabled = useSelector(state =>
     getSetting(state, "is-metabot-enabled"),
   );
+
+  const canUpload = useSelector(canUploadToQuestion(question));
+
   const isModerator = useSelector(getUserIsAdmin) && question.canWrite?.();
 
   const dispatch = useDispatch();
@@ -85,6 +91,7 @@ const QuestionActions = ({
   const canWrite = question.canWrite();
   const isSaved = question.isSaved();
   const database = question.database();
+  const canAppend = canUpload && canWrite && !!question._card.based_on_upload;
 
   const canPersistDataset =
     PLUGIN_MODEL_PERSISTENCE.isModelLevelPersistenceEnabled() &&
@@ -217,6 +224,29 @@ const QuestionActions = ({
     });
   }
 
+  const fileInputRef = useRef<HTMLInputElement>(null);
+
+  const handleUploadClick = () => {
+    if (fileInputRef.current) {
+      fileInputRef.current.click();
+    }
+  };
+
+  const handleFileUpload = (event: ChangeEvent<HTMLInputElement>) => {
+    const file = event.target.files?.[0];
+    if (file && question._card.based_on_upload) {
+      uploadFile({
+        file,
+        tableId: question._card.based_on_upload,
+      })(dispatch);
+
+      // reset the file input so that subsequent uploads of the same file trigger the change handler
+      if (fileInputRef.current?.value) {
+        fileInputRef.current.value = "";
+      }
+    }
+  };
+
   return (
     <>
       <QuestionActionsDivider />
@@ -239,6 +269,30 @@ const QuestionActions = ({
           />
         </ViewHeaderIconButtonContainer>
       </Tooltip>
+      {canAppend && (
+        <>
+          <input
+            type="file"
+            accept="text/csv"
+            id="append-file-input"
+            ref={fileInputRef}
+            onChange={handleFileUpload}
+            style={{ display: "none" }}
+          />
+          <Tooltip tooltip={t`Upload data to this model`}>
+            <ViewHeaderIconButtonContainer>
+              <Button
+                onlyIcon
+                icon="upload"
+                iconSize={HEADER_ICON_SIZE}
+                onClick={handleUploadClick}
+                color={infoButtonColor}
+                data-testid="qb-header-append-button"
+              />
+            </ViewHeaderIconButtonContainer>
+          </Tooltip>
+        </>
+      )}
       {extraButtons.length > 0 && (
         <EntityMenu
           triggerAriaLabel={t`Move, archive, and more...`}

frontend/src/metabase/query_builder/selectors.js

@@ -995,6 +995,22 @@ export const getDashboard = state => {
   return getDashboardById(state, getDashboardId(state));
 };
 
+export const canUploadToQuestion = question => state => {
+  const uploadsEnabled = getSetting(state, "uploads-enabled");
+  if (!uploadsEnabled) {
+    return false;
+  }
+  const uploadsDbId = getSetting(state, "uploads-database-id");
+  const canUploadToDb =
+    uploadsDbId === question.databaseId() &&
+    Databases.selectors
+      .getObject(state, {
+        entityId: uploadsDbId,
+      })
+      ?.canUpload();
+  return canUploadToDb;
+};
+
 export const getTemplateTags = createSelector([getCard], card =>
   getIn(card, ["dataset_query", "native", "template-tags"]),
 );