Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.44k stars 2.81k forks source link

[HOLD for payment 2022-12-20] [$1000] iOS - Attachments - Uploading image throws error "Unsupported PDF format" #12283

Closed kbecciv closed 1 year ago

kbecciv commented 1 year ago

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:

  1. Go to Settings > Camera > Format > ensure that "high-efficiency" is enabled
  2. Open the iOS app
  3. Visit any DM
  4. Take a picture with the native iOS camera, ensuring that "live" photo is disabled
  5. Tap the grey plus icon to the right of the chat input
  6. Select a image from your camera roll (for iOS should be HEIC files by default)
  7. Upload the image

Expected Result:

The image should upload natively with no errors

Actual Result:

RBR error appears, with the message "Unsupported PDF format"

Workaround:

Unknown

Platform:

Where is this issue occurring?

Version Number: 1.2.21.4

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/93399543/198847076-65bd917b-150a-41cb-8da0-a5e5bb649a6a.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0156f41c36e242a9d2
  • Upwork Job ID: 1602341181291810816
  • Last Price Increase: 2022-12-12
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

mateocole commented 1 year ago

having engineering take a look

arosiclair commented 1 year ago

I don't have a physical iPhone to repro this, but this does look to be fixable by a contributor. Marking external.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @Gonals (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

dhairyasenjaliya commented 1 year ago

hello @JmillsExpensify I reported the bug here on slack but at that time didn't get it on GH issues https://expensify.slack.com/archives/C01GTK53T8Q/p1664269336925119

mmarceau commented 1 year ago

Proposal

The reportAction object has a couple of properties that can be used to determine state of an attachment.

  1. reportAction.errors - If this exists, there was an error uploading the file so the download option shouldn't appear.
  2. reportAction.pendingAction - If this exists, the file has a pending process, which can be used to determine if an upload is in flight.

My proposal is to change the following line: https://github.com/Expensify/App/blob/0724337138a4d80f6603074cc9af5c186cb524ec/src/pages/home/report/ContextMenu/ContextMenuActions.js#L46

to:

- return isAttachment && reportAction.reportActionID;
+ return isAttachment && reportAction.reportActionID && !reportAction.errors && !reportAction.pendingAction;

I've tested this out on an actual iOS device. Please see the capture videos below. Thanks!

https://user-images.githubusercontent.com/6327057/200099158-88695f3e-3830-4abb-9025-40c8346b9566.mp4

https://user-images.githubusercontent.com/6327057/200099326-781aa320-5255-4c23-a85a-d72a04650502.mov

JmillsExpensify commented 1 year ago

@dhairyasenjaliya Thanks for flagging. I commented in the linked issue immediately above this post.

Santhosh-Sellavel commented 1 year ago

@mmarceau Your proposal looks good, but this seems to be a duplicate.

Santhosh-Sellavel commented 1 year ago

@kbecciv

Try to upload unsupported file type and long type on the error message--> Download

Can you send a screen recording for this step and a file, please?

Because I get only this error Simulator Screen Shot - iPhone 14 - 2022-11-07 at 08 17 23

kbecciv commented 1 year ago

@Santhosh-Sellavel Please attached the video, you need to turn off live mode when taking a picture before uploading in NewDot app.

https://user-images.githubusercontent.com/93399543/200347442-09252155-cace-4fb5-b1d1-ec93b01fa217.mp4

JmillsExpensify commented 1 year ago

This issue still seems like a duplicate of the crash that we're also discussing in https://github.com/Expensify/App/issues/10907. Right?

JmillsExpensify commented 1 year ago

@Gonals Do you mind chiming in on my last question? Looking at the reproduction steps, I'm failing to see the difference.

Santhosh-Sellavel commented 1 year ago

@JmillsExpensify Clicking download while Uploading is a duplicate of the other issue.

But this case here https://github.com/Expensify/App/issues/12283#issuecomment-1305774920, where we show a preview of the image they selected and followed by an error on sending is weird, I think we should do something about it

Gonals commented 1 year ago

Yes, I think @Santhosh-Sellavel. We seem to be dealing with two different problems here:

  1. Error after showing the image preview
  2. Crash downloading the attachment after failure (duplicate)
JmillsExpensify commented 1 year ago

Nice, yeah agreed. I think one is also a duplicate as well though. Let me try to find this issue, this is related to changes to the attachment flow, where everything is running through Web-PDFs.

JmillsExpensify commented 1 year ago

The main issue is here, however, @youssef-lr, I think we actually had the discussion on Web-PDFs in a DM. I think we specifically talked about this point

⁦the issue here is that some filetypes are recognized as image/pdf and Web tries to create a thumbnail for them, which results in an error

Is the no live setting on the photo this same error that you're already resolving?

trjExpensify commented 1 year ago

Error after showing the image preview - @Gonals I think one is also a duplicate as well though. Let me try to find this issue, this is related to changes to the attachment flow, where everything is running through Web-PDFs. - @JmillsExpensify

So this issue can be closed can it, if it's being taken care of elsewhere?

JmillsExpensify commented 1 year ago

Updated this issue to remove the duplicate discussion around the crash. That's handled in https://github.com/Expensify/App/issues/10907 moving forward. This issue is now focused on the on the "Unsupported PDF format" RBR error message.

JmillsExpensify commented 1 year ago

Separately, @youssef-lr still would like confirmation if you are aware of this issue or working on any PRs elsewhere that resolve this?

Santhosh-Sellavel commented 1 year ago

Updated this issue to remove the duplicate discussion around the crash. That's handled in #10907 moving forward. This issue is now focused on the on the "Unsupported PDF format" RBR error message.

Thanks, @JmillsExpensify, now its more clear!

JmillsExpensify commented 1 year ago

Anytime!

JmillsExpensify commented 1 year ago

Also an update on this issue: It's pretty tricky. I tested two flows, and received a different result each time. Basically, if you follow the testing flow on mWeb, you receive no error. It's also possible to see the (HEIC) attachment after upload. However, if you test on native iOS, then the reproduction steps above produce the error. That makes me think that we're doing some combination of file processing in one flow that doesn't hit the error, whereas the native flow is treating the file differently and so, triggers the error.

melvin-bot[bot] commented 1 year ago

@JmillsExpensify, @Gonals, @Santhosh-Sellavel Whoops! This issue is 2 days overdue. Let's get this updated quick!

Santhosh-Sellavel commented 1 year ago

Looking for proposals!

JmillsExpensify commented 1 year ago

Now that we're aligned on the issue/next steps, I'm doubling the price of this issue! Upwork job is here: https://www.upwork.com/jobs/~013c634d3ded0ca3e6. Please help us with proposals!

JmillsExpensify commented 1 year ago

Still waiting on proposals.

puneetlath commented 1 year ago

@Gonals To help us clear out the large backlog of /App bugs, we're putting the spotlight every bug in the repo already than 4 weeks old. To help unblock the roadmap and get our bug pipeline back in equilibrium, can you:

Gonals commented 1 year ago

Hey @puneetlath! So we don't plan to increase the price of this? It got doubled just a few days ago.

I'm basically unable to tackle this issue myself, as it is an iOS-specific issue that needs to be tested with the iPhone camera (so the simulator won't cut it), and I don't have an iPhone 😅 I'll reach out in eng-chat to see if any Apple people volunteers

puneetlath commented 1 year ago

Ah actually, my mistake. This isn't 4+ weeks old yet, so it doesn't have to be taken internal. I think it might be worth you asking if another engineer wants to trade you for this issue though, since you don't have iOS.

s77rt commented 1 year ago

Proposal

diff --git a/src/components/AttachmentPicker/index.native.js b/src/components/AttachmentPicker/index.native.js
index 418be07b8..aa965e5e7 100644
--- a/src/components/AttachmentPicker/index.native.js
+++ b/src/components/AttachmentPicker/index.native.js
@@ -34,6 +34,7 @@ const imagePickerOptions = {
     saveToPhotos: false,
     selectionLimit: 1,
     includeExtra: false,
+    quality: 0.95,
 };

 /**

Details

My proposal is more like a workaround, the bug is in https://github.com/react-native-image-picker/react-native-image-picker/issues/2052 I think we can proceed with the workaround without waiting for the react-native-image-picker.

Santhosh-Sellavel commented 1 year ago

Strictly No workarounds, if the issue exists at the library, it's better to address it there! If you have a solution to solve please post it here, i.e at the library itself!

s77rt commented 1 year ago

@Santhosh-Sellavel I have submitted a bug report already. I have found the root cause, proposed a solution for the library and proposed a workaround here.

Santhosh-Sellavel commented 1 year ago

@Santhosh-Sellavel I have submitted a bug report already. I have found the root cause, proposed a solution for the library, and proposed a workaround here.

If you have found the root cause then why was the workaround submitted here?

s77rt commented 1 year ago

@Santhosh-Sellavel From Expensify App perspective react-native-image-picker is the root cause. It's related to the quality prop. I know that the issue exist for setting the value equal to 1. From react-native-image-picker perspective I don't know the root cause, I just submitted a suggestion to where to start looking in the code.

Santhosh-Sellavel commented 1 year ago

As you say if setting quality 1 is causing the problem but we don't set 1 as a value. I assume it might default as 1. We know that problem is the with react-native-image-picker, then we need to fix the problem at the library side by finding the root cause and solution, thanks! If you have one please post it here and submit a PR for the same at the library repo!

JmillsExpensify commented 1 year ago

While the conversation above continues, doubling the price of this issue once more! We're now at $1,000 💰

s77rt commented 1 year ago

The issue seems to be a regression from https://github.com/react-native-image-picker/react-native-image-picker/pull/2006 I have asked if this can be reverted, then it should be sorted out, except that the returned file will seem to be double but that's just due to the conversion from heic to jpeg I suppose. We may also want to keep the quality at 0.95

mountiny commented 1 year ago

except that the returned file will seem to be double but that's just due to the conversion from heic to jpeg I suppose

I dont think reverting that fix is great, that was introduced to fix another issue. Doubling the size is not what we want and I believe it doubles the size also for images which are jpeg in the gallery

s77rt commented 1 year ago

@mountiny What I'm saying is that maybe the PR didn't fix any issue in the first place and the returned size was actually the correct size. I have downloaded the image that seems to have the size doubled, and the size was in fact correct. Uploaded image at quality 1 => size 9.6MB (this seems to be double but it's correct) => downloaded the image and the size is in fact 9.6MB ==> Correct Uploaded image at quality 0.95 => size 5.1MB => downloaded the image and the size is 5.1MB ==> Correct

Santhosh-Sellavel commented 1 year ago

@s77rt

@mountiny What I'm saying is that maybe the PR didn't fix any issue in the first place and the returned size was actually the correct size.

Which platform did you check?

s77rt commented 1 year ago

@Santhosh-Sellavel Your question is not quite clear, check what exactly?

Santhosh-Sellavel commented 1 year ago

You have confirmed here the size didn’t change right?

On which platform you verified it?

s77rt commented 1 year ago

@Santhosh-Sellavel Ah, you quoted the wrong part, your question was out of context.

  1. I have modified the code (basically a revert) so I can get it working.
  2. Send the image using quality 1 (on iOS)
  3. Downloaded the image that I sent (on Linux, but this does not matter, it just happen to be Linux)
  4. Verified that the reported size on iOS 9.6MB is indeed the size of the photo that I downloaded
s77rt commented 1 year ago

Here is the image, you can download it and you will see it's indeed 9.6MB. There was nothing wrong it just happen that the conversion at quality 1 is so inefficient that the size seems to be double.

w_8361c37cbbfcef589f6fc29b76d5de220586b302

Santhosh-Sellavel commented 1 year ago

Send the image using iOS and download it on any platforms size will change.

s77rt commented 1 year ago

Send the image using iOS and download it on any platforms size will change.

Isn't that what i just said i did?