-
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-07][$500] Page refresh causes manual request with a receipt to be stuck #30884
Comments
Job added to Upwork: https://www.upwork.com/jobs/~01b422f44a99747ac7 |
Triggered auto assignment to @lschurr ( |
Bug0 Triage Checklist (Main S/O)
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Page refresh causes manual request with a receipt to be stuck What is the root cause of that problem?Firstly, Let's see this issue: #28347 (similar case with scan flow) We discussed about this problem before with scan request. But we missed a case: the manual request by adding a receipt Note that if we upload the receipt and back to the participant page and then reload, the same bug also happens. This bug is mentioned here What changes do you think we should make in order to solve the problem?In here Line 41 in e81ed8f
We should add a check if It is a manual request and the file is lost. The app will be on the confirmation page and the receipt thumbnail is removed
With the big refactor on the request flow, This is result sol.mp4What alternative solutions did you explore? (Optional)In here Line 41 in e81ed8f
We should add a check if It is a manual request and the file is lost. The app will redirect to the uploading screen
|
ProposalPlease re-state the problem that we are trying to solve in this issue.After adding a receipt to the manual request, page refresh causes manual request to be stuck What is the root cause of that problem?When we adding a receipt to the manual request, we create blob URL and set it to the iou App/src/pages/iou/ReceiptSelector/index.js Lines 135 to 136 in 7bd51eb
App/src/pages/iou/ReceiptSelector/index.js Lines 154 to 155 in 7bd51eb
But this object URL is only valid for the current session and will be revoked once the page is refreshed. So when navigating to the confirmation page, file would be null and it'll be redirected to the manual request page or previous page. App/src/pages/iou/steps/MoneyRequestConfirmPage.js Lines 106 to 109 in 7bd51eb
This is the root cause What changes do you think we should make in order to solve the problem?I don't think we need to go back to the manual request page when object URL is not valid. Update the above code to
Result30884.mp4What alternative solutions did you explore? (Optional) |
@s-alves10 Your proposal is the same as my alternative solution 😄 (same interesting idea) |
I actually reported this one and worked on a solution that I shared on Slack. Not sure why @m-natarajan didn't include it :) could you double-check again please :), and please check the Reported GitHub ID. that's not me it should be |
Proposal by @abdel-h66 ProposalPlease re-state the problem that we are trying to solve in this issue.When a receipt is added to a Manual request, refreshing the page will cause the manual request process to be stuck. What is the root cause of that problem?
What changes do you think we should make in order to solve the problem?→ In the const isLocalReceiptFile = receiptPath.startsWith('file://') || receiptPath.startsWith('blob:'); → if this file could not be read, then we reset the money request receipt with IOU.setMoneyRequestReceipt('', '') → the final solution will be something like this if (!file) {
if(isLocalReceiptFile) {
IOU.setMoneyRequestReceipt('', '')
return;
}
Navigation.goBack(ROUTES.MONEY_REQUEST.getRoute(iouType.current, reportID.current));
} else {
const receipt = file;
receipt.state = file && isManualRequestDM ? CONST.IOU.RECEIPT_STATE.OPEN : CONST.IOU.RECEIPT_STATE.SCANREADY;
setReceiptFile(receipt);
} What alternative solutions did you explore? (Optional)N/A |
@eVoloshchak could you take a look at the discussion on this GH? |
Bump @eVoloshchak |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
Bumped in Slack as well - https://expensify.slack.com/archives/C01GTK53T8Q/p1699738567654619 |
This has been resolved in #30832 |
Ok, great! Closing this one out. |
Could you weigh in here @eVoloshchak? |
Any update on the regression here @DylanDylann @eVoloshchak? |
@DylanDylann, let's proceed with the first option here, the second would require duplicating the |
@eVoloshchak this function is written by Typescript but currently, IOU.js file don't support typescript. I suggest that we should hold this until #24926 (migrate IOU.js to typescript) is done. cc @cristipaval |
@DylanDylann, let's just revert the function back to vanilla JS. Hold on #24926 will be removed on Jan 26, but it's a huge file, and a migration to TS will take time. Let's get rid of this warning, the function is pretty small, it won't be hard to convert it to TS again |
@eVoloshchak The PR is ready for review: #35127 |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.33-5 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-07. 🎊 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:
|
@eVoloshchak @DylanDylann - could you work through the new checklist for this one? |
Regression Test Proposal
Do we agree 👍 or 👎 |
This is ready for payment, @lschurr, could you post the payment summary please? |
Yep, I will review and post payment summary today. |
Payment summary:
|
Added new QA test request: https://github.com/Expensify/Expensify/issues/367651 Closing out this issue. |
$250 approved for @eVoloshchak based on summary above. |
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.95-5
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: @abdel-h66
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1698081033316989
Action Performed:
Expected Result:
We should be able to continue to the request information page
Actual Result:
The page is stuck and keeps coming back whenever Next is clicked
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Recording.198.mp4
Expensify.bugs.mp4
Monosnap.screencast.Oct.23.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: