gabotorresruiz commented on code in PR #43736:
URL: https://github.com/apache/superset/pull/43736#discussion_r3960682631
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/GroupByFilterCard.tsx:
##########
@@ -355,6 +370,16 @@ const GroupByFilterCard: FC<GroupByFilterCardProps> = ({
[sortAscending],
);
+ // Builder-configured allowlist of columns viewers are allowed to group by.
+ const columnsAllowlist = customizationItem.controlValues?.columnsAllowlist as
+ | string[]
+ | undefined;
+
+ const allowedColumnOptions = useMemo(
Review Comment:
Just a question, not a blocker: if a viewer already has a group-by applied
and the builder later narrows the allowlist to exclude it, the applied value
survives. I verified it on this branch: with `columnsAllowlist: ['country']`
and an applied value of `state`, the Select renders the raw column name `state`
(the verbose label "State" is lost because the option is gone), and the chart
keeps grouping by the excluded column since the dataMask is untouched. Is that
the intended behavior? If so, merging the active value into
`allowedColumnOptions` would at least keep the verbose label; if not, the card
could clear an applied value that the allowlist excludes. A small test like
"keeps or clears an applied selection that a narrowed allowlist excludes" would
pin whichever way you decide.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -607,6 +614,37 @@ const FiltersConfigForm = (
[filterId, form, formChanged],
);
+ // Seed a NEW Dynamic Group By control's allowlist with every groupable
+ // column so it defaults to "all selected" (builders can then deselect to
+ // restrict). Only seeds when no allowlist is set yet — a freshly created
+ // control, or one whose dataset just changed (which clears the allowlist).
+ // An existing selection, including a deliberately narrowed or emptied one,
+ // is never overwritten. The column names come from the same source that
+ // populates the multi-select options, so the default matches exactly what
+ // the builder can choose from. Seeding does not mark the form as changed, so
+ // editing a legacy control that never stored an allowlist stays a no-op
+ // unless the builder actually narrows the selection.
+ const seedGroupByAllowlist = useCallback(
Review Comment:
Not a blocker, but this block worries me a bit. I verified on this branch
that opening the config form on a legacy Group By control (saved before this
PR, so no `columnsAllowlist`) seeds the full column list into the form values,
and `useModalSaveLogic` transforms modified customizations straight from those
form values. So the moment a builder edits that filter for any other reason and
saves, the unset allowlist silently becomes a snapshot of today's columns. From
then on, a column added to the dataset later is hidden from viewers, while an
untouched legacy filter (or an explicitly emptied one) would have offered it.
The same drift applies to every newly created control, since the seed
materializes "all columns" as an explicit list.
Are we certain we want "all columns" to freeze at seed time? Two cheaper
alternatives that keep the "all selected" UX:
- keep `undefined` as the "all" state and only write a list into the form
when the builder actually narrows it, or
- normalize on save: if the allowlist equals the full groupable column set,
collapse it back to `undefined`.
Either way, a test along the lines of "editing a legacy Group By control
leaves columnsAllowlist unset unless the builder narrows it" in
`GroupByColumnAllowlist.test.tsx` would lock the intent in. Happy to dig in
with you if you want to pair on it.
--
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]
