Add avatar upload density constraints #13899

azimgd · 2022-12-30T00:47:00Z

Details

Create a limit for Avatar resolution and validate based on allowed min-max px resolution range.

Fixed Issues

$ 13006
PROPOSAL: 13006(1367560892)

Tests (Web, Desktop, Android, mWeb Android)

Save this failing image, and any other that is less than 2048px
Open staging.new.expensify.com.
Open Settings, click on Avatar to open Profile Page.
Click on Edit photo and upload the below image
Try to scroll the slider to zoom in or zoom out the photo.
Slider is scrolled smoothly without any delay
Display an error message if a user is uploading a file/size too large (min: 80px, max: 2048px), images that are in allowed range should proceed to crop avatar screen
Ensure there are no animation (scroll) glitches between pick image and crop avatar screens

Verify that no errors appear in the JS console

Offline tests

Same as Tests

QA Steps

Same as Tests
Try various images with px density

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

WEB.mov

Mobile Web - Chrome

Screen.Recording.2023-01-02.at.23.27.49.mov

Mobile Web - Safari

Simulator.Screen.Recording.-.iPhone.13.-.2023-01-02.at.23.01.05.mp4

Desktop

Screen.Recording.2023-01-02.at.23.15.00.mov

iOS

Simulator.Screen.Recording.-.iPhone.13.-.2023-01-02.at.22.59.00.mp4

Android

Screen.Recording.2023-01-02.at.22.11.44.mov

(copied from sobit)

…ete function

melvin-bot · 2022-12-30T00:47:27Z

@MonilBhavsar Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

sobitneupane · 2022-12-30T05:04:33Z

src/CONST.js

@@ -32,6 +32,11 @@ const CONST = {
    // Minimum width and height size in px for a selected image
    AVATAR_MIN_WIDTH_PX: 80,
    AVATAR_MIN_HEIGHT_PX: 80,
+
+    // Maximum width and height size in px for a selected image
+    AVATAR_MAX_WIDTH_PX: 2048,


We are having issues when avatar height or width is greater than around 4500/4800 pixels.

Facebook uses the following resolution:

FB Avatar size:
Max Size: 2048px X 2048px
Min Size: 168px X 168px
Aspect ratio: 1:1

So, max size is set 2048px here. @tgolen Are we okay with this or want to increase this size?

While I’m also testing various resolutions on my end, could you help me with testing e.g. 4096 on different platforms including real mobile device ? @sobitneupane

We are having issues when avatar height or width is greater than around 4500/4800 pixels.

@sobitneupane could you please expand on what kind of issues we have seen?

If I'm not wrong, looks like 2048 is coming from this reference?
And, comparing to other platforms, it seems more than enough. I would be down to decrease it, but not increase it.

@shawnborton can we please have your thoughts in this case

We are having issues when avatar height or width is greater than around 4500/4800 pixels.

@sobitneupane could you please expand on what kind of issues we have seen?

This issue was reported for images larger than 5000 pixels. (https://expensify.slack.com/archives/C049HHMV9SM/p1669089889220939)

If I'm not wrong, looks like 2048 is coming from this reference? And, comparing to other platforms, it seems more than enough. I would be down to decrease it, but not increase it.

With 2048 px AVATAR_MAX_WIDTH_PX, users might face difficulty uploading profile avatar with Take photo option (Upload photo> Take photo) in native because image resolution might be greater than 2048px.

RPReplay_Final1672660161.MP4

Thank you, got it!
I did some testing on Whatsapp, and looks like it compresses the image as per their accepted criteria.
I feel like we should do the same, and not ask user to compress the image according to our standards.
If we increase the limit for now to allow photos captured directly through camera, it doesn't really fix the issue, no? or it will create a new issue - "Users on this device can't upload profile picture directly through camera"?

Just chiming in to say I agree with Monil's line of thought above. 2048 seems fine to me assuming we don't accidentally block things like camera uploads.

Why did we choose 2048? Why wouldn't we just choose the max size until we know there can be an issue with some native devices (eg. 4400)? I think that before we consider pre-compression, we should understand what the resolution is of native camera uploads first.

FYI - By default my iPhone 13 Pro takes 3024 × 4032 so I agree 2048 is too small to block on.

I can:

Increase max resolution to: 4096px

Set max resolution for camera upload to: 4096px

https://github.com/react-native-image-picker/react-native-image-picker#options

Oh, cool. I was wondering if we could specify a max for the camera.

src/languages/en.js

src/components/AvatarWithImagePicker.js

azimgd · 2023-01-02T10:38:48Z

Hey @MonilBhavsar could you help us reviewing this PR.

MonilBhavsar · 2023-01-02T10:51:51Z

Yes, and also requesting review from @tgolen

azimgd · 2023-01-02T10:52:54Z

Would be cool if we can get this easy PR merged today.

sobitneupane · 2023-01-02T11:00:30Z

Hello @MonilBhavsar, do we want to increase AVATAR_MAX_WIDTH_PX/AVATAR_MAX_HEIGHT_PX or keep it 2048? #13899 (comment)

MonilBhavsar · 2023-01-02T11:31:26Z

Commented in that thread

sobitneupane · 2023-01-02T12:19:24Z

Screenshots/Videos

Web

Screen.Recording.2023-01-02.at.16.56.11.mov

Mobile Web - Chrome

Screen.Recording.2023-01-02.at.22.08.39.mov

Mobile Web - Safari

Untitled.mov

Desktop

Screen.Recording.2023-01-02.at.22.20.26.mov

iOS

Untitled.mov

Android

Screen.Recording.2023-01-02.at.22.11.44.mov

MonilBhavsar

Commented on the thread, but I feel like this will create a new issue and add overhead to the user experience when uploading images that are captured using latest smartphones

azimgd · 2023-01-02T14:30:53Z

Here is the comment from @tgolen

I think we should concentrate on fixing the bug first, in the most simple way we can. If at some point in the future we want to add precompression, then we can do that as a feature request. As of right now, no one is requesting precompression specifically.

#13006 (comment)

azimgd · 2023-01-02T14:40:28Z

@MonilBhavsar So we all agreed on this solution with validating image resolution, would be cool if we can get this reviewed today.

tgolen · 2023-01-02T15:51:40Z

The full checklist needs to be filled out in the PR description and there is no reviewer checklist posted yet. I will review the code this morning.

tgolen · 2023-01-02T15:55:31Z

src/components/AvatarWithImagePicker.js

+            && (resolution.height <= CONST.AVATAR_MAX_HEIGHT_PX && resolution.width <= CONST.AVATAR_MAX_WIDTH_PX)
+        );
+
+        return getImageResolution(image).then(isResolutionWithinRange);


Can you please update this code so that there is no isResolutionWithinRange variable? The method can be defined directly in then() and it is easier to read and follow.

Sure, fixed.

tgolen · 2023-01-02T15:56:38Z

src/libs/fileDownload/getImageResolution.native.js

@@ -0,0 +1,11 @@
+/**
+ * Returns image dimensions for image selected from react-native-image-picker


Please add more of an explanation here of why it's OK for this method to be a no-op (since the height and width are already included with the image object)

tgolen · 2023-01-02T15:58:06Z

src/libs/fileDownload/getImageResolution.js

@@ -0,0 +1,28 @@
+/**
+ * Returns image dimensions for picked File object
+ * File object is returned as a result of a user selecting files using the <input> element


Please add more of an explanation here for the platform-specific usage of this file such as this being the best way to get the height/width of an image on web, and you can also explain why you used onload vs the other option you were considering (I remember you mentioned something about performance).

tgolen · 2023-01-02T15:59:01Z

It also looks like there are commits on this branch that are not signed, so it cannot be merged until that's fixed.

azimgd · 2023-01-02T18:46:58Z

@tgolen Increased allowed width / height to 4096px, tested everything locally and addressed all comments. Ready for re-review.

tgolen

Looks great! I love the comments. Nice work on those

tgolen · 2023-01-02T19:19:21Z

@sobitneupane I see you tested with some screenshots, were you also going to post the full checklist?

sobitneupane

@azimgd When I try to upload avatar larger than 4096px in iOS/Safari, neither resolutionContrainsts message is shown nor the image is uploaded.

Untitled.mov

azimgd · 2023-01-03T05:57:48Z

I’m pretty sure I tested it on ios/safari and it scaled down image to 2048/2047 automatically.

#13899 (comment)

Checking again.

azimgd · 2023-01-03T07:45:35Z

I was able to reproduce this on production. Can you try it as well please ?

sobitneupane · 2023-01-03T08:41:22Z

I was able to reproduce this on production. Can you try it as well please ?

Yes, I was able to reproduce it on production as well.

sobitneupane

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

azimgd · 2023-01-03T09:29:29Z

New issue where avatar crop screen won't open for large image was reported here: https://expensify.slack.com/archives/C049HHMV9SM/p1672734548804589 cc: @tgolen

src/languages/es.js

Co-authored-by: Rocio Perez-Cano <pecanoro@users.noreply.github.com>

OSBotify · 2023-01-03T15:57:38Z

✋ 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 · 2023-01-04T03:35:38Z

🚀 Deployed to staging by @tgolen in version: 1.2.48-0 🚀

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

OSBotify · 2023-01-05T16:35:47Z

🚀 Deployed to production by @luacmartins in version: 1.2.48-2 🚀

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

OSBotify · 2023-01-06T19:28:46Z

🚀 Deployed to production by @luacmartins in version: 1.2.49-0 🚀

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

azimgd added 5 commits December 30, 2022 00:51

check image constraints when image is selected at avatar picker

98ecfd3

export getImageResolution into platform specific file

b3eb495

rename getImageBlobResolution -> getImageResolution and removed obsol…

365d10a

…ete function

es translation fix

ad21504

increase max avatar size to 2048px

2ec523a

azimgd requested a review from a team as a code owner December 30, 2022 00:47

melvin-bot bot requested review from MonilBhavsar and removed request for a team December 30, 2022 00:47

sobitneupane reviewed Dec 30, 2022

View reviewed changes

src/components/AvatarWithImagePicker.js Outdated Show resolved Hide resolved

fix english message translation

ab1f5b7

MonilBhavsar requested a review from tgolen January 2, 2023 10:51

MonilBhavsar reviewed Jan 2, 2023

View reviewed changes

tgolen requested changes Jan 2, 2023

View reviewed changes

azimgd added 2 commits January 2, 2023 21:11

fix obsolete getImageResolution import

ca3f8fa

fix getImageResolution function comment

b0a46ea

azimgd force-pushed the avatar-image-constraints branch from 061b00c to b0a46ea Compare January 2, 2023 16:12

azimgd added 2 commits January 2, 2023 21:16

remove isResolutionWithinRange variable

a51823c

fix getImageResolution comments

da169c5

tgolen previously approved these changes Jan 2, 2023

View reviewed changes

AndrewGable previously approved these changes Jan 2, 2023

View reviewed changes

sobitneupane reviewed Jan 3, 2023

View reviewed changes

sobitneupane previously approved these changes Jan 3, 2023

View reviewed changes

MonilBhavsar previously approved these changes Jan 3, 2023

View reviewed changes

pecanoro requested changes Jan 3, 2023

View reviewed changes

src/languages/es.js Outdated Show resolved Hide resolved

Update src/languages/es.js

b447cbf

Co-authored-by: Rocio Perez-Cano <pecanoro@users.noreply.github.com>

azimgd dismissed stale reviews from MonilBhavsar, sobitneupane, AndrewGable, and tgolen via b447cbf January 3, 2023 12:31

pecanoro approved these changes Jan 3, 2023

View reviewed changes

MonilBhavsar approved these changes Jan 3, 2023

View reviewed changes

sobitneupane approved these changes Jan 3, 2023

View reviewed changes

tgolen approved these changes Jan 3, 2023

View reviewed changes

tgolen merged commit 2039b64 into Expensify:main Jan 3, 2023

OSBotify mentioned this pull request Jan 4, 2023

Deploy Checklist: New Expensify 2023-01-04 #13976

Closed

18 tasks

mallenexpensify mentioned this pull request Jan 9, 2023

[HOLD for payment 1/12][$8000] Profile - Create a limit for Avatar file types or size #13006

Closed

jayeshmangwani mentioned this pull request Nov 7, 2023

[$500] Profile- No error message when the image is bigger that the required pixel limit #30904

Closed

6 tasks

		@@ -0,0 +1,11 @@
		/**
		* Returns image dimensions for image selected from react-native-image-picker

Add avatar upload density constraints #13899

Add avatar upload density constraints #13899

Conversation

azimgd commented Dec 30, 2022 • edited Loading

Details

Fixed Issues

Tests (Web, Desktop, Android, mWeb Android)

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Dec 30, 2022

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

azimgd commented Jan 2, 2023

MonilBhavsar commented Jan 2, 2023

azimgd commented Jan 2, 2023

sobitneupane commented Jan 2, 2023 • edited Loading

MonilBhavsar commented Jan 2, 2023

sobitneupane commented Jan 2, 2023 • edited Loading

Screenshots/Videos

MonilBhavsar left a comment

Choose a reason for hiding this comment

azimgd commented Jan 2, 2023

azimgd commented Jan 2, 2023

tgolen commented Jan 2, 2023

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

tgolen commented Jan 2, 2023

azimgd commented Jan 2, 2023

tgolen left a comment

Choose a reason for hiding this comment

tgolen commented Jan 2, 2023

sobitneupane left a comment

Choose a reason for hiding this comment

azimgd commented Jan 3, 2023 • edited Loading

azimgd commented Jan 3, 2023

sobitneupane commented Jan 3, 2023

sobitneupane left a comment

Choose a reason for hiding this comment

Reviewer Checklist

azimgd commented Jan 3, 2023

OSBotify commented Jan 3, 2023

OSBotify commented Jan 4, 2023

OSBotify commented Jan 5, 2023

OSBotify commented Jan 6, 2023

azimgd commented Dec 30, 2022 •

edited

Loading

sobitneupane commented Jan 2, 2023 •

edited

Loading

sobitneupane commented Jan 2, 2023 •

edited

Loading

azimgd commented Jan 3, 2023 •

edited

Loading