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

Prevent large attachment #4269

Merged
merged 8 commits into from
Jul 29, 2021
Merged

Prevent large attachment #4269

merged 8 commits into from
Jul 29, 2021

Conversation

rushatgabhane
Copy link
Member

@rushatgabhane rushatgabhane commented Jul 28, 2021

Details

Acceptable API attachment size: Less than 50 MB (52428800 bytes). Source

ConfirmModal is used to display a message when a larger file is uploaded.

The following language is added.

English

Attachment too large
Attachment size is larger than 50 MB limit.

Spanish (Source & verification)

Archivo adjunto demasiado grande
El archivo adjunto supera el límite de 50 MB.

Fixed Issues

$ #3926

Tests / QA

  1. Create a dummy file of size at least 50 MB (52428800 bytes).
  • Windows: fsutil file createnew test50 52428800
  • MacOS: mkfile -n test50 50m
  1. Login to New Expensify.
  2. Navigate to any user.
  3. Press + in chat, then press Add Attachment.
  4. Select the dummy file.
  5. Verify that Attachment too large message is shown.
  6. Similarly, verify that you can upload a file smaller than 50 MB.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

English

Screen Shot 2021-07-28 at 12 25 32 PM

Spanish

Screen Shot 2021-07-28 at 12 39 48 PM

Mobile Web

Desktop

Screen Shot 2021-07-28 at 12 55 50 PM

iOS

Screen Shot 2021-07-28 at 12 54 16 PM

Android

@rushatgabhane rushatgabhane requested a review from a team as a code owner July 28, 2021 10:46
@MelvinBot MelvinBot requested review from deetergp and removed request for a team July 28, 2021 10:46
@rushatgabhane
Copy link
Member Author

I went with this style because 3 out of 3 non-tech people agreed with it.
What do you think?

Current style

image

Alternative style

image

@shawnborton
Copy link
Contributor

I agree, I like the current style you have.

Copy link
Contributor

@deetergp deetergp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had one minor style change. Otherwise looks good and tests well 👍

src/components/AttachmentModal.js Outdated Show resolved Hide resolved
Copy link
Contributor

@deetergp deetergp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@deetergp deetergp merged commit 470d9de into Expensify:main Jul 29, 2021
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.81-5🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@isagoico
Copy link

isagoico commented Aug 1, 2021

Issue found for this PR #4356

@OSBotify
Copy link
Contributor

OSBotify commented Aug 6, 2021

🚀 Deployed to production in version: 1.0.82-7🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@trjExpensify
Copy link
Contributor

trjExpensify commented Nov 16, 2022

👋 blast from the past! Heads up as part of the new BugZero checklist. This PR caused a regression whereby the max file size was set to 50 MB, but the server only allows for 25 MB. One to look out for in the future. Thanks!

CC: @mountiny @Santhosh-Sellavel @Tushu17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants