eschutho commented on code in PR #43736:
URL: https://github.com/apache/superset/pull/43736#discussion_r4109573745
##########
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:
Went with normalize-on-save. The form still shows "all selected", but the
save step collapses an allowlist covering every groupable column back to unset,
so new dataset columns stay available. Added the test you suggested ("editing a
legacy control leaves columnsAllowlist unset on save unless narrowed") plus
unit tests on the collapse. Fixed in a756611.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -1214,6 +1266,55 @@ const FiltersConfigForm = (
: FilterPanels.configuration.name,
children: (
<>
+ {isDynamicGroupBy &&
+ hasDataset &&
+ showDataset && (
+ <StyledRowFormItem
+ expanded={expanded}
+ name={[
+ 'filters',
+ filterId,
+ 'controlValues',
+ 'columnsAllowlist',
+ ]}
+ initialValue={
+ customizationToEdit?.controlValues
+ ?.columnsAllowlist
+ }
+ label={
+ <>
+ <StyledLabel>
+ {t('Groupable columns')}
+ </StyledLabel>
+
+ <InfoTooltip
+ placement="top"
+ tooltip={t(
+ 'Columns viewers are allowed to
group by. Leave empty to allow all groupable columns.',
+ )}
+ />
+ </>
+ }
+ data-test="groupby-columns-allowlist"
+ >
+ <ColumnSelect
+ mode="multiple"
+ allowClear
+ form={form}
+ filterId={filterId}
+ datasetId={datasetId}
+ datasourceType={datasourceType}
+ filterValues={(column: Column) =>
+ !!column?.filterable
+ }
Review Comment:
Good catch. The allowlist now uses the viewer's predicate
(`column?.filterable !== false`). Added a `filterable: null` case asserting
it's offered and seeded. Fixed in a756611.
##########
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:
We decided to keep it: the applied value is now included in the options so
it renders with its verbose label, and it stops being offered once the viewer
clears it. Clearing it automatically felt too surprising for charts viewers
already have grouped. Pinned with "keeps an applied selection that a narrowed
allowlist excludes, with its label". Fixed in a756611.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -1214,6 +1266,55 @@ const FiltersConfigForm = (
: FilterPanels.configuration.name,
children: (
<>
+ {isDynamicGroupBy &&
+ hasDataset &&
+ showDataset && (
+ <StyledRowFormItem
+ expanded={expanded}
+ name={[
+ 'filters',
+ filterId,
+ 'controlValues',
+ 'columnsAllowlist',
+ ]}
+ initialValue={
+ customizationToEdit?.controlValues
+ ?.columnsAllowlist
+ }
+ label={
+ <>
+ <StyledLabel>
+ {t('Groupable columns')}
+ </StyledLabel>
+
+ <InfoTooltip
+ placement="top"
+ tooltip={t(
+ 'Columns viewers are allowed to
group by. Leave empty to allow all groupable columns.',
+ )}
+ />
+ </>
+ }
+ data-test="groupby-columns-allowlist"
+ >
+ <ColumnSelect
Review Comment:
Took your suggestion: an empty selection skips the lookup. Added a test that
loading the allowlist leaves `column` untouched. Fixed in a756611.
--
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]
