sadpandajoe commented on code in PR #44803:
URL: https://github.com/apache/superset/pull/44803#discussion_r4173860920
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx:
##########
@@ -1411,3 +1433,202 @@ 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 typeof state).dataMask[filterId]).toEqual(
+ expect.objectContaining({
+ filterState: expect.objectContaining({
+ value: 'Current month',
+ }),
+ extraFormData: {
+ time_range: 'Current month',
+ },
+ }),
+ );
+
+ expect(screen.getByText('Current month')).toBeInTheDocument();
+
+ await act(async () => {
+ store.dispatch(dataMaskActions.removeDataMask(filterId));
+ jest.advanceTimersByTime(300);
+ });
+
+ expect(screen.getByText('Current month')).toBeInTheDocument();
+
+ const clearAllButton = screen.getByText('Clear all');
+ await userEvent.click(clearAllButton);
+
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
+ });
+
+ expect(screen.getByTestId(getTestId('apply-button'))).toBeEnabled();
+
+ await userEvent.click(screen.getByTestId(getTestId('apply-button')));
+
+ expect(updateDataMaskSpy).toHaveBeenLastCalledWith(
+ filterId,
+ expect.objectContaining({
+ id: filterId,
+ filterState: {
+ value: undefined,
+ validateStatus: undefined,
+ },
+ extraFormData: {},
+ }),
+ );
+
+ updateDataMaskSpy.mockRestore();
+});
+
+test('FilterBar clears selected state when cross-filtering is disabled', async
() => {
Review Comment:
Running this reset test alone leaves `fetchTimeRange` as a bare mock, so
`DateFilterLabel` calls `.then` on `undefined` before reaching the reset
assertions; the whole-file run inherits the preceding test’s implementation.
Could this test initialize its own promise-returning mock?
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx:
##########
@@ -369,9 +373,13 @@ const FilterBar: FC<FiltersBarProps> = ({
useEffect(() => {
const dashboardChanged = dashboardId !== previousDashboardId;
+ const crossFiltersDisabled =
+ previousCrossFiltersEnabled === true && crossFiltersEnabled === false;
if (dashboardChanged) {
setDataMaskSelected(() => dataMaskApplied);
+ } else if (crossFiltersDisabled) {
+ setDataMaskSelected(() => dataMaskApplied);
Review Comment:
A failed attempt to enable cross-filtering rolls the setting back from true
to false without clearing the applied masks, so this replaces any pending
native-filter selection with the old applied value. Could this reset be tied to
an actual mask clear rather than every true → false transition?
##########
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 current head still fails frontend type-checking: `typeof state` triggers
TS2352 at line 1535, and the new reset test adds TS2339 for
`crossFiltersEnabled` and `dataMask` at lines 1610 and 1632. Could we type both
fixtures and state reads so the frontend type-check passes?
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx:
##########
@@ -391,12 +391,9 @@ const FilterBar: FC<FiltersBarProps> = ({
}
});
- // Remove stale entries that no longer exist in dataMaskApplied
+ // Remove stale entries that no longer exist in the configured filters
Object.keys(updated).forEach(filterId => {
Review Comment:
The toggle reset is now handled, but disabling cross-filtering, applying a
time range from chat, then clicking the toast’s Undo still leaves that range
displayed: Undo removes the previously absent mask without changing the
setting, and Apply restores it. Could explicit Undo removals reset the selected
state too?
--
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]
