Properly highlight models as the data source (#39792)

* [HDS] Prep/cleanup steps for Highlighting the data source (#39681) * Rename `DATA_BUCKET` key to `MODELS` * Rename misleading method `isSavedQuestionSelected` is too narrow because we can also have other saved entities, such as models (and soon metrics) as the selected source. * Fix type for `selectedTableId` * Extract `sourceId` * Rename saved question instances to saved entity * Rename related components * Show models correctly in the data source picker (#39686) * Show models correctly in the data source picker * Address review comments * Destructure props * Add E2E reproduction for #39699 * Prevent accessing potentially stale value Closes #39699
metabase · Mar 7, 2024 · 56d4c48 · 56d4c48
1 parent 0a1f97f
commit 56d4c48
Show file tree

Hide file tree

Showing 11 changed files with 148 additions and 70 deletions.
diff --git a/e2e/test/scenarios/question/reproductions/39699-notebook-data-source-models.cy.spec.js b/e2e/test/scenarios/question/reproductions/39699-notebook-data-source-models.cy.spec.js
@@ -0,0 +1,50 @@
+import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
+import { SECOND_COLLECTION_ID } from "e2e/support/cypress_sample_instance_data";
+import { restore, openNotebook } from "e2e/support/helpers";
+
+const { REVIEWS_ID } = SAMPLE_DATABASE;
+
+const modelDetails = {
+  name: "GUI Model",
+  query: { "source-table": REVIEWS_ID, limit: 1 },
+  display: "table",
+  type: "model",
+  collection_id: SECOND_COLLECTION_ID,
+};
+
+describe("issue 39699", () => {
+  beforeEach(() => {
+    restore();
+    cy.signInAsAdmin();
+
+    cy.createQuestion(modelDetails, { visitQuestion: true });
+  });
+
+  it("data selector should properly show a model as the source (metabase#39699)", () => {
+    openNotebook();
+    cy.findByTestId("data-step-cell")
+      .should("have.text", modelDetails.name)
+      .click();
+
+    cy.findByTestId("saved-entity-back-navigation").should(
+      "have.text",
+      "Models",
+    );
+
+    cy.findByTestId("saved-entity-collection-tree").within(() => {
+      cy.findByLabelText("Our analytics")
+        .should("have.attr", "aria-expanded", "false")
+        .and("have.attr", "aria-selected", "false");
+      cy.findByLabelText("First collection")
+        .should("have.attr", "aria-expanded", "true")
+        .and("have.attr", "aria-selected", "false");
+      cy.findByLabelText("Second collection")
+        .should("have.attr", "aria-expanded", "false")
+        .and("have.attr", "aria-selected", "true");
+    });
+
+    cy.findByTestId("select-list")
+      .findByLabelText(modelDetails.name)
+      .should("have.attr", "aria-selected", "true");
+  });
+});
diff --git a/frontend/src/metabase/containers/DataPicker/constants.tsx b/frontend/src/metabase/containers/DataPicker/constants.tsx
@@ -9,7 +9,7 @@ import type {
 export const MIN_SEARCH_LENGTH = 2;
 
 export const DATA_BUCKET: Record<string, DataPickerDataType> = {
-  DATASETS: "models",
+  MODELS: "models",
   RAW_DATA: "raw-data",
   SAVED_QUESTIONS: "questions",
 } as const;
@@ -21,8 +21,8 @@ export const DEFAULT_DATA_PICKER_FILTERS: DataPickerFilters = {
   tables: () => true,
 };
 
-export const DATASETS_INFO_ITEM: DataTypeInfoItem = {
-  id: DATA_BUCKET.DATASETS,
+export const MODELS_INFO_ITEM: DataTypeInfoItem = {
+  id: DATA_BUCKET.MODELS,
   icon: "model",
   name: t`Models`,
   description: t`The best starting place for new questions.`,
@@ -37,7 +37,7 @@ export const RAW_DATA_INFO_ITEM: DataTypeInfoItem = {
 
 export const SAVED_QUESTIONS_INFO_ITEM: DataTypeInfoItem = {
   id: DATA_BUCKET.SAVED_QUESTIONS,
-  name: t`Saved Questions`,
   icon: "folder",
+  name: t`Saved Questions`,
   description: t`Use any question’s results to start a new question.`,
 };
diff --git a/frontend/src/metabase/containers/DataPicker/utils.ts b/frontend/src/metabase/containers/DataPicker/utils.ts
@@ -1,5 +1,5 @@
 import {
-  DATASETS_INFO_ITEM,
+  MODELS_INFO_ITEM,
   RAW_DATA_INFO_ITEM,
   SAVED_QUESTIONS_INFO_ITEM,
 } from "./constants";
@@ -17,7 +17,7 @@ export function getDataTypes({
   const dataTypes: DataTypeInfoItem[] = [];
 
   if (hasNestedQueriesEnabled && hasModels) {
-    dataTypes.push(DATASETS_INFO_ITEM);
+    dataTypes.push(MODELS_INFO_ITEM);
   }
 
   dataTypes.push(RAW_DATA_INFO_ITEM);

diff --git a/frontend/src/metabase/query_builder/components/DataSelector/DataSelector.jsx b/frontend/src/metabase/query_builder/components/DataSelector/DataSelector.jsx
@@ -12,6 +12,7 @@ import LoadingAndErrorWrapper from "metabase/components/LoadingAndErrorWrapper";
 import PopoverWithTrigger from "metabase/components/PopoverWithTrigger";
 import { DATA_BUCKET, getDataTypes } from "metabase/containers/DataPicker";
 import Databases from "metabase/entities/databases";
+import Questions from "metabase/entities/questions";
 import Schemas from "metabase/entities/schemas";
 import Search from "metabase/entities/search";
 import Tables from "metabase/entities/tables";
@@ -21,6 +22,7 @@ import { getSetting } from "metabase/selectors/settings";
 import { Icon } from "metabase/ui";
 import {
   isVirtualCardId,
+  getQuestionIdFromVirtualTableId,
   SAVED_QUESTIONS_VIRTUAL_DB_ID,
 } from "metabase-lib/metadata/utils/saved-questions";
 import { getSchemaName } from "metabase-lib/metadata/utils/schema";
@@ -36,7 +38,7 @@ import FieldPicker from "./DataSelectorFieldPicker";
 import SchemaPicker from "./DataSelectorSchemaPicker";
 import TablePicker from "./DataSelectorTablePicker";
 import { SearchResults, getSearchItemTableOrCardId } from "./data-search";
-import SavedQuestionPicker from "./saved-question-picker/SavedQuestionPicker";
+import SavedEntityPicker from "./saved-entity-picker/SavedEntityPicker";
 
 import "./DataSelector.css";
 
@@ -177,6 +179,9 @@ const DataSelector = _.compose(
       }),
       hasDataAccess: getHasDataAccess(ownProps.allDatabases ?? []),
       hasNestedQueriesEnabled: getSetting(state, "enable-nested-queries"),
+      selectedQuestion: Questions.selectors.getObject(state, {
+        entityId: getQuestionIdFromVirtualTableId(ownProps.selectedTableId),
+      }),
     }),
     {
       fetchDatabases: databaseQuery =>
@@ -185,6 +190,10 @@ const DataSelector = _.compose(
         Schemas.actions.fetchList({ dbId: databaseId }),
       fetchSchemaTables: schemaId => Schemas.actions.fetch({ id: schemaId }),
       fetchFields: tableId => Tables.actions.fetchMetadata({ id: tableId }),
+      fetchQuestion: id =>
+        Questions.actions.fetch({
+          id: getQuestionIdFromVirtualTableId(id),
+        }),
     },
   ),
 )(DataSelectorInner);
@@ -200,7 +209,8 @@ export class UnconnectedDataSelector extends Component {
       selectedTableId: props.selectedTableId,
       selectedFieldId: props.selectedFieldId,
       searchText: "",
-      isSavedQuestionPickerShown: false,
+      isSavedEntityPickerShown: false,
+      savedEntityType: null,
     };
     const computedState = this._getComputedState(props, state);
     this.state = {
@@ -217,7 +227,7 @@ export class UnconnectedDataSelector extends Component {
     selectedDataBucketId: PropTypes.string,
     selectedDatabaseId: PropTypes.number,
     selectedSchemaId: PropTypes.string,
-    selectedTableId: PropTypes.number,
+    selectedTableId: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
     selectedFieldId: PropTypes.number,
     databases: PropTypes.array.isRequired,
     setDatabaseFn: PropTypes.func,
@@ -401,18 +411,29 @@ export class UnconnectedDataSelector extends Component {
 
   async componentDidMount() {
     const { activeStep } = this.state;
+    const {
+      fetchFields,
+      fetchQuestion,
+      selectedDataBucketId,
+      selectedTableId: sourceId,
+    } = this.props;
+
     if (!this.isLoadingDatasets() && !activeStep) {
       await this.hydrateActiveStep();
     }
 
-    if (this.props.selectedDataBucketId === DATA_BUCKET.DATASETS) {
-      this.showSavedQuestionPicker();
+    if (selectedDataBucketId === DATA_BUCKET.MODELS) {
+      this.showSavedEntityPicker({ entityType: "model" });
     }
 
-    if (this.props.selectedTableId) {
-      await this.props.fetchFields(this.props.selectedTableId);
-      if (this.isSavedQuestionSelected()) {
-        this.showSavedQuestionPicker();
+    if (sourceId) {
+      await fetchFields(sourceId);
+      if (this.isSavedEntitySelected()) {
+        await fetchQuestion(sourceId);
+
+        this.showSavedEntityPicker({
+          entityType: this.props.selectedQuestion?.type(),
+        });
       }
     }
   }
@@ -512,8 +533,8 @@ export class UnconnectedDataSelector extends Component {
   async hydrateActiveStep() {
     const { steps } = this.props;
     if (
-      this.isSavedQuestionSelected() ||
-      this.state.selectedDataBucketId === DATA_BUCKET.DATASETS ||
+      this.isSavedEntitySelected() ||
+      this.state.selectedDataBucketId === DATA_BUCKET.MODELS ||
       this.state.selectedDataBucketId === DATA_BUCKET.SAVED_QUESTIONS
     ) {
       await this.switchToStep(DATABASE_STEP);
@@ -743,8 +764,11 @@ export class UnconnectedDataSelector extends Component {
     }
   };
 
-  showSavedQuestionPicker = () =>
-    this.setState({ isSavedQuestionPickerShown: true });
+  showSavedEntityPicker = ({ entityType }) =>
+    this.setState({
+      isSavedEntityPickerShown: true,
+      savedEntityType: entityType,
+    });
 
   onChangeDataBucket = async selectedDataBucketId => {
     if (selectedDataBucketId === DATA_BUCKET.RAW_DATA) {
@@ -766,7 +790,7 @@ export class UnconnectedDataSelector extends Component {
 
   onChangeDatabase = async database => {
     if (database.is_saved_questions) {
-      this.showSavedQuestionPicker();
+      this.showSavedEntityPicker({ entityType: "question" });
       return;
     }
 
@@ -849,15 +873,15 @@ export class UnconnectedDataSelector extends Component {
       : cx("flex align-center", { disabled: readOnly });
   }
 
-  handleSavedQuestionPickerClose = () => {
+  handleSavedEntityPickerClose = () => {
     const { selectedDataBucketId } = this.state;
     if (
-      selectedDataBucketId === DATA_BUCKET.DATASETS ||
+      selectedDataBucketId === DATA_BUCKET.MODELS ||
       this.hasUsableDatasets()
     ) {
       this.previousStep();
     }
-    this.setState({ isSavedQuestionPickerShown: false });
+    this.setState({ isSavedEntityPickerShown: false, savedEntityType: null });
   };
 
   renderActiveStep() {
@@ -920,9 +944,9 @@ export class UnconnectedDataSelector extends Component {
     return null;
   }
 
-  isSavedQuestionSelected = () => isVirtualCardId(this.props.selectedTableId);
+  isSavedEntitySelected = () => isVirtualCardId(this.props.selectedTableId);
 
-  handleSavedQuestionSelect = async tableOrCardId => {
+  handleSavedEntitySelect = async tableOrCardId => {
     await this.props.fetchFields(tableOrCardId);
     if (this.props.setSourceTableFn) {
       const table = this.props.metadata.table(tableOrCardId);
@@ -970,32 +994,32 @@ export class UnconnectedDataSelector extends Component {
   };
 
   getSearchInputPlaceholder = () => {
-    const { activeStep, selectedDataBucketId, isSavedQuestionPickerShown } =
+    const { activeStep, selectedDataBucketId, isSavedEntityPickerShown } =
       this.state;
     if (activeStep === DATA_BUCKET_STEP) {
       return t`Search for some data…`;
     }
-    if (selectedDataBucketId === DATA_BUCKET.DATASETS) {
+    if (selectedDataBucketId === DATA_BUCKET.MODELS) {
       return t`Search for a model…`;
     }
-    return isSavedQuestionPickerShown
+    return isSavedEntityPickerShown
       ? t`Search for a question…`
       : t`Search for a table…`;
   };
 
   getSearchModels = () => {
-    const { selectedDataBucketId, isSavedQuestionPickerShown } = this.state;
+    const { selectedDataBucketId, isSavedEntityPickerShown } = this.state;
     if (!this.props.hasNestedQueriesEnabled) {
       return ["table"];
     }
     if (!this.hasUsableDatasets()) {
-      return isSavedQuestionPickerShown ? ["card"] : ["card", "table"];
+      return isSavedEntityPickerShown ? ["card"] : ["card", "table"];
     }
     if (!selectedDataBucketId) {
       return ["card", "dataset", "table"];
     }
     return {
-      [DATA_BUCKET.DATASETS]: ["dataset"],
+      [DATA_BUCKET.MODELS]: ["dataset"],
       [DATA_BUCKET.RAW_DATA]: ["table"],
       [DATA_BUCKET.SAVED_QUESTIONS]: ["card"],
     }[selectedDataBucketId];
@@ -1009,9 +1033,10 @@ export class UnconnectedDataSelector extends Component {
   renderContent = () => {
     const {
       searchText,
-      isSavedQuestionPickerShown,
+      isSavedEntityPickerShown,
       selectedDataBucketId,
       selectedTable,
+      savedEntityType,
     } = this.state;
     const { canChangeDatabase, selectedDatabaseId } = this.props;
 
@@ -1020,8 +1045,11 @@ export class UnconnectedDataSelector extends Component {
     const isSearchActive = searchText.trim().length >= MIN_SEARCH_LENGTH;
 
     const isPickerOpen =
-      isSavedQuestionPickerShown ||
-      selectedDataBucketId === DATA_BUCKET.DATASETS;
+      isSavedEntityPickerShown || selectedDataBucketId === DATA_BUCKET.MODELS;
+
+    const isDatasets =
+      selectedDataBucketId === DATA_BUCKET.MODELS ||
+      savedEntityType === "model";
 
     if (this.isLoadingDatasets()) {
       return <LoadingAndErrorWrapper loading />;
@@ -1052,17 +1080,17 @@ export class UnconnectedDataSelector extends Component {
           )}
           {!isSearchActive &&
             (isPickerOpen ? (
-              <SavedQuestionPicker
+              <SavedEntityPicker
                 collectionName={
                   selectedTable &&
                   selectedTable.schema &&
                   getSchemaName(selectedTable.schema.id)
                 }
-                isDatasets={selectedDataBucketId === DATA_BUCKET.DATASETS}
+                isDatasets={isDatasets}
                 tableId={selectedTable?.id}
                 databaseId={currentDatabaseId}
-                onSelect={this.handleSavedQuestionSelect}
-                onBack={this.handleSavedQuestionPickerClose}
+                onSelect={this.handleSavedEntitySelect}
+                onBack={this.handleSavedEntityPickerClose}
               />
             ) : (
               this.renderActiveStep()

diff --git a/...stion-picker/SavedQuestionList.styled.tsx → ...-entity-picker/SavedEntityList.styled.tsx b/...stion-picker/SavedQuestionList.styled.tsx → ...-entity-picker/SavedEntityList.styled.tsx
@@ -4,7 +4,7 @@ import LoadingAndErrorWrapper from "metabase/components/LoadingAndErrorWrapper";
 import SelectList from "metabase/components/SelectList";
 import { breakpointMaxSmall } from "metabase/styled-components/theme/media-queries";
 
-export const SavedQuestionListRoot = styled(SelectList)`
+export const SavedEntityListRoot = styled(SelectList)`
   overflow: auto;
   width: 100%;
   padding: 0.5rem;
@@ -14,13 +14,13 @@ export const SavedQuestionListRoot = styled(SelectList)`
   }
 `;
 
-export const SavedQuestionListItem = styled(SelectList.Item)`
+export const SavedEntityListItem = styled(SelectList.Item)`
   .Icon:last-child {
     justify-self: start;
   }
 `;
 
-export const SavedQuestionListEmptyState = styled.div`
+export const SavedEntityListEmptyState = styled.div`
   margin: 7.5rem 0;
 `;