-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Improve attachment validation on the front-end #10118
Changes from all commits
1d02408
0e22b20
f01528c
1464dbe
6c50384
c417959
37068ea
a36809b
057d678
12ba8cc
dc45281
b2db4ba
daa65af
7d3a441
439f03f
0e804b2
d641756
1cccad3
414ec0d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,8 @@ import PropTypes from 'prop-types'; | |
import {View} from 'react-native'; | ||
import Str from 'expensify-common/lib/str'; | ||
import lodashGet from 'lodash/get'; | ||
import _ from 'lodash'; | ||
import lodashExtend from 'lodash/extend'; | ||
import _ from 'underscore'; | ||
import CONST from '../CONST'; | ||
import Modal from './Modal'; | ||
import AttachmentView from './AttachmentView'; | ||
|
@@ -71,15 +72,17 @@ class AttachmentModal extends PureComponent { | |
|
||
this.state = { | ||
isModalOpen: false, | ||
isConfirmModalOpen: false, | ||
isAttachmentInvalid: false, | ||
attachmentInvalidReasonTitle: null, | ||
attachmentInvalidReason: null, | ||
file: null, | ||
sourceURL: props.sourceURL, | ||
modalType: CONST.MODAL.MODAL_TYPE.CENTERED_UNSWIPEABLE, | ||
}; | ||
|
||
this.submitAndClose = this.submitAndClose.bind(this); | ||
this.closeConfirmModal = this.closeConfirmModal.bind(this); | ||
this.isValidSize = this.isValidSize.bind(this); | ||
this.validateAndDisplayFileToUpload = this.validateAndDisplayFileToUpload.bind(this); | ||
} | ||
|
||
/** | ||
|
@@ -89,22 +92,30 @@ class AttachmentModal extends PureComponent { | |
* @returns {String} | ||
*/ | ||
getModalType(sourceUrl, file) { | ||
const modalType = (sourceUrl | ||
&& (Str.isPDF(sourceUrl) || (file && Str.isPDF(file.name || this.props.translate('attachmentView.unknownFilename'))))) | ||
return ( | ||
sourceUrl | ||
&& ( | ||
Str.isPDF(sourceUrl) | ||
|| ( | ||
file | ||
&& Str.isPDF(file.name || this.props.translate('attachmentView.unknownFilename')) | ||
) | ||
) | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This just needs a few more indents and it will be ready for merge There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Haha, the logic is already melting my brain :D |
||
? CONST.MODAL.MODAL_TYPE.CENTERED_UNSWIPEABLE | ||
: CONST.MODAL.MODAL_TYPE.CENTERED; | ||
return modalType; | ||
} | ||
|
||
/** | ||
* Returns the filename split into fileName and fileExtension | ||
* | ||
* @param {String} fullFileName | ||
* @returns {Object} | ||
*/ | ||
splitExtensionFromFileName() { | ||
const fullFileName = this.props.originalFileName ? this.props.originalFileName.trim() : lodashGet(this.state, 'file.name', '').trim(); | ||
const splittedFileName = fullFileName.split('.'); | ||
const fileExtension = splittedFileName.pop(); | ||
const fileName = splittedFileName.join('.'); | ||
splitExtensionFromFileName(fullFileName) { | ||
const fileName = fullFileName.trim(); | ||
const splitFileName = fileName.split('.'); | ||
const fileExtension = splitFileName.pop(); | ||
return {fileName, fileExtension}; | ||
} | ||
|
||
|
@@ -118,7 +129,7 @@ class AttachmentModal extends PureComponent { | |
} | ||
|
||
if (this.props.onConfirm) { | ||
this.props.onConfirm(_.extend(this.state.file, {source: this.state.sourceURL})); | ||
this.props.onConfirm(lodashExtend(this.state.file, {source: this.state.sourceURL})); | ||
} | ||
|
||
this.setState({isModalOpen: false}); | ||
|
@@ -128,16 +139,70 @@ class AttachmentModal extends PureComponent { | |
* Close the confirm modal. | ||
*/ | ||
closeConfirmModal() { | ||
this.setState({isConfirmModalOpen: false}); | ||
this.setState({isAttachmentInvalid: false}); | ||
marcaaron marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
/** | ||
* Check if the attachment size is less than the API size limit. | ||
* @param {Object} file | ||
* @returns {Boolean} | ||
*/ | ||
isValidSize(file) { | ||
return !file || lodashGet(file, 'size', 0) < CONST.API_MAX_ATTACHMENT_SIZE; | ||
isValidFile(file) { | ||
if (lodashGet(file, 'size', 0) > CONST.API_ATTACHMENT_VALIDATIONS.MAX_SIZE) { | ||
this.setState({ | ||
isAttachmentInvalid: true, | ||
attachmentInvalidReasonTitle: this.props.translate('attachmentPicker.attachmentTooLarge'), | ||
attachmentInvalidReason: this.props.translate('attachmentPicker.sizeExceeded'), | ||
}); | ||
return false; | ||
} | ||
|
||
if (lodashGet(file, 'size', 0) < CONST.API_ATTACHMENT_VALIDATIONS.MIN_SIZE) { | ||
this.setState({ | ||
isAttachmentInvalid: true, | ||
attachmentInvalidReasonTitle: this.props.translate('attachmentPicker.attachmentTooSmall'), | ||
attachmentInvalidReason: this.props.translate('attachmentPicker.sizeNotMet'), | ||
}); | ||
return false; | ||
} | ||
|
||
const {fileExtension} = this.splitExtensionFromFileName(lodashGet(file, 'name', '')); | ||
if (!_.contains(CONST.API_ATTACHMENT_VALIDATIONS.ALLOWED_EXTENSIONS, fileExtension)) { | ||
const invalidReason = `${this.props.translate('attachmentPicker.notAllowedExtension')} ${CONST.API_ATTACHMENT_VALIDATIONS.ALLOWED_EXTENSIONS.join(', ')}`; | ||
this.setState({ | ||
isAttachmentInvalid: true, | ||
attachmentInvalidReasonTitle: this.props.translate('attachmentPicker.wrongFileType'), | ||
attachmentInvalidReason: invalidReason, | ||
}); | ||
return false; | ||
} | ||
|
||
return true; | ||
} | ||
|
||
/** | ||
* @param {Object} file | ||
*/ | ||
validateAndDisplayFileToUpload(file) { | ||
if (!file) { | ||
return; | ||
} | ||
Comment on lines
+186
to
+188
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NAB: I wonder when If it's empty because of an error, I think we should move this condition to Or just log something using thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't really think it's a valid edge case for |
||
|
||
if (!this.isValidFile(file)) { | ||
return; | ||
} | ||
|
||
if (file instanceof File) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kind of curious whether the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can test this out on Android. I'm 100% certain |
||
const source = URL.createObjectURL(file); | ||
const modalType = this.getModalType(source, file); | ||
this.setState({ | ||
isModalOpen: true, sourceURL: source, file, modalType, | ||
}); | ||
} else { | ||
const modalType = this.getModalType(file.uri, file); | ||
this.setState({ | ||
isModalOpen: true, sourceURL: file.uri, file, modalType, | ||
}); | ||
} | ||
} | ||
|
||
render() { | ||
|
@@ -149,7 +214,7 @@ class AttachmentModal extends PureComponent { | |
? [styles.imageModalImageCenterContainer] | ||
: [styles.imageModalImageCenterContainer, styles.p5]; | ||
|
||
const {fileName, fileExtension} = this.splitExtensionFromFileName(); | ||
const {fileName, fileExtension} = this.splitExtensionFromFileName(this.props.originalFileName || lodashGet(this.state, 'file.name', '')); | ||
|
||
return ( | ||
<> | ||
|
@@ -197,34 +262,19 @@ class AttachmentModal extends PureComponent { | |
/> | ||
)} | ||
</Modal> | ||
|
||
<ConfirmModal | ||
title={this.props.translate('attachmentPicker.attachmentTooLarge')} | ||
title={this.state.attachmentInvalidReasonTitle} | ||
Comment on lines
-201
to
+267
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change causes a regression #22665. We just reverted back the logic i.e store |
||
onConfirm={this.closeConfirmModal} | ||
onCancel={this.closeConfirmModal} | ||
isVisible={this.state.isConfirmModalOpen} | ||
prompt={this.props.translate('attachmentPicker.sizeExceeded')} | ||
isVisible={this.state.isAttachmentInvalid} | ||
prompt={this.state.attachmentInvalidReason} | ||
confirmText={this.props.translate('common.close')} | ||
shouldShowCancelButton={false} | ||
/> | ||
|
||
{this.props.children({ | ||
displayFileInModal: ({file}) => { | ||
if (!this.isValidSize(file)) { | ||
this.setState({isConfirmModalOpen: true}); | ||
return; | ||
} | ||
if (file instanceof File) { | ||
const source = URL.createObjectURL(file); | ||
const modalType = this.getModalType(source, file); | ||
this.setState({ | ||
isModalOpen: true, sourceURL: source, file, modalType, | ||
}); | ||
} else { | ||
const modalType = this.getModalType(file.uri, file); | ||
this.setState({ | ||
isModalOpen: true, sourceURL: file.uri, file, modalType, | ||
}); | ||
} | ||
}, | ||
displayFileInModal: this.validateAndDisplayFileToUpload, | ||
show: () => { | ||
this.setState({isModalOpen: true}); | ||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use empty String for attachmentInvalidReasonTitle because Confirm modal accepts string as title but we pass null which cause in this issue. Fixed it here by providing empty String.