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

[$250] PR - No error message displayed for greater than 15 mb jpg file #54700

Open
2 of 8 tasks
mitarachim opened this issue Jan 1, 2025 · 12 comments
Open
2 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 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

@mitarachim
Copy link

mitarachim commented Jan 1, 2025

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: v9.0.80-1
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: #53902
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team
Device used: Macbook Pro 2023 / Sonoma 14.4.1
App Component: Other

Action Performed:

  1. Open the New Expensify Desktop app
  2. Click on FAB > Submit expense
  3. Enter an amount, click on 'Next'
  4. Choose any recipient
  5. Click on three dots at top right corner in expense creation screen
  6. Click 'Add receipt'
  7. Select a receipt equal or greater than 10mb.

Expected Result:

Verify that 'Attachment is too large - Attachment size is larger than 10 MB limit' is displayed

Actual Result:

Able to upload the file with no issues

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6704689_1735685408972.PR_53902_Desktop.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021874862385024395150
  • Upwork Job ID: 1874862385024395150
  • Last Price Increase: 2025-01-02
Issue OwnerCurrent Issue Owner: @jayeshmangwani
@mitarachim mitarachim added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 1, 2025
Copy link

melvin-bot bot commented Jan 1, 2025

Triggered auto assignment to @MitchExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@mitarachim mitarachim changed the title PR 53902 - No error message displayed for greater than 15 mb jpg file PR - No error message displayed for greater than 15 mb jpg file Jan 1, 2025
@Themoonalsofall
Copy link
Contributor

@mitarachim @MitchExpensify Sorry for not clarifying the steps in the previous PR. Here, we only display "Attachment is too large - Attachment size is larger than the 10 MB limit" for receipts that are not images.

@mitarachim
Copy link
Author

@Themoonalsofall Okay thank you for clarify, so should be truly receipt in .jpg or .png ?

@Themoonalsofall
Copy link
Contributor

@mitarachim From what I see in the source code, for receipts in image formats like .jpg or .png, we don’t limit the file size. The size restriction only applies to formats like PDF, ZIP, and others.

@mitarachim
Copy link
Author

@Themoonalsofall already ask tester to re-test.
seems tester not available at the moment , i also try in my side and can see the restriction while using PDF larger than 10 MB

Screen.Recording.2025-01-01.at.3.02.08.PM.mp4

@MitchExpensify MitchExpensify added 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 labels Jan 2, 2025
@melvin-bot melvin-bot bot changed the title PR - No error message displayed for greater than 15 mb jpg file [$250] PR - No error message displayed for greater than 15 mb jpg file Jan 2, 2025
Copy link

melvin-bot bot commented Jan 2, 2025

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

Copy link

melvin-bot bot commented Jan 2, 2025

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

@MitchExpensify MitchExpensify moved this to Bugs and Follow Up Issues in [#whatsnext] #expense Jan 2, 2025
@nkdengineer
Copy link
Contributor

Proposal

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

What is the root cause of that problem?

For the image, we will resize it if the file is larger than 24MB based on the expected here. So it's expected that no error message displays for the greater than 15MB image. But we have a problem here: the max size image is 1024 * 1024 while the max dimension is 2400 that can causes the wrong resolution image when we upload the large image

const scaleFactor = CONST.MAX_IMAGE_DIMENSION / (width < height ? height : width);

if (!file || !Str.isImage(file.name ?? '') || (file?.size ?? 0) <= CONST.API_ATTACHMENT_VALIDATIONS.RECEIPT_MAX_SIZE) {
return Promise.resolve(file);
}
return getImageDimensionsAfterResize(file).then(({width, height}) => getImageManipulator({fileUri: file.uri ?? '', width, height, fileName: file.name ?? '', type: file.type}));

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

We should change CONST.API_ATTACHMENT_VALIDATIONS.RECEIPT_MAX_SIZE) to CONST.API_ATTACHMENT_VALIDATIONS.MAX_SIZE to match with the previous expected here

if (!file || !Str.isImage(file.name ?? '') || (file?.size ?? 0) <= CONST.API_ATTACHMENT_VALIDATIONS.RECEIPT_MAX_SIZE) {
return Promise.resolve(file);
}
return getImageDimensionsAfterResize(file).then(({width, height}) => getImageManipulator({fileUri: file.uri ?? '', width, height, fileName: file.name ?? '', type: file.type}));

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

Or we can change the CONST.MAX_IMAGE_DIMENSION to 1024 and then resize the image correctly with a max size of 10MB.

const scaleFactor = CONST.MAX_IMAGE_DIMENSION / (width < height ? height : width);

@jayeshmangwani
Copy link
Contributor

It seems this PR (#53902) has been reverted. @dominictb and @madmax330, will this issue be addressed in the new PR, or should we continue evaluating proposals on this issue?

@dominictb
Copy link
Contributor

@mitarachim Please share the attachment used in OP here so we can retest it. For any attachment-related issues, we should always share the attachment as well. Thanks.

@mitarachim
Copy link
Author

@dominictb 1 file .jpg from tester that i have compress in .zip , please extract if needed that file .
1 file .pdf from me while re-test here
20mb.pdf
Bug6704689_1735685602908!15mb.jpg.zip

@dominictb
Copy link
Contributor

@mitarachim Thanks for the responsiveness! I will retest with these files and reply here soon whether to continue this issue.

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 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
Status: Bugs and Follow Up Issues
Development

No branches or pull requests

6 participants