rusackas commented on code in PR #42592:
URL: https://github.com/apache/superset/pull/42592#discussion_r3693990931
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.test.tsx:
##########
@@ -989,6 +989,45 @@ test('shows all options when filterOption is false', async
() => {
expect(options[0]).toHaveTextContent('Server 0');
});
+test('hides a server-matched option when its label diverges from the search
term and filterOption is left at the default (regression for #42041)', async ()
=> {
Review Comment:
Renamed it to match what it actually asserts, thanks for the catch.
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.test.tsx:
##########
@@ -989,6 +989,45 @@ test('shows all options when filterOption is false', async
() => {
expect(options[0]).toHaveTextContent('Server 0');
});
+test('hides a server-matched option when its label diverges from the search
term and filterOption is left at the default (regression for #42041)', async ()
=> {
+ // Mirrors the real permissions-search bug: the remote fetch legitimately
+ // matches the raw, underscore-containing value (e.g. a schema name like
+ // "stg_silver"), but the returned option's displayed label has had
+ // underscores replaced with spaces (see formatPermissionLabel in
+ // features/roles/utils.ts). filterOption defaults to true, so AsyncSelect
+ // re-filters the already-matched options against the raw search term
+ // client-side. Since the underscore-typed search never appears as a
+ // substring of the space-formatted label, the legitimately fetched
+ // option gets hidden. Contrast with the `filterOption={false}` test
+ // above, which is the only way today's callers can avoid this.
+ const searchData = [{ label: 'stg silver', value: 100 }];
+ const loadOptions = jest.fn(async (search: string) =>
+ // totalCount must exceed the empty initial page here, otherwise
+ // AsyncSelect marks allValuesLoaded and short-circuits every later
+ // fetch, including the search request this test depends on.
+ search === ''
+ ? { data: [], totalCount: 1 }
+ : { data: searchData, totalCount: 1 },
+ );
Review Comment:
This one's just exercising AsyncSelect's rendering given an already-matched
option, not the query-to-result mapping itself. PermissionsField's own test in
RoleFormItems.test.tsx scopes the mock to stg_silver specifically and covers
that part.
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.test.tsx:
##########
@@ -989,6 +989,45 @@ test('shows all options when filterOption is false', async
() => {
expect(options[0]).toHaveTextContent('Server 0');
});
+test('hides a server-matched option when its label diverges from the search
term and filterOption is left at the default (regression for #42041)', async ()
=> {
+ // Mirrors the real permissions-search bug: the remote fetch legitimately
+ // matches the raw, underscore-containing value (e.g. a schema name like
+ // "stg_silver"), but the returned option's displayed label has had
+ // underscores replaced with spaces (see formatPermissionLabel in
+ // features/roles/utils.ts). filterOption defaults to true, so AsyncSelect
+ // re-filters the already-matched options against the raw search term
+ // client-side. Since the underscore-typed search never appears as a
+ // substring of the space-formatted label, the legitimately fetched
+ // option gets hidden. Contrast with the `filterOption={false}` test
+ // above, which is the only way today's callers can avoid this.
+ const searchData = [{ label: 'stg silver', value: 100 }];
+ const loadOptions = jest.fn(async (search: string) =>
+ // totalCount must exceed the empty initial page here, otherwise
+ // AsyncSelect marks allValuesLoaded and short-circuits every later
+ // fetch, including the search request this test depends on.
+ search === ''
+ ? { data: [], totalCount: 1 }
+ : { data: searchData, totalCount: 1 },
+ );
+
+ render(<AsyncSelect {...defaultProps} options={loadOptions} />);
+ await open();
+
+ await type('stg_silver');
+ await waitFor(() =>
+ expect(loadOptions).toHaveBeenCalledWith(
+ 'stg_silver',
+ expect.anything(),
+ expect.anything(),
+ ),
+ );
+
+ // The backend legitimately matched and returned this option (asserted
+ // above); it should render in the dropdown despite the search term using
+ // underscores while the label uses spaces.
+ expect(await findSelectOption('stg silver')).toBeInTheDocument();
Review Comment:
Fix landed alongside, filterOption={false} on the Permissions select in
RoleFormItems.tsx, so this should be green now.
##########
superset-frontend/src/features/roles/RoleFormItems.tsx:
##########
@@ -60,6 +60,13 @@ export const PermissionsField = ({
placeholder={t('Select permissions')}
options={options}
loading={loading}
+ // fetchPermissionOptions already filters server-side against the raw
+ // permission/view_menu names. AsyncSelect's default client-side
+ // re-filter checks the search term against the rendered label, but
+ // that label has had underscores replaced with spaces
+ // (formatPermissionLabel), so a raw-name search term never matches
+ // it and the correctly-fetched option gets hidden. See #42041.
+ filterOption={false}
Review Comment:
filterOption={false} disabling all client-side narrowing isn't new here,
it's the documented AsyncSelect contract already (see the 'shows all options
when filterOption is false' test above). It only bites once allValuesLoaded
flips true, which needs the whole permission set under the page size (100 by
default), a pretty small install. Given the alternative is silently hiding
legit matches, I'd rather keep this.
--
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]
