sadpandajoe commented on code in PR #45591: URL: https://github.com/apache/superset/pull/45591#discussion_r4379508750
########## superset-frontend/src/features/groups/GroupListModal.test.tsx: ########## @@ -0,0 +1,196 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { render, screen, waitFor, within } from 'spec/helpers/testing-library'; +import userEvent from '@testing-library/user-event'; +import { GroupListAddModal, GroupListEditModal } from './GroupListModal'; +import { fetchUserOptions, toUserSelectOption } from './utils'; + +jest.mock('./utils', () => ({ + ...jest.requireActual('./utils'), + fetchUserOptions: jest.fn(), +})); + +jest.mock('src/components/MessageToasts/withToasts', () => ({ + __esModule: true, + default: (Component: unknown) => Component, + useToasts: () => ({ addDangerToast: jest.fn(), addSuccessToast: jest.fn() }), +})); + +const props = { + show: true, + onHide: jest.fn(), + onSave: jest.fn(), + roles: [], +}; + +test('Users picker shows a user matched by email with the email under their name', async () => { + // The server matches email, but the option label is the display name, so + // the picker has to re-filter on the option's plain-text name and email + // props and show the email, or the match is hidden or unexplained. + jest + .mocked(fetchUserOptions) + .mockImplementation(async (filterValue: string) => + filterValue === '[email protected]' + ? { + data: [ + toUserSelectOption({ + id: 7, + username: 'jsmith', + first_name: 'Jane', + last_name: 'Smith', + email: '[email protected]', + }), + ], + totalCount: 1, + } + : // 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. + { data: [], totalCount: 1 }, + ); + + render(<GroupListAddModal {...props} />); + const combobox = screen.getByRole('combobox', { name: /users/i }); + await waitFor(() => userEvent.click(combobox)); + await userEvent.type(combobox, '[email protected]', { delay: 10 }); + + await waitFor(() => + expect(fetchUserOptions).toHaveBeenCalledWith( + '[email protected]', + expect.anything(), + expect.anything(), + expect.anything(), + ), + ); + const dropdown = within(document.querySelector('.ant-select-dropdown-list')!); + expect(await dropdown.findByText('Jane Smith')).toBeInTheDocument(); + expect(dropdown.getByText('[email protected]')).toBeInTheDocument(); +}); + +test('Users picker shows a user matched by username even though neither line shows it', async () => { + // The server matches username, but the option shows only the display name + // and email. The client-side re-filter must also check the username prop, + // or a user found by an opaque SSO username is hidden. + jest + .mocked(fetchUserOptions) + .mockImplementation(async (filterValue: string) => + filterValue === '00f09b2e' + ? { + data: [ + toUserSelectOption({ + id: 8, + username: '00f09b2e-f4fb-48a3-80fc-53ccd9528021', + first_name: 'Bob', + last_name: 'Jones', + email: '[email protected]', + }), + ], + totalCount: 1, + } + : { data: [], totalCount: 1 }, + ); + + render(<GroupListAddModal {...props} />); + const combobox = screen.getByRole('combobox', { name: /users/i }); + await waitFor(() => userEvent.click(combobox)); + await userEvent.type(combobox, '00f09b2e', { delay: 10 }); + + await waitFor(() => + expect(fetchUserOptions).toHaveBeenCalledWith( + '00f09b2e', + expect.anything(), + expect.anything(), + expect.anything(), + ), + ); + const dropdown = within(document.querySelector('.ant-select-dropdown-list')!); + expect(await dropdown.findByText('Bob Jones')).toBeInTheDocument(); + expect(dropdown.getByText('[email protected]')).toBeInTheDocument(); + expect(dropdown.queryByText(/00f09b2e/)).not.toBeInTheDocument(); +}); + +test('Users picker orders users by display name rather than by id', async () => { + // Ids 1, 2, 3 are Zoe, Amy, Max. AsyncSelect's default comparator ranks only + // string labels and falls back to the option value, so without + // userOptionSortComparator the React-element labels render in id order. + jest.mocked(fetchUserOptions).mockResolvedValue({ + data: [ + toUserSelectOption({ id: 1, username: 'zoe', first_name: 'Zoe' }), + toUserSelectOption({ id: 2, username: 'amy', first_name: 'Amy' }), + toUserSelectOption({ id: 3, username: 'max', first_name: 'Max' }), + ], + totalCount: 3, + }); + + render(<GroupListAddModal {...props} />); + const combobox = screen.getByRole('combobox', { name: /users/i }); + await waitFor(() => userEvent.click(combobox)); + + const optionTitles = () => + Array.from( + document.querySelectorAll('.ant-select-dropdown .ant-select-item-option'), + ).map(option => option.getAttribute('title')); + await waitFor(() => expect(optionTitles()).toHaveLength(3)); + expect(optionTitles()).toEqual(['Amy', 'Max', 'Zoe']); +}); + +test('Users picker options are announced by name rather than by user id', async () => { + // The virtualized option list derives an aria-label only from a string + // label, so a React-element label would leave the numeric id as the name. + jest.mocked(fetchUserOptions).mockResolvedValue({ + data: [toUserSelectOption({ id: 42, username: 'amy', first_name: 'Amy' })], + totalCount: 1, + }); + + render(<GroupListAddModal {...props} />); + const combobox = screen.getByRole('combobox', { name: /users/i }); + await waitFor(() => userEvent.click(combobox)); + + expect( + await screen.findByRole('option', { name: 'Amy' }), + ).toBeInTheDocument(); + expect(screen.queryByRole('option', { name: '42' })).not.toBeInTheDocument(); +}); + +test('Edit modal announces a preselected user outside the first page by name', async () => { + // A member missing from the fetched page is rebuilt by AsyncSelect from the + // selected value, which has to keep the option's aria-label: the React label + // gives the virtualized list no accessible name of its own. + jest.mocked(fetchUserOptions).mockResolvedValue({ + data: [toUserSelectOption({ id: 1, username: 'zoe', first_name: 'Zoe' })], + totalCount: 50, + }); + const group = { + id: 9, + name: 'analysts', + label: 'Analysts', + description: '', + roles: [], + users: [{ id: 42, username: 'amy', first_name: 'Amy', last_name: '' }], + }; + + render(<GroupListEditModal {...props} group={group} />); + const combobox = screen.getByRole('combobox', { name: /users/i }); + await waitFor(() => userEvent.click(combobox)); + + expect( + await screen.findByRole('option', { name: 'Amy' }), Review Comment: This hydration test still passes if `GroupListModal` goes back to `{ value: user.id, label: getUserDisplayLabel(user) }` for `group.users`: a plain string label already gives the option the name "Amy", and the fixture has no email. So an off-page group member losing its email/username filter props (the part the Role modal test now pins) would go unnoticed. Could this case give Amy an email, then type an email-only substring and assert she stays in the dropdown while the on-page user drops out? ########## superset-frontend/src/features/groups/utils.ts: ########## @@ -18,10 +18,80 @@ */ import { t } from '@apache-superset/core/translation'; import { SupersetClient } from '@superset-ui/core'; +import { + DEFAULT_SORT_COMPARATOR, + type LabeledValue, + stripSurroundingQuotes, +} from '@superset-ui/core/components'; import rison from 'rison'; +import { + SUBJECT_DETAIL_PROP, + SUBJECT_OPTION_FILTER_PROPS, + SUBJECT_TEXT_LABEL_PROP, + SubjectSelectLabel, +} from 'src/features/subjects/SubjectSelectLabel'; import { getUserDisplayLabel } from 'src/features/users/utils'; import { FormValues } from './types'; +type UserRecord = { id: number; email?: string } & Parameters< + typeof getUserDisplayLabel +>[0]; + +/** + * Option props AsyncSelect's client-side re-filter should match against for + * user options: the display name, the email and the username. These mirror + * the columns the server-side `user_all_text` filter searches, so a user the + * server matched is never hidden by the picker. + */ +export const USER_OPTION_FILTER_PROPS = [ + ...SUBJECT_OPTION_FILTER_PROPS, + 'username', +]; + +/** + * Build a select option for a user picker. + * + * The option shows the email under the display name, like the subject-based + * owner pickers do, so that a server-side match on email is visible to the + * person searching. Plain-text copies of both lines, plus the username, are + * attached under the `USER_OPTION_FILTER_PROPS` keys so AsyncSelect's + * client-side re-filter can match them (a React label is otherwise filtered + * as an empty string), and the display name under `title` so ListView can + * serialize the chosen option's label into the URL. The display name is also + * the option's `aria-label`, since the virtualized option list only derives + * one from a string label and would otherwise announce the numeric user id. + * The username is not rendered, so a username match surfaces the user + * without explaining why. + */ +export const toUserSelectOption = (user: UserRecord) => { + const textLabel = getUserDisplayLabel(user); + return { + value: user.id, + label: SubjectSelectLabel({ label: textLabel, secondaryLabel: user.email }), + title: textLabel, + 'aria-label': textLabel, Review Comment: Picking from the list is covered now, but pasting a name is still a path that skips it. `getPastedTextValue` finds the full option from the fetched page and then returns only `{ label, value }`, and `onPaste` sets that straight into the selected value without going through `handleOnSelect`, so `pickedOptionsRef` never sees it. Paste an off-page user's name into the Users picker, clear the search, and the rebuilt option again has no `aria-label`, `title` or filter props, so it is announced by id and drops out of an email search. Could the paste path record the resolved option as well? -- 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]
