Re: [PR] feat(dashboard): pre-filter time grain [superset]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #38922:
URL: https://github.com/apache/superset/pull/38922#discussion_r3039268813


##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/transformers/timeGrains.test.ts:
##########
@@ -0,0 +1,86 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { NativeFilterType, NativeFilterScope } from '@superset-ui/core';
+import { getInitialDataMask } from 'src/dataMask/reducer';
+
+const mockScope: NativeFilterScope = {
+  rootPath: ['ROOT_ID'],
+  excluded: [],
+};
+
+const mockDataMask = getInitialDataMask();
+
+test('time_grains field is omitted when all grains are selected', () => {
+  // When all time grains are selected/allowed, time_grains should be undefined
+  const filterOutput = {
+    id: 'NATIVE_FILTER-abc123',
+    type: NativeFilterType.NativeFilter,
+    name: 'Time Grain Filter',
+    filterType: 'filter_timegrain',
+    description: '',
+    targets: [],
+    scope: mockScope,
+    controlValues: {},
+    defaultDataMask: mockDataMask,
+    cascadeParentIds: [],
+    adhoc_filters: undefined,
+    time_range: undefined,
+    granularity_sqla: undefined,
+    time_grains: undefined, // omitted when all selected
+    sortMetric: null,
+  };
+
+  expect(filterOutput.time_grains).toBeUndefined();
+});
+
+test('time_grains field is omitted when empty array is reset', () => {
+  // When user clears all selections (empty array), it should be treated as undefined
+  const emptySelection = [];
+  const shouldOmit = emptySelection.length === 0;
+
+  expect(shouldOmit).toBe(true);
+});
+
+test('time_grains allows partial selection of grains', () => {
+  const allGrains = ['PT1M', 'PT1H', 'P1D', 'P1W', 'P1M', 'P0.25Y', 'P1Y'];
+  const selectedGrains = ['P1D', 'P1W', 'P1M'];
+
+  expect(selectedGrains).toEqual(expect.arrayContaining(['P1D', 'P1W', 'P1M']));
+  expect(selectedGrains.length).toBe(3);
+  expect(selectedGrains.length).toBeLessThan(allGrains.length);
+});
+
+test('backward compatibility: missing time_grains defaults to show all', () => {
+  // Existing filter configs without time_grains key should show all options
+  const legacyFilter = {
+    id: 'NATIVE_FILTER-legacy',
+    type: NativeFilterType.NativeFilter,
+    name: 'Legacy Time Grain Filter',
+    filterType: 'filter_timegrain',
+    targets: [],
+    // Note: no time_grains field
+  };
+
+  // When time_grains is undefined, runtime should show all options
+  const runtime_allowlist =
+    legacyFilter['time_grains' as keyof typeof legacyFilter];
+  const shouldShowAll = !runtime_allowlist || runtime_allowlist.length === 0;
+
+  expect(shouldShowAll).toBe(true);
+});

Review Comment:
   This test file doesn’t exercise any production code (it only asserts properties of locally constructed objects/arrays), so it won’t catch regressions in the actual transformer logic. Consider either deleting it (since `TimeGrainsTransformer.integration.test.ts` already covers `transformFilterForSave`) or rewriting it to import and test the real transformer behavior (e.g., empty array -> omitted/undefined).
   ```suggestion
   import { getInitialDataMask } from 'src/dataMask/reducer';
   
   test('getInitialDataMask returns the default empty data mask state', () => {
     const dataMask = getInitialDataMask();
   
     expect(dataMask).toEqual(
       expect.objectContaining({
         extraFormData: {},
         filterState: {},
         ownState: {},
       }),
     );
   });
   ```



##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx:
##########
@@ -239,11 +271,19 @@ const FilterValue: FC<FilterValueProps> = ({
             const result = 'result' in json ? json.result[0] : json;
             if (response.status === 200) {
               setState([result as ChartDataResponseResult]);

Review Comment:
   When `FeatureFlag.GlobalAsyncQueries` is enabled and the initial `getChartDataRequest` returns `200`, the time grain allowlist is not applied (only the `202` async-result path and the non-async path filter results). This makes `time_grains` behave inconsistently depending on whether the response is sync vs async; apply the allowlist to the `200` branch as well before calling `setState`.
   ```suggestion
                 setState(
                   applyTimeGrainAllowlist(
                     filterType,
                     allowedTimeGrains,
                     [result as ChartDataResponseResult],
                   ),
                 );
   ```



##########
superset-frontend/packages/superset-ui-core/src/query/types/Dashboard.ts:
##########
@@ -81,6 +81,7 @@ export type Filter = {
   granularity?: string;
   time_grain_sqla?: string;
   time_range?: string;
+  time_grains?: string[] | undefined; // selected time grains allowlist; undefined means all are allowed

Review Comment:
   `time_grains?: string[] | undefined` is redundant because the optional property marker (`?`) already implies `undefined`. Consider changing this to `time_grains?: string[]` to match the surrounding fields and avoid triggering `no-redundant-type-constituents`/style lint rules.
   ```suggestion
     time_grains?: string[]; // selected time grains allowlist; undefined means all are allowed
   ```



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org