alexandrusoare commented on code in PR #37681:
URL: https://github.com/apache/superset/pull/37681#discussion_r2877286632
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -821,6 +821,11 @@ const FiltersConfigForm = (
);
return (
<Tabs
+ allowOverflow={false}
+ contentHeight={630}
Review Comment:
Was wondering how did we choose this value of 630?
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/utils.test.ts:
##########
@@ -17,402 +17,771 @@
* under the License.
*/
-import { DataMaskStateWithId, Filter } from '@superset-ui/core';
+import {
+ DataMaskStateWithId,
+ DataRecordValue,
+ Filter,
+ FilterState,
+} from '@superset-ui/core';
import {
checkIsApplyDisabled,
checkIsValidateError,
checkIsMissingRequiredValue,
+ getOnlyExtraFormData,
+ getFiltersToApply,
} from './utils';
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
-describe('FilterBar Utils - Validation and Apply Logic', () => {
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
- describe('checkIsValidateError', () => {
- test('should return true when no filters have validation errors', () => {
- const dataMask: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- validateStatus: undefined,
- value: ['CA'],
- },
- extraFormData: {},
- },
- 'filter-2': {
- id: 'filter-2',
- filterState: {
- validateStatus: undefined,
- value: ['NY'],
- },
- extraFormData: {},
- },
- };
-
- expect(checkIsValidateError(dataMask)).toBe(true);
- });
-
- test('should return false when any filter has validation error', () => {
- const dataMask: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- validateStatus: 'error',
- value: undefined,
- },
- extraFormData: {},
- },
- 'filter-2': {
- id: 'filter-2',
- filterState: {
- validateStatus: undefined,
- value: ['NY'],
- },
- extraFormData: {},
- },
- };
-
- expect(checkIsValidateError(dataMask)).toBe(false);
- });
-
- test('should handle empty dataMask', () => {
- const dataMask: DataMaskStateWithId = {};
- expect(checkIsValidateError(dataMask)).toBe(true);
- });
-
- test('should handle filters without filterState', () => {
- const dataMask: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- extraFormData: {},
- },
- };
-
- expect(checkIsValidateError(dataMask)).toBe(true);
- });
+// Factory functions for test data
+function createDataMaskEntry(
+ id: string,
+ overrides: {
+ value?: unknown;
+ validateStatus?: 'error' | undefined;
+ extraFormData?: Record<string, unknown>;
+ } = {},
+) {
+ const { value, validateStatus, extraFormData = {} } = overrides;
+ return {
+ id,
+ filterState: {
+ value,
+ validateStatus,
+ },
+ extraFormData,
+ };
+}
+
+function createFilter(
+ id: string,
+ overrides: {
+ enableEmptyFilter?: boolean;
+ controlValues?: Record<string, unknown>;
+ } = {},
+): Filter {
+ const { enableEmptyFilter, controlValues = {} } = overrides;
+ return {
+ id,
+ controlValues: {
+ ...(enableEmptyFilter !== undefined && { enableEmptyFilter }),
+ ...controlValues,
+ },
+ } as unknown as Filter;
+}
+
+function createExtraFormDataWithFilter(
+ col: string,
+ val: DataRecordValue[],
+ op: 'IN' | 'NOT IN' = 'IN',
+) {
+ return {
+ filters: [{ col, op, val }],
+ };
+}
+
+// getOnlyExtraFormData tests
+test('getOnlyExtraFormData extracts extraFormData from all filters when no
filterIds provided', () => {
+ const dataMask: DataMaskStateWithId = {
+ 'filter-1': {
+ id: 'filter-1',
+ filterState: { value: ['CA'] },
+ extraFormData: { filters: [{ col: 'state', op: 'IN', val: ['CA'] }] },
+ },
+ 'filter-2': {
+ id: 'filter-2',
+ filterState: { value: ['NY'] },
+ extraFormData: { filters: [{ col: 'city', op: 'IN', val: ['NY'] }] },
+ },
+ };
+
+ const result = getOnlyExtraFormData(dataMask);
+
+ expect(result).toEqual({
+ 'filter-1': { filters: [{ col: 'state', op: 'IN', val: ['CA'] }] },
+ 'filter-2': { filters: [{ col: 'city', op: 'IN', val: ['NY'] }] },
});
+});
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
- describe('checkIsMissingRequiredValue', () => {
- test('should return true for required filter with undefined value', () => {
- const filter = {
- id: 'test-filter',
- controlValues: {
- enableEmptyFilter: true,
- },
- } as unknown as Filter;
-
- const filterState = {
- value: undefined,
- };
-
- expect(checkIsMissingRequiredValue(filter, filterState)).toBe(true);
- });
-
- test('should return true for required filter with null value', () => {
- const filter = {
- id: 'test-filter',
- controlValues: {
- enableEmptyFilter: true,
- },
- } as unknown as Filter;
-
- const filterState = {
- value: null,
- };
-
- expect(checkIsMissingRequiredValue(filter, filterState)).toBe(true);
- });
-
- test('should return false for required filter with valid value', () => {
- const filter = {
- id: 'test-filter',
- controlValues: {
- enableEmptyFilter: true,
- },
- } as unknown as Filter;
-
- const filterState = {
- value: ['CA'],
- };
-
- expect(checkIsMissingRequiredValue(filter, filterState)).toBe(false);
- });
-
- test('should return false for non-required filter with undefined value',
() => {
- const filter = {
- id: 'test-filter',
- controlValues: {
- enableEmptyFilter: false,
- },
- } as unknown as Filter;
-
- const filterState = {
- value: undefined,
- };
-
- expect(checkIsMissingRequiredValue(filter, filterState)).toBe(false);
- });
-
- test('should return false for filter without controlValues', () => {
- const filter = {
- id: 'test-filter',
- } as Filter;
-
- const filterState = {
- value: undefined,
- };
-
- // checkIsMissingRequiredValue returns undefined when controlValues is
missing
- // undefined is falsy, so we check for truthiness instead of exact false
- expect(checkIsMissingRequiredValue(filter, filterState)).toBeFalsy();
- });
+test('getOnlyExtraFormData only extracts extraFormData for specified
filterIds', () => {
+ const dataMask: DataMaskStateWithId = {
+ 'filter-1': {
+ id: 'filter-1',
+ filterState: { value: ['CA'] },
+ extraFormData: { filters: [{ col: 'state', op: 'IN', val: ['CA'] }] },
+ },
+ 'filter-2': {
+ id: 'filter-2',
+ filterState: { value: ['NY'] },
+ extraFormData: { filters: [{ col: 'city', op: 'IN', val: ['NY'] }] },
+ },
+ 'filter-3': {
+ id: 'filter-3',
+ filterState: { value: ['Product'] },
+ extraFormData: {
+ filters: [{ col: 'product', op: 'IN', val: ['Product'] }],
+ },
+ },
+ };
+
+ const filterIds = new Set(['filter-1', 'filter-3']);
+ const result = getOnlyExtraFormData(dataMask, filterIds);
+
+ expect(result).toEqual({
+ 'filter-1': { filters: [{ col: 'state', op: 'IN', val: ['CA'] }] },
+ 'filter-3': { filters: [{ col: 'product', op: 'IN', val: ['Product'] }] },
});
+ expect(result).not.toHaveProperty('filter-2');
+});
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
- describe('checkIsApplyDisabled', () => {
- test('should return true when filters have validation errors', () => {
- const dataMaskSelected: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- validateStatus: 'error',
- value: undefined,
- },
- extraFormData: {},
- },
- };
-
- const dataMaskApplied: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- value: ['CA'],
- },
- extraFormData: {
- filters: [{ col: 'state', op: 'IN', val: ['CA'] }],
- },
- },
- };
-
- const filters: Filter[] = [
- {
- id: 'filter-1',
- controlValues: {
- enableEmptyFilter: true,
- },
- } as unknown as Filter,
- ];
-
- expect(
- checkIsApplyDisabled(dataMaskSelected, dataMaskApplied, filters),
- ).toBe(true);
- });
-
- test('should return false when selected and applied states differ', () => {
- const dataMaskSelected: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- validateStatus: undefined,
- value: ['NY'],
- },
- extraFormData: {
- filters: [{ col: 'state', op: 'IN', val: ['NY'] }],
- },
- },
- };
-
- const dataMaskApplied: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- value: ['CA'],
- },
- extraFormData: {
- filters: [{ col: 'state', op: 'IN', val: ['CA'] }],
- },
- },
- };
-
- const filters: Filter[] = [
- {
- id: 'filter-1',
- controlValues: {
- enableEmptyFilter: false,
- },
- } as unknown as Filter,
- ];
-
- expect(
- checkIsApplyDisabled(dataMaskSelected, dataMaskApplied, filters),
- ).toBe(false);
- });
-
- test('should return true when selected and applied states are identical',
() => {
- const dataMaskSelected: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- validateStatus: undefined,
- value: ['CA'],
- },
- extraFormData: {
- filters: [{ col: 'state', op: 'IN', val: ['CA'] }],
- },
- },
- };
-
- const dataMaskApplied: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- value: ['CA'],
- },
- extraFormData: {
- filters: [{ col: 'state', op: 'IN', val: ['CA'] }],
- },
- },
- };
-
- const filters: Filter[] = [
- {
- id: 'filter-1',
- controlValues: {
- enableEmptyFilter: false,
- },
- } as unknown as Filter,
- ];
-
- expect(
- checkIsApplyDisabled(dataMaskSelected, dataMaskApplied, filters),
- ).toBe(true);
- });
-
- test('should return true when required filter is missing value in selected
state', () => {
- const dataMaskSelected: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- validateStatus: undefined,
- value: undefined,
- },
- extraFormData: {},
- },
- };
-
- const dataMaskApplied: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- value: ['CA'],
- },
- extraFormData: {
- filters: [{ col: 'state', op: 'IN', val: ['CA'] }],
- },
- },
- };
-
- const filters: Filter[] = [
- {
- id: 'filter-1',
- controlValues: {
- enableEmptyFilter: true, // Required filter
- },
- } as unknown as Filter,
- ];
-
- expect(
- checkIsApplyDisabled(dataMaskSelected, dataMaskApplied, filters),
- ).toBe(true);
- });
-
- test('should handle filter count mismatch', () => {
- const dataMaskSelected: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- validateStatus: undefined,
- value: ['CA'],
- },
- extraFormData: {
- filters: [{ col: 'state', op: 'IN', val: ['CA'] }],
- },
- },
- 'filter-2': {
- id: 'filter-2',
- filterState: {
- validateStatus: undefined,
- value: ['Product A'],
- },
- extraFormData: {
- filters: [{ col: 'product', op: 'IN', val: ['Product A'] }],
- },
- },
- };
-
- const dataMaskApplied: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- value: ['CA'],
- },
- extraFormData: {
- filters: [{ col: 'state', op: 'IN', val: ['CA'] }],
- },
- },
- // Missing filter-2
- };
-
- const filters: Filter[] = [
- { id: 'filter-1', controlValues: {} } as unknown as Filter,
- { id: 'filter-2', controlValues: {} } as unknown as Filter,
- ];
-
- expect(
- checkIsApplyDisabled(dataMaskSelected, dataMaskApplied, filters),
- ).toBe(true);
- });
-
- test('should handle validation status recalculation scenario', () => {
- // Scenario: Filter was required and had error, then user selected value
- // The validateStatus should be cleared and Apply should be enabled
-
- const dataMaskSelected: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- validateStatus: undefined, // Error cleared after selection
- value: ['CA'],
- },
- extraFormData: {
- filters: [{ col: 'state', op: 'IN', val: ['CA'] }],
- },
- },
- };
-
- const dataMaskApplied: DataMaskStateWithId = {
- 'filter-1': {
- id: 'filter-1',
- filterState: {
- value: undefined, // Previously cleared
- },
- extraFormData: {},
- },
- };
-
- const filters: Filter[] = [
- {
- id: 'filter-1',
- controlValues: {
- enableEmptyFilter: true,
- },
- } as unknown as Filter,
- ];
-
- // Should be enabled because states differ and no validation errors
- expect(
- checkIsApplyDisabled(dataMaskSelected, dataMaskApplied, filters),
- ).toBe(false);
- });
- });
+test('getOnlyExtraFormData returns empty object when filterIds is empty set',
() => {
+ const dataMask: DataMaskStateWithId = {
+ 'filter-1': {
+ id: 'filter-1',
+ filterState: { value: ['CA'] },
+ extraFormData: { filters: [{ col: 'state', op: 'IN', val: ['CA'] }] },
+ },
+ };
+
+ const filterIds = new Set<string>();
+ const result = getOnlyExtraFormData(dataMask, filterIds);
+
+ expect(result).toEqual({});
+});
+
+// checkIsValidateError tests
+test('checkIsValidateError returns true when no filters have validation
errors', () => {
+ const dataMask: DataMaskStateWithId = {
+ 'filter-1': createDataMaskEntry('filter-1', { value: ['CA'] }),
+ 'filter-2': createDataMaskEntry('filter-2', { value: ['NY'] }),
+ };
+
+ expect(checkIsValidateError(dataMask)).toBe(true);
+});
+
+test('checkIsValidateError returns false when any filter has validation
error', () => {
+ const dataMask: DataMaskStateWithId = {
+ 'filter-1': createDataMaskEntry('filter-1', { validateStatus: 'error' }),
+ 'filter-2': createDataMaskEntry('filter-2', { value: ['NY'] }),
+ };
+
+ expect(checkIsValidateError(dataMask)).toBe(false);
+});
+
+test('checkIsValidateError handles empty dataMask', () => {
+ const dataMask: DataMaskStateWithId = {};
+ expect(checkIsValidateError(dataMask)).toBe(true);
Review Comment:
I do agree with this point
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx:
##########
@@ -144,12 +144,12 @@ const publishDataMask = debounce(
// it when necessary. We strip any prefix so that history.replace adds
it back and doesn't
// double it up.
const appRoot = applicationRoot();
- let replacement_pathname = window.location.pathname;
- if (appRoot !== '/' && replacement_pathname.startsWith(appRoot)) {
- replacement_pathname = replacement_pathname.substring(appRoot.length);
+ let replacementPathname = window.location.pathname;
+ if (appRoot !== '/' && replacementPathname.startsWith(appRoot)) {
+ replacementPathname = replacementPathname.substring(appRoot.length);
}
- history.location.pathname = replacement_pathname;
history.replace({
+ pathname: replacementPathname,
search: newParams.toString(),
Review Comment:
What do you think about this?
--
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]
