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 for payment 2023-12-08] [$1000] IOU - Tapping download on manual money request receipt app crashes #29788

Closed
1 of 6 tasks
lanitochka17 opened this issue Oct 17, 2023 · 73 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff

Comments

@lanitochka17
Copy link

lanitochka17 commented Oct 17, 2023

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.85-0

Reproducible in staging?: Yes

Reproducible in production?: No

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. Launch app
  2. Tap fab
  3. Tap request money-- Manual
  4. Enter an amount
  5. Tap next
  6. Select a Workspace
  7. Tap 3 dots and add the below attached receipt or any image by clicking " choose a document"
  8. Tap request
  9. Tap on the IOU
  10. Tap on the receipt
  11. Tap on 3 dots
  12. Tap download

Expected Result:

When user taps download in receipt page, receipt must be downloaded

Actual Result:

When user taps download in receipt page, app crashes

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

Android: Native
Bug6240242_1697536635359.steps_crash.1.mp4

Bug6240242_1697536533241!w_32904c599d480e967b4c3bba8e072b06c866bb21-2023-10-17_09_38_19 470-2023-10-17_09_41_03 755 (2)

logs 3.txt

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01728d6678a4828e28
  • Upwork Job ID: 1714368001279598592
  • Last Price Increase: 2023-10-30
  • Automatic offers:
    • situchan | Contributor | 27247568
    • b4s36t4 | Contributor | 27652633
@lanitochka17 lanitochka17 added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 17, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 17, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Oct 17, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@melvin-bot
Copy link

melvin-bot bot commented Oct 17, 2023

Triggered auto assignment to @marcochavezf (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@marcochavezf
Copy link
Contributor

Posted in wave4 in case someone has more context about the issue

@pradeepmdk
Copy link
Contributor

pradeepmdk commented Oct 17, 2023

Proposal

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

Tapping download on manual money request receipt app crash

What is the root cause of that problem?

Screenshot 2023-10-17 at 9 51 35 PM

const isLocalFile = url.startsWith('file://');

local file referring to the file:/ so isLocalFile is false so its pointing the HTTP URL so that its crashing

this plugin https://www.npmjs.com/package/react-native-document-picker return this format only in fileCopyUri

[{"fileCopyUri": "file:/data/user/0/com.expensify.chat.dev/cache/676d92f0-d911-4f36-b514-09171f701681/w_1616fc868aa6834d5082e20f9b40b9a667511f8f-2023-10-17%2016_18_46.350.jpg", "name": "w_1616fc868aa6834d5082e20f9b40b9a667511f8f-2023-10-17 16_18_46.350.jpg", "size": 932381, "type": "image/jpeg", "uri": "content://com.android.providers.media.documents/document/image%3A1000045261"}]

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

we can check with const isLocalFile = url.startsWith('file:/');

or here

uri: fileData.fileCopyUri || fileData.uri,

we should change file:/ -> file:// in fileCopyUri

and here https://github.com/Expensify/App/blob/5ad33724f2926f1b687bbac2b3d8fcfced5eb997/src/components/AttachmentModal.js#L185C32-L185C32

we need check const isLocalFile = url.startsWith('file:/'); and source is number we should ingore isAuthTokenRequired .

@francoisl
Copy link
Contributor

@pradeepmdk do you see any recent changes/pull request that could have introduced this crash? All the code you linked to doesn't seem to have changed in a few months, so I'm curious why we're only seeing this now. Or maybe it's because we just started using AttachmentPicker in a new component.

Anyway, thanks for the proposal, I'll run some tests to see if it fixes the crash.

@situchan
Copy link
Contributor

For deploy blocker issues, offending PR should be pointed out in the root cause of proposal.

@b4s36t4
Copy link
Contributor

b4s36t4 commented Oct 17, 2023

I just have updated the app from playstore and issue is also there (v1.3.84-10)means this is not a deploy blocker :)

@situchan
Copy link
Contributor

@lanitochka17 can you please confirm that this is not reproducible on production?

@b4s36t4
Copy link
Contributor

b4s36t4 commented Oct 17, 2023

Proposal

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

IOU - Tapping download on manual money request receipt app crashes

What is the root cause of that problem?

This is an issue with upstream.

Maybe due to slow-network or some other reason the receipt image is pointed to local which causing the issue.

The RCA for the Crash is correct provided by pradeepmdk i.e accessing file:/ with only one leading slash.

But the real RCA is from the upstream. As per the documentation when we pass copyTo option the library tries to copy the file to temp folder and return the path of the copied file in which the return value from the copy function in upstream is returning file: type URI.

https://docs.oracle.com/javase%2F7%2Fdocs%2Fapi%2F%2F/java/io/File.html#toURI()

react-native-documents/document-picker#527

https://github.com/rnmods/react-native-document-picker/blob/e1000fd9a93ce3568b1d891917ceae064bed415a/android/src/main/java/com/reactnativedocumentpicker/DocumentPickerModule.java#L315

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

We can upgrade the package v8.2.1 which uses Uri from android API instead of Java File API

https://developer.android.com/reference/android/net/Uri#fromFile(java.io.File)

https://github.com/rnmods/react-native-document-picker/blob/5dd2e4063a9806581d99563dce48e1ce2b474264/android/src/main/java/com/reactnativedocumentpicker/DocumentPickerModule.java#L324

What alternative solutions did you explore? (Optional)

NA

@b4s36t4
Copy link
Contributor

b4s36t4 commented Oct 17, 2023

I think the issue exists before but we were not able to catch this because we added scan/preview/ menu options feature recently and also I think because of the file-size or some other reason the API request hasn't been completed fully by the time when we open modal so thus it is pointing the image to local path which let the team to catch this issue.

@marcochavezf
Copy link
Contributor

I haven't been able to corroborate this issue in production but potentially is not a blocker as @b4s36t4 stated above. Since there are some proposals I will make it external to assign @situchan as C+ (already DM'ed) to move forward.

@marcochavezf marcochavezf added the External Added to denote the issue can be worked on by a contributor label Oct 17, 2023
@melvin-bot melvin-bot bot changed the title IOU - Tapping download on manual money request receipt app crashes [$500] IOU - Tapping download on manual money request receipt app crashes Oct 17, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 17, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01728d6678a4828e28

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 17, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 17, 2023

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

@melvin-bot melvin-bot bot added Internal Requires API changes or must be handled by Expensify staff and removed External Added to denote the issue can be worked on by a contributor labels Nov 14, 2023
Copy link

melvin-bot bot commented Nov 14, 2023

Current assignee @situchan is eligible for the Internal assigner, not assigning anyone new.

@b4s36t4
Copy link
Contributor

b4s36t4 commented Nov 17, 2023

@situchan PR is up #31499.

@dylanexpensify
Copy link
Contributor

bump @situchan ^^

@b4s36t4
Copy link
Contributor

b4s36t4 commented Nov 22, 2023

@dylanexpensify @situchan Asked for a review in the PR, I updated it only yesterday. He might check it today.

@isabelastisser
Copy link
Contributor

@situchan what's the update here?

@situchan
Copy link
Contributor

PR is in review

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Dec 1, 2023
@melvin-bot melvin-bot bot changed the title [$1000] IOU - Tapping download on manual money request receipt app crashes [HOLD for payment 2023-12-08] [$1000] IOU - Tapping download on manual money request receipt app crashes Dec 1, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Dec 1, 2023
Copy link

melvin-bot bot commented Dec 1, 2023

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Dec 1, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.6-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-12-08. 🎊

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.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Dec 1, 2023

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:

  • [@situchan] The PR that introduced the bug has been identified. Link to the PR:
  • [@situchan] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@situchan] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@situchan] Determine if we should create a regression test for this bug.
  • [@situchan] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@isabelastisser / @dylanexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@dylanexpensify
Copy link
Contributor

Payment upcoming!

@isabelastisser isabelastisser added Daily KSv2 and removed Weekly KSv2 labels Dec 5, 2023
@melvin-bot melvin-bot bot added Daily KSv2 and removed Daily KSv2 labels Dec 8, 2023
@isabelastisser
Copy link
Contributor

@situchan please complete the checklist above. Thanks!

@situchan
Copy link
Contributor

situchan commented Dec 8, 2023

The bug always existed after we enable local file download. Was issue in upstream.
This was caught by QA team so no need another regression test

@isabelastisser
Copy link
Contributor

The payments were made in Upwork. All set!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff
Projects
None yet
Development

No branches or pull requests