-
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-10-25] [$500] IOU - Request money is not successful after refreshing in confirm page #28347
Comments
ProposalPlease re-state the problem that we are trying to solve in this issue.Request money is not successful after refreshing in confirm page What is the root cause of that problem?
In the web, we are creating a new Object URL to save the file. But when we reload the page that object URL is removed (reference for this point: https://stackoverflow.com/questions/13966186/how-long-does-a-blob-persist) What changes do you think we should make in order to solve the problem?Let's see another place where we also use a new Object URL to save the file Screen.Recording.2023-09-15.at.17.43.22.movIn adding an attachment, when reloading we hide the selected attachment modal. So, I suggest that we check if the url Object is removed, we will redirect to ReceiptSelector page. To do that, in here
Add this code to check objectURL is valid or not
And then in here
we only need to check receiptState and request money
What alternative solutions did you explore? (Optional)We can save uploaded files to localStorage |
Triggered auto assignment to @johncschuster ( |
Job added to Upwork: https://www.upwork.com/jobs/~011e5061c23f7df911 |
Bug0 Triage Checklist (Main S/O)
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr ( |
@DylanDylann Does your alternative solution use Onyx to save the URL object? The better fix should make the submit still processable. |
@mollfpr I can't reproduce this bug when reloading the App redirects to the initial page. Do you want to fix as suggestion here
|
@DylanDylann The expected result should keep the user on the same page after reloading and the submit request is successful. From your RCA the issue is because the object URL is removed after reloading, so I'm thinking your alternative solution is to keep the object URL and set it back. |
We need to wait for this reverted PR #28410 to be merged. So we can move forward this bug |
@DylanDylann Where's the revert issue? |
@mollfpr We had a blocker that blocked this issue on the main yesterday. But now It is resolved. Please ignore the above comment |
To do that, I have 2 suggestions:
I understand the benefit from your viewpoint But both ways are complicated. I see It's a bit overkill for this issue. Note that
Anyways, Let's confirm the expected before moving forward. @shawnborton Please help to give some thought from UX perspective on this issue. |
Hmm I am a bit confused here about this one. @alitoshmatov since you reported this one, can you walk me through when exactly you think a user would experience this? I am having a hard time understanding why a user would refresh the app after they upload a receipt. |
@mollfpr Could you check this comment? |
Thanks @DylanDylann. It seems reasonable to redirect the user back. @luacmartins I think you already agree with the expected result, and the proposal looks good! 🎀 👀 🎀 C+ reviewed! |
Current assignee @luacmartins is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new. |
This might need to put on hold for #26538 |
Thanks for the link @aimane-chnaif. We'll refactor navigation for multi-step flows, but I still think the changes to readFileAsync mentioned in this proposal are good and shouldn't overlap with that refactor, so I think we can move on with this issue. |
@mollfpr The PR is ready for review |
Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:
On to the next one 🚀 |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.86-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 2023-10-25. 🎊 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:
As a reminder, here are the bonuses/penalties that should be applied for any External 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:
|
Waiting to issue payment on |
https://github.com/Expensify/App/pull/23770/files#r1371795681
The regression step should be enough.
Only testable in Web, mWeb, and Desktop.
@johncschuster Could you give us the payment summary? I'll request the payment in NewDot, thanks! |
Thanks for filling the checklist. 👍 to the regression tests |
@johncschuster Could you give us the payment summary? Thank you! |
@johncschuster, @luacmartins, @mollfpr, @DylanDylann Whoops! This issue is 2 days overdue. Let's get this updated quick! |
Just pending payment |
On it! |
Payment Summary:External issue reporter: @alitoshmatov - $50 - paid via Upwork |
Payment has been issued via Upwork |
$500 payment approved for @mollfpr 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!
Action Performed:
Expected Result:
Money should be requested and it should show scanning
Actual Result:
Nothing happens
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.74-2
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
Notes/Photos/Videos: Any additional supporting documentation
Screen.Recording.2023-09-27.at.14.36.23.mov
bandicam.2023-09-28.00-54-33-234.mp4
Expensify/Expensify Issue URL:
Issue reported by: @alitoshmatov
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694717028781839
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: