Skip to content
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 #33518] [$500] Scan - Confirmation page resets to empty field when previous scan request creation fails #35064

Closed
6 tasks done
lanitochka17 opened this issue Jan 24, 2024 · 34 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@lanitochka17
Copy link

lanitochka17 commented Jan 24, 2024

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.4.31-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:

  1. Go to workspace chat
  2. Create a scan request with an invalid file that will result in an error. (use corrupted pdf attached)
  3. Create another scan request and reach the confirmation page

Expected Result:

will not change when the previous scan request creation fails

Actual Result:

When the first request creation results in error, the confirmation page for the second request is reset to empty fields

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6353338_1706109563449.1000009575.mp4

default-file-icon (9)

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01c62efac50e9080f0
  • Upwork Job ID: 1750180736709853184
  • Last Price Increase: 2024-02-14
@lanitochka17 lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 24, 2024
@melvin-bot melvin-bot bot changed the title Scan - Confirmation page resets to empty field when previous scan request creation fails [$500] Scan - Confirmation page resets to empty field when previous scan request creation fails Jan 24, 2024
Copy link

melvin-bot bot commented Jan 24, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01c62efac50e9080f0

Copy link

melvin-bot bot commented Jan 24, 2024

Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 24, 2024
Copy link

melvin-bot bot commented Jan 24, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @abdulrahuman5196 (External)

@lanitochka17
Copy link
Author

We think that this bug might be related to #wave5.
CC @dylanexpensify

@bernhardoj
Copy link
Contributor

bernhardoj commented Jan 24, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

The request confirmation info becomes empty when there is a failed scan request.

What is the root cause of that problem?

The money request create page depends on transaction draft onyx. When there is a request money fails, it will clear the transaction draft onyx.

App/src/libs/actions/IOU.js

Lines 539 to 543 in a8a3a4c

{
onyxMethod: Onyx.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${CONST.IOU.OPTIMISTIC_TRANSACTION_ID}`,
value: null,
},

What changes do you think we should make in order to solve the problem?

Don't clear the transaction draft onyx when fails because we already did it in optimistic data.

this issue most likely happen on split bill too

@melvin-bot melvin-bot bot added the Overdue label Jan 29, 2024
@abdulrahuman5196
Copy link
Contributor

abdulrahuman5196 commented Jan 29, 2024

Will be working on proposal review today.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jan 29, 2024
Copy link

melvin-bot bot commented Jan 31, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@abdulrahuman5196
Copy link
Contributor

Still pending review. Will close out before tomorrow.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jan 31, 2024
@abdulrahuman5196
Copy link
Contributor

Still pending review. Will close out before tomorrow.

Still working on review. Will check or re-assign accordingly

@melvin-bot melvin-bot bot removed the Overdue label Feb 5, 2024
Copy link

melvin-bot bot commented Feb 7, 2024

@JmillsExpensify @abdulrahuman5196 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@melvin-bot melvin-bot bot added the Overdue label Feb 7, 2024
Copy link

melvin-bot bot commented Feb 7, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@JmillsExpensify
Copy link

@abdulrahuman5196 Any update on your review?

@melvin-bot melvin-bot bot removed the Overdue label Feb 7, 2024
@abdulrahuman5196
Copy link
Contributor

Hi, Sorry for the delay. Will take care of this in my morning or will re-assign tomorrow night.

@abdulrahuman5196
Copy link
Contributor

Reviewing now

@abdulrahuman5196
Copy link
Contributor

Hi @JmillsExpensify , the root cause is same as this issue - #33518. And would be fixed if we fix that issue.

we can put this issue on hold for the same.

Copy link

melvin-bot bot commented Feb 14, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@JmillsExpensify
Copy link

Adding a hold per the above comment.

@melvin-bot melvin-bot bot removed the Overdue label Feb 14, 2024
@JmillsExpensify JmillsExpensify changed the title [$500] Scan - Confirmation page resets to empty field when previous scan request creation fails [HOLD #33518] [$500] Scan - Confirmation page resets to empty field when previous scan request creation fails Feb 14, 2024
@abdulrahuman5196
Copy link
Contributor

@abdulrahuman5196 I see. Please correct me if I'm wrong, but I think it's not possible to have overlapping transaction drafts if we clear out the draft on optimistic data, even when creating multiple transactions while offline using OPTIMISTIC_TRANSACTION_ID.

Here is how it would run

1. Start a new money request

2. Confirm the money request

3. The draft of OPTIMISTIC_TRANSACTION_ID is cleared (optimistically)

4. Start a new money request with the same OPTIMISTIC_TRANSACTION_ID

5. The draft is new

If I understand correctly, OPTIMISTIC_TRANSACTION_ID is only being used when creating a new money request.

@bernhardoj I am not actually getting your point.

The key for storing in Onyx is still the same and whereever we use the draft data the same key is used. This causes collision since the same key will be used for the new draft request as well.

key: `${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${CONST.IOU.OPTIMISTIC_TRANSACTION_ID}`,

Onyx.set(`${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${newTransactionID}`, {

I hope this clears up the context. If you have some suggestions or proposal to fix the same. Could you kindly comment in the other linked issue?

@abdulrahuman5196
Copy link
Contributor

abdulrahuman5196 commented Feb 16, 2024

because we already did it in optimistic data.

Ah!! @bernhardoj I got your point. It seems in scan flow of this particular issue, we already delete the transaction data optimistically itself, so there is no point in deleting it again in failure data or success data.

But the same is not being done currently in optimistic data of split bill. We don't delete it optimistically as I checked, so if its not deleted we would be using that optimistic data at some place was my thought process. Let me check again if we can use #33518 (comment) proposal itself to solve the case similar to how scan flow is handled.

Copy link

melvin-bot bot commented Feb 20, 2024

@JmillsExpensify, @abdulrahuman5196 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@JmillsExpensify
Copy link

Still on hold for the linked issue.

Copy link

melvin-bot bot commented Feb 26, 2024

@JmillsExpensify, @abdulrahuman5196 Huh... This is 4 days overdue. Who can take care of this?

@abdulrahuman5196
Copy link
Contributor

The PR for the hold is in staging. @JmillsExpensify / @lanitochka17 Could you kindly re-test the issue in staging?

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Feb 27, 2024
Copy link

melvin-bot bot commented Mar 1, 2024

@JmillsExpensify, @abdulrahuman5196 Whoops! This issue is 2 days overdue. Let's get this updated quick!

Copy link

melvin-bot bot commented Mar 5, 2024

@JmillsExpensify, @abdulrahuman5196 Still overdue 6 days?! Let's take care of this!

@JmillsExpensify
Copy link

Still held on linked issue.

@melvin-bot melvin-bot bot removed the Overdue label Mar 6, 2024
@abdulrahuman5196
Copy link
Contributor

The linked issue changes are already in production.

@JmillsExpensify / @lanitochka17 Could you kindly re-test and close this issue, if its not reproducible?

@lanitochka17
Copy link
Author

Issue is not reproducible on the latest build 1.4.48-0

Screen_Recording_20240307_234924_New.Expensify.mp4

@melvin-bot melvin-bot bot added the Overdue label Mar 11, 2024
Copy link

melvin-bot bot commented Mar 12, 2024

@JmillsExpensify, @abdulrahuman5196 Eep! 4 days overdue now. Issues have feelings too...

@JmillsExpensify
Copy link

Looks like QA isn't able to reproduce this issue, so I'm going to close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests

4 participants