From a1b302a1b3aa3a6eccb1e9a8914e3a739ebcb223 Mon Sep 17 00:00:00 2001
From: Alexander Polyankin <alexander.polyankin@metabase.com>
Date: Tue, 7 Feb 2023 21:33:44 +0200
Subject: [PATCH] Show query error in the parameter modal (#28119)
---
.../ValuesSourceModal.unit.spec.tsx | 47 +++++++++++++++++-
.../ValuesSourceTypeModal.tsx | 48 +++++++++++--------
.../test/__support__/server-mocks/dataset.ts | 4 ++
3 files changed, 78 insertions(+), 21 deletions(-)
diff --git a/frontend/src/metabase/parameters/components/ValuesSourceModal/ValuesSourceModal.unit.spec.tsx b/frontend/src/metabase/parameters/components/ValuesSourceModal/ValuesSourceModal.unit.spec.tsx
index d6468b4e693..e87a9b72ea5 100644
--- a/frontend/src/metabase/parameters/components/ValuesSourceModal/ValuesSourceModal.unit.spec.tsx
+++ b/frontend/src/metabase/parameters/components/ValuesSourceModal/ValuesSourceModal.unit.spec.tsx
@@ -12,6 +12,7 @@ import {
import {
setupCardsEndpoints,
setupCollectionsEndpoints,
+ setupErrorParameterValuesEndpoints,
setupParameterValuesEndpoints,
setupUnauthorizedCardsEndpoints,
} from "__support__/server-mocks";
@@ -135,6 +136,10 @@ describe("ValuesSourceModal", () => {
}),
});
+ expect(
+ await screen.findByText(/We don’t have any cached values/),
+ ).toBeInTheDocument();
+
userEvent.click(screen.getByRole("radio", { name: "Custom list" }));
expect(screen.getByRole("radio", { name: "Custom list" })).toBeChecked();
expect(screen.getByRole("textbox")).toHaveValue("A\nB");
@@ -241,7 +246,7 @@ describe("ValuesSourceModal", () => {
});
});
- it("should display an error message when the user has no access to the card", async () => {
+ it("should display a message when the user has no access to the card", async () => {
setup({
parameter: createMockUiParameter({
values_source_type: "card",
@@ -263,6 +268,37 @@ describe("ValuesSourceModal", () => {
).toBeInTheDocument();
});
+ it("should display a message when there is an error in the underlying query", async () => {
+ setup({
+ parameter: createMockUiParameter({
+ values_source_type: "card",
+ values_source_config: {
+ card_id: 1,
+ value_field: ["field", 2, null],
+ },
+ }),
+ cards: [
+ createMockCard({
+ id: 1,
+ name: "Products",
+ result_metadata: [
+ createMockField({
+ id: 2,
+ display_name: "Category",
+ base_type: "type/Text",
+ semantic_type: "type/Category",
+ }),
+ ],
+ }),
+ ],
+ hasParameterValuesError: true,
+ });
+
+ expect(
+ await screen.findByText("An error occurred in your query"),
+ ).toBeInTheDocument();
+ });
+
it("should copy card values when switching to custom list", async () => {
setup({
parameter: createMockUiParameter({
@@ -339,6 +375,7 @@ interface SetupOpts {
parameterValues?: ParameterValues;
cards?: Card[];
hasDataAccess?: boolean;
+ hasParameterValuesError?: boolean;
}
const setup = ({
@@ -346,6 +383,7 @@ const setup = ({
parameterValues = createMockParameterValues(),
cards = [],
hasDataAccess = true,
+ hasParameterValuesError = false,
}: SetupOpts = {}) => {
const scope = nock(location.origin);
const onSubmit = jest.fn();
@@ -354,7 +392,12 @@ const setup = ({
if (hasDataAccess) {
setupCollectionsEndpoints(scope, [createMockCollection(ROOT_COLLECTION)]);
setupCardsEndpoints(scope, cards);
- setupParameterValuesEndpoints(scope, parameterValues);
+
+ if (!hasParameterValuesError) {
+ setupParameterValuesEndpoints(scope, parameterValues);
+ } else {
+ setupErrorParameterValuesEndpoints(scope);
+ }
} else {
setupUnauthorizedCardsEndpoints(scope, cards);
}
diff --git a/frontend/src/metabase/parameters/components/ValuesSourceModal/ValuesSourceTypeModal.tsx b/frontend/src/metabase/parameters/components/ValuesSourceModal/ValuesSourceTypeModal.tsx
index b1dfbe5c445..4e48aba8e0e 100644
--- a/frontend/src/metabase/parameters/components/ValuesSourceModal/ValuesSourceTypeModal.tsx
+++ b/frontend/src/metabase/parameters/components/ValuesSourceModal/ValuesSourceTypeModal.tsx
@@ -1,7 +1,7 @@
import React, {
ChangeEvent,
useCallback,
- useEffect,
+ useLayoutEffect,
useMemo,
useState,
} from "react";
@@ -201,20 +201,20 @@ const FieldSourceModal = ({
onChangeSourceType,
onChangeSourceConfig,
}: FieldSourceModalProps) => {
- const parameterValues = useParameterValues({
+ const { values, isLoading } = useParameterValues({
parameter,
sourceType,
sourceConfig,
onFetchParameterValues,
});
- const parameterValuesText = useMemo(
- () => getValuesText(getSourceValues(parameterValues)),
- [parameterValues],
+ const valuesText = useMemo(
+ () => getValuesText(getSourceValues(values)),
+ [values],
);
const hasFields = getFields(parameter).length > 0;
- const hasEmptyValues = parameterValues && parameterValues.length === 0;
+ const hasEmptyValues = !isLoading && values.length === 0;
return (
<ModalBodyWithPane>
@@ -223,7 +223,7 @@ const FieldSourceModal = ({
<ModalLabel>{t`Where values should come from`}</ModalLabel>
<SourceTypeOptions
parameter={parameter}
- parameterValues={parameterValues}
+ parameterValues={values}
sourceType={sourceType}
sourceConfig={sourceConfig}
onChangeSourceType={onChangeSourceType}
@@ -241,7 +241,7 @@ const FieldSourceModal = ({
{t`We don’t have any cached values for the connected fields. Try one of the other options, or change this widget to a search box.`}
</ModalEmptyState>
) : (
- <ModalTextArea value={parameterValuesText} readOnly fullWidth />
+ <ModalTextArea value={valuesText} readOnly fullWidth />
)}
</ModalMain>
</ModalBodyWithPane>
@@ -279,16 +279,16 @@ const CardSourceModal = ({
return getFieldByReference(fields, sourceConfig.value_field);
}, [fields, sourceConfig]);
- const parameterValues = useParameterValues({
+ const { values, isError } = useParameterValues({
parameter,
sourceType,
sourceConfig,
onFetchParameterValues,
});
- const parameterValuesText = useMemo(
- () => getValuesText(getSourceValues(parameterValues)),
- [parameterValues],
+ const valuesText = useMemo(
+ () => getValuesText(getSourceValues(values)),
+ [values],
);
const handleFieldChange = useCallback(
@@ -308,7 +308,7 @@ const CardSourceModal = ({
<ModalLabel>{t`Where values should come from`}</ModalLabel>
<SourceTypeOptions
parameter={parameter}
- parameterValues={parameterValues}
+ parameterValues={values}
sourceType={sourceType}
sourceConfig={sourceConfig}
onChangeSourceType={onChangeSourceType}
@@ -354,8 +354,10 @@ const CardSourceModal = ({
<ModalEmptyState>{t`Pick a model or question`}</ModalEmptyState>
) : !selectedField ? (
<ModalEmptyState>{t`Pick a column`}</ModalEmptyState>
+ ) : isError ? (
+ <ModalEmptyState>{t`An error occurred in your query`}</ModalEmptyState>
) : (
- <ModalTextArea value={parameterValuesText} readOnly fullWidth />
+ <ModalTextArea value={valuesText} readOnly fullWidth />
)}
</ModalMain>
</ModalBodyWithPane>
@@ -446,6 +448,12 @@ const getSourceTypeOptions = (
];
};
+interface ParameterValuesState {
+ values: ParameterValue[];
+ isLoading?: boolean;
+ isError?: boolean;
+}
+
interface UseParameterValuesOpts {
parameter: Parameter;
sourceType: ValuesSourceType;
@@ -461,7 +469,7 @@ const useParameterValues = ({
sourceConfig,
onFetchParameterValues,
}: UseParameterValuesOpts) => {
- const [values, setValues] = useState<ParameterValue[]>();
+ const [state, setState] = useState<ParameterValuesState>({ values: [] });
const handleFetchValues = useSafeAsyncFunction(onFetchParameterValues);
const isValidSource = isValidSourceConfig(sourceType, sourceConfig);
@@ -474,15 +482,17 @@ const useParameterValues = ({
[initialParameter, sourceType, sourceConfig],
);
- useEffect(() => {
+ useLayoutEffect(() => {
if (isValidSource) {
+ setState(({ values }) => ({ values, isLoading: true }));
+
handleFetchValues({ parameter })
- .then(({ values }) => setValues(values))
- .catch(() => setValues([]));
+ .then(({ values }) => setState({ values }))
+ .catch(() => setState({ values: [], isError: true }));
}
}, [parameter, isValidSource, handleFetchValues]);
- return values;
+ return state;
};
const mapStateToProps = (
diff --git a/frontend/test/__support__/server-mocks/dataset.ts b/frontend/test/__support__/server-mocks/dataset.ts
index b87e3ca5d39..a50cbe36e12 100644
--- a/frontend/test/__support__/server-mocks/dataset.ts
+++ b/frontend/test/__support__/server-mocks/dataset.ts
@@ -7,3 +7,7 @@ export function setupParameterValuesEndpoints(
) {
scope.post("/api/dataset/parameter/values").reply(200, parameterValues);
}
+
+export function setupErrorParameterValuesEndpoints(scope: Scope) {
+ scope.post("/api/dataset/parameter/values").reply(500);
+}
--
GitLab