QT/superset2
1
0
Fork 0
mirror of https://github.com/apache/superset.git synced 2026-04-16 06:34:52 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(dashboard): prevent Apply button from disabling when required filters are auto-applied (#38479)

Browse Source 
Co-authored-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
yousoph
2026-03-09 09:29:43 -07:00
committed by GitHub
parent e70c7944b7
commit 62cebc8a0e
3 changed files with 255 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx
View File

@@ -238,7 +238,19 @@ const FilterBar: FC<FiltersBarProps> = ({
needsAutoApply);
if (shouldDispatch) {
dispatch(updateDataMask(filter.id, dataMask));
// Strip validateStatus before dispatching to Redux
// validateStatus is UI-only state and shouldn't persist in Redux
const { filterState, ...restDataMask } = dataMask;
const dataMaskForRedux = filterState
? {
...restDataMask,
filterState: {
...filterState,
validateStatus: undefined,
},
}
: dataMask;
dispatch(updateDataMask(filter.id, dataMaskForRedux));
}
// Mark filter as initialized after getting its first value WITH extraFormData

219
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/utils.test.ts
View File

@@ -326,7 +326,9 @@ describe('FilterBar Utils - Validation and Apply Logic', () => {
).toBe(true);
});
test('should handle filter count mismatch', () => {
test('should enable Apply when new filter is selected', () => {
// User selects a new filter that hasn't been applied yet
// Apply should be ENABLED to allow applying the new selection
const dataMaskSelected: DataMaskStateWithId = {
'filter-1': {
id: 'filter-1',
@@ -360,7 +362,7 @@ describe('FilterBar Utils - Validation and Apply Logic', () => {
filters: [{ col: 'state', op: 'IN', val: ['CA'] }],
},
},
// Missing filter-2
// filter-2 not yet applied
};
const filters: Filter[] = [
@@ -370,7 +372,7 @@ describe('FilterBar Utils - Validation and Apply Logic', () => {
expect(
checkIsApplyDisabled(dataMaskSelected, dataMaskApplied, filters),
).toBe(true);
).toBe(false);
});
test('should handle validation status recalculation scenario', () => {
@@ -414,5 +416,216 @@ describe('FilterBar Utils - Validation and Apply Logic', () => {
checkIsApplyDisabled(dataMaskSelected, dataMaskApplied, filters),
).toBe(false);
});
test('should not disable Apply when required filter is auto-applied (bug fix)', () => {
// Bug scenario: Filter A has "required" + "select first by default"
// Filter A auto-applies and is synced to selected
// User changes Filter B (adds it to selected, not in applied yet)
// Apply button should be ENABLED (changes exist and required filter has value)
const dataMaskSelected: DataMaskStateWithId = {
'filter-country': {
id: 'filter-country',
filterState: {
validateStatus: undefined,
value: ['USA'], // Auto-applied value (synced from applied)
},
extraFormData: {
filters: [{ col: 'country', op: 'IN', val: ['USA'] }],
},
},
'filter-product': {
id: 'filter-product',
filterState: {
validateStatus: undefined,
value: ['Product A'], // User changed this filter
},
extraFormData: {
filters: [{ col: 'product_line', op: 'IN', val: ['Product A'] }],
},
},
};
const dataMaskApplied: DataMaskStateWithId = {
'filter-country': {
id: 'filter-country',
filterState: {
value: ['USA'], // Already applied
},
extraFormData: {
filters: [{ col: 'country', op: 'IN', val: ['USA'] }],
},
},
// filter-product not yet applied
};
const filters: Filter[] = [
{
id: 'filter-country',
controlValues: {
enableEmptyFilter: true, // Required filter
},
} as unknown as Filter,
{
id: 'filter-product',
controlValues: {
enableEmptyFilter: false,
},
} as unknown as Filter,
];
// Should be ENABLED - Filter B has changes to apply and Filter A (required) has value
expect(
checkIsApplyDisabled(dataMaskSelected, dataMaskApplied, filters),
).toBe(false);
});
test('should not disable Apply when required filter is only in applied state during sync gap', () => {
// Edge case: Required filter auto-applied but not yet synced to selected
// The filter is NOT in dataMaskSelected at all (key doesn't exist)
// This can happen during the React state update cycle
// Apply button should still be enabled if other filters have changes
const dataMaskSelected: DataMaskStateWithId = {
'filter-product': {
id: 'filter-product',
filterState: {
validateStatus: undefined,
value: ['Product B'],
},
extraFormData: {
filters: [{ col: 'product_line', op: 'IN', val: ['Product B'] }],
},
},
// filter-country not yet in selected (sync gap) - KEY DOESN'T EXIST
};
const dataMaskApplied: DataMaskStateWithId = {
'filter-country': {
id: 'filter-country',
filterState: {
value: ['USA'], // Auto-applied, has value
},
extraFormData: {
filters: [{ col: 'country', op: 'IN', val: ['USA'] }],
},
},
// filter-product not yet applied
};
const filters: Filter[] = [
{
id: 'filter-country',
controlValues: {
enableEmptyFilter: true, // Required filter
},
} as unknown as Filter,
{
id: 'filter-product',
controlValues: {
enableEmptyFilter: false,
},
} as unknown as Filter,
];
// Should be ENABLED - Required filter has value in applied state (auto-applied),
// and it's not in selected at all (not explicitly cleared by user)
expect(
checkIsApplyDisabled(dataMaskSelected, dataMaskApplied, filters),
).toBe(false);
});
test('should disable Apply when user explicitly clears a required filter value', () => {
// Different from sync gap: filter IS in selected state but with undefined value
// This means user explicitly cleared it
// Apply should be DISABLED because it's a required filter
const dataMaskSelected: DataMaskStateWithId = {
'filter-1': {
id: 'filter-1',
filterState: {
validateStatus: undefined,
value: undefined, // User explicitly cleared the value
},
extraFormData: {},
},
};
const dataMaskApplied: DataMaskStateWithId = {
'filter-1': {
id: 'filter-1',
filterState: {
value: ['CA'], // Previously had a value
},
extraFormData: {
filters: [{ col: 'state', op: 'IN', val: ['CA'] }],
},
},
};
const filters: Filter[] = [
{
id: 'filter-1',
controlValues: {
enableEmptyFilter: true, // Required filter
},
} as unknown as Filter,
];
// Should be DISABLED - User cleared a required filter
// Even though it has value in applied state, the selected state shows user intent
expect(
checkIsApplyDisabled(dataMaskSelected, dataMaskApplied, filters),
).toBe(true);
});
test('should enable Apply when filter has value but needs extraFormData update (PR #36927 regression test)', () => {
// Original bug from PR #36927: defaultDataMask has value but empty extraFormData
// The filter loads with a value in applied state but no extraFormData
// Then the filter plugin generates extraFormData and updates selected state
// Apply should be ENABLED to allow the extraFormData to be applied
const dataMaskSelected: DataMaskStateWithId = {
'filter-1': {
id: 'filter-1',
filterState: {
validateStatus: undefined,
value: ['value1', 'value2'], // Same value
},
extraFormData: {
// Now has extraFormData generated by filter plugin
filters: [
{ col: 'test_column', op: 'IN', val: ['value1', 'value2'] },
],
},
},
};
const dataMaskApplied: DataMaskStateWithId = {
'filter-1': {
id: 'filter-1',
filterState: {
value: ['value1', 'value2'], // Has value from defaultDataMask
},
extraFormData: {}, // But extraFormData is empty (the bug scenario)
},
};
const filters: Filter[] = [
{
id: 'filter-1',
controlValues: {
enableEmptyFilter: true, // Required filter
},
} as unknown as Filter,
];
// Should be ENABLED because extraFormData changed (not equal)
// even though the value is the same
// This allows the auto-apply logic to update the applied state with extraFormData
expect(
checkIsApplyDisabled(dataMaskSelected, dataMaskApplied, filters),
).toBe(false);
});
});
});

38
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/utils.ts
View File

@@ -61,15 +61,32 @@ export const checkIsApplyDisabled = (
return true;
}
const dataSelectedValues = Object.values(dataMaskSelected);
const dataAppliedValues = Object.values(dataMaskApplied);
// Check if any required filter is missing a value
// For filters that may have been auto-applied (e.g., requiredFirst filters),
// check both selected and applied states to avoid false positives during initialization
const hasMissingRequiredFilter = filters.some(filter => {
const selectedDataMask = dataMaskSelected?.[filter?.id];
const selectedState = selectedDataMask?.filterState;
const appliedState = dataMaskApplied?.[filter?.id]?.filterState;
const hasMissingRequiredFilter = filters.some(filter =>
checkIsMissingRequiredValue(
filter,
dataMaskSelected?.[filter?.id]?.filterState,
),
);
// If filter has value in selected state, it's not missing
if (selectedState?.value !== null && selectedState?.value !== undefined) {
return false;
}
// If filter is not in selected state at all (not initialized yet),
// check if it was auto-applied and has value in applied state
// This handles the case where auto-applied filters haven't synced to selected yet
if (!selectedDataMask) {
if (appliedState?.value !== null && appliedState?.value !== undefined) {
return false; // Not missing, it's auto-applied
}
}
// Otherwise, check if it's actually a required filter with missing value
// This includes cases where user explicitly cleared the value (selectedDataMask exists but value is undefined)
return checkIsMissingRequiredValue(filter, selectedState);
});
const selectedExtraFormData = getOnlyExtraFormData(dataMaskSelected);
const appliedExtraFormData = getOnlyExtraFormData(dataMaskApplied);
@@ -80,10 +97,7 @@ export const checkIsApplyDisabled = (
{ ignoreUndefined: true },
);
const result =
areEqual ||
dataSelectedValues.length !== dataAppliedValues.length ||
hasMissingRequiredFilter;
const result = areEqual || hasMissingRequiredFilter;
return result;
};