Make "Save" Modal Context Aware (#30558)

e2e/test/scenarios/models/create.cy.spec.js

-import { restore, visitCollection } from "e2e/support/helpers";
+import {
+  getCollectionIdFromSlug,
+  modal,
+  popover,
+  restore,
+  visitCollection,
+} from "e2e/support/helpers";
 
 const modelName = "A name";
 
@@ -12,7 +18,7 @@ describe("scenarios > models > create", () => {
   it("creates a native query model via the New button", () => {
     cy.visit("/");
 
-    goFromHomePageToNewNativeQueryModelPage();
+    navigateToNewModelPage();
 
     // Cancel creation with confirmation modal
     // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
@@ -21,7 +27,7 @@ describe("scenarios > models > create", () => {
     cy.findByText("Discard").click();
 
     // Now we will create a model
-    goFromHomePageToNewNativeQueryModelPage();
+    navigateToNewModelPage();
 
     // Clicking on metadata should not work until we run a query
     cy.findByTestId("editor-tabs-metadata").should("be.disabled");
@@ -42,12 +48,53 @@ describe("scenarios > models > create", () => {
 
     checkIfPinned();
   });
+
+  it("suggest the currently viewed collection when saving a new native query", () => {
+    getCollectionIdFromSlug("third_collection", THIRD_COLLECTION_ID => {
+      visitCollection(THIRD_COLLECTION_ID);
+    });
+
+    navigateToNewModelPage();
+    cy.get(".ace_editor").should("be.visible").type("select * from ORDERS");
+
+    cy.findByTestId("dataset-edit-bar").within(() => {
+      cy.contains("button", "Save").click();
+    });
+    modal().within(() => {
+      cy.findByTestId("select-button").should("have.text", "Third collection");
+    });
+  });
+
+  it("suggest the currently viewed collection when saving a new structured query", () => {
+    getCollectionIdFromSlug("third_collection", THIRD_COLLECTION_ID => {
+      visitCollection(THIRD_COLLECTION_ID);
+    });
+
+    navigateToNewModelPage("structured");
+
+    popover().within(() => {
+      cy.findByText("Sample Database").click();
+      cy.findByText("Orders").click();
+    });
+
+    cy.findByTestId("dataset-edit-bar").within(() => {
+      cy.contains("button", "Save").click();
+    });
+
+    modal().within(() => {
+      cy.findByTestId("select-button").should("have.text", "Third collection");
+    });
+  });
 });
 
-function goFromHomePageToNewNativeQueryModelPage() {
+function navigateToNewModelPage(queryType = "native") {
   cy.findByText("New").click();
   cy.findByText("Model").click();
-  cy.findByText("Use a native query").click();
+  if (queryType === "structured") {
+    cy.findByText("Use the notebook editor").click();
+  } else {
+    cy.findByText("Use a native query").click();
+  }
 }
 
 function checkIfPinned() {

e2e/test/scenarios/native/native.cy.spec.js

@@ -7,6 +7,8 @@ import {
   rightSidebar,
   filter,
   filterField,
+  getCollectionIdFromSlug,
+  visitCollection,
 } from "e2e/support/helpers";
 
 import { SAMPLE_DB_ID } from "e2e/support/cypress_data";
@@ -30,6 +32,22 @@ describe("scenarios > question > native", () => {
     cy.contains("18,760");
   });
 
+  it("should suggest the currently viewed collection when saving question", () => {
+    getCollectionIdFromSlug("third_collection", THIRD_COLLECTION_ID => {
+      visitCollection(THIRD_COLLECTION_ID);
+    });
+    openNativeEditor({ fromCurrentPage: true }).type(
+      "select count(*) from orders",
+    );
+
+    cy.findByTestId("qb-header").within(() => {
+      cy.findByText("Save").click();
+    });
+    modal().within(() => {
+      cy.findByTestId("select-button").should("have.text", "Third collection");
+    });
+  });
+
   it("displays an error", () => {
     openNativeEditor().type("select * from not_a_table");
     runQuery();

e2e/test/scenarios/question/new.cy.spec.js

@@ -8,6 +8,8 @@ import {
   getCollectionIdFromSlug,
   saveQuestion,
   getPersonalCollectionName,
+  visitCollection,
+  modal,
 } from "e2e/support/helpers";
 
 import { SAMPLE_DB_ID, USERS } from "e2e/support/cypress_data";
@@ -255,4 +257,28 @@ describe("scenarios > question > new", () => {
     // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
     cy.findByText("37.65");
   });
+
+  it("should suggest the currently viewed collection when saving question", () => {
+    getCollectionIdFromSlug("third_collection", THIRD_COLLECTION_ID => {
+      visitCollection(THIRD_COLLECTION_ID);
+    });
+
+    cy.findByLabelText("Navigation bar").within(() => {
+      cy.findByText("New").click();
+    });
+
+    popover().findByText("Question").click();
+
+    popover().within(() => {
+      cy.findByText("Sample Database").click();
+      cy.findByText("Orders").click();
+    });
+
+    cy.findByTestId("qb-header").within(() => {
+      cy.findByText("Save").click();
+    });
+    modal().within(() => {
+      cy.findByTestId("select-button").should("have.text", "Third collection");
+    });
+  });
 });

frontend/src/metabase/components/NewItemMenu/NewItemMenu.tsx

@@ -31,6 +31,15 @@ export interface NewItemMenuProps {
   onChangeLocation: (nextLocation: LocationDescriptor) => void;
 }
 
+type NewMenuItem = {
+  title: string;
+  icon: string;
+  link?: LocationDescriptor;
+  event?: string;
+  action?: () => void;
+  onClose?: () => void;
+};
+
 const NewItemMenu = ({
   className,
   collectionId,
@@ -61,7 +70,7 @@ const NewItemMenu = ({
   );
 
   const menuItems = useMemo(() => {
-    const items = [];
+    const items: NewMenuItem[] = [];
 
     if (hasDataAccess) {
       items.push({
@@ -70,6 +79,7 @@ const NewItemMenu = ({
         link: Urls.newQuestion({
           mode: "notebook",
           creationType: "custom_question",
+          collectionId,
         }),
         event: `${analyticsContext};New Question Click;`,
         onClose: onCloseNavbar,
@@ -83,6 +93,7 @@ const NewItemMenu = ({
         link: Urls.newQuestion({
           type: "native",
           creationType: "native_question",
+          collectionId,
         }),
         event: `${analyticsContext};New SQL Query Click;`,
         onClose: onCloseNavbar,
@@ -103,12 +114,15 @@ const NewItemMenu = ({
         event: `${analyticsContext};New Collection Click;`,
       },
     );
-
     if (hasNativeWrite) {
+      const collectionQuery = collectionId
+        ? `?collectionId=${collectionId}`
+        : "";
+
       items.push({
         title: t`Model`,
         icon: "model",
-        link: "/model/new",
+        link: `/model/new${collectionQuery}`,
         event: `${analyticsContext};New Model Click;`,
         onClose: onCloseNavbar,
       });
@@ -125,13 +139,14 @@ const NewItemMenu = ({
 
     return items;
   }, [
-    hasModels,
     hasDataAccess,
     hasNativeWrite,
-    hasDatabaseWithJsonEngine,
-    hasDatabaseWithActionsEnabled,
     analyticsContext,
+    hasModels,
+    hasDatabaseWithActionsEnabled,
+    collectionId,
     onCloseNavbar,
+    hasDatabaseWithJsonEngine,
   ]);
 
   return (

frontend/src/metabase/lib/card.js

@@ -45,6 +45,7 @@ function getCleanCard(card) {
 
   return {
     name: card.name,
+    collectionId: card.collectionId,
     description: card.description,
     dataset_query: dataset_query,
     display: card.display,

frontend/src/metabase/models/containers/NewModelOptions/NewModelOptions.jsx

-/* eslint-disable react/prop-types */
-import { Component } from "react";
-import { connect } from "react-redux";
-import { push } from "react-router-redux";
-import { t } from "ttag";
-import _ from "underscore";
-
-import { Grid } from "metabase/components/Grid";
-import AdminAwareEmptyState from "metabase/components/AdminAwareEmptyState";
-import NewModelOption from "metabase/models/components/NewModelOption";
-
-import MetabaseSettings from "metabase/lib/settings";
-import * as Urls from "metabase/lib/urls";
-import Databases from "metabase/entities/databases";
-import { getHasDataAccess, getHasNativeWrite } from "metabase/selectors/data";
-
-import {
-  OptionsGridItem,
-  OptionsRoot,
-  EducationalButton,
-} from "./NewModelOptions.styled";
-
-const EDUCATIONAL_LINK = MetabaseSettings.learnUrl("data-modeling/models");
-
-const mapStateToProps = (state, { databases = [] }) => ({
-  hasDataAccess: getHasDataAccess(databases),
-  hasNativeWrite: getHasNativeWrite(databases),
-});
-
-const mapDispatchToProps = {
-  push,
-};
-
-class NewModelOptions extends Component {
-  componentDidMount() {
-    const { location, push } = this.props;
-    if (Object.keys(location.query).length > 0) {
-      const { database, table, ...options } = location.query;
-      push(
-        Urls.newQuestion({
-          ...options,
-          databaseId: database ? parseInt(database) : undefined,
-          tableId: table ? parseInt(table) : undefined,
-        }),
-      );
-    }
-  }
-
-  render() {
-    const { hasDataAccess, hasNativeWrite } = this.props;
-
-    if (!hasDataAccess && !hasNativeWrite) {
-      return (
-        <div className="full-height flex align-center justify-center">
-          <NoDatabasesEmptyState />
-        </div>
-      );
-    }
-
-    // Determine how many items will be shown based on permissions etc so we can make sure the layout adapts
-    const itemsCount = (hasDataAccess ? 1 : 0) + (hasNativeWrite ? 1 : 0);
-
-    return (
-      <OptionsRoot>
-        <Grid className="justifyCenter">
-          {hasDataAccess && (
-            <OptionsGridItem itemsCount={itemsCount}>
-              <NewModelOption
-                image="app/img/notebook_mode_illustration"
-                title={t`Use the notebook editor`}
-                description={t`This automatically inherits metadata from your source tables, and gives your models drill-through.`}
-                width={180}
-                to={Urls.newQuestion({
-                  mode: "query",
-                  creationType: "custom_question",
-                  dataset: true,
-                })}
-                data-metabase-event="New Model; Custom Question Start"
-              />
-            </OptionsGridItem>
-          )}
-          {hasNativeWrite && (
-            <OptionsGridItem itemsCount={itemsCount}>
-              <NewModelOption
-                image="app/img/sql_illustration"
-                title={t`Use a native query`}
-                description={t`You can always fall back to a SQL or native query, which is a bit more manual.`}
-                to={Urls.newQuestion({
-                  mode: "query",
-                  type: "native",
-                  creationType: "native_question",
-                  dataset: true,
-                })}
-                width={180}
-                data-metabase-event="New Model; Native Query Start"
-              />
-            </OptionsGridItem>
-          )}
-        </Grid>
-
-        <EducationalButton
-          target="_blank"
-          href={EDUCATIONAL_LINK}
-          className="mt4"
-        >
-          {t`What's a model?`}
-        </EducationalButton>
-      </OptionsRoot>
-    );
-  }
-}
-
-const NoDatabasesEmptyState = user => (
-  <AdminAwareEmptyState
-    title={t`Metabase is no fun without any data`}
-    adminMessage={t`Your databases will appear here once you connect one`}
-    message={t`Databases will appear here once your admins have added some`}
-    image="app/assets/img/databases-list"
-    adminAction={t`Connect a database`}
-    adminLink="/admin/databases/create"
-    user={user}
-  />
-);
-
-export default _.compose(
-  Databases.loadList({
-    loadingAndErrorWrapper: false,
-  }),
-  connect(mapStateToProps, mapDispatchToProps),
-)(NewModelOptions);

frontend/src/metabase/models/containers/NewModelOptions/NewModelOptions.tsx

+import { t } from "ttag";
+import _ from "underscore";
+
+import { Location } from "history";
+import { Grid } from "metabase/components/Grid";
+import NewModelOption from "metabase/models/components/NewModelOption";
+
+import MetabaseSettings from "metabase/lib/settings";
+import * as Urls from "metabase/lib/urls";
+import Databases from "metabase/entities/databases";
+import { getHasDataAccess, getHasNativeWrite } from "metabase/selectors/data";
+
+import { useSelector } from "metabase/lib/redux";
+import AdminAwareEmptyState from "metabase/components/AdminAwareEmptyState";
+import Database from "metabase-lib/metadata/Database";
+import {
+  OptionsGridItem,
+  OptionsRoot,
+  EducationalButton,
+} from "./NewModelOptions.styled";
+
+const EDUCATIONAL_LINK = MetabaseSettings.learnUrl("data-modeling/models");
+
+interface NewModelOptionsProps {
+  databases?: Database[];
+  location: Location;
+}
+
+const NewModelOptions = (props: NewModelOptionsProps) => {
+  const hasDataAccess = useSelector(() =>
+    getHasDataAccess(props.databases ?? []),
+  );
+  const hasNativeWrite = useSelector(() =>
+    getHasNativeWrite(props.databases ?? []),
+  );
+
+  const collectionId = Urls.extractEntityId(
+    props.location.query.collectionId as string,
+  );
+
+  if (!hasDataAccess && !hasNativeWrite) {
+    return (
+      <div className="full-height flex align-center justify-center">
+        <NoDatabasesEmptyState />
+      </div>
+    );
+  }
+
+  // Determine how many items will be shown based on permissions etc so we can make sure the layout adapts
+  const itemsCount = (hasDataAccess ? 1 : 0) + (hasNativeWrite ? 1 : 0);
+
+  return (
+    <OptionsRoot>
+      <Grid className="justifyCenter">
+        {hasDataAccess && (
+          <OptionsGridItem itemsCount={itemsCount}>
+            <NewModelOption
+              image="app/img/notebook_mode_illustration"
+              title={t`Use the notebook editor`}
+              description={t`This automatically inherits metadata from your source tables, and gives your models drill-through.`}
+              width={180}
+              to={Urls.newQuestion({
+                mode: "query",
+                creationType: "custom_question",
+                dataset: true,
+                collectionId,
+              })}
+              data-metabase-event="New Model; Custom Question Start"
+            />
+          </OptionsGridItem>
+        )}
+        {hasNativeWrite && (
+          <OptionsGridItem itemsCount={itemsCount}>
+            <NewModelOption
+              image="app/img/sql_illustration"
+              title={t`Use a native query`}
+              description={t`You can always fall back to a SQL or native query, which is a bit more manual.`}
+              to={Urls.newQuestion({
+                mode: "query",
+                type: "native",
+                creationType: "native_question",
+                dataset: true,
+                collectionId,
+              })}
+              width={180}
+              data-metabase-event="New Model; Native Query Start"
+            />
+          </OptionsGridItem>
+        )}
+      </Grid>
+
+      <EducationalButton
+        target="_blank"
+        href={EDUCATIONAL_LINK}
+        className="mt4"
+      >
+        {t`What's a model?`}
+      </EducationalButton>
+    </OptionsRoot>
+  );
+};
+const NoDatabasesEmptyState = () => (
+  <AdminAwareEmptyState
+    title={t`Metabase is no fun without any data`}
+    adminMessage={t`Your databases will appear here once you connect one`}
+    message={t`Databases will appear here once your admins have added some`}
+    image="app/assets/img/databases-list"
+    adminAction={t`Connect a database`}
+    adminLink="/admin/databases/create"
+  />
+);
+// eslint-disable-next-line import/no-default-export -- deprecated usage
+export default _.compose(
+  Databases.loadList({
+    loadingAndErrorWrapper: false,
+  }),
+)(NewModelOptions);

frontend/src/metabase/query_builder/components/DataSelector/DataSelector.jsx

 /* eslint-disable react/prop-types */
-import {
-  createRef,
-  createElement,
-  Component,
-  useEffect,
-  useState,
-} from "react";
+import { createRef, createElement, Component } from "react";
 import { connect } from "react-redux";
 import PropTypes from "prop-types";
 import { t } from "ttag";
@@ -25,14 +19,11 @@ import Schemas from "metabase/entities/schemas";
 import Tables from "metabase/entities/tables";
 import Search from "metabase/entities/search";
 
-import { PLUGIN_MODERATION } from "metabase/plugins";
-
 import { getMetadata } from "metabase/selectors/metadata";
 import { getHasDataAccess } from "metabase/selectors/data";
 import { getSchemaName } from "metabase-lib/metadata/utils/schema";
 import {
   isVirtualCardId,
-  getQuestionVirtualTableId,
   SAVED_QUESTIONS_VIRTUAL_DB_ID,
 } from "metabase-lib/metadata/utils/saved-questions";
 import {
@@ -47,8 +38,6 @@ import SchemaPicker from "./DataSelectorSchemaPicker";
 import FieldPicker from "./DataSelectorFieldPicker";
 import TablePicker from "./DataSelectorTablePicker";
 import {
-  CollectionDatasetSelectList,
-  CollectionDatasetAllDataLink,
   EmptyStateContainer,
   TableSearchContainer,
 } from "./DataSelector.styled";
@@ -70,10 +59,6 @@ const TABLE_STEP = "TABLE";
 // chooses a table field (table has already been selected)
 const FIELD_STEP = "FIELD";
 
-// allows to choose one of collection's dataset (requires collectionId prop)
-// is used while adding a question with the "+" button on collection page
-const COLLECTION_DATASET_STEP = "COLLECTION_DATASET";
-
 export const DataSourceSelector = props => (
   <DataSelector
     steps={[DATA_BUCKET_STEP, DATABASE_STEP, SCHEMA_STEP, TABLE_STEP]}
@@ -83,31 +68,6 @@ export const DataSourceSelector = props => (
   />
 );
 
-export const CollectionDatasetOrDataSourceSelector = ({
-  hasCollectionDatasetsStep,
-  ...props
-}) => {
-  const [collectionDatasetsShown, setCollectionDatasetsShown] = useState(
-    !!hasCollectionDatasetsStep,
-  );
-
-  const steps = collectionDatasetsShown
-    ? [COLLECTION_DATASET_STEP]
-    : [DATA_BUCKET_STEP, DATABASE_STEP, SCHEMA_STEP, TABLE_STEP];
-
-  return (
-    <DataSelector
-      steps={steps}
-      combineDatabaseSchemaSteps
-      getTriggerElementContent={TableTriggerContent}
-      {...props}
-      onCloseCollectionDatasets={() => {
-        setCollectionDatasetsShown(false);
-      }}
-    />
-  );
-};
-
 export const DatabaseDataSelector = props => (
   <DataSelector
     steps={[DATABASE_STEP]}
@@ -352,18 +312,21 @@ export class UnconnectedDataSelector extends Component {
         tables = schemas[0].tables;
       }
     }
+
     function setSelectedSchema(schema) {
       selectedSchema = schema;
       if (!tables && schema) {
         tables = schema.tables;
       }
     }
+
     function setSelectedTable(table) {
       selectedTable = table;
       if (!fields && table) {
         fields = table.fields;
       }
     }
+
     function setSelectedField(field) {
       selectedField = field;
     }
@@ -914,15 +877,6 @@ export class UnconnectedDataSelector extends Component {
     };
 
     switch (this.state.activeStep) {
-      case COLLECTION_DATASET_STEP:
-        return (
-          <CollectionDatasetPicker
-            {...props}
-            collectionId={this.props.collectionId}
-            handleCollectionDatasetSelect={this.handleCollectionDatasetSelect}
-            onSeeAllData={this.handleCollectionDatasetsPickerClose}
-          />
-        );
       case DATA_BUCKET_STEP:
         return <DataBucketPicker {...props} />;
       case DATABASE_STEP:
@@ -981,24 +935,6 @@ export class UnconnectedDataSelector extends Component {
       searchText,
     });
 
-  handleCollectionDatasetSelect = async dataset => {
-    const tableId = getQuestionVirtualTableId(dataset.id);
-    await this.props.fetchFields(tableId);
-    if (this.props.setSourceTableFn) {
-      this.props.setSourceTableFn(tableId);
-    }
-    this.popover.current.toggle();
-    this.props.onCloseCollectionDatasets();
-    this.switchToStep(TABLE_STEP);
-  };
-
-  handleCollectionDatasetsPickerClose = () => {
-    this.props.onCloseCollectionDatasets();
-    this.switchToStep(
-      this.hasUsableDatasets() ? DATA_BUCKET_STEP : DATABASE_STEP,
-    );
-  };
-
   handleSearchItemSelect = async item => {
     const table = convertSearchResultToTableLikeItem(item);
     await this.props.fetchFields(table.id);
@@ -1153,74 +1089,3 @@ export class UnconnectedDataSelector extends Component {
     return this.renderContent();
   }
 }
-
-const CollectionDatasetPicker = ({
-  collectionId,
-  handleCollectionDatasetSelect,
-  onSeeAllData,
-}) => {
-  return (
-    <Search.ListLoader
-      query={{
-        collection: collectionId,
-        models: ["dataset"],
-      }}
-      loadingAndErrorWrapper={false}
-    >
-      {({ list: datasets }) => (
-        <CollectionDatasetList
-          datasets={datasets}
-          onSelect={handleCollectionDatasetSelect}
-          onSeeAllData={onSeeAllData}
-        />
-      )}
-    </Search.ListLoader>
-  );
-};
-
-function CollectionDatasetList({ datasets, onSelect, onSeeAllData }) {
-  useEffect(() => {
-    if (datasets?.length === 0) {
-      onSeeAllData();
-    } else if (datasets?.length === 1) {
-      onSelect(datasets[0]);
-    }
-  }, [datasets, onSelect, onSeeAllData]);
-
-  // If there are no datasets, in a collection, we just switch to the normal picker
-  // If there is exactly one dataset, we select it and close the picker
-  // The loading indicator is still shown for both cases to prevent flickering
-  // Example: spinner > one dataset shown > it gets selected > the selector closes, everything flickers
-  if (!datasets || datasets.length === 0 || datasets.length === 1) {
-    return <LoadingAndErrorWrapper loading />;
-  }
-
-  return (
-    <CollectionDatasetSelectList>
-      {datasets.map(dataset => {
-        return (
-          <CollectionDatasetSelectList.Item
-            key={dataset.id}
-            name={dataset.name}
-            onSelect={() => onSelect(dataset)}
-            size="small"
-            icon={{ name: "model", size: 16 }}
-            rightIcon={PLUGIN_MODERATION.getStatusIcon(
-              dataset.moderated_status,
-            )}
-          />
-        );
-      })}
-      <CollectionDatasetAllDataLink
-        key="all-data"
-        onSelect={onSeeAllData}
-        as={props => <li {...props} />}
-      >
-        <CollectionDatasetAllDataLink.Content>
-          {t`All data`}
-          <Icon key="icon" name="chevronright" size={12} />
-        </CollectionDatasetAllDataLink.Content>
-      </CollectionDatasetAllDataLink>
-    </CollectionDatasetSelectList>
-  );
-}

frontend/src/metabase/query_builder/components/notebook/steps/DataStep.tsx

@@ -2,7 +2,7 @@ import * as React from "react";
 import { connect } from "react-redux";
 import { t } from "ttag";
 
-import { CollectionDatasetOrDataSourceSelector } from "metabase/query_builder/components/DataSelector";
+import { DataSourceSelector } from "metabase/query_builder/components/DataSelector";
 import { getDatabasesList } from "metabase/query_builder/selectors";
 
 import type { TableId } from "metabase-types/api";
@@ -29,13 +29,6 @@ function DataStep({
   const table = query.table();
   const canSelectTableColumns = table && query.isRaw() && !readOnly;
 
-  const hasCollectionDatasetsStep =
-    question &&
-    !question.isSaved() &&
-    !question.databaseId() &&
-    !question.tableId() &&
-    question.collectionId() !== undefined;
-
   return (
     <NotebookCell color={color}>
       <NotebookCellItem
@@ -55,10 +48,9 @@ function DataStep({
         rightContainerStyle={FIELDS_PICKER_STYLES.notebookRightItemContainer}
         data-testid="data-step-cell"
       >
-        <CollectionDatasetOrDataSourceSelector
+        <DataSourceSelector
           hasTableSearch
           collectionId={question.collectionId()}
-          hasCollectionDatasetsStep={hasCollectionDatasetsStep}
           databaseQuery={{ saved: true }}
           selectedDatabaseId={query.databaseId()}
           selectedTableId={query.tableId()}