From da96c24f80cadc5ac8f44af0ef9d14327b751500 Mon Sep 17 00:00:00 2001
From: Romeo Van Snick <romeo@romeovansnick.be>
Date: Tue, 12 Mar 2024 08:27:15 +0100
Subject: [PATCH] Split up "Doing science..." message (#39619)
* Convert QueryVisualization to a functional component
* Add a custom message after loading for a while
* Add test for slow database message
* Loop through different custom messages
* Use cy.clock to test timeouts
* Make getLoadingMessage a function
---
.../visualizations-tabular/table.cy.spec.js | 19 ++
.../metabase-enterprise/settings/selectors.ts | 5 +-
.../settings/selectors.unit.spec.ts | 6 +-
.../whitelabel/lib/loading-message.ts | 20 ++-
.../MetabotQueryBuilder.tsx | 14 +-
frontend/src/metabase/plugins/index.ts | 3 +-
.../components/QueryVisualization.jsx | 168 ++++++++----------
.../QueryVisualization.unit.spec.tsx | 41 +++++
.../query_builder/containers/QueryBuilder.jsx | 2 -
.../whitelabel/tests/common.unit.spec.ts | 21 ++-
.../whitelabel/tests/enterprise.unit.spec.ts | 21 ++-
.../whitelabel/tests/premium.unit.spec.ts | 19 +-
12 files changed, 214 insertions(+), 125 deletions(-)
create mode 100644 frontend/src/metabase/query_builder/components/QueryVisualization.unit.spec.tsx
diff --git a/e2e/test/scenarios/visualizations-tabular/table.cy.spec.js b/e2e/test/scenarios/visualizations-tabular/table.cy.spec.js
index bada9fc232a..09b73e718d1 100644
--- a/e2e/test/scenarios/visualizations-tabular/table.cy.spec.js
+++ b/e2e/test/scenarios/visualizations-tabular/table.cy.spec.js
@@ -326,6 +326,25 @@ describe("scenarios > visualizations > table", () => {
expect(isScrollableHorizontally($popover[0])).to.be.false;
});
});
+
+ it("should show the slow loading text when the query is taking too long", () => {
+ openOrdersTable({ mode: "notebook" });
+
+ cy.intercept("POST", "/api/dataset", req => {
+ req.on("response", res => {
+ res.setDelay(10000);
+ });
+ });
+
+ cy.button("Visualize").click();
+
+ cy.clock();
+ cy.tick(1000);
+ cy.findByTestId("query-builder-main").findByText("Doing science...");
+
+ cy.tick(5000);
+ cy.findByTestId("query-builder-main").findByText("Waiting for results...");
+ });
});
describe("scenarios > visualizations > table > conditional formatting", () => {
diff --git a/enterprise/frontend/src/metabase-enterprise/settings/selectors.ts b/enterprise/frontend/src/metabase-enterprise/settings/selectors.ts
index 668a6a4290c..c9d1d07ce4d 100644
--- a/enterprise/frontend/src/metabase-enterprise/settings/selectors.ts
+++ b/enterprise/frontend/src/metabase-enterprise/settings/selectors.ts
@@ -17,8 +17,9 @@ const getCustomLogoUrl = (settingValues: EnterpriseSettings) => {
export const getLogoUrl = (state: EnterpriseState) =>
getCustomLogoUrl(getSettings(state));
-export const getLoadingMessage = (state: EnterpriseState) =>
- LOADING_MESSAGE_BY_SETTING[getSetting(state, "loading-message")];
+export const getLoadingMessage = (state: EnterpriseState) => {
+ return LOADING_MESSAGE_BY_SETTING[getSetting(state, "loading-message")].value;
+};
// eslint-disable-next-line no-literal-metabase-strings -- This is a Metabase string we want to keep. It's used for comparison.
const DEFAULT_APPLICATION_NAME = "Metabase";
diff --git a/enterprise/frontend/src/metabase-enterprise/settings/selectors.unit.spec.ts b/enterprise/frontend/src/metabase-enterprise/settings/selectors.unit.spec.ts
index 4605cca0a80..dc10c95c0a9 100644
--- a/enterprise/frontend/src/metabase-enterprise/settings/selectors.unit.spec.ts
+++ b/enterprise/frontend/src/metabase-enterprise/settings/selectors.unit.spec.ts
@@ -50,7 +50,7 @@ describe("getLoadingMessage", () => {
const expectedLoadingMessage = "Doing science...";
- expect(getLoadingMessage(states)).toBe(expectedLoadingMessage);
+ expect(getLoadingMessage(states)(false)).toBe(expectedLoadingMessage);
});
it('should show correct loading message when "loading-message" is set to "running-query"', () => {
@@ -63,7 +63,7 @@ describe("getLoadingMessage", () => {
const expectedLoadingMessage = "Loading results...";
("");
- expect(getLoadingMessage(states)).toBe(expectedLoadingMessage);
+ expect(getLoadingMessage(states)(false)).toBe(expectedLoadingMessage);
});
it('should show correct loading message when "loading-message" is set to "loading-results"', () => {
@@ -75,7 +75,7 @@ describe("getLoadingMessage", () => {
const expectedLoadingMessage = "Running query...";
- expect(getLoadingMessage(states)).toBe(expectedLoadingMessage);
+ expect(getLoadingMessage(states)(false)).toBe(expectedLoadingMessage);
});
});
diff --git a/enterprise/frontend/src/metabase-enterprise/whitelabel/lib/loading-message.ts b/enterprise/frontend/src/metabase-enterprise/whitelabel/lib/loading-message.ts
index 190652ff2cd..cc0534aa7ca 100644
--- a/enterprise/frontend/src/metabase-enterprise/whitelabel/lib/loading-message.ts
+++ b/enterprise/frontend/src/metabase-enterprise/whitelabel/lib/loading-message.ts
@@ -1,13 +1,23 @@
import { t } from "ttag";
export const LOADING_MESSAGE_BY_SETTING = {
- "doing-science": t`Doing science...`,
- "running-query": t`Running query...`,
- "loading-results": t`Loading results...`,
+ "doing-science": {
+ name: t`Doing science...`,
+ value: (isSlow: boolean) =>
+ isSlow ? t`Waiting for results...` : t`Doing science...`,
+ },
+ "running-query": {
+ name: t`Running query...`,
+ value: (_: boolean) => t`Running query...`,
+ },
+ "loading-results": {
+ name: t`Loading results...`,
+ value: (_: boolean) => t`Loading results...`,
+ },
};
export const getLoadingMessageOptions = () =>
- Object.entries(LOADING_MESSAGE_BY_SETTING).map(([value, name]) => ({
+ Object.entries(LOADING_MESSAGE_BY_SETTING).map(([value, option]) => ({
+ name: option.name,
value,
- name,
}));
diff --git a/frontend/src/metabase/metabot/components/MetabotQueryBuilder/MetabotQueryBuilder.tsx b/frontend/src/metabase/metabot/components/MetabotQueryBuilder/MetabotQueryBuilder.tsx
index 48a09940292..7bccda9cbde 100644
--- a/frontend/src/metabase/metabot/components/MetabotQueryBuilder/MetabotQueryBuilder.tsx
+++ b/frontend/src/metabase/metabot/components/MetabotQueryBuilder/MetabotQueryBuilder.tsx
@@ -2,7 +2,6 @@ import { connect } from "react-redux";
import { t } from "ttag";
import { getResponseErrorMessage } from "metabase/lib/errors";
-import { getWhiteLabeledLoadingMessage } from "metabase/selectors/whitelabel";
import type { Dataset } from "metabase-types/api";
import type { MetabotQueryStatus, State } from "metabase-types/store";
@@ -28,7 +27,6 @@ import {
} from "./MetabotQueryBuilder.styled";
interface StateProps {
- loadingMessage: string;
queryStatus: MetabotQueryStatus;
queryResults: [Dataset] | null;
queryError: unknown;
@@ -38,7 +36,6 @@ interface StateProps {
type MetabotQueryBuilderProps = StateProps;
const mapStateToProps = (state: State): StateProps => ({
- loadingMessage: getWhiteLabeledLoadingMessage(state),
queryStatus: getQueryStatus(state),
queryResults: getQueryResults(state),
queryError: getQueryError(state),
@@ -46,7 +43,6 @@ const mapStateToProps = (state: State): StateProps => ({
});
const MetabotQueryBuilder = ({
- loadingMessage,
queryStatus,
queryResults,
queryError,
@@ -60,7 +56,7 @@ const MetabotQueryBuilder = ({
<QueryBuilderRoot>
{hasResults && <MetabotQueryEditor />}
{isRunning ? (
- <QueryRunningState loadingMessage={loadingMessage} />
+ <RunningStateRoot />
) : hasErrors ? (
<QueryErrorState queryError={queryError} />
) : hasResults ? (
@@ -81,14 +77,6 @@ const QueryIdleState = () => {
);
};
-interface QueryRunningStateProps {
- loadingMessage: string;
-}
-
-const QueryRunningState = ({ loadingMessage }: QueryRunningStateProps) => {
- return <RunningStateRoot loadingMessage={loadingMessage} />;
-};
-
interface QueryErrorStateProps {
queryError: unknown;
}
diff --git a/frontend/src/metabase/plugins/index.ts b/frontend/src/metabase/plugins/index.ts
index 600d387d6d9..703d82c94a7 100644
--- a/frontend/src/metabase/plugins/index.ts
+++ b/frontend/src/metabase/plugins/index.ts
@@ -133,7 +133,8 @@ export const PLUGIN_IS_PASSWORD_USER: ((user: User) => boolean)[] = [];
// selectors that customize behavior between app versions
export const PLUGIN_SELECTORS = {
canWhitelabel: (_state: State) => false,
- getLoadingMessage: (_state: State) => t`Doing science...`,
+ getLoadingMessage: (_state: State) => (isSlow: boolean) =>
+ isSlow ? t`Waiting for results...` : t`Doing science...`,
getIsWhiteLabeling: (_state: State) => false,
// eslint-disable-next-line no-literal-metabase-strings -- This is the actual Metabase name, so we don't want to translate it.
getApplicationName: (_state: State) => "Metabase",
diff --git a/frontend/src/metabase/query_builder/components/QueryVisualization.jsx b/frontend/src/metabase/query_builder/components/QueryVisualization.jsx
index 2031d2f0ea3..11fc11c62f1 100644
--- a/frontend/src/metabase/query_builder/components/QueryVisualization.jsx
+++ b/frontend/src/metabase/query_builder/components/QueryVisualization.jsx
@@ -1,9 +1,12 @@
/* eslint-disable react/prop-types */
import cx from "classnames";
-import { Component } from "react";
+import { useState } from "react";
+import { useTimeout } from "react-use";
import { t } from "ttag";
import LoadingSpinner from "metabase/components/LoadingSpinner";
+import { useSelector } from "metabase/lib/redux";
+import { getWhiteLabeledLoadingMessage } from "metabase/selectors/whitelabel";
import { HARD_ROW_LIMIT } from "metabase-lib/queries/utils";
import RunButtonWithTooltip from "./RunButtonWithTooltip";
@@ -11,85 +14,63 @@ import { VisualizationError } from "./VisualizationError";
import VisualizationResult from "./VisualizationResult";
import Warnings from "./Warnings";
-export default class QueryVisualization extends Component {
- constructor(props, context) {
- super(props, context);
- this.state = {};
- }
+const SLOW_MESSAGE_TIMEOUT = 4000;
- static defaultProps = {
- // NOTE: this should be more dynamic from the backend, it's set based on the query lang
- maxTableRows: HARD_ROW_LIMIT,
- };
+export default function QueryVisualization(props) {
+ const {
+ className,
+ question,
+ isRunning,
+ isObjectDetail,
+ isResultDirty,
+ isNativeEditorOpen,
+ result,
+ maxTableRows = HARD_ROW_LIMIT,
+ } = props;
- runQuery = () => {
- const { isResultDirty } = this.props;
- // ignore the cache if we're hitting "Refresh" (which we only show if isResultDirty = false)
- this.props.runQuestionQuery({ ignoreCache: !isResultDirty });
- };
+ const [warnings, setWarnings] = useState([]);
- handleUpdateWarnings = warnings => {
- this.setState({ warnings });
- };
-
- render() {
- const {
- className,
- question,
- isRunning,
- isObjectDetail,
- isResultDirty,
- isNativeEditorOpen,
- result,
- loadingMessage,
- } = this.props;
-
- return (
- <div className={cx(className, "relative stacking-context full-height")}>
- {isRunning ? (
- <VisualizationRunningState
- className="spread z2"
- loadingMessage={loadingMessage}
- />
- ) : null}
- <VisualizationDirtyState
- {...this.props}
- hidden={!isResultDirty || isRunning || isNativeEditorOpen}
- className="spread z2"
+ return (
+ <div className={cx(className, "relative stacking-context full-height")}>
+ {isRunning ? <VisualizationRunningState className="spread z2" /> : null}
+ <VisualizationDirtyState
+ {...props}
+ hidden={!isResultDirty || isRunning || isNativeEditorOpen}
+ className="spread z2"
+ />
+ {!isObjectDetail && (
+ <Warnings
+ warnings={warnings}
+ className="absolute top right mt2 mr2 z2"
+ size={18}
/>
- {!isObjectDetail && (
- <Warnings
- warnings={this.state.warnings}
- className="absolute top right mt2 mr2 z2"
- size={18}
+ )}
+ <div
+ className={cx("spread Visualization z1", {
+ "Visualization--loading": isRunning,
+ })}
+ >
+ {result?.error ? (
+ <VisualizationError
+ className="spread"
+ error={result.error}
+ via={result.via}
+ question={question}
+ duration={result.duration}
/>
- )}
- <div
- className={cx("spread Visualization z1", {
- "Visualization--loading": isRunning,
- })}
- >
- {result?.error ? (
- <VisualizationError
- className="spread"
- error={result.error}
- via={result.via}
- question={question}
- duration={result.duration}
- />
- ) : result?.data ? (
- <VisualizationResult
- {...this.props}
- className="spread"
- onUpdateWarnings={this.handleUpdateWarnings}
- />
- ) : !isRunning ? (
- <VisualizationEmptyState className="spread" />
- ) : null}
- </div>
+ ) : result?.data ? (
+ <VisualizationResult
+ {...props}
+ maxTableRows={maxTableRows}
+ className="spread"
+ onUpdateWarnings={setWarnings}
+ />
+ ) : !isRunning ? (
+ <VisualizationEmptyState className="spread" />
+ ) : null}
</div>
- );
- }
+ </div>
+ );
}
export const VisualizationEmptyState = ({ className }) => (
@@ -98,22 +79,29 @@ export const VisualizationEmptyState = ({ className }) => (
</div>
);
-export const VisualizationRunningState = ({
- className = "",
- loadingMessage,
-}) => (
- <div
- className={cx(
- className,
- "Loading flex flex-column layout-centered text-brand",
- )}
- >
- <LoadingSpinner />
- <h2 className="Loading-message text-brand text-uppercase my3">
- {loadingMessage}
- </h2>
- </div>
-);
+export function VisualizationRunningState({ className = "" }) {
+ const [isSlow] = useTimeout(SLOW_MESSAGE_TIMEOUT);
+
+ const loadingMessage = useSelector(getWhiteLabeledLoadingMessage);
+
+ // show the slower loading message only when the loadingMessage is
+ // not customised
+ const message = loadingMessage(isSlow());
+
+ return (
+ <div
+ className={cx(
+ className,
+ "Loading flex flex-column layout-centered text-brand",
+ )}
+ >
+ <LoadingSpinner />
+ <h2 className="Loading-message text-brand text-uppercase my3">
+ {message}
+ </h2>
+ </div>
+ );
+}
export const VisualizationDirtyState = ({
className,
diff --git a/frontend/src/metabase/query_builder/components/QueryVisualization.unit.spec.tsx b/frontend/src/metabase/query_builder/components/QueryVisualization.unit.spec.tsx
new file mode 100644
index 00000000000..d4834a40c68
--- /dev/null
+++ b/frontend/src/metabase/query_builder/components/QueryVisualization.unit.spec.tsx
@@ -0,0 +1,41 @@
+import { renderWithProviders, screen } from "__support__/ui";
+import { PLUGIN_SELECTORS } from "metabase/plugins";
+
+import { VisualizationRunningState } from "./QueryVisualization";
+
+type SetupOpts = {
+ customMessage?: (isSlow: boolean) => string;
+};
+
+function setup({ customMessage }: SetupOpts = {}) {
+ if (customMessage) {
+ jest
+ .spyOn(PLUGIN_SELECTORS, "getLoadingMessage")
+ .mockImplementation(() => customMessage);
+ }
+
+ renderWithProviders(<VisualizationRunningState />);
+}
+
+describe("VisualizationRunningState", () => {
+ it("should render the different loading messages after a while", () => {
+ jest.useFakeTimers();
+
+ setup();
+ expect(screen.getByText("Doing science...")).toBeInTheDocument();
+
+ jest.advanceTimersByTime(5000);
+ expect(screen.getByText("Waiting for results...")).toBeInTheDocument();
+ });
+
+ it("should only render the custom loading message when it was customized", () => {
+ const customMessage = (isSlow: boolean) =>
+ isSlow ? `Custom message (slow)...` : `Custom message...`;
+
+ setup({ customMessage });
+ expect(screen.getByText("Custom message...")).toBeInTheDocument();
+
+ jest.advanceTimersByTime(5000);
+ expect(screen.getByText("Custom message (slow)...")).toBeInTheDocument();
+ });
+});
diff --git a/frontend/src/metabase/query_builder/containers/QueryBuilder.jsx b/frontend/src/metabase/query_builder/containers/QueryBuilder.jsx
index 13165b9ffd2..64c32b17180 100644
--- a/frontend/src/metabase/query_builder/containers/QueryBuilder.jsx
+++ b/frontend/src/metabase/query_builder/containers/QueryBuilder.jsx
@@ -26,7 +26,6 @@ import {
getUserIsAdmin,
canManageSubscriptions,
} from "metabase/selectors/user";
-import { getWhiteLabeledLoadingMessage } from "metabase/selectors/whitelabel";
import * as actions from "../actions";
import View from "../components/view/View";
@@ -166,7 +165,6 @@ const mapStateToProps = (state, props) => {
documentTitle: getDocumentTitle(state),
pageFavicon: getPageFavicon(state),
isLoadingComplete: getIsLoadingComplete(state),
- loadingMessage: getWhiteLabeledLoadingMessage(state),
reportTimezone: getSetting(state, "report-timezone-long"),
diff --git a/frontend/src/metabase/selectors/whitelabel/tests/common.unit.spec.ts b/frontend/src/metabase/selectors/whitelabel/tests/common.unit.spec.ts
index de6de6d3fc1..5b72bb1a5d1 100644
--- a/frontend/src/metabase/selectors/whitelabel/tests/common.unit.spec.ts
+++ b/frontend/src/metabase/selectors/whitelabel/tests/common.unit.spec.ts
@@ -12,19 +12,34 @@ describe("getWhiteLabeledLoadingMessage (OSS)", () => {
it("should return 'Doing science...' when loading-message is set to 'doing-science'", () => {
const { getState } = setup({ loadingMessage: "doing-science" });
- expect(getWhiteLabeledLoadingMessage(getState())).toBe("Doing science...");
+ expect(getWhiteLabeledLoadingMessage(getState())(false)).toBe(
+ "Doing science...",
+ );
+ expect(getWhiteLabeledLoadingMessage(getState())(true)).toBe(
+ "Waiting for results...",
+ );
});
it("should return 'Doing science...' when loading-message is set to 'loading-results'", () => {
const { getState } = setup({ loadingMessage: "loading-results" });
- expect(getWhiteLabeledLoadingMessage(getState())).toBe("Doing science...");
+ expect(getWhiteLabeledLoadingMessage(getState())(false)).toBe(
+ "Doing science...",
+ );
+ expect(getWhiteLabeledLoadingMessage(getState())(true)).toBe(
+ "Waiting for results...",
+ );
});
it("should return 'Doing science...' when loading-message is set to 'running-query'", () => {
const { getState } = setup({ loadingMessage: "running-query" });
- expect(getWhiteLabeledLoadingMessage(getState())).toBe("Doing science...");
+ expect(getWhiteLabeledLoadingMessage(getState())(false)).toBe(
+ "Doing science...",
+ );
+ expect(getWhiteLabeledLoadingMessage(getState())(true)).toBe(
+ "Waiting for results...",
+ );
});
});
diff --git a/frontend/src/metabase/selectors/whitelabel/tests/enterprise.unit.spec.ts b/frontend/src/metabase/selectors/whitelabel/tests/enterprise.unit.spec.ts
index 7417f79774b..32c8a42486b 100644
--- a/frontend/src/metabase/selectors/whitelabel/tests/enterprise.unit.spec.ts
+++ b/frontend/src/metabase/selectors/whitelabel/tests/enterprise.unit.spec.ts
@@ -17,19 +17,34 @@ describe("getWhiteLabeledLoadingMessage (EE without token)", () => {
it("should return 'Doing science...' when loading-message is set to 'doing-science'", () => {
const { getState } = setup({ loadingMessage: "doing-science" });
- expect(getWhiteLabeledLoadingMessage(getState())).toBe("Doing science...");
+ expect(getWhiteLabeledLoadingMessage(getState())(false)).toBe(
+ "Doing science...",
+ );
+ expect(getWhiteLabeledLoadingMessage(getState())(true)).toBe(
+ "Waiting for results...",
+ );
});
it("should return 'Doing science...' when loading-message is set to 'loading-results'", () => {
const { getState } = setup({ loadingMessage: "loading-results" });
- expect(getWhiteLabeledLoadingMessage(getState())).toBe("Doing science...");
+ expect(getWhiteLabeledLoadingMessage(getState())(false)).toBe(
+ "Doing science...",
+ );
+ expect(getWhiteLabeledLoadingMessage(getState())(true)).toBe(
+ "Waiting for results...",
+ );
});
it("should return 'Doing science...' when loading-message is set to 'running-query'", () => {
const { getState } = setup({ loadingMessage: "running-query" });
- expect(getWhiteLabeledLoadingMessage(getState())).toBe("Doing science...");
+ expect(getWhiteLabeledLoadingMessage(getState())(false)).toBe(
+ "Doing science...",
+ );
+ expect(getWhiteLabeledLoadingMessage(getState())(true)).toBe(
+ "Waiting for results...",
+ );
});
});
diff --git a/frontend/src/metabase/selectors/whitelabel/tests/premium.unit.spec.ts b/frontend/src/metabase/selectors/whitelabel/tests/premium.unit.spec.ts
index db3c29eab40..7b8fbb663c1 100644
--- a/frontend/src/metabase/selectors/whitelabel/tests/premium.unit.spec.ts
+++ b/frontend/src/metabase/selectors/whitelabel/tests/premium.unit.spec.ts
@@ -21,13 +21,21 @@ describe("getWhiteLabeledLoadingMessage (EE with token)", () => {
it("should return 'Doing science...' when loading-message is set to 'doing-science'", () => {
const { getState } = setup({ loadingMessage: "doing-science" });
- expect(getWhiteLabeledLoadingMessage(getState())).toBe("Doing science...");
+ expect(getWhiteLabeledLoadingMessage(getState())(false)).toBe(
+ "Doing science...",
+ );
+ expect(getWhiteLabeledLoadingMessage(getState())(true)).toBe(
+ "Waiting for results...",
+ );
});
it("should return 'Loading results...' when loading-message is set to 'loading-results'", () => {
const { getState } = setup({ loadingMessage: "loading-results" });
- expect(getWhiteLabeledLoadingMessage(getState())).toBe(
+ expect(getWhiteLabeledLoadingMessage(getState())(false)).toBe(
+ "Loading results...",
+ );
+ expect(getWhiteLabeledLoadingMessage(getState())(true)).toBe(
"Loading results...",
);
});
@@ -35,7 +43,12 @@ describe("getWhiteLabeledLoadingMessage (EE with token)", () => {
it("should return 'Running query...' when loading-message is set to 'running-query'", () => {
const { getState } = setup({ loadingMessage: "running-query" });
- expect(getWhiteLabeledLoadingMessage(getState())).toBe("Running query...");
+ expect(getWhiteLabeledLoadingMessage(getState())(false)).toBe(
+ "Running query...",
+ );
+ expect(getWhiteLabeledLoadingMessage(getState())(true)).toBe(
+ "Running query...",
+ );
});
});
--
GitLab