-
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 2024-02-19] [$500] Distance - Receipt shows original amount and distance instead of TBD when editing distance offline #33362
Comments
Job added to Upwork: https://www.upwork.com/jobs/~01d635e8deae97ab55 |
Triggered auto assignment to @kevinksullivan ( |
Bug0 Triage Checklist (Main S/O)
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.The amount and distance are not showing TBD. The values are still showing the original value before edit. What is the root cause of that problem?We don't use the same condition to show TDB as we do in App/src/components/DistanceEReceipt.js Line 38 in 249096e
App/src/components/ReportActionItem/MoneyRequestView.js Lines 108 to 110 in 249096e
What changes do you think we should make in order to solve the problem?We should use the same condition to show the amount and merchant as we do in App/src/components/DistanceEReceipt.js Line 38 in 249096e
App/src/components/ReportActionItem/MoneyRequestView.js Lines 108 to 110 in 249096e
What alternative solutions did you explore? (Optional)NA |
ProposalPlease re-state the problem that we are trying to solve in this issue.Amount and Distance are not replaced with TBD in the eReceipt. What is the root cause of that problem?We currently show TBD only if the request has no initial amount: App/src/components/DistanceEReceipt.js Line 38 in fa87f8b
What changes do you think we should make in order to solve the problem?We should add a check for const formattedTransactionAmount = !transaction.isLoading && transactionAmount ? CurrencyUtils.convertToDisplayString(transactionAmount, transactionCurrency) : translate('common.tbd');
const formattedMerchant = transaction.isLoading ? transactionMerchant.replace(CONST.REGEX.FIRST_SPACE, translate('common.tbd')) : transactionMerchant; And use this App/src/components/DistanceEReceipt.js Line 77 in fa87f8b
What alternative solutions did you explore? (Optional)Instead of using This Lines 936 to 946 in 249096e
|
Hi @parasharrajat please review the proposal! |
Checking this in 2 hours. |
ProposalPlease re-state the problem that we are trying to solve in this issue.Distance - Receipt shows original amount and distance instead of TBD when editing distance offline What is the root cause of that problem?This issue was supposed to be part of PR #30232 but I was advised that it was out of scope. The root cause arises when the waypoints are changed and no condition or transaction flag is there to check whether the amount and distance have changed and are pending recalculation. The amount and merchant are not modified in the What changes do you think we should make in order to solve the problem?
Important Note |
Regarding the notes from the proposal above:
This should have been handled in #33060, but that was closed as not important at the moment. If it should still be fixed – the original issue should be reopened.
This should be handled in #28965, where the green receipt thumbnail will be replaced by an actual map. Besides these points, the rest is almost the same as in the first 2 proposals. |
@kevinksullivan, @parasharrajat Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
I will review this by tomorrow. |
@parasharrajat, would you know why we display the App/src/components/DistanceEReceipt.js Lines 67 to 70 in 7b836cf
|
Thank you @paultsimura for your feedback on my proposal. After reading #28965, I would like to ask you a question.
Depending on the user's internet connection, if the user changes the waypoints while the device is offline then the The default receipt will be displayed instead of the Is this correct? Thank you 😄 |
It's good to solve other inconsistencies but I will let @kevinksullivan decide those if they should be solved in this issue. @kevinksullivan There are some suggestions from #33362 (comment) but the are out of scope of the expected behaviour of this issue. Should we pursue those as well?
|
@paultsimura Can you please share the context where this was discussed? |
Sorry, I don't remember the exact issues as they were eventually closed (that's why we don't see I just remember the pro-points were:
|
@kevinksullivan Can you please confirm this #33362 (comment)? |
We need to discuss the above point before moving forward. I am one step away from finalizing the review. |
Hmm, I don't have much idea of this part. I will let know after some testing. Meanwhile, @Tony-MK Do you mind proposing the suggestions as problems following https://github.com/Expensify/App/blob/main/contributingGuides/HOW_TO_CREATE_A_PLAN.md#step-1-define-the-problem? Why should we make these changes? |
Thanks. The PR is ready for review: #34135 |
As I mentioned here, we need to update the regression tests for offline distance requests to match up with this change. @kevinksullivan would you please help out with that? The new expected results can be found in the QA/test steps on the PR. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.39-8 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 2024-02-19. 🎊 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:
|
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. |
False alert – I used this issue as a reference. |
I think the issue is eligible for a bonus due to the scope change on the PR. @paultsimura did great job in handling and managing the discussion till the end. |
@amyevans, @paultsimura, @kevinksullivan, @parasharrajat Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
Payment summary:
|
Payment requested as per #33362 (comment) |
$500 approved for @parasharrajat |
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: v1.4.14-2
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:
Precondition: There is a distance expense request in the workspace chat.
Expected Result:
The amount and distance will show TBD.
Actual Result:
The amount and distance are not showing TBD. The values are still showing the original value before edit.
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6321335_1703081373259.bandicam_2023-12-15_03-44-39-835.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @kevinksullivanThe text was updated successfully, but these errors were encountered: