sadpandajoe commented on code in PR #45591:
URL: https://github.com/apache/superset/pull/45591#discussion_r4367707715
##########
superset-frontend/src/features/groups/utils.ts:
##########
@@ -18,10 +18,75 @@
*/
import { t } from '@apache-superset/core/translation';
import { SupersetClient } from '@superset-ui/core';
+import {
+ DEFAULT_SORT_COMPARATOR,
+ type LabeledValue,
+} 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 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
}),
Review Comment:
Making `label` a React element also breaks the Select's string-label paths
beyond paste. Typing a loaded name and then a comma matches against string
labels, so "Jane Smith," clears the input without selecting Jane (the new title
fallback only covers clipboard paste, and `reconcileTokensRef` returns early
when `allowNewOptions` is false). The virtualized option list also builds each
option's `aria-label` only from a string label, so a screen reader announces
the numeric user id instead of the name. Would an explicit `aria-label` on the
option, plus a title-aware token match for these pickers, keep both working?
##########
superset-frontend/src/features/groups/utils.ts:
##########
@@ -47,8 +112,13 @@ export const fetchUserOptions = async (
pageSize: number,
addDangerToast: (msg: string) => void,
) => {
- const query = rison.encode({
- filter: filterValue,
+ // The ListView filter path passes the raw input, so trim here to keep both
+ // callers from sending a whitespace-only search.
+ const searchTerm = filterValue.trim();
Review Comment:
`searchTerm` is sent to the server with any surrounding double quotes, but
AsyncSelect's client filter strips them (`stripSurroundingQuotes`). With more
than one page of users, typing `"Jane Sm"` first shows Jane from the
already-loaded options, then the server search for the literal `"Jane Sm"`
returns nothing and replaces the list with an empty result. Should the picker
strip surrounding quotes before building the `user_all_text` value so both
sides agree?
--
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]
