Allow multiple avatar image types

[Beamanator]

Details

Fixed Issues

$ https://github.com/Expensify/App/issues/11063

Tests

1. Download sample jpg, jpeg, gif, and bmp files from this site: https://filesamples.com/formats/bmp (or use your own if you have any)
2. Try uploading each of them as your profile avatar or a workspace avatar, one at a time
3. 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 (use this image if you need: https://user-images.githubusercontent.com/3885503/196921487-45bf3d63-7e42-4f4c-83d1-e8b40e519bf6.png)
   • Note: You can view the server response easily by searching the console for messages with onyxApiUpdate

Offline

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

• [x] Verify that no errors appear in the JS console

QA Steps

Same as above - 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:

1. Open the chrome developer tools (Command + Option + I)
2. Open "Application" -> IndexedDB -> OnyxDB (click "Refresh Database")
   • 
3. Expand OnyxDB, click keyvaluepairs, search for key personalDetails, expand your username, find avatar
4. Verify the URL has the same extension at the end as the image type you just uploaded. Example:
   • 

• [ ] Verify that no errors appear in the JS console

PR Author Checklist

• [x] I linked the correct issue in the ### Fixed Issues section above
• [x] I wrote clear testing steps that cover the changes made in this PR
  • [x] I added steps for local testing in the Tests section
  • [x] I added steps for Staging and/or Production testing in the QA steps section
  • [x] I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • [x] 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)
• [x] I included screenshots or videos for tests on all platforms
• [x] I ran the tests on all platforms & verified they passed on:
  • [x] iOS / native
  • [x] Android / native
  • [x] iOS / Safari
  • [x] Android / Chrome
  • [x] MacOS / Chrome
  • [x] MacOS / Desktop
• [x] I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• [x] I followed proper code patterns (see Reviewing the code)
  • [x] 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)
  • [x] I verified that comments were added to code that is not self explanatory
  • [x] 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.
  • [x] I verified any copy / text shown in the product was added in all src/languages/* files
  • [x] 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.
  • [x] 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.
  • [x] I verified the JSDocs style guidelines (in STYLE.md) were followed
• [x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• [x] I followed the guidelines as stated in the Review Guidelines
• [x] I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• [x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• [x] 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
• [x] If a new component is created I verified that:
  • [x] A similar component doesn't exist in the codebase
  • [x] All props are defined accurately and each prop has a /** comment above it */
  • [x] The file is named correctly
  • [x] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • [x] The only data being stored in the state is data necessary for rendering and nothing else
  • [x] 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)
  • [x] 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)
  • [x] All JSX used for rendering exists in the render method
  • [x] 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
• [x] If a new CSS style is added I verified that:
  • [x] A similar style doesn't already exist
  • [x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• [x] 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)
• [x] 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.
• [x] I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

PR Reviewer Checklist

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

• [ ] 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

Web & iOS App

https://user-images.githubusercontent.com/3885503/201423215-13cccadf-eddb-4096-9777-5e1e3e92d347.mov

Mobile Web - Chrome & Mobile Web - Safari

https://user-images.githubusercontent.com/3885503/201441006-c1b8870c-5fe9-4b01-990e-628ba5543123.mov

Desktop & Android

https://user-images.githubusercontent.com/3885503/201443023-6f39d84a-4bb0-43e4-b374-df160b992aa7.mov

[Beamanator]

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

[melvin-bot[bot]]

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

Ready for review! Have fun testing uploading images :D

[Santhosh-Sellavel]

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

https://user-images.githubusercontent.com/85645967/201728207-82c97f64-3f6d-4ac4-8c59-921db7146602.mov

[Beamanator]

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

[Beamanator]

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

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

[Beamanator]

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

[Santhosh-Sellavel]

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

[Santhosh-Sellavel]

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

https://user-images.githubusercontent.com/85645967/202246685-854b8420-fba4-4458-ab56-280cef55b57f.mov

[Santhosh-Sellavel]

@Beamanator

Offline case does not work as expected

https://user-images.githubusercontent.com/85645967/202253737-8d9b3e69-c3f8-4d9b-b1ab-de8c4c63b7a7.mov

[Santhosh-Sellavel]

Error is thrown while uploading BMP in iOS

https://user-images.githubusercontent.com/85645967/202258729-5d48fb94-727f-4e13-81cb-624a55ab4571.mp4

GIF extension not maintained while testing in iOS refer here

https://user-images.githubusercontent.com/85645967/202258031-2d5d05c1-51db-4684-b670-2c19122eed03.mov

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

cc: @Beamanator

[Beamanator]

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

[Beamanator]

Ok Finally trying to get to your review comments, sorry for the delay @Santhosh-Sellavel !

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

This is interesting, I believe avatarHighResolution is an old Onyx key and @jasperhuangg and I forgot to remove this in the back-end 🤔 I don't see it used in App anywhere currently so as long as there's no bugs here I think we can leave this be, and I'll work with @jasperhuangg to remove it from the back-end in a follow-up PR - made this internal issue: https://github.com/Expensify/Expensify/issues/244808

Offline case does not work as expected

Interesting, I wasn't able to reproduce 🤔 Maybe because I pulled main? Would you please try that case again and let me know?

Error is thrown while uploading BMP in iOS

Innnteresting, seems this is an issue on iOS 15? https://developer.apple.com/forums/thread/715255

My simulator is 15.5 but in that link someone reported the bug on 15.7 🤔

It looks like we use https://github.com/react-native-image-picker/react-native-image-picker for the image picker on native, so I am thinking we can make a follow-up issue to get this fixed (maybe it's a bug in that 3rd party lib, maybe it's a bug somewhere else). I don't really want to slow this down a lot based on this error, what do you think? Maybe we could just not let BMPs be selectable on iOS for now?

GIF extension not maintained while testing in iOS refer here

Interesting, one time I debugged, I found the picture i thought was a .gif was sent as type "application/octet-stream" to the server side, I wonder if that's an iOS thing? I see that the library RNImageManipulator is getting the wrong filename & uri - it keeps adding .jpg to the end of each, even for gifs 🤔

@Santhosh-Sellavel from looking at the docs: https://github.com/oguzhnatly/react-native-image-manipulator#arguments - I see the only 3 saveOptions (3rd param for RNImageManipulator.manipulate) are compress, format, and base64, and it looks like format only allows the options for jpg and png 😢 I think we either leave it as is, or we don't allow BMP / GIF for iOS for now, and make a separate issue to try to enable those formats in the future.

How are you seeing devtools for iOS native app by the way? When I try to open Debugger or React DevTools I see these errors (below) - did you install Flipper?

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

Hmm this is also a weird one, I found that it's failing in https://github.com/oguzhnatly/react-native-image-manipulator - just by adding a .catch(console.error) at the end of RNImageManipulator.manipulate - since gifs are also a problem with iOS, maybe it's a related android error - also something I'm thinking about postponing to a different issue (& blocking gif uploads for all native devices

[Santhosh-Sellavel]

How are you seeing devtools for iOS native app by the way? When I try to open Debugger or React DevTools I see these errors (below) - did you install Flipper?

Weird I'm too seeing the same error, I didn't need that most of the time. Never stuck with the iOS-only issue so mostly used the web to debug also android works. As both android & iOS nearly freezes up my system I never do that mostly.

😃 So just use old-school methods just use Log statements

[Santhosh-Sellavel]

From here,

My to-do is just verifying offline cases again. Please update the PR description about what to ignore from testing, I'll get to it tomorrow thanks!

If I am missing anything let me know @Beamanator

[Beamanator]

@Santhosh-Sellavel good call I can just Log next time 😅

I updated the testing steps, noting that gif and bmp currently don't work well on Native devices / Apps, so we'll deal with those in a follow-up issue.

Note: I was able to get GIF and BMP images to work on iOS Safari, but when I tried on Android Chrome, clicking "Upload image" did nothing 😅 Like I didn't even get a document / image picker 🤔 I assume you will / have already come across this when testing this PR, could you drop a comment to let me know if you hit this bug or it's just me, and also if you DON'T hit that bug, are you able to upload GIF and BMP images on Android Chrome? (Note: I've been making sure the uploaded image format works by keeping a web instance running at the same time, so when I upload on iOS Safari, the new URL gets sent to web where I can easily debug)

Follow-up issue to fix Android / iOS issues: https://github.com/Expensify/App/issues/13038

[Beamanator]

Bump @Santhosh-Sellavel if you have time today please test this time consuming PR 😅 🙏

[Santhosh-Sellavel]

please test this time consuming PR 😅

😄

[Santhosh-Sellavel]

but when I tried on Android Chrome, clicking "Upload image" did nothing 😅 Like I didn't even get a document / image picker 🤔 I assume you will / have already come across this when testing this PR

Yeah, I would have stopped after testing mWeb safari, yeah this issue occurring on the android emulator

https://user-images.githubusercontent.com/85645967/204358242-e46c1a5f-6f3f-4fa3-8c8e-01feef7ed7c7.mov

No error logs as well.

https://user-images.githubusercontent.com/85645967/204359444-da0abd5e-15a0-4c4b-8533-187fa77eeca7.mov

More Investigation

Thought it was due to the old chrome version, but it occurring other versions too.

Maybe this is the emulator-only issue, I tested it on the real device it's working.

[Santhosh-Sellavel]

Offline case does not work as expected

Interesting, I wasn't able to reproduce 🤔 Maybe because I pulled main? Would you please try that case again and let me know?

Issue still occurs on the web

https://user-images.githubusercontent.com/85645967/204363707-6d0ae1fb-2600-41cf-97f4-39b5d0e60a03.mov

[Beamanator]

@Santhosh-Sellavel I wonderrrr if the weird behavior you're seeing is because you have "Disable caching" checked?

[Santhosh-Sellavel]

Yeah might be, good catch! AFK now, will test again later and let you know. We should be good to merge this today!

[Beamanator]

Hopeeeeefully that's the source of your offline issues 🤔 And sounds good, I'll see the results tomorrow morning probably! 😅

[Santhosh-Sellavel]

@Santhosh-Sellavel I wonderrrr if the weird behavior you're seeing is because you have "Disable caching" checked?

@Beamanator That's the issue. We should show disabled everywhere or just on the profile page?

https://user-images.githubusercontent.com/85645967/204562210-cd7a4cd6-9bac-47c6-89f0-811870007773.mov

[Beamanator]

Aah good question @Santhosh-Sellavel ! By design, just on the Profile page - but maybe we can take a follow-up to check if the profile avatar in the LHN should also be grayed out when we upload offline! Good Q!

[jasperhuangg]

@Beamanator @Santhosh-Sellavel Hey guys, just catching up here. Thanks for all the discussion, and good job working out so many random kinks with file uploads, I can only imagine how tedious it was!

Just to confirm I agree that we should hold gifs/bmps to a different issue. Those definitely seem like an edge case, especially on mobile devices where people don't often work with those types of file types.

[jasperhuangg]

By design, just on the Profile page - but maybe we can take a follow-up to check if the profile avatar in the LHN should also be grayed out when we upload offline!

I feel like if we can get a quick second opinion on this from design soon we can hopefully knock this out today. Otherwise, NAB and we can hold to a separate issue.

@Expensify/design When we upload an avatar image offline we grey out the avatar shown on the profile page. Should we also grey out the avatars shown in all other places (initial settings page, LHN, etc)? See the video below for context:

Screen.Recording.2022-11-29.at.8.23.23.PM.mov

[shawnborton]

Hmm I guess that would make sense from a consistency stand point. What do you think?

[Beamanator]

Hmm I guess that would make sense from a consistency stand point. What do you think?

Trueee, here's all the places I could think of that would need to have the new gray profile picture:

1. In chat reports
2. Settings button in LHN
3. User details page - for yourself
4. Initial Settings page
5. ~Profile page~ (already done)