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

FormulaWidget: implement new custom operation mechanism #699

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,13 @@

## Not released

- FormulaWidget custom aggregation expression support and fixes [#699](https://github.com/CartoDB/carto-react/pull/699)
- custom aggregation expression e.g `operation=custom / operationExp = SUM(revenue) * 100`
- new onStateChange callback
- display empty/error state as '-'
- useWidgetFetch to ignore outdated results
- Fix: Long values cause text-overflow

## 2.1

### 2.1.2 (2023-06-26)
Expand All @@ -12,7 +19,7 @@
### 2.1.1 (2023-06-22)

- Fix spatial filter was not being applied to Timeseries widgets [#719](https://github.com/CartoDB/carto-react/pull/719)
- Bugfix: The pagination is out of alignment [#711](https://github.com/CartoDB/carto-react/pull/711).
- Bugfix: The pagination is out of alignment [#711](https://github.com/CartoDB/carto-react/pull/711).

### 2.1.0 (2023-06-16)

Expand Down
2 changes: 1 addition & 1 deletion lerna.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@
],
"npmClient": "yarn",
"useWorkspaces": true,
"version": "2.1.2"
"version": "2.1.3-alpha-custom-agg-formula.0"
}
18 changes: 13 additions & 5 deletions packages/react-api/__tests__/api/model.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,20 +35,28 @@ describe('model', () => {
});
});

test('works correctly with a query source', () => {
executeModel({ model: 'formula', source: QUERY_SOURCE, params: DEFAULT_PARAMS });
test('works correctly with a query source that fits GET url length', () => {
const longButStillFittingGetSource = {
...QUERY_SOURCE,
data: 'a'.repeat(1819)
};
executeModel({
model: 'formula',
source: longButStillFittingGetSource,
params: DEFAULT_PARAMS
});

expect(mockedMakeCall).toHaveBeenCalledWith({
credentials: TABLE_SOURCE.credentials,
opts: { method: 'GET' },
url: 'https://gcp-us-east1.api.carto.com/v3/sql/carto-ps-bq-developers/model/formula?type=query&source=SELECT+*+FROM+%60cartobq.public_account.seattle_collisions%60&params=%7B%22column%22%3A%22__test__%22%2C%22operation%22%3A%22avg%22%7D&queryParameters=&filters=%7B%7D&filtersLogicalOperator=AND'
url: `https://gcp-us-east1.api.carto.com/v3/sql/carto-ps-bq-developers/model/formula?type=query&source=${longButStillFittingGetSource.data}&params=%7B%22column%22%3A%22__test__%22%2C%22operation%22%3A%22avg%22%7D&queryParameters=&filters=%7B%7D&filtersLogicalOperator=AND`
});
});

test('works correctly when the source is very large', () => {
test('uses POST for when URL length exceeds limit', () => {
const longQuerySource = {
...QUERY_SOURCE,
data: 'a'.repeat(2048)
data: 'a'.repeat(1820)
};

executeModel({ model: 'formula', source: longQuerySource, params: DEFAULT_PARAMS });
Expand Down
2 changes: 1 addition & 1 deletion packages/react-api/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@carto/react-api",
"version": "2.1.2",
"version": "2.1.3-alpha-custom-agg-formula.0",
"description": "CARTO for React - Api",
"author": "CARTO Dev Team",
"keywords": [
Expand Down
4 changes: 2 additions & 2 deletions packages/react-api/src/api/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { checkCredentials, makeCall } from './common';
import { MAP_TYPES, API_VERSIONS } from '@deck.gl/carto/typed';
import { _assert as assert } from '@carto/react-core/';

const URL_LENGTH = 2048;
import { REQUEST_GET_MAX_URL_LENGTH } from '@carto/react-core';

const AVAILABLE_MODELS = [
'category',
Expand Down Expand Up @@ -81,7 +81,7 @@ export function executeModel(props) {
}

const urlWithSearchParams = url + '?' + new URLSearchParams(queryParams).toString();
const isGet = urlWithSearchParams.length <= URL_LENGTH;
const isGet = urlWithSearchParams.length <= REQUEST_GET_MAX_URL_LENGTH;
if (isGet) {
url = urlWithSearchParams;
} else {
Expand Down
2 changes: 1 addition & 1 deletion packages/react-auth/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@carto/react-auth",
"version": "2.1.2",
"version": "2.1.3-alpha-custom-agg-formula.0",
"description": "CARTO for React - Auth",
"author": "CARTO Dev Team",
"keywords": [
Expand Down
2 changes: 1 addition & 1 deletion packages/react-basemaps/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@carto/react-basemaps",
"version": "2.1.2",
"version": "2.1.3-alpha-custom-agg-formula.0",
"description": "CARTO for React - Basemaps",
"keywords": [
"carto",
Expand Down
2 changes: 1 addition & 1 deletion packages/react-core/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@carto/react-core",
"version": "2.1.2",
"version": "2.1.3-alpha-custom-agg-formula.0",
"description": "CARTO for React - Core",
"author": "CARTO Dev Team",
"keywords": [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,6 @@ export enum AggregationTypes {
AVG = 'avg',
MIN = 'min',
MAX = 'max',
SUM = 'sum'
SUM = 'sum',
CUSTOM = 'custom'
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,8 @@ export const AggregationTypes = Object.freeze({
MAX: 'max',

/** Sum */
SUM: 'sum'
SUM: 'sum',

/** Custom aggregation expression */
CUSTOM: 'custom'
});
2 changes: 1 addition & 1 deletion packages/react-redux/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@carto/react-redux",
"version": "2.1.2",
"version": "2.1.3-alpha-custom-agg-formula.0",
"description": "CARTO for React - Redux",
"author": "CARTO Dev Team",
"keywords": [
Expand Down
10 changes: 10 additions & 0 deletions packages/react-ui/__tests__/widgets/FormulaWidgetUI.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,16 @@ describe('FormulaWidgetUI', () => {
expect(await screen.findByText(0)).toBeInTheDocument();
});

test('should render undefined as -', async () => {
render(<FormulaWidgetUI data={undefined} />);
expect(await screen.findByText('-')).toBeInTheDocument();
});

test('should render null as -', async () => {
render(<FormulaWidgetUI data={null} />);
expect(await screen.findByText('-')).toBeInTheDocument();
});

test('with currency formatter', () => {
const DATA = '1234';
render(<FormulaWidgetUI data={DATA} formatter={currencyFormatter} />);
Expand Down
2 changes: 1 addition & 1 deletion packages/react-ui/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@carto/react-ui",
"version": "2.1.2",
"version": "2.1.3-alpha-custom-agg-formula.0",
"description": "CARTO for React - UI",
"author": "CARTO Dev Team",
"keywords": [
Expand Down
13 changes: 11 additions & 2 deletions packages/react-ui/src/widgets/FormulaWidgetUI/FormulaWidgetUI.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ function FormulaWidgetUI(props) {
requestRef
});
} else {
setValue(data);
setValue(data !== null && data !== undefined ? data : '-');
}
// eslint-disable-next-line react-hooks/exhaustive-deps
return () => cancelAnimationFrame(requestRef.current);
Expand All @@ -63,6 +63,7 @@ function FormulaWidgetUI(props) {
const formattedValue = formatter(value);

const isComplexFormat = typeof formattedValue === 'object' && formattedValue !== null;
const isDisabled = formattedValue === '-';

if (isLoading) return <FormulaSkeleton />;

Expand All @@ -73,7 +74,15 @@ function FormulaWidgetUI(props) {
<Suffix>{formattedValue.suffix}</Suffix>
</Typography>
) : (
<Typography variant='h5' component='div' weight='medium'>
<Typography
variant='h5'
component='div'
weight='medium'
color={isDisabled ? 'text.disabled' : 'default'}
whiteSpace='nowrap'
textOverflow='ellipsis'
overflow='hidden'
>
{formattedValue}
</Typography>
);
Expand Down
75 changes: 70 additions & 5 deletions packages/react-widgets/__tests__/hooks/useWidgetFetch.test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
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 { act, render, screen, waitFor } from '@testing-library/react';
import React from 'react';
import useWidgetFetch, {
selectGeometryToIntersect
Expand Down Expand Up @@ -69,6 +69,7 @@ describe('useWidgetFetch', () => {

it('should work correctly (no remote attempt)', async () => {
const onError = jest.fn();
const onStateChange = jest.fn();
const modelFn = jest
.fn()
.mockImplementation(
Expand All @@ -84,7 +85,8 @@ describe('useWidgetFetch', () => {
params: PARAMS_MOCK,
global: false,
attemptRemoteCalculation: false,
onError
onError,
onStateChange
}}
/>
);
Expand All @@ -99,11 +101,16 @@ describe('useWidgetFetch', () => {
});

expect(screen.getByText('loading')).toBeInTheDocument();
await waitFor(() => expect(onStateChange).toBeCalledWith({ state: 'loading' }));

await act(() => sleep(250));

expect(screen.getByText('data')).toBeInTheDocument();

expect(onStateChange).toBeCalledWith({ state: 'success', data: 'data' });

onStateChange.mockReset();

modelFn.mockImplementation(
() => new Promise((resolve, reject) => setTimeout(() => reject('ERROR'), 100))
);
Expand All @@ -117,7 +124,8 @@ describe('useWidgetFetch', () => {
params: PARAMS_MOCK,
global: true,
attemptRemoteCalculation: false,
onError
onError,
onStateChange
}}
/>
);
Expand All @@ -131,11 +139,14 @@ describe('useWidgetFetch', () => {
spatialFilter: null // never in global mode
});

expect(screen.getByText('loading')).toBeInTheDocument();
expect(onStateChange).toBeCalledWith({ state: 'loading' });

await waitFor(() => expect(onStateChange).toBeCalledWith({ state: 'loading' }));
await act(() => sleep(250));
expect(screen.queryByText('loading')).not.toBeInTheDocument();

expect(onError).toBeCalledTimes(1);
expect(onStateChange).toBeCalledWith({ state: 'error', error: 'ERROR' });

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

Expand All @@ -162,6 +173,7 @@ describe('useWidgetFetch', () => {

it('should work correctly (non-global, remote attempt)', async () => {
const onError = jest.fn();
const onStateChange = jest.fn();
const modelFn = jest
.fn()
.mockImplementation(
Expand All @@ -177,7 +189,8 @@ describe('useWidgetFetch', () => {
params: PARAMS_MOCK,
global: false,
attemptRemoteCalculation: true,
onError
onError,
onStateChange
}}
/>
);
Expand All @@ -190,6 +203,10 @@ describe('useWidgetFetch', () => {
remoteCalculation: true,
spatialFilter: viewportSpatialFilter
});
await waitFor(() => expect(onStateChange).toBeCalledWith({ state: 'loading' }));
await waitFor(() =>
expect(onStateChange).toBeCalledWith({ state: 'success', data: 'data' })
);
});

it('should work correctly (global, remote attempt)', async () => {
Expand Down Expand Up @@ -223,6 +240,54 @@ describe('useWidgetFetch', () => {
spatialFilter: null // no spatial filter for glboal case
});
});

it('should ignore results of outdated requests', async () => {
const onStateChange = jest.fn();

let modelCallCount = 0;
const modelFn = jest.fn().mockImplementation(async () => {
modelCallCount++;
if (modelCallCount === 1) {
await sleep(100);
return 'firstSlowOutdated';
}
if (modelCallCount === 2) {
await sleep(50);
return 'secondFast';
}
});

const defaultArgs = {
id: 'test',
dataSource: 'test',
params: PARAMS_MOCK,
global: false,
attemptRemoteCalculation: true,
onStateChange
};
const { rerender } = render(<TestComponent modelFn={modelFn} args={defaultArgs} />);

await act(() => sleep(10));

rerender(
<TestComponent
modelFn={modelFn}
args={{ ...defaultArgs, params: { ...defaultArgs.params, x: 'xxx' } }}
/>
);

await act(() => sleep(200));

// Test modelFn is called with the right params
expect(modelFn).toBeCalledTimes(2);
// expect(onStateChange).toBeCalledTimes(2);
expect(onStateChange).toBeCalledWith({ state: 'loading' });
expect(onStateChange).not.toBeCalledWith({
state: 'success',
data: 'firstSlowOutdated'
});
expect(onStateChange).toBeCalledWith({ state: 'success', data: 'secondFast' });
});
});

// Aux
Expand Down
3 changes: 2 additions & 1 deletion packages/react-widgets/__tests__/models/FormulaModel.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ describe('getFormula', () => {
connection: '__test_connection__'
},
operation: AggregationTypes.SUM,
operationExp: 'abc',
column: 'column_1',
global: true
};
Expand All @@ -81,7 +82,7 @@ describe('getFormula', () => {
expect(mockedExecuteModel).toHaveBeenCalledWith({
model: 'formula',
opts: { abortController: undefined },
params: { column: 'column_1', operation: 'sum' },
params: { column: 'column_1', operation: 'sum', operationExp: 'abc' },
source: {
connection: '__test_connection__',
credentials: { accessToken: '__test_token__', apiVersion: 'v3' },
Expand Down
2 changes: 1 addition & 1 deletion packages/react-widgets/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@carto/react-widgets",
"version": "2.1.2",
"version": "2.1.3-alpha-custom-agg-formula.0",
"description": "CARTO for React - Widgets",
"author": "CARTO Dev Team",
"keywords": [
Expand Down
Loading