Improve attachment validation on the front-end #10118
Conversation
melvin-bot
bot
requested review from
marcochavezf
and removed request for
a team
| return; | ||
| } | ||
|
|
||
| if (file instanceof File) { |
There was a problem hiding this comment.
Kind of curious whether the File is going to have a size property. Might be good to test on native and log out the value just to be sure (can help with this if you're running into issues).
There was a problem hiding this comment.
I can test this out on Android. I'm 100% certain File will always have size, but good to verify anyway.
|
Updated |
| if (!file) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
NAB: I wonder when file is empty and if it's an uncovered edge case.
If it's empty because of an error, I think we should move this condition to isValidFile function and populate attachmentInvalidReasonTitle with this.props.translate('attachmentPicker.attachmentError').
Or just log something using Log.hmmm.
thoughts?
There was a problem hiding this comment.
I don't really think it's a valid edge case for file to be empty, but it protects fatal errors in the case that it is. I'd probably be more comfortable removing this condition entirely. We seem to be paranoid about this object, and I'm not 100% why.
Co-authored-by: Marco Chávez <marcochavezf@gmail.com>
Co-authored-by: Marco Chávez <marcochavezf@gmail.com>
| && Str.isPDF(file.name || this.props.translate('attachmentView.unknownFilename')) | ||
| ) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
This just needs a few more indents and it will be ready for merge 
There was a problem hiding this comment.
Haha, the logic is already melting my brain :D
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by @marcaaron in version: 1.1.87-0 🚀
|
|
🚀 Deployed to production by @yuwenmemon in version: 1.1.87-9 🚀
|
|
You didn't add video extensions ('mp4,mov,...'), no one can send a video file now, Is it ok? Line 17 in 48737f3 |
|
Hm, that was the list of extensions that the server was preventing. You could try allowing that extension, and then see if the server allows it to be uploaded. In that case, I must have missed a few extensions and can add them. Would you mind trying that out @gladiator-1 ? |
|
@tgolen I tried it in the morning and it worked fine. |
|
OK, I can add mov and mp4 to the list. Any others? |
|
Per the checklist noting, that there was a bug regression coming from this PR (or this took the bug from previous issues). The text of the error messages in modal should have a proper punctuation in the end (the main message of the error, not the title). Keep that in mind for future, thanks! #14200 |
| @@ -71,15 +72,17 @@ class AttachmentModal extends PureComponent { | |||
|
|
|||
| this.state = { | |||
| isModalOpen: false, | |||
| isConfirmModalOpen: false, | |||
| isAttachmentInvalid: false, | |||
| attachmentInvalidReasonTitle: null, | |||
| title={this.props.translate('attachmentPicker.attachmentTooLarge')} | ||
| title={this.state.attachmentInvalidReasonTitle} |
There was a problem hiding this comment.
This change causes a regression #22665. We just reverted back the logic i.e store translationKey in the state instead of value. Translated while passing
Adds validation for minimum file size and file type.
Fixed Issues
$ #10050
Tests
.x) and upload itQA Steps
Same as tests
Screenshots
Web
Mobile Web
Desktop
iOS
I'm struggling to get iOS running (see slack)
Android
I couldn't really find any way to test the errors because it's difficult to force those kind of size images on the emulator. I tried downloading some images from the web, but I still had trouble finding them to upload into the app.