Allow multiple avatar image types by Beamanator · Pull Request #12549 · Expensify/App

Beamanator · 2022-11-08T11:42:04Z

Details

The main purpose of this PR is to allow users to upload multiple types of images as avatars (workspace & user avatars). Ideally we'd support at least jpg, jpeg (which are the same as jpg), gif, png, and bmp. However, I investigated some errors found by @Santhosh-Sellavel here (my investigation here) where I found that RNImageManipulator currently only supports png and jpg image types so on native we can currently only upload those types of avatar images on native.

In order to not let this PR stay open for too long, I'm proposing we don't worry about those cases for now, and we create follow up issues to support those image types in that Library, or at least investigate what we should do for those image types (maybe we'd be ok just saving them as jpgs? To be discussed later).

Fixed Issues

$ #11063

Tests

Download sample jpg, jpeg, gif, and bmp files from this site: https://filesamples.com/formats/bmp (or use your own if you have any). Also get a png file with some transparent part (use this image if you need: https://user-images.githubusercontent.com/3885503/196921487-45bf3d63-7e42-4f4c-83d1-e8b40e519bf6.png)
On Web / Desktop / iOS Safari / Android Chrome, try uploading each of them as your profile avatar or a workspace avatar, one at a time
For each, verify the upload succeeds & the URL for the image has the correct extension (ex: for the png image, the image URL should end with .png)
- For the png, make sure the image has some transparent part, and that the transparency is preserved.
- Note: jpg and jpeg are the same so uploading a jpg file will change to a jpeg file extension
- Note: You can view the server response easily by searching the console for messages with onyxApiUpdate
On Native devices / Apps, gif and bmp files have issues (see investigation) so please don't worry about testing those image types on those platforms, we'll handle them in a follow-up issue. For this PR, follow steps 2 & 3 for Native devices, trying to upload jpg, jpeg, and png images only

Offline

Have two devices logged in to the same account
On one device, disconnect from the internet
Upload a new avatar (user or workspace)
Verify the avatar appears grayed out
Connect to internet, verify the avatar uploads & updates on both devices
Verify the uploaded image URL has the correct file extension (via tips above)

Verify that no errors appear in the JS console

QA Steps

Same as above tests - Note: after uploading on mobile, you can use Chrome to check that the uploaded image URL is correct (has the correct extension at the end) like this:

Open the chrome developer tools (Command + Option + I)
Open "Application" -> IndexedDB -> OnyxDB (click "Refresh Database")
Expand OnyxDB, click keyvaluepairs, search for key personalDetails, expand your username, find avatar
Verify the URL has the same extension at the end as the image type you just uploaded. Example:
- Note: jpg and jpeg are the same so uploading a jpg file will change to a jpeg file extension

Verify that no errors appear in the JS console

PR Author Checklist

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

Screenshots

Web & iOS App

Screen.Recording.2022-11-11.at.12.02.09.PM.mov

Mobile Web - Chrome & Mobile Web - Safari

Screen.Recording.2022-11-11.at.12.26.09.PM.mov

Desktop & Android

Screen.Recording.2022-11-11.at.3.11.48.PM.mov

Beamanator · 2022-11-08T12:01:22Z

Will add screenshots once https://github.com/Expensify/Web-Expensify/pull/35489 is merged

…icAvatarImageTypes

Beamanator · 2022-11-11T19:58:51Z

src/components/AvatarWithImagePicker.js

+                    isAvatarCropModalOpen: true,
+                    imageUri: image.uri,
+                    imageName: image.name,
+                    imageType: image.type,


FYI this (image type) is the main "new thing" that we're passing down to cropOrRotateImage

Beamanator · 2022-11-11T19:59:33Z

src/libs/cropOrRotateImage/index.js

 function convertCanvasToFile(canvas, options = {}) {
    return new Promise((resolve) => {
        canvas.toBlob((blob) => {
-            const file = new File([blob], `${options.name || 'fileName'}.jpg`, {type: 'image/jpeg'});


Idk why we were appending .jpg to every file name before 😅

melvin-bot · 2022-11-11T23:15:32Z

@Santhosh-Sellavel @jasperhuangg One of you needs to 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]

Beamanator · 2022-11-11T23:17:00Z

Ready for review! Have fun testing uploading images :D

Santhosh-Sellavel · 2022-11-14T17:42:01Z

@Beamanator jpg is shown jpeg refer to the video below!

Initially, I checked jpeg extension, looks correct. Started recording from there testing in the following order gif, jpg, bmp.

Look at the timestamp 0:22 to 0:40 in the video below, that's where I'm checking .jpg extension.

Screen.Recording.2022-11-14.at.11.03.27.PM.mov

Santhosh-Sellavel

#12549 (comment)

Beamanator · 2022-11-14T21:49:20Z

@Santhosh-Sellavel interesting catch, I'll check that out now!

Beamanator · 2022-11-14T21:56:25Z

@Santhosh-Sellavel apparently JPG and JPEG are the same actually 😅

I found lots of articles explaining, here's just one: https://www.howtogeek.com/796389/jpg-vs.-jpeg-are-they-the-same-thing/

So I don't think this is a blocker 👍

Santhosh-Sellavel · 2022-11-15T00:41:35Z

Let's update that in pr description jpg/jpeg will shown as jpeg in url

Beamanator · 2022-11-15T18:32:30Z

Ok I'll update the OP @Santhosh-Sellavel but can you please finish your review & add the checklist?? 🙏

Santhosh-Sellavel · 2022-11-16T16:31:26Z

Will test today, but this PR is time-consuming 😅

Santhosh-Sellavel · 2022-11-16T17:10:14Z

avatarHighResolution is not set properly while changing the image, but on force reload it gets updated.

Screen.Recording.2022-11-16.at.10.36.42.PM.mov

Santhosh-Sellavel · 2022-11-16T17:41:38Z

@Beamanator

Offline case does not work as expected

Offline_Desktop_Web.mov

Santhosh-Sellavel · 2022-11-16T18:06:44Z

Error is thrown while uploading BMP in iOS

Simulator.Screen.Recording.-.iPhone.14.-.2022-11-16.at.23.35.16.mp4

GIF extension not maintained while testing in iOS refer here

MObile_Test.mov

Console error is thrown while uploading gif in Android, unable to upload here.

cc: @Beamanator

Beamanator · 2022-11-16T19:14:21Z

Wow thanks so much for the thorough tests @Santhosh-Sellavel - I'd definitely recommend requested increased compensation for this review :D

Sadly I'm going OOO till Monday so I won't be able to fix the items you mentioned till early next week

Santhosh-Sellavel · 2022-11-30T15:45:52Z

@Beamanator 😞 Sorry, I was waiting for the discussion outcome! Will add it soon!

Santhosh-Sellavel · 2022-11-30T15:46:33Z

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:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- 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 was added in all src/languages/* files
- 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 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.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Web.mov

Mobile Web - Chrome

Android.mWeb.mov

Mobile Web - Safari

iOS_All_Files_as_JPEG.mov

Desktop

Desktop.mov

iOS

iOS.mov

Android

Android.mov

Beamanator · 2022-11-30T17:07:51Z

No worries @Santhosh-Sellavel thanks for sticking with me 😅

Santhosh-Sellavel · 2022-11-30T18:41:39Z

AH Finally completed, After double-checking multiple times, crazy PR!

Tested Formats PNG, GIF, JPEG, and BMP.

Web, Desktop - all works fine maintained file typed even after uploading. Uploaded Images Loading fine on all platforms.

Android & iOS - Only tested PNG and JPEG. All is good here too.

Mobile Web things went crazy here, after verifying multiple times

Android mWeb

Old Story, Android Chrome uploading works after updating the chrome from the play store.

PNG, GIF, JPEG - Formats works as expected file types are maintained after upload.
BMP - Able to upload, but seems converted as JPEG and uploaded.

Android_Mweb_bmp.mov

iOS mWeb

Able to upload all formats, but all are converted as JPEG and uploaded.

Is converting to jPEG an issue? @Beamanator

iOS_All_Files_as_JPEG.mov

Santhosh-Sellavel

Checklist: #12549 (comment)

Need clarification on this #12549 (comment), otherwise looks good!

Beamanator · 2022-12-01T09:57:52Z

Thanks again for the comprehensive notes & review @Santhosh-Sellavel - the one thing I don't like about turning all image types into jpeg is if we get rid of the transparency of PNGs - I think that's my only "concern" / blocker. I'll test that on android Chrome & iOS safari - if that works fine I think we're good to move forward, otherwise maybeeeee not

Beamanator · 2022-12-01T10:15:35Z

Hmm so I just tested iPhone 13 simulator (iOS v15.5), Safari - and I successfully uploaded a .png, a.k.a. the file type in the avatar URL remained .png - sorry to be super annoying but are you 100% sure your image that says .png is actually a .png?

Actually the funny thing is on the same iPhone ^ sim, I uploaded bmp & gifs, and the URL extensions were correct 🤷 (same as what I reported here). Maybe @jasperhuangg can help us test just on this platform to see what results he gets?

Tested Android Chrome (updated my simulator's Chrome app & SDK version too): Got same result as you (everything worked, except .bmp got converted to .jpeg - which is fine for now

Beamanator · 2022-12-01T10:40:27Z

So I'd say the next steps should be:

@jasperhuangg (or someone else if you don't have time) test uploading png, bmp & gif images as user or workspace avatars on iOS Safari (note: this site can help you download images of specific file types: https://filesamples.com/formats/bmp)
If at least png files can be uploaded fine & have the correct extension after upload, we're good to merge
If not, we might have to figure out why it works for me & doesn't work for others 😅

Santhosh-Sellavel · 2022-12-01T14:30:00Z

Please try in latest iOS Version. I tested on 16.1

Santhosh-Sellavel · 2022-12-01T14:30:27Z

We are in a never ending loop here.

Beamanator · 2022-12-01T15:36:10Z

Lol! Ok I'll work on getting upgraded sims 👍

Beamanator · 2022-12-01T15:39:43Z

Hah I don't see iOS v16.1 available for my simulators - maybe I need to update xcode?

Beamanator · 2022-12-01T15:41:05Z

@Santhosh-Sellavel are you testing on a simulator with iOS 16.1 or a physical device? Please let me know which device as well

Santhosh-Sellavel · 2022-12-01T16:07:13Z

@Beamanator NVM it's working as expected for me now on iOS mWEB, I'm not sure what I did wrong yesterday. I did the same thing today but verified it on the actual device's local storage of iOS mWeb. As updating in one platform should update in another platform I used desktop app storage, which worked right for all other platforms. Maybe it had some sync issues, as my network was also unstable yesterday. So All looks good!

PNG_BMP

BMP_PNG_iOS_mWEB.mov

JPEG_GIF

JPEG_GIF_iOSMWeb.mov

Santhosh-Sellavel · 2022-12-01T16:10:11Z

@Beamanator But BMP to JPEG on Android mWEB still occurring for me though, I believe we are okay with it!

Screen.Recording.2022-12-01.at.9.38.32.PM.mov

Santhosh-Sellavel

Looks good to me!

jasperhuangg · 2022-12-01T17:59:13Z

Hey @Beamanator! I was able to successfully upload a png image on iOS Safari, and I was able to verify that the file extension of the uploaded file is still png. I think the transparency is preserved?

I also repeated with bmp and gif and it seems to be working fine.

The only issue I'm seeing is that the background of the cropping modal is black, which can conflict with transparent images that have black elements (as in the recording). This can probably be fixed in a followup issue.

Untitled.mov

jasperhuangg

Looks good!

jasperhuangg · 2022-12-01T18:02:52Z

Merging via [2] from #12549 (comment)

OSBotify · 2022-12-01T18:07:45Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

Beamanator · 2022-12-01T18:34:43Z

Niceee thanks y'all 👍 Will look at the other weird things y'all mentioned tomorrow 😅

Beamanator · 2022-12-02T10:07:04Z

@jasperhuangg - FYI i made a new issue about the black background when editing images here: #13266

Feel free to add your thoughts, I referenced your vid!

OSBotify · 2022-12-02T19:43:54Z

🚀 Deployed to staging by @jasperhuangg in version: 1.2.36-0 🚀

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

OSBotify · 2022-12-08T02:03:57Z

🚀 Deployed to production by @francoisl in version: 1.2.36-4 🚀

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

Beamanator added 2 commits November 8, 2022 13:40

Pass avatar image type down

510bfe0

Pass image type down & use original filename

8105cb8

Beamanator self-assigned this Nov 8, 2022

Beamanator changed the title ~~Allow multiple avatar image types~~ [HOLD Web-E#35489] Allow multiple avatar image types Nov 8, 2022

Beamanator mentioned this pull request Nov 8, 2022

[HOLD for payment 2022-12-15] [Image] Web - Profile - The white background in the image changes to black #11063

Closed

Lint

b9840f2

Merge branch 'main' of github.com:Expensify/App into beaman-passDynam…

6bb5063

…icAvatarImageTypes

Beamanator commented Nov 11, 2022

View reviewed changes

Beamanator marked this pull request as ready for review November 11, 2022 23:15

Beamanator requested a review from a team as a code owner November 11, 2022 23:15

melvin-bot bot requested review from Santhosh-Sellavel and jasperhuangg and removed request for a team November 11, 2022 23:15

Beamanator changed the title ~~[HOLD Web-E#35489] Allow multiple avatar image types~~ Allow multiple avatar image types Nov 11, 2022

Santhosh-Sellavel reviewed Nov 14, 2022

View reviewed changes

trjExpensify mentioned this pull request Nov 16, 2022

[Tracking][Image][Polish] Image-related feature request and improvements #10894

Closed

39 tasks

Santhosh-Sellavel reviewed Nov 30, 2022

View reviewed changes

Santhosh-Sellavel approved these changes Dec 1, 2022

View reviewed changes

jasperhuangg approved these changes Dec 1, 2022

View reviewed changes

jasperhuangg merged commit ff497b0 into main Dec 1, 2022

jasperhuangg deleted the beaman-passDynamicAvatarImageTypes branch December 1, 2022 18:02

Beamanator mentioned this pull request Dec 2, 2022

[HOLD for payment 2023-01-13] Uploading all-black PNG avatar makes it impossible to see the image #13266

Closed

OSBotify mentioned this pull request Dec 2, 2022

Deploy Checklist: New Expensify 2022-12-02 #13283

Closed

38 tasks

Beamanator mentioned this pull request Feb 2, 2023

[HOLD for payment 2023-03-01] [$1000] Allow avatar image uploads: gif on Android #14751

Closed

Beamanator mentioned this pull request Feb 10, 2023

[HOLD for payment 2023-02-23] [$1000] Allow avatar image uploads: bmp on iOS #15035

Closed

6 tasks

Conversation

Beamanator commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline

QA Steps

PR Author Checklist

PR Reviewer Checklist

Screenshots

Web & iOS App

Mobile Web - Chrome & Mobile Web - Safari

Desktop & Android

Uh oh!

Beamanator commented Nov 8, 2022

Uh oh!

Beamanator Nov 11, 2022

Choose a reason for hiding this comment

Uh oh!

Beamanator Nov 11, 2022

Choose a reason for hiding this comment

Uh oh!

melvin-bot bot commented Nov 11, 2022

Uh oh!

Beamanator commented Nov 11, 2022

Uh oh!

Santhosh-Sellavel commented Nov 14, 2022

Uh oh!

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

Uh oh!

Beamanator commented Nov 14, 2022

Uh oh!

Beamanator commented Nov 14, 2022

Uh oh!

Santhosh-Sellavel commented Nov 15, 2022

Uh oh!

Beamanator commented Nov 15, 2022

Uh oh!

Santhosh-Sellavel commented Nov 16, 2022

Uh oh!

Santhosh-Sellavel commented Nov 16, 2022

Uh oh!

Santhosh-Sellavel commented Nov 16, 2022

Uh oh!

Santhosh-Sellavel commented Nov 16, 2022

Error is thrown while uploading BMP in iOS

GIF extension not maintained while testing in iOS refer here

Console error is thrown while uploading gif in Android, unable to upload here.

Uh oh!

Beamanator commented Nov 16, 2022

Uh oh!

Santhosh-Sellavel commented Nov 30, 2022

Uh oh!

Santhosh-Sellavel commented Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

Beamanator commented Nov 30, 2022

Uh oh!

Santhosh-Sellavel commented Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Android mWeb

iOS mWeb

Uh oh!

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

Uh oh!

Beamanator commented Dec 1, 2022

Uh oh!

Beamanator commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Beamanator commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Santhosh-Sellavel commented Dec 1, 2022

Uh oh!

Beamanator commented Nov 8, 2022 •

edited

Loading

Santhosh-Sellavel commented Nov 30, 2022 •

edited

Loading

Santhosh-Sellavel commented Nov 30, 2022 •

edited

Loading

Beamanator commented Dec 1, 2022 •

edited

Loading

Beamanator commented Dec 1, 2022 •

edited

Loading

Santhosh-Sellavel commented Dec 1, 2022 •

edited

Loading

jasperhuangg commented Dec 1, 2022 •

edited

Loading