-
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-09] [$500] Manual request - Amount is not shown in the confirmation page after adding a receipt #29854
Comments
Triggered auto assignment to @joekaufmanexpensify ( |
Job added to Upwork: https://www.upwork.com/jobs/~01e9f9de38a831eccf |
Bug0 Triage Checklist (Main S/O)
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @ArekChr ( |
Proposalfrom: @cooldev900 Please re-state the problem that we are trying to solve in this issue.The amount doesn't show when getting back confirmation page after adding receipt. What is the root cause of that problem?The root cause is that props.receiptPath is set when we get back to confirmation page after adding the receipt. App/src/components/MoneyRequestConfirmationList.js Lines 316 to 317 in 9d79fd1
So props.receiptPath && isTypeRequest) || isDistanceRequestWithoutRoute turns to true and then text becomes only Request without amount.
What changes do you think we should make in order to solve the problem?We have to check if formattedAmount has value.
What alternative solutions did you explore? (Optional)N/A |
ProposalPlease re-state the problem that we are trying to solve in this issue.Amount is not shown in the confirmation page after adding a receipt. What is the root cause of that problem?App/src/components/MoneyRequestConfirmationList.js Lines 576 to 580 in 410a843
Amount is not shown because
The value of What changes do you think we should make in order to solve the problem?
I think we need to update So we can update the property like above code. What alternative solutions did you explore? (Optional)N/A |
ProposalPlease re-state the problem that we are trying to solve in this issueThere is no amount data shown in the confirmation's page Request button for Request Money - Manual request -> upon adding receipt from the more 3 dot menu. What is the root cause of that problem?App/src/components/MoneyRequestConfirmationList.js Lines 312 to 328 in 935e891
In the code above, the else if condition The else condition would actually return in this case The reason why the else if condition mentioned above does not return the
step which does not take place in the case of Request money -> Scan / Distance because in these 2 cases the amount is calculated / estimated from either the receipt itself and not shown on the Request button label or the waypoints distance which does show the amount on the Request button label. What changes do you think we should make in order to solve the problem?Carefully taking into account the 2 Request money -> Scan / Distance cases, my changes to solve this problem would be to change the else if condition from: - else if ((props.receiptPath && isTypeRequest) || isDistanceRequestWithoutRoute)
to
+ else if ((props.receiptPath && isTypeRequest && props.iouAmount === 0) || isDistanceRequestWithoutRoute) adding a check for the If there is an amount, which in the case of this issue is because it is required in the step 1 of the Manual request type, then the else if would not pass and we would return the else which will show label Result:MacOS: Chrome / SafariScreen.Recording.2023-10-18.at.19.08.29.mov |
Reproduced, and this def seems like a bug. @ArekChr thoughts on any of these proposals? |
ProposalPlease re-state the problem that we are trying to solve in this issue.Manual request - Amount is not shown in the confirmation page after adding a receipt What is the root cause of that problem?When we attach a receipt to a manual request, we update the property in the What changes do you think we should make in order to solve the problem?→ To fix the first issue of displaying the missing amount field after attaching the receipt, in the iou object the amount is already set, we can take advantage of that and update the props passed to in
→ For the button label, that is managed by if (isSplitBill && props.iouAmount === 0) {
text = translate('iou.split');
} else if(props.receiptPath && props.iouAmount !== 0) {
text = translate('iou.requestAmount', {amount: formattedAmount});
} else if ((props.receiptPath && isTypeRequest) || isDistanceRequestWithoutRoute) {
text = translate('iou.request');
} else {
const translationKey = isSplitBill ? 'iou.splitAmount' : 'iou.requestAmount';
text = translate(translationKey, {amount: formattedAmount});
} What alternative solutions did you explore? (Optional)N/A |
Pending review of proposals |
@cooldev900 Understood the root cause first and his proposal solves the problem effectively. 🎀 👀 🎀 C+ reviewed |
Triggered auto assignment to @bondydaa, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@ArekChr The solution I offered addresses both problems: it rectifies the display of the amount field and ensures the corresponding update to the CTA button |
@ArekChr The reason I posted my proposal was because I tested @cooldev900's first and if merged with the changes he proposed, it would cause a regression in the following scenario:
As mentioned in my proposal, my changes account for Scan / Distance requests as to not create any regressions, plus it's cleaner code wise because it doesn't nest if within else if. |
how about this? @ikevin127 |
Well, that's for you and the C+ to thoroughly test and review said proposal in the future before deciding to post or go with said proposal 🙂
I take that back, seems I wasn't aware of some unwritten rules like:
I still support my proposal for the reasons I mentioned in my last comment. Edit: |
Also the chosen proposal in lacking the part where we need to show the Amount field in the form.
The expected results clearly state the presence of both the field and the amount in the CTA |
@bondydaa @joekaufmanexpensify |
@ikevin127, Thanks for pointing this out and for the Slack discussion link. I've already chosen @cooldev900's proposal earlier, but I see now that I might have missed a potential regression you caught. I'll pay close attention to this during the code review and also when reviewing the next proposals, Thanks and apologise for the inattention. |
@ArekChr Sorry for the way I approached this, I was a bit to harsh. I wasn't aware of some of the unwritten rules when it comes to proposals and the way they are selected by C+, as somebody mentioned in the Slack discussion:
I had a similar situation with this one in another issue, with me being the first to propose and the second proposal had minor adjustments compared to mine and the 2nd got selected, hence my confusion in this case. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.94-2 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-09. 🎊 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:
|
@ArekChr could you take care of BZ checklist this week so we can prep to issue payments? TY! |
Regression Test Proposal
Do we agree 👍 or 👎 |
BZ checklist complete! |
All set to issue payment. Since @ArekChr is paid separately, the only payment due here is $500 to @cooldev900, paid via Upwork. Will pay this tomorrow! |
@cooldev900 offer sent for $500! |
@joekaufmanexpensify |
@cooldev900 $500 sent and contract ended! |
All set. 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!
Issue found when executing PR: #29474
Version Number: v1.3.86-1
Reproducible in staging?: Y
Reproducible in production?: Y
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 amount field and amount entered should be visible in the confirmation page.
Actual Result:
There is no any data about amount requested in the confirmation page.
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
Bug6241500_1697620412215.20231018_101618.mp4
MacOS: Desktop
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: