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

Avatar Image Upload Limit #5900

Merged
merged 14 commits into from
Oct 19, 2021
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/CONST.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const NEW_EXPENSIFY_URL = 'https://new.expensify.com';
const CONST = {
// 50 megabytes in bytes
API_MAX_ATTACHMENT_SIZE: 52428800,
AVATAR_MAX_ATTACHMENT_SIZE_MB: 3,
Copy link
Member

Choose a reason for hiding this comment

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

Could you please change it to Bytes? Most of the time arithmetic operations will be in bytes.

APP_DOWNLOAD_LINKS: {
ANDROID: 'https://play.google.com/store/apps/details?id=com.expensify.chat',
IOS: 'https://apps.apple.com/us/app/expensify-cash/id1530278510',
Expand Down
3 changes: 2 additions & 1 deletion src/components/AttachmentPicker/index.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import React, {Component} from 'react';
import {Alert, Linking, View} from 'react-native';
import {launchImageLibrary} from 'react-native-image-picker';
import RNDocumentPicker from 'react-native-document-picker';
import lodashGet from 'lodash/get';
import {propTypes as basePropTypes, defaultProps} from './AttachmentPickerPropTypes';
import styles from '../../styles/styles';
import Popover from '../Popover';
Expand Down Expand Up @@ -65,7 +66,7 @@ function getDataForUpload(fileData) {
name: fileData.fileName || fileData.name || 'chat_attachment',
type: fileData.type,
uri: fileData.uri,
size: fileData.size,
size: lodashGet(fileData, 'fileSize', lodashGet(fileData, 'size')),
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need lodashGet here? will FileData ever be undefined?

Copy link
Member

Choose a reason for hiding this comment

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

Still a question?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh I did answer this yesterday, can't find the comment.

Earlier we had fileData.size but it gives size is undefined. I checked it returns fileData.fileSize. So I've used both to keep it backward compatible.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. I think it is better like this.

Filedata.filesize || filedata.size

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. I thought generally we use lodash.get for optional checks.

};
}

Expand Down
43 changes: 42 additions & 1 deletion src/components/AvatarWithImagePicker.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
Pressable, View, Animated, StyleSheet,
} from 'react-native';
import PropTypes from 'prop-types';
import lodashGet from 'lodash/get';
import Avatar from './Avatar';
import Icon from './Icon';
import PopoverMenu from './PopoverMenu';
Expand All @@ -13,6 +14,7 @@ import {
import styles from '../styles/styles';
import themeColors from '../styles/themes/default';
import AttachmentPicker from './AttachmentPicker';
import ConfirmModal from './ConfirmModal';
import withLocalize, {withLocalizePropTypes} from './withLocalize';
import variables from '../styles/variables';
import CONST from '../CONST';
Expand Down Expand Up @@ -52,6 +54,9 @@ const propTypes = {
/** Size of Indicator */
size: PropTypes.string,
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add the correct type here?

Suggested change
size: PropTypes.string,
size: size: PropTypes.oneOf([CONST.AVATAR_SIZE.LARGE, CONST.AVATAR_SIZE.DEFAULT]),


/** Max image size */
maxUploadSizeInMB: PropTypes.number,
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this prop? I don't think we want to customize this ever.


...withLocalizePropTypes,
};

Expand All @@ -64,14 +69,18 @@ const defaultProps = {
isUsingDefaultAvatar: false,
isUploading: false,
size: CONST.AVATAR_SIZE.DEFAULT,
maxUploadSizeInMB: undefined,
};

class AvatarWithImagePicker extends React.Component {
constructor(props) {
super(props);
this.animation = new SpinningIndicatorAnimation();
this.setUploadLimitModalVisibility = this.setUploadLimitModalVisibility.bind(this);
this.isValidSize = this.isValidSize.bind(this);
this.state = {
isMenuVisible: false,
isMaxUploadSizeModalOpen: false,
};
}

Expand All @@ -93,6 +102,23 @@ class AvatarWithImagePicker extends React.Component {
this.animation.stop();
}

/**
* Toggle max upload limit mModal visibility
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Toggle max upload limit mModal visibility
* Toggle max upload limit modal's visibility

* @param {Boolean} isVisible
*/
setUploadLimitModalVisibility(isVisible) {
this.setState({isMaxUploadSizeModalOpen: isVisible});
}

/**
* Check if the attachment size is less than allowed size.
* @param {Object} image
* @returns {Boolean}
*/
isValidSize(image) {
return !image || !this.props.maxUploadSizeInMB || lodashGet(image, 'size', 0) < (this.props.maxUploadSizeInMB * 1024 * 1024);
Copy link
Member

Choose a reason for hiding this comment

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

When the image is null, it returns true.

}

/**
* Create menu items list for avatar menu
*
Expand All @@ -106,7 +132,13 @@ class AvatarWithImagePicker extends React.Component {
text: this.props.translate('avatarWithImagePicker.uploadPhoto'),
onSelected: () => {
openPicker({
onPicked: this.props.onImageSelected,
onPicked: (image) => {
if (!this.isValidSize(image)) {
this.setUploadLimitModalVisibility(true);
return;
}
this.props.onImageSelected(image);
},
});
},
},
Expand Down Expand Up @@ -200,6 +232,15 @@ class AvatarWithImagePicker extends React.Component {
</AttachmentPicker>
</View>
</Pressable>
<ConfirmModal
title={this.props.translate('avatarWithImagePicker.imageUploadFailed')}
onConfirm={() => this.setUploadLimitModalVisibility(false)}
onCancel={() => this.setUploadLimitModalVisibility(false)}
isVisible={this.state.isMaxUploadSizeModalOpen}
prompt={this.props.translate('avatarWithImagePicker.sizeExceeded', {maxUploadSizeInMB: this.props.maxUploadSizeInMB})}
Copy link
Member

Choose a reason for hiding this comment

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

You can use x * 1024 * 1024 here instead.

confirmText={this.props.translate('common.close')}
shouldShowCancelButton={false}
/>
</View>
);
}
Expand Down
2 changes: 2 additions & 0 deletions src/languages/en.js
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,8 @@ export default {
uploadPhoto: 'Upload photo',
removePhoto: 'Remove photo',
editImage: 'Edit photo',
imageUploadFailed: 'Image upload failed',
sizeExceeded: ({maxUploadSizeInMB}) => `The selected image exceeds the maximum upload size of ${maxUploadSizeInMB}MB.`,
},
profilePage: {
profile: 'Profile',
Expand Down
2 changes: 2 additions & 0 deletions src/languages/es.js
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,8 @@ export default {
uploadPhoto: 'Subir foto',
removePhoto: 'Eliminar foto',
editImage: 'Editar foto',
imageUploadFailed: 'Error al cargar la imagen',
sizeExceeded: ({maxUploadSizeInMB}) => `La imagen supera el tamaño máximo de ${maxUploadSizeInMB} MB.`,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sizeExceeded: ({maxUploadSizeInMB}) => `La imagen supera el tamaño máximo de ${maxUploadSizeInMB} MB.`,
sizeExceeded: ({maxUploadSizeInMB}) => `La imagen supera el tamaño máximo de ${maxUploadSizeInMB}MB.`,

},
profilePage: {
profile: 'Perfil',
Expand Down
1 change: 1 addition & 0 deletions src/pages/settings/Profile/ProfilePage.js
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ class ProfilePage extends Component {
isUsingDefaultAvatar={this.props.myPersonalDetails.avatar.includes('/images/avatars/avatar')}
anchorPosition={styles.createMenuPositionProfile}
size={CONST.AVATAR_SIZE.LARGE}
maxUploadSizeInMB={CONST.AVATAR_MAX_ATTACHMENT_SIZE_MB}
Copy link
Member

Choose a reason for hiding this comment

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

Not needed based on my comment above.

/>
<Text style={[styles.mt6, styles.mb6]}>
{this.props.translate('profilePage.tellUsAboutYourself')}
Expand Down
1 change: 1 addition & 0 deletions src/pages/workspace/WorkspaceSettingsPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ class WorkspaceSettingsPage extends React.Component {
isUsingDefaultAvatar={!this.state.previewAvatarURL}
onImageSelected={this.uploadAvatar}
onImageRemoved={this.removeAvatar}
maxUploadSizeInMB={CONST.AVATAR_MAX_ATTACHMENT_SIZE_MB}
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

/>

<ExpensiTextInput
Expand Down