-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[HOLD for payment 2023-11-13] [$500] Chat - After creating a money request, email are first displayed, then changed to set name #30456
Comments
Job added to Upwork: https://www.upwork.com/jobs/~01ef7eaea70e5f02e1 |
Triggered auto assignment to @conorpendergrast ( |
Bug0 Triage Checklist (Main S/O)
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @s77rt ( |
I am checking this issue 👀 |
Proposalfrom: @cooldev900 Please re-state the problem that we are trying to solve in this issue.After creating a money request, email are first displayed, then changed to set name in LHN. What is the root cause of that problem?The root cause is because it returns displayName at the first time after creating a money request instead of firstname and lastname. Lines 1175 to 1179 in cb5aeb1
Lines 1189 to 1197 in cb5aeb1
What changes do you think we should make in order to solve the problem?So we need to check if the personalDetail includes firstName and lastNae and if it doesn't include, we can return displayName.
What alternative solutions did you explore? (Optional)If we don't want to change getDisplayNameForParticipant function, the backend should return correct displayName that include firstName and lastName in personalDetail. And we need to sync personalDetails data to backend when selecting participants in requesting money and also other user selection cases. ResultVideo_2023-10-26_215420-mac-chrome_20231026215533.mp4 |
@cooldev900 Thanks for the proposal. I don't think your RCA is correct. From what I can tell and based on the attached screenshot, we re-creating the personalDetails item object even though we have it. And by doing so we are overwriting the displayName field. Can you please recheck if that's the case? |
@s77rt |
@cooldev900 That's not what I meant. This is the flow:
The bug is on step 2, which means that the bug is on the frontend and not the backend.. |
oh, let me re-check now. |
It is personalDetails item before requesting money.
It is one right after requesting money.
We are overwriting displayName twice after requesting money. |
@cooldev900 Can you please update your proposal fixing this issue? |
Yes, I am doing. |
Proposalfrom: @cooldev900 Please re-state the problem that we are trying to solve in this issue.After creating a money request, email are first displayed, then changed to set name in LHN. What is the root cause of that problem?The root cause is because we didn't set displayName in participant object so that displayName is set to payerEmail in optimisticPersonalDetailListAction. Line 561 in cb5aeb1
App/src/libs/OptionsListUtils.js Lines 199 to 224 in cb5aeb1
App/src/libs/OptionsListUtils.js Lines 571 to 590 in cb5aeb1
What changes do you think we should make in order to solve the problem?So we need to create displayName property in both getPolicyExpenseReportOption and getParticipantsOption.
What alternative solutions did you explore? (Optional)We could change displayName to text here.
|
Same issue as #25927 which gets closed, so I copied my proposal from there. ProposalPlease re-state the problem that we are trying to solve in this issue.When we request money with a user we never chatted before with a display name but we have their contact (personal detail), their display name will be changed to their login. I can only reproduce this case on a high-traffic account where we have other user accounts without ever chatting with them. What is the root cause of that problem?When we make a money request, we check if we should create an optimistic personal detail. Lines 559 to 570 in a59d56f
What changes do you think we should make in order to solve the problem?I don't know if there will be a real case where we have the user's personal details but not their report/chat. Instead of checking whether we have a report/chat with the user, we can check if we already have the user's personal details.
Just like what we did in IOU.startSplitBill Lines 1471 to 1486 in a59d56f
|
@cooldev900 Thanks for the update. I still don't think your RCA is correct. The root cause is the displayName being overwritten in the first place. For future proposal please don't write a new comment but instead update the existing one. |
@bernhardoj Thanks for the proposal. Your RCA is correct. The solution looks good to me but we can keep the *The other issue was fixed by #29279 🎀 👀 🎀 C+ reviewed |
Triggered auto assignment to @grgia, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
📣 @s77rt 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
Thanks for the proposal @bernhardoj, all yours! |
PR is ready cc: @s77rt |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.95-9 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2023-11-13. 🎊 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
Regression Test Proposal
|
Payouts due:Issue Reporter: N/A - Applause
Upwork job is here. |
Regression test request created: https://github.com/Expensify/Expensify/issues/336527 All done, thanks everyone! |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 1.3.91-6
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
Action Performed:
Expected Result:
The immediately installed username is displayed at the top of the page
Actual Result:
At the top of the page, first, the email is displayed, and then it changes to the set username
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Bug6252081_1698337680025.Recording__6558.mp4
MacOS: Desktop
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: