bito-code-review[bot] commented on code in PR #38568: URL: https://github.com/apache/superset/pull/38568#discussion_r2915431855
########## superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/TimeGrainPreFilter.test.tsx: ########## @@ -0,0 +1,127 @@ +/** + * 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 { render, screen, userEvent, waitFor } from 'spec/helpers/testing-library'; +import { Form, Select } from '@superset-ui/core/components'; +import { CollapsibleControl } from './CollapsibleControl'; +import { getTimeGrainOptions } from './utils'; + +/** + * Tests for the Time Grain Pre-filter feature (CollapsibleControl + Select). + * + * These tests verify: + * - Options are rendered in database order (no client-side sorting) + * - A saved subset of time_grains is loaded and shown as selected values + * - The CollapsibleControl checkbox shows/hides the Select + * - Toggling the checkbox off clears the underlying selection + */ + +const TIME_GRAIN_TUPLES: [string, string][] = [ + ['PT1S', 'Second'], + ['PT1M', 'Minute'], + ['PT1H', 'Hour'], + ['P1D', 'Day'], + ['P1W', 'Week'], +]; + +const renderPreFilter = (props: { + savedGrains?: string[]; + initialChecked?: boolean; + onChangeMock?: jest.Mock; +}) => { + const { savedGrains, initialChecked = false, onChangeMock = jest.fn() } = props; + const options = getTimeGrainOptions(TIME_GRAIN_TUPLES); + return render( + <Form> + <Form.Item name="time_grains" initialValue={savedGrains}> + <CollapsibleControl + title="Pre-filter available values" + initialValue={initialChecked} + onChange={onChangeMock} + > + <Select + mode="multiple" + ariaLabel="Time grain options" + options={options} + sortComparator={() => 0} + /> + </CollapsibleControl> + </Form.Item> + </Form>, + ); +}; + +test('time grain options are displayed in database order (no sorting)', async () => { + renderPreFilter({ initialChecked: true }); + + // Open the dropdown + const select = screen.getByRole('combobox'); + await userEvent.click(select); + + await waitFor(() => { + const options = screen.getAllByRole('option'); + const labels = options.map(o => o.textContent); + // Options must follow database order: Second, Minute, Hour, Day, Week + expect(labels).toEqual(['Second', 'Minute', 'Hour', 'Day', 'Week']); + }); +}); + +test('CollapsibleControl checkbox shows and hides the grain Select', async () => { + renderPreFilter({}); + + // Initially unchecked — select should be hidden + expect(screen.queryByRole('combobox')).not.toBeInTheDocument(); + + const checkbox = screen.getByRole('checkbox', { + name: /Pre-filter available values/i, + }); + await userEvent.click(checkbox); + + // After checking — select should appear + expect(screen.getByRole('combobox')).toBeInTheDocument(); + + await userEvent.click(checkbox); + + // After unchecking — select should disappear + expect(screen.queryByRole('combobox')).not.toBeInTheDocument(); +}); + +test('CollapsibleControl starts expanded when initialValue is true', () => { + renderPreFilter({ initialChecked: true }); + + // When initialValue=true the checkbox is checked and children are visible + expect(screen.getByRole('combobox')).toBeInTheDocument(); + const checkbox = screen.getByRole('checkbox', { + name: /Pre-filter available values/i, + }); + expect(checkbox).toBeChecked(); +}); + +test('onChange is called with correct value when checkbox is toggled', async () => { + const onChangeMock = jest.fn(); + renderPreFilter({ onChangeMock }); + + const checkbox = screen.getByRole('checkbox', { + name: /Pre-filter available values/i, + }); + await userEvent.click(checkbox); + expect(onChangeMock).toHaveBeenCalledWith(true); + + await userEvent.click(checkbox); + expect(onChangeMock).toHaveBeenCalledWith(false); +}); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing test for saved time grain selection</b></div> <div id="fix"> The test comments indicate verification of 'A saved subset of time_grains is loaded and shown as selected values', but no test checks that savedGrains values appear selected in the multiple Select. Add a test to render with savedGrains=['PT1D', 'P1W'] and assert the selected values match. </div> </div> <small><i>Code Review Run #1c78ce</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx: ########## @@ -1036,425 +1041,494 @@ const FiltersConfigForm = ( items={[ ...(itemTypeField !== 'filter_time' ? [ - { - key: `${filterId}-${FilterPanels.configuration.key}`, - forceRender: true, - label: isChartCustomization - ? CustomizationPanels.configuration.name - : FilterPanels.configuration.name, - children: ( - <> - {canDependOnOtherFilters && - (hasAvailableFilters || - dependencies.length > 0) && ( - <StyledRowFormItem + { + key: `${filterId}-${FilterPanels.configuration.key}`, + forceRender: true, + label: isChartCustomization + ? CustomizationPanels.configuration.name + : FilterPanels.configuration.name, + children: ( + <> + {canDependOnOtherFilters && + (hasAvailableFilters || + dependencies.length > 0) && ( + <StyledRowFormItem + expanded={expanded} + name={[ + 'filters', + filterId, + 'dependencies', + ]} + initialValue={dependencies} + > + <DependencyList + availableFilters={availableFilters} + dependencies={dependencies} + onDependenciesChange={dependencies => { + setNativeFilterFieldValues( + form, + filterId, + { + dependencies, + }, + ); + forceUpdate(); + validateDependencies(); + formChanged(); + }} + getDependencySuggestion={() => + getDependencySuggestion(filterId) + } + > + {hasTimeDependency + ? timeColumn + : undefined} + </DependencyList> + </StyledRowFormItem> + )} + {hasDataset && hasAdditionalFilters && ( + <FormItem + name={['filters', filterId, 'preFilter']} + > + <CollapsibleControl + initialValue={hasPreFilter} + title={t('Pre-filter available values')} + tooltip={t(`Add filter clauses to control the filter's source query, + though only in the context of the autocomplete i.e., these conditions + do not impact how the filter is applied to the dashboard. This is useful + when you want to improve the query's performance by only scanning a subset + of the underlying data or limit the available values displayed in the filter.`)} + onChange={checked => { + formChanged(); + if (checked) { + validatePreFilter(); + } + }} + > + <StyledRowSubFormItem expanded={expanded} name={[ 'filters', filterId, - 'dependencies', + 'adhoc_filters', + ]} + css={{ width: INPUT_WIDTH }} + initialValue={ + filterToEdit?.adhoc_filters + } + required + rules={[ + { + validator: preFilterValidator, + }, ]} - initialValue={dependencies} > - <DependencyList - availableFilters={availableFilters} - dependencies={dependencies} - onDependenciesChange={dependencies => { + <AdhocFilterControl + columns={ + datasetDetails?.columns?.filter( + (c: ColumnMeta) => c.filterable, + ) || [] + } + savedMetrics={ + datasetDetails?.metrics || [] + } + datasource={datasetDetails} + onChange={( + filters: AdhocFilterClass[], + ) => { setNativeFilterFieldValues( form, filterId, { - dependencies, + adhoc_filters: filters, }, ); forceUpdate(); - validateDependencies(); formChanged(); - }} - getDependencySuggestion={() => - getDependencySuggestion(filterId) - } - > - {hasTimeDependency - ? timeColumn - : undefined} - </DependencyList> - </StyledRowFormItem> - )} - {hasDataset && hasAdditionalFilters && ( - <FormItem - name={['filters', filterId, 'preFilter']} - > - <CollapsibleControl - initialValue={hasPreFilter} - title={t('Pre-filter available values')} - tooltip={t(`Add filter clauses to control the filter's source query, - though only in the context of the autocomplete i.e., these conditions - do not impact how the filter is applied to the dashboard. This is useful - when you want to improve the query's performance by only scanning a subset - of the underlying data or limit the available values displayed in the filter.`)} - onChange={checked => { - formChanged(); - if (checked) { validatePreFilter(); + }} + label={ + <span> + <StyledLabel> + {t('Pre-filter')} + </StyledLabel> + {!hasTimeRange && ( + <StyledAsterisk /> + )} + </span> } - }} - > - <StyledRowSubFormItem + /> + </StyledRowSubFormItem> + {showTimeRangePicker && ( + <StyledRowFormItem expanded={expanded} name={[ 'filters', filterId, - 'adhoc_filters', + 'time_range', ]} - css={{ width: INPUT_WIDTH }} + label={ + <StyledLabel> + {t('Time range')} + </StyledLabel> + } initialValue={ - filterToEdit?.adhoc_filters + filterToEdit?.time_range || + t('No filter') } - required + required={!hasAdhoc} rules={[ { validator: preFilterValidator, }, ]} > - <AdhocFilterControl - columns={ - datasetDetails?.columns?.filter( - (c: ColumnMeta) => c.filterable, - ) || [] - } - savedMetrics={ - datasetDetails?.metrics || [] - } - datasource={datasetDetails} - onChange={( - filters: AdhocFilterClass[], - ) => { + <DateFilterComponent + name="time_range" + onChange={timeRange => { setNativeFilterFieldValues( form, filterId, { - adhoc_filters: filters, + time_range: timeRange, }, ); forceUpdate(); formChanged(); validatePreFilter(); }} - label={ - <span> - <StyledLabel> - {t('Pre-filter')} - </StyledLabel> - {!hasTimeRange && ( - <StyledAsterisk /> - )} - </span> - } /> - </StyledRowSubFormItem> - {showTimeRangePicker && ( - <StyledRowFormItem - expanded={expanded} - name={[ - 'filters', - filterId, - 'time_range', - ]} - label={ - <StyledLabel> - {t('Time range')} - </StyledLabel> - } - initialValue={ - filterToEdit?.time_range || - t('No filter') - } - required={!hasAdhoc} - rules={[ - { - validator: preFilterValidator, - }, - ]} - > - <DateFilterComponent - name="time_range" - onChange={timeRange => { - setNativeFilterFieldValues( - form, - filterId, - { - time_range: timeRange, - }, - ); - forceUpdate(); - formChanged(); - validatePreFilter(); - }} - /> - </StyledRowFormItem> - )} - {hasTimeRange && !hasTimeDependency - ? timeColumn - : undefined} - </CollapsibleControl> - </FormItem> - )} - {itemTypeField !== 'filter_range' ? ( + </StyledRowFormItem> + )} + {hasTimeRange && !hasTimeDependency + ? timeColumn + : undefined} + </CollapsibleControl> + </FormItem> + )} + {itemTypeField === 'filter_timegrain' && + hasDataset && + datasetDetails?.time_grain_sqla && + datasetDetails.time_grain_sqla.length > 0 && ( <FormItem - name={['filters', filterId, 'sortFilter']} - initialValue={hasSorting} + name={[ + 'filters', + filterId, + 'preFilterTimegrain', + ]} > <CollapsibleControl - initialValue={hasSorting} - title={ - isChartCustomization - ? t('Sort display control values') - : t('Sort filter values') - } + initialValue={hasTimeGrainPreFilter} + title={t('Pre-filter available values')} + tooltip={t(`Add filter clauses to control the filter's source query, + though only in the context of the autocomplete i.e., these conditions + do not impact how the filter is applied to the dashboard. This is useful + when you want to improve the query's performance by only scanning a subset + of the underlying data or limit the available values displayed in the filter.`)} onChange={checked => { - onSortChanged(checked || undefined); + if (!checked) { + setNativeFilterFieldValues( + form, + filterId, + { time_grains: undefined }, + ); + forceUpdate(); + } formChanged(); }} > - <StyledRowFormItem - expanded={expanded} + <FormItem name={[ 'filters', filterId, - 'controlValues', - 'sortAscending', + 'time_grains', ]} - initialValue={sort} - label={ - <StyledLabel> - {t('Sort type')} - </StyledLabel> - } + initialValue={filterToEdit?.time_grains} + {...getFiltersConfigModalTestId( + 'time-grain-allowlist', + )} > - <Radio.GroupWrapper - options={[ - { - value: true, - label: t('Sort ascending'), - }, - { - value: false, - label: t('Sort descending'), - }, - ]} - onChange={value => { - onSortChanged(value.target.value); + <Select + mode="multiple" + ariaLabel={t('Time grain options')} + options={getTimeGrainOptions( + datasetDetails.time_grain_sqla, + )} + sortComparator={() => 0} + onChange={(values: string[]) => { + setNativeFilterFieldValues( + form, + filterId, + { + time_grains: + values.length > 0 + ? values + : undefined, + }, + ); + forceUpdate(); formChanged(); }} + css={{ width: INPUT_WIDTH }} /> - </StyledRowFormItem> - {hasMetrics && ( - <StyledRowSubFormItem - expanded={expanded} - name={[ - 'filters', - filterId, - 'controlValues', - 'sortMetric', - ]} - initialValue={ - filterToEdit?.controlValues - ?.sortMetric ?? - customizationToEdit?.controlValues - ?.sortMetric - } - label={ - <> - <StyledLabel> - {t('Sort Metric')} - </StyledLabel> - - <InfoTooltip - placement="top" - tooltip={t( - 'If a metric is specified, sorting will be done based on the metric value', - )} - /> - </> - } - data-test="field-input" - > - <Select - allowClear - ariaLabel={t('Sort metric')} - name="sortMetric" - options={metrics.map( - (metric: Metric) => ({ - value: metric.metric_name, - label: - metric.verbose_name ?? - metric.metric_name, - }), - )} - onChange={value => { - if (value !== undefined) { - const previous = - form.getFieldValue( - 'filters', - )?.[filterId].controlValues || - {}; - setNativeFilterFieldValues( - form, - filterId, - { - controlValues: { - ...previous, - sortMetric: value, - }, - }, - ); - forceUpdate(); - } - formChanged(); - }} - /> - </StyledRowSubFormItem> - )} + </FormItem> </CollapsibleControl> </FormItem> - ) : ( - <> - <FormItem + )} + {itemTypeField !== 'filter_range' ? ( + <FormItem + name={['filters', filterId, 'sortFilter']} + initialValue={hasSorting} + > + <CollapsibleControl + initialValue={hasSorting} + title={ + isChartCustomization + ? t('Sort display control values') + : t('Sort filter values') + } + onChange={checked => { + onSortChanged(checked || undefined); + formChanged(); + }} + > + <StyledRowFormItem + expanded={expanded} name={[ 'filters', filterId, - 'rangeFilter', + 'controlValues', + 'sortAscending', ]} + initialValue={sort} + label={ + <StyledLabel> + {t('Sort type')} + </StyledLabel> + } > - <CollapsibleControl - initialValue={hasEnableSingleValue} - title={t('Single Value')} - onChange={checked => { - onEnableSingleValueChanged( - checked - ? SingleValueType.Exact - : undefined, - ); + <Radio.GroupWrapper + options={[ + { + value: true, + label: t('Sort ascending'), + }, + { + value: false, + label: t('Sort descending'), + }, + ]} + onChange={value => { + onSortChanged(value.target.value); formChanged(); }} - > - <StyledRowFormItem - expanded={expanded} - name={[ - 'filters', - filterId, - 'controlValues', - 'enableSingleValue', - ]} - initialValue={enableSingleValue} - label={ + /> + </StyledRowFormItem> + {hasMetrics && ( + <StyledRowSubFormItem + expanded={expanded} + name={[ + 'filters', + filterId, + 'controlValues', + 'sortMetric', + ]} + initialValue={ + filterToEdit?.controlValues + ?.sortMetric ?? + customizationToEdit?.controlValues + ?.sortMetric + } + label={ + <> <StyledLabel> - {t('Single value type')} + {t('Sort Metric')} </StyledLabel> - } - > - <Radio.GroupWrapper - onChange={value => { - onEnableSingleValueChanged( - value.target.value, + + <InfoTooltip + placement="top" + tooltip={t( + 'If a metric is specified, sorting will be done based on the metric value', + )} + /> + </> + } + data-test="field-input" + > + <Select + allowClear + ariaLabel={t('Sort metric')} + name="sortMetric" + options={metrics.map( + (metric: Metric) => ({ + value: metric.metric_name, + label: + metric.verbose_name ?? + metric.metric_name, + }), + )} Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Form update fails on clear</b></div> <div id="fix"> The onChange handler for the sort metric Select only updates the form when value is defined, but when cleared (value undefined), it should set sortMetric to undefined to properly clear the field. This prevents stale state from persisting after clearing the selection. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` - onChange={value => { - if (value !== undefined) { - const previous = - form.getFieldValue( - 'filters', - )?.[filterId].controlValues || {}; - setNativeFilterFieldValues( - form, - filterId, - { - controlValues: { - ...previous, - sortMetric: value, - }, - }, - ); - forceUpdate(); - } - formChanged(); - }} + onChange={value => { + const previous = + form.getFieldValue( + 'filters', + )?.[filterId].controlValues || {}; + setNativeFilterFieldValues( + form, + filterId, + { + controlValues: { + ...previous, + sortMetric: value, + }, + }, + ); + forceUpdate(); + formChanged(); + }} ``` </div> </details> </div> <small><i>Code Review Run #1c78ce</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- 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]
