gabotorresruiz commented on code in PR #43736:
URL: https://github.com/apache/superset/pull/43736#discussion_r4085034569
##########
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:
This block worries me a bit. The allowlist select narrows with
`!!column?.filterable` (line 1308), while the viewer builds its options with
`col.filterable !== false` (`GroupByFilterCard.tsx:339`). `filterable` is
nullable in `superset/connectors/sqla/models.py:1065` (Python-side
`default=True`, no `nullable=False`), so the API can return `null` for a column
and the two predicates then disagree.
I verified both halves on this branch. With a dataset whose third column is
`{ column_name: 'legacy_col', filterable: null }`, `seedGroupByAllowlist`
writes `["country","state"]` and silently drops `legacy_col`. Rendering
`GroupByFilterCard` against that same dataset offers `legacy_col` while no
allowlist is stored, and stops offering it once `columnsAllowlist: ['country',
'state']` is stored. So a builder who creates a Group By control and narrows
nothing still ends up hiding a column viewers could group by before.
Cheapest fix is to hand the allowlist the viewer's predicate:
```tsx
filterValues={(column: Column) => column?.filterable !== false}
```
A case in `GroupByColumnAllowlist.test.tsx` with a `filterable: null`
column, asserting it lands in the seed, would lock it in. The current tests
miss it because both mocked columns are explicitly `filterable: true`.
##########
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:
Not a blocker, and nothing reaches it today. This `ColumnSelect` does not
pass `formField`, so it falls back to `'column'` (`ColumnSelect.tsx:62`), and
its `resetColumnField()` writes `['filters', filterId, 'column'] = null`
whenever none of the current allowlist values match a loaded dataset column
(`ColumnSelect.tsx:145` and `:168`).
I verified it on this branch: for a Group By customization with no
`columnsAllowlist`, `filters[<id>].column` flips to `null` once the columns
load, while the same harness on `7024401` leaves it untouched. It is harmless
right now only because the main control items are gated behind
`!isChartCustomization` (`FiltersConfigForm.tsx:1244`), so `column` is never
populated for a chart customization in the first place.
Since this select owns its own value and an empty selection is legitimate,
skipping the lookup when there is nothing to look up looks like cheap insurance:
```tsx
const lookupValue = ensureIsArray(value);
const valueExists =
lookupValue.length === 0 ||
result.columns.some((column: Column) =>
lookupValue.includes(column.column_name),
);
```
Or am I misunderstanding something here?
--
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]
