sadpandajoe commented on code in PR #44803:
URL: https://github.com/apache/superset/pull/44803#discussion_r4161606719
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx:
##########
@@ -1411,3 +1432,147 @@ test('FilterBar with orientation=Vertical renders
Vertical layout (sanity counte
screen.queryByRole('img', { name: 'setting' }),
).not.toBeInTheDocument();
});
+
+test('FilterBar preserves a selected time range when its applied data mask is
removed', async () => {
+ setupTimeRangeMocks();
+
+ mockedFetchTimeRange.mockResolvedValue({
+ value: '2021-04-07T00:00:00 ≤ ds < 2021-04-14T00:00:00',
+ });
+
+ fetchMock.post('glob:*/api/v1/chart/data', {
+ result: [
+ {
+ data: [{ ds: '2021-04-14T00:00:00' }],
+ colnames: ['ds'],
+ coltypes: [2],
+ applied_filters: [],
+ },
+ ],
+ });
+
+ const filterId = 'NATIVE_FILTER-keep-time-range';
+ const updateDataMaskSpy = jest.spyOn(dataMaskActions, 'updateDataMask');
+
+ const filter = createFilter({
+ id: filterId,
+ name: 'Time range',
+ filterType: 'filter_time',
+ targets: [{ datasetId: 7, column: { name: 'ds' } }],
+ defaultDataMask: {
+ filterState: { value: 'Last week' },
+ extraFormData: { time_range: 'Last week' },
+ },
+ chartsInScope: [18],
+ });
+
+ const state = createStateWithFilter(
+ filter,
+ createDataMask(filterId, 'Last week', {
+ time_range: 'Last week',
+ }),
+ {
+ filterBarOrientation: FilterBarOrientation.Horizontal,
+ metadata: {
+ native_filter_configuration: [filter],
+ chart_configuration: {},
+ },
+ },
+ );
+
+ const store = createStore(state, reducerIndex);
+
+ render(<FilterBar orientation={FilterBarOrientation.Horizontal} />, {
+ store,
+ useDnd: true,
+ useRouter: true,
+ });
+
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
+ });
+
+ await waitFor(() => {
+ expect(screen.queryByTestId('loading-indicator')).not.toBeInTheDocument();
+ });
+
+ expect(screen.getByText('Last week')).toBeInTheDocument();
+
+ await userEvent.click(screen.getByText('Last week'));
+
+ const rangeType = screen.getByLabelText('Range type');
+ await userEvent.click(rangeType);
+ await userEvent.click(screen.getByText('Current'));
+
+ await userEvent.click(screen.getByRole('radio', { name: 'Current month' }));
+
+ const timeFilterApply = screen.getByTestId(
+ 'date-filter-control__apply-button',
+ );
+ expect(timeFilterApply).not.toBeNull();
+ expect(timeFilterApply).not.toBeDisabled();
+
+ await userEvent.click(timeFilterApply!);
+
+ const filterBarApply = screen.getByTestId(getTestId('apply-button'));
+ expect(filterBarApply).toBeEnabled();
+
+ await userEvent.click(filterBarApply);
+
+ expect(updateDataMaskSpy).toHaveBeenCalledWith(
+ filterId,
+ expect.objectContaining({
+ filterState: expect.objectContaining({
+ value: 'Current month',
+ }),
+ extraFormData: {
+ time_range: 'Current month',
+ },
+ }),
+ );
+
+ expect((store.getState() as RootState).dataMask[filterId]).toEqual(
Review Comment:
The new `RootState` assertion fails TypeScript checking (TS2352):
`createStore` accepts its reducers as `object`, so its inferred state exposes
only `queryApi`, not the dashboard fields. Could we type the test store/state
assertion compatibly so the frontend type-check passes?
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx:
##########
@@ -1411,3 +1413,73 @@ test('FilterBar with orientation=Vertical renders
Vertical layout (sanity counte
screen.queryByRole('img', { name: 'setting' }),
).not.toBeInTheDocument();
});
+
+test('FilterBar keeps a configured filter selected when its applied data mask
is removed', async () => {
+ fetchMock.post(
+ 'glob:*/api/v1/chart/data',
+ {
+ result: [
+ {
+ data: [{ region: 'East' }, { region: 'West' }],
+ colnames: ['region'],
+ coltypes: [1],
+ applied_filters: [],
+ },
+ ],
+ },
+ { name: 'configured-filter-selected-chart-data' },
+ );
+
+ const filterId = 'NATIVE_FILTER-keep-selected';
+ const filter = createFilter({
+ id: filterId,
+ name: 'Region',
+ filterType: 'filter_select',
+ targets: [{ datasetId: 7, column: { name: 'region' } }],
+ chartsInScope: [18],
+ });
+
+ const state = createStateWithFilter(
+ filter,
+ createDataMask(filterId, ['East'], {
+ filters: [{ col: 'region', op: 'IN', val: ['East'] }],
+ }),
+ {
+ filterBarOrientation: FilterBarOrientation.Vertical,
+ metadata: {
+ native_filter_configuration: [filter],
+ chart_configuration: {},
+ },
+ },
+ );
+
+ const store = createStore(state, reducerIndex);
+
+ render(
+ <FilterBar
+ orientation={FilterBarOrientation.Vertical}
+ verticalConfig={{
+ width: 280,
+ height: 400,
+ offset: 0,
+ ...createOpenedBarProps(),
+ }}
+ />,
+ { store, useDnd: true, useRouter: true },
+ );
+
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
+ });
+
+ expect(screen.getByText('Region')).toBeInTheDocument();
+ expect(screen.getByTitle('East')).toBeInTheDocument();
+
+ await act(async () => {
Review Comment:
The fail-on-base result can still come solely from the explicit
`removeDataMask` step: the popup flow starts with an already-applied Last week
range, whereas #44680 starts with an empty applied entry and loses the label
while that entry remains present. Could we also cover that empty-entry →
Current month → Apply → retained label and Clear All sequence without directly
deleting the applied mask, and verify that sequence fails on base?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
