Copilot commented on code in PR #36062:
URL: https://github.com/apache/superset/pull/36062#discussion_r2519375293
##########
superset/daos/dashboard.py:
##########
@@ -423,6 +423,72 @@ def update_native_filters_config(
return updated_configuration
+ @classmethod
+ def update_chart_customizations_config(
+ cls,
+ dashboard: Dashboard | None = None,
+ attributes: dict[str, Any] | None = None,
+ ) -> list[dict[str, Any]]:
+ if not dashboard:
+ raise DashboardUpdateFailedError("Dashboard not found")
+
+ if attributes:
+ metadata = json.loads(dashboard.json_metadata or "{}")
Review Comment:
The method doesn't handle the case when `attributes` is `None` or empty
dict. If `attributes` is `None`, the function will return
`updated_configuration` which is undefined, causing an `UnboundLocalError`. Add
validation at the start or initialize `updated_configuration = []` before the
`if attributes:` block.
##########
superset-frontend/src/dataMask/reducer.ts:
##########
@@ -202,19 +245,35 @@ const dataMaskReducer = produce(
item.customization !== null &&
'column' in item.customization
) {
- const customizationFilterId = `chart_customization_${(item as {
id: string }).id}`;
+ const customizationFilterId = (item as { id: string }).id;
+
+ // Only set metadata default if not already loaded from backend
+ const customization = item.customization as {
+ column?: string | string[];
+ defaultDataMask?: { filterState?: { value?: string[] } };
+ };
Review Comment:
Multiple type assertions with `as` suggest the types are not well-defined.
Consider defining proper TypeScript interfaces for chart customization items to
avoid repeated type casting and improve type safety throughout the codebase.
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx:
##########
@@ -105,11 +112,19 @@ const FilterValue: FC<FilterControlProps> = ({
validateStatus,
clearAllTrigger,
onClearAllComplete,
+ isCustomization = false,
}) => {
const { id, targets, filterType, adhoc_filters, time_range } = filter;
const metadata = getChartMetadataRegistry().get(filterType);
const dependencies = useFilterDependencies(id, dataMaskSelected);
const shouldRefresh = useShouldFilterRefresh();
+
+ const behaviors = useMemo(
+ () => [
+ isCustomization ? Behavior.ChartCustomization : Behavior.NativeFilter,
+ ],
+ [isCustomization],
+ );
Review Comment:
[nitpick] The `behaviors` array should be defined outside the component or
as a constant map since it has only two possible values. Creating a new array
on every render (even with useMemo) is unnecessary. Consider: `const behaviors
= isCustomization ? [Behavior.ChartCustomization] : [Behavior.NativeFilter];`
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx:
##########
@@ -109,106 +120,88 @@ function FiltersConfigModal({
}: FiltersConfigModalProps) {
const dispatch = useDispatch();
const theme = useTheme();
-
const [form] = Form.useForm<NativeFiltersForm>();
-
const configFormRef = useRef<any>();
- // the filter config from redux state, this does not change until modal is
closed.
const filterConfig = useFilterConfiguration();
const filterConfigMap = useFilterConfigMap();
+ const chartCustomizationConfig = useChartCustomizationConfiguration();
+ const chartCustomizationConfigMap = useChartCustomizationConfigMap();
- // this state contains the changes that we'll be sent through the PATCH
endpoint
- const [filterChanges, setFilterChanges] = useState<FilterChangesType>(
- DEFAULT_FILTER_CHANGES,
- );
-
- const resetFilterChanges = () => {
- setFilterChanges(DEFAULT_FILTER_CHANGES);
- };
-
- const handleModifyFilter = useCallback(
- (filterId: string) => {
- if (!filterChanges.modified.includes(filterId)) {
- setFilterChanges(prev => ({
- ...prev,
- modified: [...prev.modified, filterId],
- }));
- }
- },
- [filterChanges.modified],
- );
-
- // new filter ids belong to filters have been added during
- // this configuration session, and only exist in the form state until we
submit.
- const [newFilterIds, setNewFilterIds] = useState<string[]>(
- DEFAULT_EMPTY_FILTERS,
- );
+ const [filterChanges, setFilterChanges] =
+ useState<FilterChangesType>(DEFAULT_CHANGES);
+ const [chartCustomizationChanges, setChartCustomizationChanges] =
+ useState<FilterChangesType>(DEFAULT_CHANGES);
- // store ids of filters that have been removed with the time they were
removed
- // so that we can disappear them after a few secs.
- // filters are still kept in state until form is submitted.
+ const [newFilterIds, setNewFilterIds] =
+ useState<string[]>(DEFAULT_EMPTY_ARRAY);
const [removedFilters, setRemovedFilters] = useState<
Record<string, FilterRemoval>
- >(DEFAULT_REMOVED_FILTERS);
+ >(DEFAULT_REMOVED_ITEMS);
+ const [newChartCustomizationIds, setNewChartCustomizationIds] =
+ useState<string[]>(DEFAULT_EMPTY_ARRAY);
+ const [removedChartCustomizations, setRemovedChartCustomizations] = useState<
+ Record<string, FilterRemoval>
+ >(DEFAULT_REMOVED_ITEMS);
const [saveAlertVisible, setSaveAlertVisible] = useState<boolean>(false);
+ const [formValues, setFormValues] =
+ useState<NativeFiltersForm>(DEFAULT_FORM_VALUES);
+ const [erroredFilters, setErroredFilters] =
+ useState<string[]>(DEFAULT_EMPTY_ARRAY);
+ const [erroredCustomizations, setErroredCustomizations] =
+ useState<string[]>(DEFAULT_EMPTY_ARRAY);
+ const [expanded, setExpanded] = useState(false);
+ const [activeCollapseKeys, setActiveCollapseKeys] = useState<string[]>([
+ 'filters',
+ 'chartCustomizations',
+ ]);
- // The full ordered set of ((original + new) - completely removed) filter ids
- // Use this as the canonical list of what filters are being configured!
- // This includes filter ids that are pending removal, so check for that.
const filterIds = useMemo(
+ () => uniq([...getFilterIds(filterConfig), ...newFilterIds]),
+ [filterConfig, newFilterIds],
+ );
+
+ const chartCustomizationIds = useMemo(
() =>
- uniq([...getFilterIds(filterConfig), ...newFilterIds]).filter(
- id => !removedFilters[id] || removedFilters[id]?.isPending,
- ),
- [filterConfig, newFilterIds, removedFilters],
+ uniq([
+ ...getChartCustomizationIds(chartCustomizationConfig),
+ ...newChartCustomizationIds,
+ ]),
+ [chartCustomizationConfig, newChartCustomizationIds],
);
- // open the first filter in the list to start
- const initialCurrentFilterId = initialFilterId ?? filterIds[0];
- const [currentFilterId, setCurrentFilterId] = useState(
+ const initialCurrentFilterId =
+ initialFilterId ?? filterIds[0] ?? chartCustomizationIds[0] ?? '';
+ const [currentItemId, setCurrentItemId] = useState<string>(
initialCurrentFilterId,
);
- const [erroredFilters, setErroredFilters] = useState<string[]>(
- DEFAULT_EMPTY_FILTERS,
+ const [renderedFilters, setRenderedFilters] = useState<string[]>(
+ initialCurrentFilterId && isFilterId(initialCurrentFilterId)
+ ? [initialCurrentFilterId]
+ : [],
);
Review Comment:
[nitpick] The initialization logic for `renderedFilters` and
`renderedCustomizations` is duplicated. Consider extracting this into a helper
function or consolidating the logic to reduce duplication and improve
maintainability.
--
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]
