sadpandajoe commented on code in PR #45591:
URL: https://github.com/apache/superset/pull/45591#discussion_r4368809579
##########
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:
Both `roleUsers` mappings now go through `toUserSelectOption`, but no test
reaches them: `RoleFormItems.test.tsx` feeds already-built
`toUserSelectOption(...)` values straight into a Form, and the existing
`RoleListEditModal.test.tsx` only checks displayed names and submitted ids.
Example: reverting lines 234 and 319 to `{ value: user.id, label:
getUserDisplayLabel(user) }` still passes every test, yet an off-page role
member would lose its email/username filter props and its name-based option
announcement when the edit modal opens. Could a case open the real edit modal
with a role member outside the mocked first page and assert the option is named
by display name and still matches an email search?
##########
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:
The spread covers values supplied up front, but a user picked from search
results is still stored as the bare `{ label, value }` that `handleOnSelect`
receives. Example: with more than one page of users, search an off-page name,
select them, and let auto-clear restore the base options. The selected entry is
rebuilt by the same fallback from `selectValue`, so it again has no
`aria-label`, `title`, or filter props, and the option is announced by numeric
id and drops out of an email search. The new tests only seed initial values.
Could the selection path merge the chosen option's extra props (or look them up
from the options just fetched), with a test that selects an off-page user and
then clears the search?
--
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]
