fix: Attendees are not copied in alphabetical order when they are arranged alphabetically#89175
Conversation
|
@nyomanjyotisa Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da3a27bc02
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c0c801dcd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
trjExpensify
left a comment
There was a problem hiding this comment.
Attendee pills rollout follow-up. 👍
|
Also, could you fix this issue where we still shows MacOS-Chrome.mp4 |
…ill-workflow-attendees # Conflicts: # src/libs/Notification/LocalNotification/BrowserNotifications.ts # src/libs/OptionsListUtils/index.ts # src/libs/ReportNameUtils.ts # src/libs/ReportUtils.ts
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid-Native.mp4Android: mWeb ChromeAndroid-mWeb.Chrome.mp4iOS: HybridAppiOS-Native.mp4iOS: mWeb SafariiOS-mWeb.Safari.mp4MacOS: Chrome / SafariMacOS-Chrome.mp4 |
|
🚧 @Gonals has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/Gonals in version: 9.3.67-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. This is a bug fix ensuring attendees display in consistent alphabetical order across all views (merge details page, hover-copy, accessibility labels). The changes are purely code-level sorting logic and don't introduce new features, change UI labels, or modify user-facing workflows. The existing help site articles about attendees don't reference sort order, so no documentation updates are needed. |
Explanation of Change
On the merged-expense flow, attendees rendered as sorted pills but the hover-Copy button and the merge Select details page returned them in insertion order. Pills also stripped
@expensify.smswhile these other paths leaked it.Sort once at the data layer; let consumers opt in.
getAttendeesListDisplayString(src/libs/TransactionUtils): accepts an optionallocaleCompare(sorts when passed, preserves insertion order otherwise) and stripsStr.removeSMSDomainfrom each name + the sort key. Fixes the merge details page and the hover-Copy value. Sort is case-insensitive to matchsortAlphabetically(the pill sort).enrichAndSortAttendees(new helper insrc/libs/AttendeeUtils): enriches attendees with livepersonalDetailsand sorts once.MoneyRequestViewandAttendeeFieldcall it at the top; the same canonical list feeds pills, hover Copy, accessibility label, and the missing-attendees violation check.localeComparefromuseLocalize(DetailsReviewPage→getDisplayValue/buildMergeFieldsData). Non-React paths (ModifiedExpenseMessage→ 5 callers) intentionally skip sorting — no custom collator in utils, no threading ripple.actualAttendees.map(...).join(', ')/iouAttendees?.map(...).join(', ')expressions inMoneyRequestViewandAttendeeFieldwith calls togetAttendeesListDisplayString.Fixed Issues
$ #89130
PROPOSAL:
Tests
Setup (precondition):
A. Hover-Copy on merge confirm page (deploy blocker repro)
Offline tests
Same as tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Macbook-Chrome.mp4