sadpandajoe commented on code in PR #44366:
URL: https://github.com/apache/superset/pull/44366#discussion_r4130613451
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx:
##########
@@ -532,6 +535,28 @@ test('deletes a filter including dependencies', async ()
=> {
);
}, 30000);
+test('shows the dependency control on first render for a saved cascade
filter', () => {
+ const nativeFilterConfig = [
+ buildNativeFilter('NATIVE_FILTER-1', 'state', ['NATIVE_FILTER-2']),
+ buildNativeFilter('NATIVE_FILTER-2', 'country', []),
+ ];
+ const state = {
+ ...defaultState(),
+ dashboardInfo: {
+ metadata: {
+ native_filter_configuration: nativeFilterConfig,
+ },
+ },
+ dashboardLayout,
+ };
+ defaultRender(state, { ...props, createNewOnOpen: false });
+
+ // No interaction: the dependency control and its saved parent must be
+ // visible as soon as the modal opens on a filter that already has a
+ // cascade parent, without waiting for a rerender.
+ expect(getCheckbox(DEPENDENCIES_REGEX)).toBeChecked();
Review Comment:
The comment above says the saved parent itself must be visible, but the only
assertion is `getCheckbox(DEPENDENCIES_REGEX)).toBeChecked()`, which only
proves the checkbox is checked. `DependencyList` can render a "(deleted or
invalid type)" placeholder instead of the real parent in some states, and this
assertion would still pass. Could the test also assert the rendered parent
label/dependency-list content, and could the comment be narrowed to what's
actually checked?
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -477,9 +477,11 @@ const FiltersConfigForm = (
formFilter?.filterType,
);
Review Comment:
`canDependOnOtherFilters` just below was fixed to derive from
`itemTypeField` because `formFilter?.filterType` can be `undefined` on the
first render before the antd Form hydrates. `hasAdditionalFilters` here has the
identical read and the identical exposure: for a saved Select/Range filter,
`FILTERS_WITH_ADHOC_FILTERS.includes(undefined)` is `false` on that first
commit, so the pre-filter/adhoc-filters section is omitted from the initial
paint (it self-corrects once the mount-effect refresh forces a re-render, so
this reads as a first-paint flash rather than a lasting hide).
```suggestion
const hasAdditionalFilters = FILTERS_WITH_ADHOC_FILTERS.includes(
itemTypeField,
);
```
Should this use `itemTypeField` the same way, and is a first-paint
regression test worth adding here too, alongside the one just added for the
dependency control?
--
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]
