-
Notifications
You must be signed in to change notification settings - Fork 3.5k
fix: attendee field doesn't show the assigned person #81483
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Code Review SummaryI've reviewed PR #81483. Here are my findings:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca87f6b725
ℹ️ 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".
src/libs/TransactionUtils/index.ts
Outdated
| const [creatorDetails] = getPersonalDetailsByIDs({accountIDs: [creatorAccountID], currentUserAccountID: currentUserPersonalDetails?.accountID}); | ||
| const creatorEmail = creatorDetails?.login ?? ''; | ||
| const creatorEmail = creatorDetails?.login ?? creatorDetails?.displayName ?? ''; | ||
| const creatorDisplayName = creatorDetails?.displayName ?? creatorEmail; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle empty login strings when building attendee email
When creatorDetails.login is an empty string (the case described in the PR), the ?? operator will not fall back, so creatorEmail remains '' and the function still returns no attendee. That means the assigned person will continue to be missing in the attendee field for that scenario. If the intent is to treat empty string as missing, this needs an explicit empty-string check (e.g., ||) instead of nullish coalescing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Done ✅ |
|
@NJ-2020 We have lint issues |
|
Done |
trjExpensify
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks to be addressing a follow-up from a PR @ikevin127 and @JS00001 were working on to introduce category required attendees. 👍
|
@NJ-2020 We have spell-check issue, could you help check? |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-02-11.at.16.55.15.movAndroid: mWeb ChromeScreen.Recording.2026-02-11.at.16.56.52.moviOS: HybridAppScreen.Recording.2026-02-11.at.16.55.47.moviOS: mWeb SafariScreen.Recording.2026-02-11.at.16.56.29.movMacOS: Chrome / SafariScreen.Recording.2026-02-11.at.16.51.03.mov |
|
🚧 @JS00001 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. |
Explanation of Change
Fallback
creatorDetails?.logintocreatorDetails?.displayNameto handle empty stringFixed Issues
$ #80687
PROPOSAL: #80687 (comment)
Tests
Preconditions: Rules are enabled > Enable Attendee tracking
Offline tests
Same as Tests
QA Steps
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
Screen.Recording.2026-02-06.at.17.41.15.mov
Android: mWeb Chrome
Screen.Recording.2026-02-06.at.17.33.17.mov
iOS: Native
Screen.Recording.2026-02-06.at.17.17.57.mov
iOS: mWeb Safari
Screen.Recording.2026-02-06.at.17.27.48.mov
MacOS: Chrome / Safari
Screen.Recording.2026-02-06.at.17.25.03.mov