sfirke commented on code in PR #45591:
URL: https://github.com/apache/superset/pull/45591#discussion_r4379229615
##########
superset-frontend/src/features/roles/RoleListEditModal.tsx:
##########
@@ -231,10 +231,7 @@ function RoleListEditModal({
useEffect(() => {
if (!loadingRoleUsers && formRef.current) {
- const userOptions = roleUsers.map(user => ({
- value: user.id,
- label: getUserDisplayLabel(user),
- }));
+ const userOptions = roleUsers.map(toUserSelectOption);
Review Comment:
Good call, added in fb97802bef. The new `RoleListEditModal` test opens the
real edit modal with a role member (Amy Adams, `[email protected]`) who isn't
on the picker's first page. It asserts the option is announced as "Amy Adams"
rather than "42", then types `aadams@` and checks Amy still matches while the
on-page user drops out. Reverting both mappings to `{ value: user.id, label:
getUserDisplayLabel(user) }` fails it.
One note: the test really pins the line-234 effect. `roleUsers` is empty on
first render, so the `initialValues` mapping at line 319 never carries a user
when the form mounts. I kept it consistent anyway.
---
🤖 _Drafted by Claude Code, reviewed and approved by @sfirke._
##########
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:
Confirmed, thanks. `handleOnSelect` only receives `{ label, value }`, so a
user picked from search results lost their extra props as soon as the base page
came back. `AsyncSelect` now keeps the full option it's handed on select, keyed
by value, and the selected-value fallback merges it back in. That keeps the
`aria-label`, `title` and filter props. I kept this out of the selected value
itself so the `onChange` payload is unchanged for other `AsyncSelect` consumers.
The new `AsyncSelect` test searches, picks an option that isn't on the base
page with a React label, clears the search, and asserts the option is still
named by display name and not by id. It fails without the fix. Pushed in
fb97802bef.
---
🤖 _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]
