Merge pull request #3738 from metabase/qb-bug-fixes

Query builder bug fixes

frontend/src/metabase/components/AddToDashSelectDashModal.jsx

@@ -6,6 +6,7 @@ import ModalContent from "metabase/components/ModalContent.jsx";
 import SortableItemList from 'metabase/components/SortableItemList.jsx';
 
 import Urls from "metabase/lib/urls";
+import { DashboardApi } from "metabase/services";
 
 import moment from 'moment';
 
@@ -24,13 +25,13 @@ export default class AddToDashSelectDashModal extends Component {
 
     static propTypes = {
         card: PropTypes.object.isRequired,
-        dashboardApi: PropTypes.object.isRequired,
         closeFn: PropTypes.func.isRequired,
         onChangeLocation: PropTypes.func.isRequired
     };
 
     async loadDashboardList() {
-        var dashboards = await this.props.dashboardApi.list({ f: "all" });
+        // TODO: reduxify
+        var dashboards = await DashboardApi.list({ f: "all" });
         for (var dashboard of dashboards) {
             dashboard.updated_at = moment(dashboard.updated_at);
         }
@@ -43,7 +44,8 @@ export default class AddToDashSelectDashModal extends Component {
     }
 
     async createDashboard(newDashboard) {
-        let dashboard = await this.props.dashboardApi.create(newDashboard);
+        // TODO: reduxify
+        let dashboard = await DashboardApi.create(newDashboard);
         // this.props.notifyDashboardCreatedFn(dashboard);
         this.addToDashboard(dashboard);
     }

frontend/src/metabase/lib/query.js

@@ -574,6 +574,20 @@ var Query = {
         console.warn("Unknown field type: ", field);
     },
 
+    getFieldPath(fieldId, tableDef) {
+        let path = [];
+        while (fieldId != null) {
+            let field = Table.getField(tableDef, fieldId);
+            path.unshift(field);
+            fieldId = field && field.parent_id;
+        }
+        return path;
+    },
+
+    getFieldPathName(fieldId, tableDef) {
+        return Query.getFieldPath(fieldId, tableDef).map(f => f && f.display_name).join(": ")
+    },
+
     getDatetimeUnit(field) {
         if (field.length === 4) {
             return field[3]; // deprecated

frontend/src/metabase/query_builder/actions.js

@@ -20,7 +20,7 @@ import { getEngineNativeType, formatJsonQuery } from "metabase/lib/engine";
 import { defer } from "metabase/lib/promise";
 import { applyParameters } from "metabase/meta/Card";
 
-import { getParameters, getNativeDatabases } from "./selectors";
+import { isDirty, getParameters, getNativeDatabases } from "./selectors";
 
 import { MetabaseApi, CardApi, UserApi } from "metabase/services";
 
@@ -92,9 +92,16 @@ export const updateUrl = createThunkAction(UPDATE_URL, (card, isDirty = false, r
     }
 );
 
+export const RESET_QB = "metabase/qb/RESET_QB";
+export const resetQB = createAction(RESET_QB);
+
 export const INITIALIZE_QB = "INITIALIZE_QB";
 export const initializeQB = createThunkAction(INITIALIZE_QB, (location, params) => {
     return async (dispatch, getState) => {
+        // do this immediately to ensure old state is cleared before the user sees it
+        dispatch(resetQB());
+        dispatch(cancelQuery());
+
         const { currentUser } = getState();
 
         let card, databases, originalCard, uiControls = {};
@@ -387,10 +394,6 @@ export const setParameterValue = createAction(SET_PARAMETER_VALUE, (parameterId,
 export const NOTIFY_CARD_CREATED = "NOTIFY_CARD_CREATED";
 export const notifyCardCreatedFn = createThunkAction(NOTIFY_CARD_CREATED, (card) => {
     return (dispatch, getState) => {
-        dispatch(loadMetadataForCard(card));
-
-        // we do this to force the indication of the fact that the card should not be considered dirty when the url is updated
-        dispatch(runQuery(card, false));
         dispatch(updateUrl(card, false));
 
         MetabaseAnalytics.trackEvent("QueryBuilder", "Create Card", card.dataset_query.type);
@@ -402,10 +405,6 @@ export const notifyCardCreatedFn = createThunkAction(NOTIFY_CARD_CREATED, (card)
 export const NOTIFY_CARD_UPDATED = "NOTIFY_CARD_UPDATED";
 export const notifyCardUpdatedFn = createThunkAction("NOTIFY_CARD_UPDATED", (card) => {
     return (dispatch, getState) => {
-        dispatch(loadMetadataForCard(card));
-
-        // we do this to force the indication of the fact that the card should not be considered dirty when the url is updated
-        dispatch(runQuery(card, false));
         dispatch(updateUrl(card, false));
 
         MetabaseAnalytics.trackEvent("QueryBuilder", "Update Card", card.dataset_query.type);
@@ -791,7 +790,8 @@ export const runQuery = createThunkAction(RUN_QUERY, (card, shouldUpdateUrl = tr
             dispatch(queryErrored(startTime, error));
         }
 
-        if (card && card.id) {
+        // use the CardApi.query if the query is saved and not dirty so users with view but not create permissions can see it.
+        if (card && card.id && !isDirty(state)) {
             CardApi.query({ cardId: card.id, parameters: card.dataset_query.parameters }, { cancelled: cancelQueryDeferred.promise }).then(onQuerySuccess, onQueryError);
         } else {
             MetabaseApi.dataset(card.dataset_query, { cancelled: cancelQueryDeferred.promise }).then(onQuerySuccess, onQueryError);

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

@@ -10,6 +10,7 @@ import QueryDefinitionTooltip from "./QueryDefinitionTooltip.jsx";
 import { isDate, getIconForField } from 'metabase/lib/schema_metadata';
 import { parseFieldBucketing, parseFieldTarget } from "metabase/lib/query_time";
 import { stripId, singularize } from "metabase/lib/formatting";
+import Query from "metabase/lib/query";
 
 import _ from "underscore";
 
@@ -61,7 +62,7 @@ export default class FieldList extends Component {
         let mainSection = {
             name: singularize(tableName),
             items: specialOptions.concat(fieldOptions.fields.map(field => ({
-                name: field.display_name,
+                name: Query.getFieldPathName(field.id, tableMetadata),
                 value: field.id,
                 field: field
             })))
@@ -70,7 +71,7 @@ export default class FieldList extends Component {
         let fkSections = fieldOptions.fks.map(fk => ({
             name: stripId(fk.field.display_name),
             items: fk.fields.map(field => ({
-                name: field.display_name,
+                name: Query.getFieldPathName(field.id, tableMetadata),
                 value: ["fk->", fk.field.id, field.id],
                 field: field
             }))

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

 import React, { Component, PropTypes } from "react";
 
-import i from 'icepick';
-
 import Icon from "metabase/components/Icon.jsx";
 
 import Query from "metabase/lib/query";
@@ -30,7 +28,9 @@ export default class FieldName extends Component {
 
         let parts = [];
 
-        if (fieldTarget) {
+        if (fieldTarget && !fieldTarget.field) {
+            parts.push(<span className="text-error" key="field">Missing Field</span>);
+        } else if (fieldTarget) {
             // fk path
             for (let [index, fkField] of Object.entries(fieldTarget.path)) {
                 parts.push(<span key={"fkName"+index}>{stripId(fkField.display_name)}</span>);
@@ -38,7 +38,7 @@ export default class FieldName extends Component {
             }
             // target field itself
             // using i.getIn to avoid exceptions when field is undefined
-            parts.push(<span key="field">{i.getIn(fieldTarget, ['field', 'display_name'])}</span>);
+            parts.push(<span key="field">{Query.getFieldPathName(fieldTarget.field.id, tableMetadata)}</span>);
             // datetime-field unit
             if (fieldTarget.unit != null) {
                 parts.push(<span key="unit">{": " + formatBucketing(fieldTarget.unit)}</span>);

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

@@ -15,6 +15,8 @@ import QuestionSavedModal from 'metabase/components/QuestionSavedModal.jsx';
 import SaveQuestionModal from 'metabase/components/SaveQuestionModal.jsx';
 import Tooltip from "metabase/components/Tooltip.jsx";
 
+import { CardApi, RevisionApi } from "metabase/services";
+
 import MetabaseAnalytics from "metabase/lib/analytics";
 import Query from "metabase/lib/query";
 import { cancelable } from "metabase/lib/promise";
@@ -44,9 +46,6 @@ export default class QueryHeader extends Component {
         originalCard: PropTypes.object,
         isEditing: PropTypes.bool.isRequired,
         tableMetadata: PropTypes.object, // can't be required, sometimes null
-        cardApi: PropTypes.object.isRequired,
-        dashboardApi: PropTypes.object.isRequired,
-        revisionApi: PropTypes.object.isRequired,
         onSetCardAttribute: PropTypes.func.isRequired,
         reloadCardFn: PropTypes.func.isRequired,
         setQueryModeFn: PropTypes.func.isRequired,
@@ -84,7 +83,8 @@ export default class QueryHeader extends Component {
             Query.cleanQuery(card.dataset_query.query);
         }
 
-        this.requesetPromise = cancelable(this.props.cardApi.create(card));
+        // TODO: reduxify
+        this.requesetPromise = cancelable(CardApi.create(card));
         return this.requesetPromise.then(newCard => {
             this.props.notifyCardCreatedFn(newCard);
 
@@ -108,7 +108,8 @@ export default class QueryHeader extends Component {
             Query.cleanQuery(card.dataset_query.query);
         }
 
-        this.requesetPromise = cancelable(this.props.cardApi.update(card));
+        // TODO: reduxify
+        this.requesetPromise = cancelable(CardApi.update(card));
         return this.requesetPromise.then(updatedCard => {
             if (this.props.fromUrl) {
                 this.onGoBack();
@@ -137,7 +138,8 @@ export default class QueryHeader extends Component {
     }
 
     async onDelete() {
-        await this.props.cardApi.delete({ 'cardId': this.props.card.id });
+        // TODO: reduxify
+        await CardApi.delete({ 'cardId': this.props.card.id });
         this.onGoBack();
         MetabaseAnalytics.trackEvent("QueryBuilder", "Delete");
     }
@@ -155,12 +157,14 @@ export default class QueryHeader extends Component {
     }
 
     async onFetchRevisions({ entity, id }) {
-        var revisions = await this.props.revisionApi.list({ entity, id });
+        // TODO: reduxify
+        var revisions = await RevisionApi.list({ entity, id });
         this.setState({ revisions });
     }
 
     onRevertToRevision({ entity, id, revision_id }) {
-        return this.props.revisionApi.revert({ entity, id, revision_id });
+        // TODO: reduxify
+        return RevisionApi.revert({ entity, id, revision_id });
     }
 
     onRevertedRevision() {
@@ -392,7 +396,6 @@ export default class QueryHeader extends Component {
                 <Modal isOpen={this.state.modal === "add-to-dashboard"} onClose={this.onCloseModal}>
                     <AddToDashSelectDashModal
                         card={this.props.card}
-                        dashboardApi={this.props.dashboardApi}
                         closeFn={this.onCloseModal}
                         onChangeLocation={this.props.onChangeLocation}
                     />

frontend/src/metabase/query_builder/containers/QueryBuilder.jsx

@@ -43,7 +43,7 @@ import {
 import * as actions from "../actions";
 import { push } from "react-router-redux";
 
-import { CardApi, DashboardApi, RevisionApi, MetabaseApi } from "metabase/services";
+import { MetabaseApi } from "metabase/services";
 
 function cellIsClickable(queryResult, rowIndex, columnIndex) {
     if (!queryResult) return false;
@@ -97,9 +97,6 @@ const mapStateToProps = (state, props) => {
         isRunning:                 state.qb.uiControls.isRunning,
         isRunnable:                getIsRunnable(state),
 
-        cardApi:                   CardApi,
-        dashboardApi:              DashboardApi,
-        revisionApi:               RevisionApi,
         loadTableAndForeignKeysFn: loadTableAndForeignKeys,
         autocompleteResultsFn:     (prefix) => autocompleteResults(state.qb.card, prefix),
         cellIsClickableFn:         (rowIndex, columnIndex) => cellIsClickable(state.qb.queryResult, rowIndex, columnIndex)
@@ -159,6 +156,9 @@ export default class QueryBuilder extends Component {
     }
 
     componentWillUnmount() {
+        // cancel the query if one is running
+        this.props.cancelQuery();
+
         window.removeEventListener("resize", this.handleResize);
         document.addEventListener("keydown", this.handleKeyDown);
     }

frontend/src/metabase/query_builder/reducers.js

@@ -2,6 +2,7 @@ import { handleActions } from "redux-actions";
 import i from "icepick";
 
 import {
+    RESET_QB,
     INITIALIZE_QB,
     TOGGLE_DATA_REFERENCE,
     TOGGLE_TEMPLATE_TAGS_EDITOR,
@@ -70,6 +71,7 @@ export const uiControls = handleActions({
 
 // the card that is actively being worked on
 export const card = handleActions({
+    [RESET_QB]: { next: (state, { payload }) => null },
     [INITIALIZE_QB]: { next: (state, { payload }) => payload ? payload.card : null },
     [RELOAD_CARD]: { next: (state, { payload }) => payload },
     [CANCEL_EDITING]: { next: (state, { payload }) => payload },
@@ -113,11 +115,13 @@ export const databases = handleActions({
 
 // the table actively being queried against.  this is only used for MBQL queries.
 export const tableMetadata = handleActions({
+    [RESET_QB]: { next: (state, { payload }) => null },
     [LOAD_DATABASE]: { next: (state, { payload }) => null},
     [LOAD_TABLE_METADATA]: { next: (state, { payload }) => payload && payload.table ? payload.table : state }
 }, null);
 
 export const tableForeignKeys = handleActions({
+    [RESET_QB]: { next: (state, { payload }) => null },
     [LOAD_DATABASE]: { next: (state, { payload }) => null},
     [LOAD_TABLE_METADATA]: { next: (state, { payload }) => payload && payload.foreignKeys ? payload.foreignKeys : state }
 }, null);
@@ -134,9 +138,9 @@ export const tableForeignKeyReferences = handleActions({
 
 // the result of a query execution.  optionally an error if the query fails to complete successfully.
 export const queryResult = handleActions({
+    [RESET_QB]: { next: (state, { payload }) => null },
     [QUERY_COMPLETED]: { next: (state, { payload }) => payload.queryResult },
-    [QUERY_ERRORED]: { next: (state, { payload }) => payload ? payload : state },
-    [INITIALIZE_QB]: { next: (state, { payload }) => null }
+    [QUERY_ERRORED]: { next: (state, { payload }) => payload ? payload : state }
 }, null);
 
 // promise used for tracking a query execution in progress.  when a query is started we capture this.