fix(charts): Time range filters are not being applied to charts that were overwritten (#23589)

superset-frontend/src/explore/actions/saveModalActions.js

@@ -19,6 +19,7 @@
 import rison from 'rison';
 import { SupersetClient, t } from '@superset-ui/core';
 import { addSuccessToast } from 'src/components/MessageToasts/actions';
+import { isEmpty } from 'lodash';
 import { buildV1ChartDataPayload } from '../exploreUtils';
 
 const ADHOC_FILTER_REGEX = /^adhoc_filters/;
@@ -76,19 +77,35 @@ export function removeSaveModalAlert() {
   return { type: REMOVE_SAVE_MODAL_ALERT };
 }
 
-export const getSlicePayload = (
-  sliceName,
-  formDataWithNativeFilters,
-  dashboards,
-  owners,
-) => {
-  const adhocFilters = Object.entries(formDataWithNativeFilters).reduce(
+const extractAddHocFiltersFromFormData = formDataToHandle =>
+  Object.entries(formDataToHandle).reduce(
     (acc, [key, value]) =>
       ADHOC_FILTER_REGEX.test(key)
         ? { ...acc, [key]: value?.filter(f => !f.isExtra) }
         : acc,
     {},
   );
+
+export const getSlicePayload = (
+  sliceName,
+  formDataWithNativeFilters,
+  dashboards,
+  owners,
+  formDataFromSlice = {},
+) => {
+  let adhocFilters = extractAddHocFiltersFromFormData(
+    formDataWithNativeFilters,
+  );
+
+  // Retain adhoc_filters from the slice if no adhoc_filters are present
+  // after overwriting a chart.  This ensures the dashboard can continue
+  // to filter the chart. Before, any time range filter applied in the dashboard
+  // would end up as an extra filter and when overwriting the chart the original
+  // time range adhoc_filter was lost
+  if (isEmpty(adhocFilters?.adhoc_filters) && !isEmpty(formDataFromSlice)) {
+    adhocFilters = extractAddHocFiltersFromFormData(formDataFromSlice);
+  }
+
   const formData = {
     ...formDataWithNativeFilters,
     ...adhocFilters,
@@ -157,8 +174,9 @@ const addToasts = (isNewSlice, sliceName, addedToDashboard) => {
 
 //  Update existing slice
 export const updateSlice =
-  ({ slice_id: sliceId, owners }, sliceName, dashboards, addedToDashboard) =>
+  (slice, sliceName, dashboards, addedToDashboard) =>
   async (dispatch, getState) => {
+    const { slice_id: sliceId, owners, form_data: formDataFromSlice } = slice;
     const {
       explore: {
         form_data: { url_params: _, ...formData },
@@ -167,7 +185,13 @@ export const updateSlice =
     try {
       const response = await SupersetClient.put({
         endpoint: `/api/v1/chart/${sliceId}`,
-        jsonPayload: getSlicePayload(sliceName, formData, dashboards, owners),
+        jsonPayload: getSlicePayload(
+          sliceName,
+          formData,
+          dashboards,
+          owners,
+          formDataFromSlice,
+        ),
       });
 
       dispatch(saveSliceSuccess());

superset-frontend/src/explore/actions/saveModalActions.test.js

@@ -31,6 +31,7 @@ import {
   SAVE_SLICE_FAILED,
   SAVE_SLICE_SUCCESS,
   updateSlice,
+  getSlicePayload,
 } from './saveModalActions';
 
 /**
@@ -339,3 +340,124 @@ test('getSliceDashboards with slice handles failure', async () => {
   expect(dispatch.callCount).toBe(1);
   expect(dispatch.getCall(0).args[0].type).toBe(SAVE_SLICE_FAILED);
 });
+
+describe('getSlicePayload', () => {
+  const sliceName = 'Test Slice';
+  const formDataWithNativeFilters = {
+    datasource: '22__table',
+    viz_type: 'pie',
+    adhoc_filters: [],
+  };
+  const dashboards = [5];
+  const owners = [1];
+  const formDataFromSlice = {
+    datasource: '22__table',
+    viz_type: 'pie',
+    adhoc_filters: [
+      {
+        clause: 'WHERE',
+        subject: 'year',
+        operator: 'TEMPORAL_RANGE',
+        comparator: 'No filter',
+        expressionType: 'SIMPLE',
+      },
+    ],
+  };
+
+  test('should return the correct payload when no adhoc_filters are present in formDataWithNativeFilters', () => {
+    const result = getSlicePayload(
+      sliceName,
+      formDataWithNativeFilters,
+      dashboards,
+      owners,
+      formDataFromSlice,
+    );
+    expect(result).toHaveProperty('params');
+    expect(result).toHaveProperty('slice_name', sliceName);
+    expect(result).toHaveProperty(
+      'viz_type',
+      formDataWithNativeFilters.viz_type,
+    );
+    expect(result).toHaveProperty('datasource_id', 22);
+    expect(result).toHaveProperty('datasource_type', 'table');
+    expect(result).toHaveProperty('dashboards', dashboards);
+    expect(result).toHaveProperty('owners', owners);
+    expect(result).toHaveProperty('query_context');
+    expect(JSON.parse(result.params).adhoc_filters).toEqual(
+      formDataFromSlice.adhoc_filters,
+    );
+  });
+
+  test('should return the correct payload when adhoc_filters are present in formDataWithNativeFilters', () => {
+    const formDataWithAdhocFilters = {
+      ...formDataWithNativeFilters,
+      adhoc_filters: [
+        {
+          clause: 'WHERE',
+          subject: 'year',
+          operator: 'TEMPORAL_RANGE',
+          comparator: 'No filter',
+          expressionType: 'SIMPLE',
+        },
+      ],
+    };
+    const result = getSlicePayload(
+      sliceName,
+      formDataWithAdhocFilters,
+      dashboards,
+      owners,
+      formDataFromSlice,
+    );
+    expect(result).toHaveProperty('params');
+    expect(result).toHaveProperty('slice_name', sliceName);
+    expect(result).toHaveProperty(
+      'viz_type',
+      formDataWithAdhocFilters.viz_type,
+    );
+    expect(result).toHaveProperty('datasource_id', 22);
+    expect(result).toHaveProperty('datasource_type', 'table');
+    expect(result).toHaveProperty('dashboards', dashboards);
+    expect(result).toHaveProperty('owners', owners);
+    expect(result).toHaveProperty('query_context');
+    expect(JSON.parse(result.params).adhoc_filters).toEqual(
+      formDataWithAdhocFilters.adhoc_filters,
+    );
+  });
+
+  test('should return the correct payload when formDataWithNativeFilters has a filter with isExtra set to true', () => {
+    const formDataWithAdhocFiltersWithExtra = {
+      ...formDataWithNativeFilters,
+      adhoc_filters: [
+        {
+          clause: 'WHERE',
+          subject: 'year',
+          operator: 'TEMPORAL_RANGE',
+          comparator: 'No filter',
+          expressionType: 'SIMPLE',
+          isExtra: true,
+        },
+      ],
+    };
+    const result = getSlicePayload(
+      sliceName,
+      formDataWithAdhocFiltersWithExtra,
+      dashboards,
+      owners,
+      formDataFromSlice,
+    );
+    expect(result).toHaveProperty('params');
+    expect(result).toHaveProperty('slice_name', sliceName);
+    expect(result).toHaveProperty(
+      'viz_type',
+      formDataWithAdhocFiltersWithExtra.viz_type,
+    );
+    expect(result).toHaveProperty('datasource_id', 22);
+    expect(result).toHaveProperty('datasource_type', 'table');
+    expect(result).toHaveProperty('dashboards', dashboards);
+    expect(result).toHaveProperty('owners', owners);
+    expect(result).toHaveProperty('query_context');
+    expect(JSON.parse(result.params).adhoc_filters).toEqual(
+      formDataFromSlice.adhoc_filters,
+    );
+  });
+});