gabotorresruiz commented on code in PR #43736:
URL: https://github.com/apache/superset/pull/43736#discussion_r4190159349
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/transformers/customizationTransformer.ts:
##########
@@ -96,6 +102,52 @@ function buildCustomizationTarget(
return buildNativeFilterTarget(formInputs);
}
+/**
+ * A Group By allowlist that selects every groupable column is equivalent to
+ * "no restriction". Collapse it back to unset so the saved config does not
+ * freeze a snapshot of the dataset's columns: a column added to the dataset
+ * later then stays available to viewers, exactly like a control that never
+ * stored an allowlist.
+ */
+export function collapseFullColumnsAllowlist(
+ controlValues: ChartCustomizationsFormItem['controlValues'] | undefined,
+ groupableColumns: string[] | undefined,
+): ChartCustomizationsFormItem['controlValues'] {
+ const allowlist = controlValues?.columnsAllowlist;
+ if (
+ !controlValues ||
+ !Array.isArray(allowlist) ||
+ !groupableColumns?.length
+ ) {
+ return controlValues ?? {};
+ }
+ const selected = new Set<string>(allowlist);
+ if (!groupableColumns.every(column => selected.has(column))) {
Review Comment:
Normalize-on-save is the right call and I am not asking for the snapshot
behavior back. One consequence is worth naming though, because it is the mirror
image of the bug it fixes: the test here is "the allowlist covers every
groupable column", not "equals", so a saved narrowing collapses to unset as
soon as the dataset shrinks down to it.
I ran `collapseFullColumnsAllowlist({ canSelectMultiple: true,
columnsAllowlist: ['country', 'state', 'dropped_col'] }, ['country', 'state'])`
on this branch and got `{ canSelectMultiple: true }` back. So a builder who
allowed only `country` and `state` out of a wider set, and whose dataset later
lost `dropped_col`, has the restriction dropped by the next save they make for
some unrelated reason, and anything added to the dataset afterwards becomes
groupable for viewers. The form has the evidence in hand on that very render,
since `unavailableAllowlistColumns` is warning about `dropped_col` at the same
time.
Requiring equality keeps that case restricted:
```ts
const selected = new Set<string>(allowlist);
if (
allowlist.length !== groupableColumns.length ||
!groupableColumns.every(column => selected.has(column))
) {
return controlValues;
}
```
The case where the allowlist is exactly the current groupable set is
genuinely ambiguous and I think your reading of it is the defensible one, so I
would leave that half alone. A case in `customizationTransformer.test.ts` like
"keeps an allowlist that still carries a column the dataset no longer has"
would pin whichever way you go.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ColumnSelect.tsx:
##########
@@ -139,20 +195,25 @@ export function ColumnSelect({
'columns.is_dttm',
'columns.type_generic',
'columns.filterable',
+ ...(showVerboseNames ? ['columns.verbose_name'] : []),
],
})}`,
})
.then(({ json: { result } }) => {
- const lookupValue = Array.isArray(value) ? value : [value];
- const valueExists = result.columns.some((column: Column) =>
- lookupValue?.includes(column.column_name),
- );
- if (!valueExists) {
+ if (isStale()) {
+ return;
+ }
+ if (!isValueInColumns(result.columns)) {
resetColumnField();
}
setColumns(result.columns);
+ onColumnsLoaded?.(filterColumnNames(result.columns), requestKey);
}, handleError)
Review Comment:
Just a question, not a blocker: `isStale()` guards the resolve path and the
`finally`, but `handleError` is still the bare rejection handler, so a failure
for a dataset the builder already switched away from still toasts. Deliberate,
or worth the same guard?
--
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]
