[HOLD for payment 2022-11-16] [$250] [Image] Cant Share attachments between 40-49 mb - reported by @Tushu17

[mvtglobally]

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 chat
2. Select Actions -> Add attachment
3. Add an around 45 mb video/file (file in tread)

Expected Result:

Error should be shown immediately since the limit of the file we can at the moment process is 25MB. Let's not have the user wait for the upload if we know it cannot succeed.

Actual Result:

Attachment kept loading until you refresh the page, attachment disappears after refreshing.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.1.43-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

VID_374491118_125842_369.mp4

Expensify/Expensify Issue URL:
Issue reported by: @Tushu17
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1644521208448039

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @mallenexpensify (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[mallenexpensify]

Well... that is a weird one, nice catch @Tushu17
I was able to reproduce, got this is logs

The pdf file @Tushu17 was using is here in slack

If/when we fix this, it'd be great to know why it's happening and to make sure we fix it for all file sizes.

[melvin-bot]

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

[marcochavezf]

Marking this issue as internal, because the logs show that there's a size issue on backend:

2022-03-22T20:13:58.941746+00:00 expensidev2004 php-fpm: QHP2Bx /api.php marco+test1@expensify.com !ecash! ?report? [info] Benchmark started:PDFBoxAPI - SPost2
2022-03-22T20:13:58.952725+00:00 expensidev2004 nginx: 2022/03/22 20:13:58 [error] 164010#164010: *2365 client intended to send too large body: 66021374 bytes, client: 127.0.0.1, server: pdfs.expensify.com.dev, request: "POST /api.php HTTP/1.0", host: "pdfs.expensify.com.dev"

[melvin-bot]

Triggered auto assignment to @michaelhaxhiu (Internal) because issue was reported by a contributor who needs to be compensated if this issue is fixed

[marcochavezf]

I'm going to send this issue to the pool, I have my plate full atm. But I think we should check why the file size increases in the backend to 66 mb when the original attachment size is 46 mb.

[michaelhaxhiu]

Posted in #engineering here to see if we can find a volunteer.

[thienlnam]

But I think we should check why the file size increases in the backend to 66 mb when the original attachment size is 46 mb.

Ah man this has been an issue for a while - IIRC if you copied and pasted the image it would increase in size. When we send an image to the PDF box we try to generate a thumbnail and convert it into a PDF so that could also be the case.

An easy workaround could be to increase the allowed file size in php.ini https://github.com/Expensify/Web-Expensify/blob/5f23cbb798713a7689c8ca55f2db87aa02178ae4/lib/ReportUtils.php#L289-L293. https://github.com/Expensify/Web-Expensify/blob/5f23cbb798713a7689c8ca55f2db87aa02178ae4/_tools/ci/php.ini#L133-L135

[michaelhaxhiu]

Related - #4356

[melvin-bot]

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

[Tushu17]

I've raised the PR. Unfortunately I wasn't able to run android app now, So i didn't include android app recording or screenshot but I had tested my solution on android app.

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist will need to be completed before the issue can be closed:

• A regression test has been added or updated so that the same bug will not reach production again. Link to the updated test here: https://github.com/Expensify/Expensify/issues/243486
• The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/4269/files
• 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: Prevent large attachment #4269 (comment)
• A discussion in #contributor-plus 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: https://expensify.slack.com/archives/C02NK2DQWUX/p1668642018740719
• Payment has been made to the issue reporter (if applicable)
• Payment has been made to the contributor that fixed the issue (if applicable)
• Payment has been made to the contributor+ that helped on the issue (if applicable)

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.25-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Decrease attachment size limit #12497

If no regressions arise, payment will be issued on 2022-11-16. 🎊

[trjExpensify]

This one is due for payment tomorrow. I'm stepping into this issue late for Matt. @mountiny @Santhosh-Sellavel @Tushu17 can you help with sharing any insight on these two items of the checklist?

• The PR that introduced the bug has been identified. Link to the PR:
• 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:

Am I right in thinking the title of this issue is outdated based on where we landed here? We didn't increase the file size limit to 40-49mb, but rather we decreased it to 24MB because the maximum file size the server allows is 25 MB?

In terms of where the bug came from then, was it this PR that set the limit to 50 MB incorrectly instead of <25 MB?

I wonder if a code comment would be helpful here to prevent that from happening again? I.e

// 25 MB is the maximum the server allows so we don't exceed that file size limit

[mountiny]

@trjExpensify That is correct, it must have been this PR https://github.com/Expensify/App/pull/10118/files.

It is hard to check though since this limit is set on servers not even in API so that is why this was missed.

I agree a comment would be handy but also I think anyone who wants to raise the limit should be wondering why the limit is set to what it it. I have created a quick follow up PR to add a comment there #12738

[trjExpensify]

@trjExpensify That is correct, it must have been this PR https://github.com/Expensify/App/pull/10118/files.

It is hard to check though since this limit is set on servers not even in API so that is why this was missed.

As in, that explains why it was likely done incorrectly, but nevertheless it's the "offending" PR?

I agree a comment would be handy but also I think anyone who wants to raise the limit should be wondering why the limit is set to what it it. I have created a quick follow up PR to add a comment there #12738

Awesome, thanks for adding this!

[mountiny]

As in, that explains why it was likely done incorrectly, but nevertheless it's the "offending" PR?

It is technically, yes.

[flodnv]

That is correct, it must have been this PR #10118 (files).

Hm? This PR did not introduce the 50MB...

[mountiny]

:ohnothing:

Colours were never my strength

[mountiny]

This is the PR which introduced it https://github.com/Expensify/App/pull/4269/files, back in bronze age I believe, judging based off the number of annual rings.

[trjExpensify]

Cool, taken care of all the items on the checklist including paying everyone out. Discussion posted here. Will reopen if something comes of that. Thanks everyone! 🎉

[trjExpensify]

@Santhosh-Sellavel reminded me that this PR was raised after Nov 4th and was merged on Nov 7th, so we owe @Tushu17 & @Santhosh-Sellavel a $125 bonus each. Offers have been sent here: https://www.upwork.com/jobs/~010f6b384191b40d0c

[michaelhaxhiu]

Nice work on getting this one finalized everyone!

[trjExpensify]

Cool, done!