Avatar Image Upload Limit #5900

akshayasalvi · 2021-10-15T19:40:13Z

Details

Added upload limit to AvatarImage in Workspace
Added the same for Profile Image as well

Fixed Issues

$ #5778

Tests

Tested by uploading images of different sizes in all platforms.
Tested modal with language changes.

QA Steps

Go to any workspace via Sidebar
Click on the Workspace Title to edit it.
Try uploading an image of size > 3MB, it should throw an error
Upload an image within the limit, it should successfully allow uploading

Tested On

Screenshots

Web

web-upload-limit-compressed.mp4

Mobile Web

Desktop

iOS

Android

…image-size-limit

parasharrajat · 2021-10-15T20:09:46Z

src/components/AttachmentPicker/index.native.js

@@ -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')),


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

Still a question?

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.

Ok. I think it is better like this.

Filedata.filesize || filedata.size

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

parasharrajat · 2021-10-15T20:12:07Z

src/components/AvatarWithImagePicker.js

@@ -52,6 +54,9 @@ const propTypes = {
    /** Size of Indicator */
    size: PropTypes.string,

+    /** Max image size */
+    maxUploadSizeInMB: PropTypes.number,


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

parasharrajat · 2021-10-15T20:13:35Z

src/components/AvatarWithImagePicker.js

@@ -52,6 +54,9 @@ const propTypes = {
    /** Size of Indicator */
    size: PropTypes.string,


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]),

parasharrajat · 2021-10-15T20:14:41Z

src/components/AvatarWithImagePicker.js

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

+    /**
+     *  Toggle max upload limit mModal visibility


Suggested change

* Toggle max upload limit mModal visibility

* Toggle max upload limit modal's visibility

parasharrajat · 2021-10-15T20:17:30Z

src/CONST.js

@@ -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,


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

parasharrajat · 2021-10-15T20:18:16Z

src/components/AvatarWithImagePicker.js

+     * @returns {Boolean}
+     */
+    isValidSize(image) {
+        return !image || !this.props.maxUploadSizeInMB || lodashGet(image, 'size', 0) < (this.props.maxUploadSizeInMB * 1024 * 1024);


When the image is null, it returns true.

parasharrajat · 2021-10-15T20:18:52Z

src/components/AvatarWithImagePicker.js

+                    onConfirm={() => this.setUploadLimitModalVisibility(false)}
+                    onCancel={() => this.setUploadLimitModalVisibility(false)}
+                    isVisible={this.state.isMaxUploadSizeModalOpen}
+                    prompt={this.props.translate('avatarWithImagePicker.sizeExceeded', {maxUploadSizeInMB: this.props.maxUploadSizeInMB})}


You can use x * 1024 * 1024 here instead.

parasharrajat · 2021-10-15T20:19:42Z

src/languages/es.js

@@ -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.`,


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.`,

parasharrajat · 2021-10-15T20:20:07Z

src/pages/settings/Profile/ProfilePage.js

@@ -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}


Not needed based on my comment above.

parasharrajat · 2021-10-15T20:20:14Z

src/pages/workspace/WorkspaceSettingsPage.js

@@ -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}


akshayasalvi · 2021-10-16T11:15:31Z

@parasharrajat PR Updated

parasharrajat

A few changes.

parasharrajat · 2021-10-16T15:06:12Z

src/CONST.js

@@ -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: 3145728,


I hoped you would change the name AVATAR_MAX_ATTACHMENT_SIZE ~~_MB~~. Don't forget to update the references.

parasharrajat · 2021-10-16T15:07:12Z

src/components/AttachmentPicker/index.native.js

@@ -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')),


Still a question?

akshayasalvi · 2021-10-16T16:21:56Z

Sorry about the var name, will update

akshayasalvi · 2021-10-17T13:19:38Z

@parasharrajat PR updated. Any other changes?

parasharrajat

LGTM. tests well.

Ready for final review and possibly for merge after N6.cc: @pecanoro

akshayasalvi · 2021-10-17T13:39:22Z

Thanks, @parasharrajat for the detailed review. Appreciate it.

pecanoro · 2021-10-18T15:52:34Z

src/components/AvatarWithImagePicker.js

+     * @returns {Boolean}
+     */
+    isValidSize(image) {
+        return image && lodashGet(image, 'size', 0) < CONST.AVATAR_MAX_ATTACHMENT_SIZE;


Why are we passing the size as a prop but then using the constant directly?

@pecanoro size is for the size of the Avatar image - Large or default. Wasn't added in this PR. I just updated proptypes.

and for upload size limit I had added props but based on the feedback removed, so using CONST directly.

parasharrajat · 2021-10-19T15:48:31Z

@akshayasalvi Could you please merge main into your PR? I want to make sure there are no issues as a lot has been changed in N6. Thanks for your patience.

akshayasalvi · 2021-10-19T16:00:26Z

@akshayasalvi Could you please merge main into your PR? I want to make sure there are no issues as a lot has been changed in N6. Thanks for your patience.

On it for all PRs

…image-size-limit

akshayasalvi · 2021-10-19T16:10:39Z

Merged main and retested @parasharrajat @pecanoro

parasharrajat

Thank you.

OSBotify · 2021-10-19T17:25:34Z

✋ 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 · 2021-10-23T01:32:45Z

🚀 Deployed to staging by @pecanoro in version: 1.1.8-10 🚀

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

OSBotify · 2021-10-28T20:57:37Z

🚀 Deployed to production by @roryabraham in version: 1.1.10-2 🚀

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

akshayasalvi added 8 commits October 15, 2021 04:16

Updated params and config

1e74b9e

Added components for ConfirmModal along with state and prop chanes

c28e73e

Made translation dynamic and added isValidSize

4357ffc

Fixed label typos

754045f

Modified checks to show the error block and corrected translations

921a577

Added limit to the profile picture as well

6d5f1fb

Merge branch 'main' of github-personal:akshayasalvi/App into wkspace-…

601151a

…image-size-limit

Fixed size param for native attachment picker

d9c7583

akshayasalvi requested a review from a team as a code owner October 15, 2021 19:40

MelvinBot requested review from pecanoro and removed request for a team October 15, 2021 19:40

Added jsdoc comment for the function

40efded

parasharrajat suggested changes Oct 15, 2021

View reviewed changes

Removed prop and converted calculations to bytes

9f8c0d9

parasharrajat suggested changes Oct 16, 2021

View reviewed changes

akshayasalvi added 2 commits October 16, 2021 22:17

Change variable name for avatar max size

e288d0c

Changed lodash to || for size var

5fadd0a

Removed lodash.get ref

470b3c3

parasharrajat approved these changes Oct 17, 2021

View reviewed changes

pecanoro reviewed Oct 18, 2021

View reviewed changes

Merge branch 'main' of github-personal:akshayasalvi/App into wkspace-…

37fb1fc

…image-size-limit

pecanoro approved these changes Oct 19, 2021

View reviewed changes

parasharrajat approved these changes Oct 19, 2021

View reviewed changes

pecanoro merged commit 73b7551 into Expensify:main Oct 19, 2021

OSBotify mentioned this pull request Oct 23, 2021

Deploy Checklist: New Expensify 2021-10-23 #6017

Closed

akshayasalvi mentioned this pull request Nov 12, 2021

[HOLD for payment 2021-12-27] $500 - solution + reporting bonus. Android/iOS - User is able to upload a video of >50mb - Reported by: @akshayasalvi #5918

Closed

Jag96 mentioned this pull request Dec 21, 2021

iOS - Profile - Error message when uploading a photo using camera #6628

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avatar Image Upload Limit #5900

Avatar Image Upload Limit #5900

akshayasalvi commented Oct 15, 2021 •

edited

Loading

parasharrajat Oct 15, 2021

parasharrajat Oct 16, 2021

akshayasalvi Oct 17, 2021

parasharrajat Oct 17, 2021

akshayasalvi Oct 17, 2021

parasharrajat Oct 15, 2021

parasharrajat Oct 15, 2021

parasharrajat Oct 15, 2021

parasharrajat Oct 15, 2021

parasharrajat Oct 15, 2021

parasharrajat Oct 15, 2021

parasharrajat Oct 15, 2021

parasharrajat Oct 15, 2021

parasharrajat Oct 15, 2021

akshayasalvi commented Oct 16, 2021

parasharrajat left a comment

parasharrajat Oct 16, 2021

parasharrajat Oct 16, 2021

akshayasalvi commented Oct 16, 2021

akshayasalvi commented Oct 17, 2021

parasharrajat left a comment

akshayasalvi commented Oct 17, 2021

pecanoro Oct 18, 2021

akshayasalvi Oct 18, 2021

parasharrajat commented Oct 19, 2021

akshayasalvi commented Oct 19, 2021

akshayasalvi commented Oct 19, 2021 •

edited

Loading

parasharrajat left a comment

OSBotify commented Oct 19, 2021

OSBotify commented Oct 23, 2021

OSBotify commented Oct 28, 2021

		@@ -52,6 +54,9 @@ const propTypes = {
		/** Size of Indicator */
		size: PropTypes.string,

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

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

	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.`,

Avatar Image Upload Limit #5900

Avatar Image Upload Limit #5900

Conversation

akshayasalvi commented Oct 15, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshayasalvi commented Oct 16, 2021

parasharrajat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshayasalvi commented Oct 16, 2021

akshayasalvi commented Oct 17, 2021

parasharrajat left a comment

Choose a reason for hiding this comment

akshayasalvi commented Oct 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat commented Oct 19, 2021

akshayasalvi commented Oct 19, 2021

akshayasalvi commented Oct 19, 2021 • edited Loading

parasharrajat left a comment

Choose a reason for hiding this comment

OSBotify commented Oct 19, 2021

OSBotify commented Oct 23, 2021

OSBotify commented Oct 28, 2021

akshayasalvi commented Oct 15, 2021 •

edited

Loading

akshayasalvi commented Oct 19, 2021 •

edited

Loading