Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add warning to widgets when column is missing #427

Merged
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
- Ensure source exists in HistogramWidget before getting stats [#426](https://github.com/CartoDB/carto-react/pull/426)
- Use en dash for intervals instead of hyphen [#428](https://github.com/CartoDB/carto-react/pull/428) and [#429](https://github.com/CartoDB/carto-react/pull/429)
- Remove widgets dropping features warning in global mode [#430](https://github.com/CartoDB/carto-react/pull/430)
- Add warning to widgets when column is missing [#427](https://github.com/CartoDB/carto-react/pull/427)

## 1.3

Expand Down
10 changes: 10 additions & 0 deletions packages/react-api/src/api/common.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,17 @@
import { InvalidColumnError } from '@carto/react-core/';

/**
* Return more descriptive error from API
*/
export function dealWithApiError({ response, data }) {
if (data.error === 'Column not found') {
throw new InvalidColumnError(`${data.error} ${data.column_name}`);
}

if (data.error?.includes('Missing columns')) {
throw new InvalidColumnError(data.error);
}

switch (response.status) {
case 401:
throw new Error('Unauthorized access. Invalid credentials');
Expand Down
7 changes: 6 additions & 1 deletion packages/react-api/src/api/stats.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { assert, checkCredentials, makeCall } from './common';
import { MAP_TYPES, API_VERSIONS } from '@deck.gl/carto';
import { getTileJson } from './tilejson';
import { InvalidColumnError } from '@carto/react-core/';

/**
* Execute a stats service request.
Expand All @@ -27,7 +28,11 @@ export async function getStats(props) {
const tileJson = await getTileJson({ source });
const tileStatsAttributes = tileJson.tilestats.layers[0].attributes;
const columnStats = tileStatsAttributes.find(({ attribute }) => attribute === column);
assert(columnStats, 'getStats: column not found in tileset attributes');

if (!columnStats) {
throw new InvalidColumnError(`${column} not found in tileset attributes`);
}

return columnStats;
} else {
const url = buildUrl(source, column);
Expand Down
2 changes: 2 additions & 0 deletions packages/react-core/src/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ export {

export { getMonday } from './utils/dateUtils';

export { InvalidColumnError } from './utils/InvalidColumnError';

export { debounce } from './utils/debounce';
export { throttle } from './utils/throttle';
export { randomString } from './utils/randomString';
Expand Down
2 changes: 2 additions & 0 deletions packages/react-core/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ export {

export { getMonday } from './utils/dateUtils';

export { InvalidColumnError } from './utils/InvalidColumnError';

export { debounce } from './utils/debounce';
export { throttle } from './utils/throttle';
export { randomString } from './utils/randomString';
Expand Down
15 changes: 15 additions & 0 deletions packages/react-core/src/utils/InvalidColumnError.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
const NAME = 'InvalidColumnError';
const ERR_START_MESSAGE = `${NAME}: `;

export class InvalidColumnError extends Error {
constructor(message) {
Copy link
Contributor

@zbigg zbigg Jun 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My proposition would be to change this to

super(`ERR_INVALID_COLUMN: ${message || default}`)

and in is just test for existence of 'ERR_INVALID_COLUMN: ' in error message.
This is still string test, but at least based on something that looks like error code and is universal and doesn't depend on particular details like adding "Uncaught..." by particular runtime.

super(`${ERR_START_MESSAGE}${message}`);
this.name = NAME;
}

static is(error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know.

This whole mechanism of detecting type of exception by details of how react-workers pass errors seems fishy.

First of all, this is not "general" check for InvalidColumnError, but only for errors from worker which seem to have "Uncaught ${error.name} ..." injected at beginning of message.

It would be really better to have error codes, and proper error propagation between workers and main thread.

Anyway, i. am worried that all errors throws by "remote" api calls don't have "Uncaught: InvalidColumnError" prefix and thus don't pass this test ... and as result don't trigger warning.

return (
error instanceof InvalidColumnError || error.message?.includes(ERR_START_MESSAGE)
);
}
}
32 changes: 30 additions & 2 deletions packages/react-widgets/__tests__/hooks/useWidgetFetch.test.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { InvalidColumnError } from '@carto/react-core/';
import { DEFAULT_INVALID_COLUMN_ERR } from '../../src/widgets/utils/constants';
import { act, render, screen } from '@testing-library/react';
import React from 'react';
import useWidgetFetch from '../../src/hooks/useWidgetFetch';
Expand Down Expand Up @@ -79,17 +81,43 @@ describe('useWidgetFetch', () => {
await act(() => sleep(250));
expect(screen.queryByText('loading')).not.toBeInTheDocument();

expect(onError).toBeCalled();
expect(onError).toBeCalledTimes(1);

modelFn.mockRejectedValue(new InvalidColumnError('Invalid column'));

rerender(
<TestComponent
modelFn={modelFn}
args={{
id: 'test',
dataSource: 'test',
params: PARAMS_MOCK,
global: false,
onError
}}
/>
);

expect(screen.getByText('loading')).toBeInTheDocument();
await act(() => sleep(250));
expect(screen.queryByText('loading')).not.toBeInTheDocument();
expect(onError).toBeCalledTimes(1);
expect(screen.queryByText(DEFAULT_INVALID_COLUMN_ERR)).toBeInTheDocument();
});
});

// Aux
function TestComponent({ modelFn, args }) {
const { data, isLoading } = useWidgetFetch(modelFn, args);
const { data, isLoading, warning } = useWidgetFetch(modelFn, args);

if (isLoading) {
return <div>loading</div>;
}

if (warning) {
return <div>{warning}</div>;
}

return <div>{data}</div>;
}

Expand Down
14 changes: 11 additions & 3 deletions packages/react-widgets/src/hooks/useWidgetFetch.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import { selectAreFeaturesReadyForSource } from '@carto/react-redux/';
import { InvalidColumnError } from '@carto/react-core';
import { selectAreFeaturesReadyForSource } from '@carto/react-redux';
import { dequal } from 'dequal';
import { useState } from 'react';
import { useSelector } from 'react-redux';
import { DEFAULT_INVALID_COLUMN_ERR } from '../widgets/utils/constants';
import useCustomCompareEffect from './useCustomCompareEffect';
import useWidgetSource from './useWidgetSource';

Expand All @@ -12,6 +14,7 @@ export default function useWidgetFetch(
// State
const [data, setData] = useState();
const [isLoading, setIsLoading] = useState(false);
const [warning, setWarning] = useState('');

const isSourceReady = useSelector(
(state) => global || selectAreFeaturesReadyForSource(state, dataSource)
Expand All @@ -21,6 +24,7 @@ export default function useWidgetFetch(
useCustomCompareEffect(
() => {
setIsLoading(true);
setWarning('');

if (source && isSourceReady && enabled) {
modelFn({
Expand All @@ -34,7 +38,11 @@ export default function useWidgetFetch(
}
})
.catch((error) => {
if (onError) onError(error);
if (InvalidColumnError.is(error)) {
setWarning(DEFAULT_INVALID_COLUMN_ERR);
} else if (onError) {
onError(error);
}
})
.finally(() => {
setIsLoading(false);
Expand All @@ -45,5 +53,5 @@ export default function useWidgetFetch(
dequal
);

return { data, isLoading, isSourceReady, source };
return { data, isLoading, isSourceReady, source, warning };
}
7 changes: 6 additions & 1 deletion packages/react-widgets/src/widgets/BarWidget.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,11 @@ function BarWidget({
}) {
const dispatch = useDispatch();

const { data: _data = [], isLoading } = useWidgetFetch(getCategories, {
const {
data: _data = [],
isLoading,
warning
} = useWidgetFetch(getCategories, {
id,
dataSource,
params: {
Expand Down Expand Up @@ -132,6 +136,7 @@ function BarWidget({
<WrapperWidgetUI title={title} isLoading={isLoading} {...wrapperProps}>
<WidgetWithAlert
dataSource={dataSource}
warning={warning}
global={global}
droppingFeaturesAlertProps={droppingFeaturesAlertProps}
noDataAlertProps={noDataAlertProps}
Expand Down
7 changes: 6 additions & 1 deletion packages/react-widgets/src/widgets/CategoryWidget.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,11 @@ function CategoryWidget(props) {
useWidgetFilterValues({ dataSource, id, column, type: FilterTypes.IN }) ||
EMPTY_ARRAY;

const { data = [], isLoading } = useWidgetFetch(getCategories, {
const {
data = [],
isLoading,
warning
} = useWidgetFetch(getCategories, {
id,
dataSource,
params: {
Expand Down Expand Up @@ -100,6 +104,7 @@ function CategoryWidget(props) {
<WrapperWidgetUI title={title} isLoading={isLoading} {...wrapperProps}>
<WidgetWithAlert
dataSource={dataSource}
warning={warning}
global={global}
droppingFeaturesAlertProps={droppingFeaturesAlertProps}
noDataAlertProps={noDataAlertProps}
Expand Down
7 changes: 6 additions & 1 deletion packages/react-widgets/src/widgets/FormulaWidget.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,11 @@ function FormulaWidget({
wrapperProps,
droppingFeaturesAlertProps
}) {
const { data = { value: undefined }, isLoading } = useWidgetFetch(getFormula, {
const {
data = { value: undefined },
isLoading,
warning
} = useWidgetFetch(getFormula, {
id,
dataSource,
params: {
Expand All @@ -53,6 +57,7 @@ function FormulaWidget({
<WrapperWidgetUI title={title} isLoading={isLoading} {...wrapperProps}>
<WidgetWithAlert
dataSource={dataSource}
warning={warning}
global={global}
droppingFeaturesAlertProps={droppingFeaturesAlertProps}
>
Expand Down
21 changes: 19 additions & 2 deletions packages/react-widgets/src/widgets/HistogramWidget.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import useWidgetFetch from '../hooks/useWidgetFetch';
import { _getStats } from '@carto/react-api';
import useWidgetSource from '../hooks/useWidgetSource';
import WidgetWithAlert from './utils/WidgetWithAlert';
import { InvalidColumnError } from '@carto/react-core/';
import { DEFAULT_INVALID_COLUMN_ERR } from './utils/constants';

const EMPTY_ARRAY = [];

Expand Down Expand Up @@ -63,6 +65,8 @@ function HistogramWidget({

const [[min, max], setMinMax] = useState([_min, _max]);

const [_warning, setWarning] = useState('');

const source = useWidgetSource({ dataSource, id });

const hasMinMax =
Expand All @@ -73,12 +77,20 @@ function HistogramWidget({

useEffect(() => {
if (!hasMinMax && source) {
setWarning('');

_getStats({ column, source })
.then((res) => {
const { min, max } = res;
setMinMax([min, max]);
})
.catch((err) => onError?.(err));
.catch((err) => {
if (InvalidColumnError.is(err)) {
setWarning(DEFAULT_INVALID_COLUMN_ERR);
} else if (onError) {
onError(err);
}
});
}
}, [column, source, onError, hasMinMax]);

Expand All @@ -96,7 +108,11 @@ function HistogramWidget({
return [];
}, [min, max, _ticks, bins, hasMinMax]);

let { data = EMPTY_ARRAY, isLoading } = useWidgetFetch(getHistogram, {
let {
data = EMPTY_ARRAY,
isLoading,
warning = _warning
} = useWidgetFetch(getHistogram, {
id,
dataSource,
params: {
Expand Down Expand Up @@ -165,6 +181,7 @@ function HistogramWidget({
<WrapperWidgetUI title={title} {...wrapperProps} isLoading={isLoading}>
<WidgetWithAlert
dataSource={dataSource}
warning={warning}
global={global}
droppingFeaturesAlertProps={droppingFeaturesAlertProps}
noDataAlertProps={noDataAlertProps}
Expand Down
7 changes: 6 additions & 1 deletion packages/react-widgets/src/widgets/PieWidget.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,11 @@ function PieWidget({
useWidgetFilterValues({ dataSource, id, column, type: FilterTypes.IN }) ||
EMPTY_ARRAY;

const { data = [], isLoading } = useWidgetFetch(getCategories, {
const {
data = [],
isLoading,
warning
} = useWidgetFetch(getCategories, {
id,
dataSource,
params: {
Expand Down Expand Up @@ -102,6 +106,7 @@ function PieWidget({
<WrapperWidgetUI title={title} isLoading={isLoading} {...wrapperProps}>
<WidgetWithAlert
dataSource={dataSource}
warning={warning}
global={global}
droppingFeaturesAlertProps={droppingFeaturesAlertProps}
noDataAlertProps={noDataAlertProps}
Expand Down
7 changes: 6 additions & 1 deletion packages/react-widgets/src/widgets/ScatterPlotWidget.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,11 @@ function ScatterPlotWidget({
noDataAlertProps,
droppingFeaturesAlertProps
}) {
const { data = [], isLoading } = useWidgetFetch(getScatter, {
const {
data = [],
isLoading,
warning
} = useWidgetFetch(getScatter, {
id,
dataSource,
params: {
Expand All @@ -60,6 +64,7 @@ function ScatterPlotWidget({
<WrapperWidgetUI title={title} isLoading={isLoading} {...wrapperProps}>
<WidgetWithAlert
dataSource={dataSource}
warning={warning}
global={global}
droppingFeaturesAlertProps={droppingFeaturesAlertProps}
noDataAlertProps={noDataAlertProps}
Expand Down
4 changes: 3 additions & 1 deletion packages/react-widgets/src/widgets/TableWidget.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ function TableWidget({
data = { data: EMPTY_ARRAY, totalCount: 0 },
isLoading,
isSourceReady,
source
source,
warning
} = useWidgetFetch(getTable, {
id,
dataSource,
Expand Down Expand Up @@ -85,6 +86,7 @@ function TableWidget({
<WrapperWidgetUI title={title} {...wrapperProps} isLoading={isLoading}>
<WidgetWithAlert
dataSource={dataSource}
warning={warning}
global={global}
droppingFeaturesAlertProps={droppingFeaturesAlertProps}
noDataAlertProps={noDataAlertProps}
Expand Down
9 changes: 7 additions & 2 deletions packages/react-widgets/src/widgets/TimeSeriesWidget.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ const STEP_SIZE_RANGE_MAPPING = {

/**
* Renders a <TimeSeriesWidget /> component
* @param props
* @param {object} props
* @param {string} props.id - ID for the widget instance.
* @param {string} props.title - Title to show in the widget header.
* @param {string} props.dataSource - ID of the data source to get the data from.
Expand Down Expand Up @@ -119,7 +119,11 @@ function TimeSeriesWidget({
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [stepSize]);

const { data = [], isLoading } = useWidgetFetch(getTimeSeries, {
const {
data = [],
isLoading,
warning
} = useWidgetFetch(getTimeSeries, {
id,
dataSource,
params: {
Expand Down Expand Up @@ -231,6 +235,7 @@ function TimeSeriesWidget({
>
<WidgetWithAlert
dataSource={dataSource}
warning={warning}
global={global}
droppingFeaturesAlertProps={droppingFeaturesAlertProps}
noDataAlertProps={noDataAlertProps}
Expand Down
Loading