sfirke commented on code in PR #45591:
URL: https://github.com/apache/superset/pull/45591#discussion_r4380344042
##########
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:
Thanks, this is the third path where a selected value lost its full option,
so I've fixed the cause instead of adding a fourth special case. `AsyncSelect`
now routes every fetch through one helper that remembers each option returned,
keyed by value. When a selected value has to be rebuilt because it's missing
from the loaded options, the remembered option fills in its `aria-label`,
`title` and filter props. Clicking, pasting, tokenizing and preset values all
resolve to options that came from a fetch, so they're all covered without
per-path bookkeeping, and the click-path recording from the last round is gone.
The new test pastes an option with a React label that isn't on the loaded
page, and asserts it's named by display name and not by id. With the memory
disabled, both that test and the picked-from-search test fail. Pushed in
43757b983c.
---
🤖 _Drafted by Claude Code, reviewed and approved by @sfirke._
##########
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:
Agreed. Amy now has an email, and the test types `aadams@` and asserts she
stays in the list while the on-page user drops out. Mapping `group.users` back
to a plain `{ value, label }` fails it. Pushed in 43757b983c.
---
🤖 _Drafted by Claude Code, reviewed and approved by @sfirke._
--
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]
