Skip to content

Commit

Permalink
feat(native-filters): Hide time filters if loaded datasets don't have…
Browse files Browse the repository at this point in the history 
… temporal columns (#15225)

* feat(native-filters): Hide time filters if loaded datasets don't have temporal columns

* Remove "px" suffixes to fix warnings

* Disable an option instead of hiding filter types

* Fix tests

* Add 2 more tests
  • Loading branch information
@kgabryje
kgabryje committed Jun 17, 2021
1 parent ea8507b commit c7c6375
Show file tree
Hide file tree
Showing 4 changed files with 119 additions and 43 deletions.
1 change: 1 addition & 0 deletions superset-frontend/spec/fixtures/mockDatasource.js
View file
Expand Up @@ -164,6 +164,7 @@ export default {
column_name: 'num_girls',
},
],
column_types: [0, 1, 2],
id,
granularity_sqla: [['ds', 'ds']],
name: 'birth_names',
Expand Down
110 changes: 74 additions & 36 deletions ...board/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
View file
Expand Up @@ -26,6 +26,7 @@ import {
styled,
SupersetApiError,
t,
GenericDataType,
} from '@superset-ui/core';
import {
ColumnMeta,
Expand Down Expand Up @@ -102,7 +103,7 @@ export const StyledFormItem = styled(FormItem)`
margin-bottom: ${({ theme }) => theme.gridUnit * 4}px;
& .ant-form-item-label {
padding-bottom: 0px;
padding-bottom: 0;
}
& .ant-form-item-control-input {
Expand All @@ -111,12 +112,12 @@ export const StyledFormItem = styled(FormItem)`
`;

export const StyledRowFormItem = styled(FormItem)`
margin-bottom: 0px;
padding-bottom: 0px;
margin-bottom: 0;
padding-bottom: 0;
min-width: 50%;
& .ant-form-item-label {
padding-bottom: 0px;
padding-bottom: 0;
}
.ant-form-item-control-input-content > div > div {
Expand Down Expand Up @@ -154,26 +155,26 @@ const StyledCollapse = styled(Collapse)`
margin-right: ${({ theme }) => theme.gridUnit * -4}px;
border-left: 1px solid ${({ theme }) => theme.colors.grayscale.light2};
border-top: 1px solid ${({ theme }) => theme.colors.grayscale.light2};
border-radius: 0px;
border-radius: 0;
.ant-collapse-header {
border-bottom: 1px solid ${({ theme }) => theme.colors.grayscale.light2};
border-top: 1px solid ${({ theme }) => theme.colors.grayscale.light2};
margin-top: -1px;
border-radius: 0px;
border-radius: 0;
}
.ant-collapse-content {
border: 0px;
border: 0;
}
.ant-collapse-content-box {
padding-top: ${({ theme }) => theme.gridUnit * 2}px;
}
&.ant-collapse > .ant-collapse-item {
border: 0px;
border-radius: 0px;
border: 0;
border-radius: 0;
}
`;

Expand All @@ -182,17 +183,17 @@ const StyledTabs = styled(Tabs)`
position: sticky;
margin-left: ${({ theme }) => theme.gridUnit * -4}px;
margin-right: ${({ theme }) => theme.gridUnit * -4}px;
top: 0px;
top: 0;
background: white;
z-index: 1;
}
.ant-tabs-nav-list {
padding: 0px;
padding: 0;
}
.ant-form-item-label {
padding-bottom: 0px;
padding-bottom: 0;
}
`;

Expand Down Expand Up @@ -245,6 +246,8 @@ const FILTERS_WITHOUT_COLUMN = [

const FILTERS_WITH_ADHOC_FILTERS = ['filter_select', 'filter_range'];

const TIME_FILTERS = ['filter_time', 'filter_timegrain', 'filter_timecolumn'];

const BASIC_CONTROL_ITEMS = ['enableEmptyFilter', 'multiSelect'];

// TODO: Rename the filter plugins and remove this mapping
Expand All @@ -257,6 +260,11 @@ const FILTER_TYPE_NAME_MAPPING = {
[t('Group By')]: t('Group by'),
};

// TODO: add column_types field to DatasourceMeta
const hasTemporalColumns = (
dataset: DatasourceMeta & { column_types: GenericDataType[] },
) => dataset?.column_types?.includes(GenericDataType.TEMPORAL);

/**
* The configuration form for a specific filter.
* Assigns field values to `filters[filterId]` in the form.
Expand All @@ -283,7 +291,7 @@ const FiltersConfigForm = (

const forceUpdate = useForceUpdate();
const [datasetDetails, setDatasetDetails] = useState<Record<string, any>>();
const defaultFormFilter = useMemo(() => {}, []);
const defaultFormFilter = useMemo(() => ({}), []);
const formFilter =
form.getFieldValue('filters')?.[filterId] || defaultFormFilter;

Expand All @@ -299,6 +307,22 @@ const FiltersConfigForm = (
({ datasources }) => datasources,
);

const doLoadedDatasetsHaveTemporalColumns = useMemo(
() =>
Object.values(loadedDatasets).some(dataset =>
hasTemporalColumns(dataset),
),
[loadedDatasets],
);

const showTimeRangePicker = useMemo(() => {
const currentDataset = Object.values(loadedDatasets).find(
dataset => dataset.id === formFilter.dataset?.value,
);

return currentDataset ? hasTemporalColumns(currentDataset) : true;
}, [formFilter.dataset?.value, loadedDatasets]);

// @ts-ignore
const hasDataset = !!nativeFilterItems[formFilter?.filterType]?.value
?.datasourceCount;
Expand Down Expand Up @@ -576,9 +600,21 @@ const FiltersConfigForm = (
const mappedName = name
? FILTER_TYPE_NAME_MAPPING[name]
: undefined;
const isDisabled =
TIME_FILTERS.includes(filterType) &&
!doLoadedDatasetsHaveTemporalColumns;
return {
value: filterType,
label: mappedName || name,
label: isDisabled ? (
<Tooltip
title={t('Datasets do not contain a temporal column')}
>
{mappedName || name}
</Tooltip>
) : (
mappedName || name
),
isDisabled,
};
})}
onChange={({ value }: { value: string }) => {
Expand Down Expand Up @@ -845,28 +881,30 @@ const FiltersConfigForm = (
}
/>
</StyledRowFormItem>
<StyledRowFormItem
name={['filters', filterId, 'time_range']}
label={<StyledLabel>{t('Time range')}</StyledLabel>}
initialValue={filterToEdit?.time_range || 'No filter'}
required={!hasAdhoc}
rules={[
{
validator: preFilterValidator,
},
]}
>
<DateFilterControl
name="time_range"
onChange={timeRange => {
setNativeFilterFieldValues(form, filterId, {
time_range: timeRange,
});
forceUpdate();
validatePreFilter();
}}
/>
</StyledRowFormItem>
{showTimeRangePicker && (
<StyledRowFormItem
name={['filters', filterId, 'time_range']}
label={<StyledLabel>{t('Time range')}</StyledLabel>}
initialValue={filterToEdit?.time_range || 'No filter'}
required={!hasAdhoc}
rules={[
{
validator: preFilterValidator,
},
]}
>
<DateFilterControl
name="time_range"
onChange={timeRange => {
setNativeFilterFieldValues(form, filterId, {
time_range: timeRange,
});
forceUpdate();
validatePreFilter();
}}
/>
</StyledRowFormItem>
)}
{hasTimeRange && (
<StyledRowFormItem
name={['filters', filterId, 'granularity_sqla']}
Expand Down
6 changes: 4 additions & 2 deletions ...tend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts
View file
Expand Up @@ -19,7 +19,7 @@
import { flatMapDeep } from 'lodash';
import { FormInstance } from 'antd/lib/form';
import React from 'react';
import { CustomControlItem } from '@superset-ui/chart-controls';
import { CustomControlItem, DatasourceMeta } from '@superset-ui/chart-controls';

const FILTERS_FIELD_NAME = 'filters';

Expand Down Expand Up @@ -64,7 +64,9 @@ type DatasetSelectValue = {
label: string;
};

export const datasetToSelectOption = (item: any): DatasetSelectValue => ({
export const datasetToSelectOption = (
item: DatasourceMeta & { table_name: string },
): DatasetSelectValue => ({
value: item.id,
label: item.table_name,
});
45 changes: 40 additions & 5 deletions ...end/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx
View file
Expand Up @@ -16,7 +16,10 @@
* specific language governing permissions and limitations
* under the License.
*/
import React from 'react';
import { Preset } from '@superset-ui/core';
import fetchMock from 'fetch-mock';
import userEvent, { specialChars } from '@testing-library/user-event';
import {
SelectFilterPlugin,
RangeFilterPlugin,
Expand All @@ -25,9 +28,7 @@ import {
TimeGrainFilterPlugin,
} from 'src/filters/components';
import { render, screen, waitFor } from 'spec/helpers/testing-library';
import fetchMock from 'fetch-mock';
import React from 'react';
import userEvent, { specialChars } from '@testing-library/user-event';
import mockDatasource, { datasourceId } from 'spec/fixtures/mockDatasource';
import {
FiltersConfigModal,
FiltersConfigModalProps,
Expand All @@ -49,7 +50,18 @@ class MainPreset extends Preset {
}

const initialStoreState = {
datasources: [{ id: 1, table_name: 'Datasource 1' }],
datasources: mockDatasource,
};

const storeWithDatasourceWithoutTemporalColumns = {
...initialStoreState,
datasources: {
...initialStoreState.datasources,
[datasourceId]: {
...initialStoreState.datasources[datasourceId],
column_types: [0, 1],
},
},
};

fetchMock.get('glob:*/api/v1/dataset/1', {
Expand Down Expand Up @@ -114,6 +126,7 @@ const DEFAULT_VALUE_REQUIRED_REGEX = /^default value is required$/i;
const PARENT_REQUIRED_REGEX = /^parent filter is required$/i;
const PRE_FILTER_REQUIRED_REGEX = /^pre-filter is required$/i;
const FILL_REQUIRED_FIELDS_REGEX = /fill all required fields to enable/;
const TIME_RANGE_PREFILTER_REGEX = /^time range$/i;

const props: FiltersConfigModalProps = {
isOpen: true,
Expand All @@ -128,7 +141,7 @@ beforeAll(() => {

function defaultRender(
overrides?: Partial<FiltersConfigModalProps>,
initialState?: {},
initialState = initialStoreState,
) {
return render(<FiltersConfigModal {...props} {...overrides} />, {
useRedux: true,
Expand Down Expand Up @@ -243,6 +256,20 @@ test('renders a time grain filter type', () => {
expect(screen.queryByText(ADVANCED_REGEX)).not.toBeInTheDocument();
});

test('render time filter types as disabled if there are no temporal columns in the dataset', () => {
defaultRender(undefined, storeWithDatasourceWithoutTemporalColumns);
userEvent.click(screen.getByText(VALUE_REGEX));
expect(screen.getByText(TIME_RANGE_REGEX).closest('div')).toHaveClass(
'Select__option--is-disabled',
);
expect(screen.getByText(TIME_GRAIN_REGEX).closest('div')).toHaveClass(
'Select__option--is-disabled',
);
expect(screen.getByText(TIME_COLUMN_REGEX).closest('div')).toHaveClass(
'Select__option--is-disabled',
);
});

test('validates the name', async () => {
defaultRender();
userEvent.click(screen.getByRole('button', { name: SAVE_REGEX }));
Expand Down Expand Up @@ -287,6 +314,14 @@ test('validates the pre-filter value', async () => {
).toBeInTheDocument();
});

test("doesn't render time range pre-filter if there are no temporal columns in datasource", async () => {
defaultRender(undefined, storeWithDatasourceWithoutTemporalColumns);
userEvent.click(screen.getByText(ADVANCED_REGEX));
userEvent.click(getCheckbox(PRE_FILTER_REGEX));
expect(
screen.queryByText(TIME_RANGE_PREFILTER_REGEX),
).not.toBeInTheDocument();
});
/*
TODO
adds a new value filter type with all fields filled
Expand Down

0 comments on commit c7c6375

Please sign in to comment.