[HOLD for payment 2023-02-23] [$1000] Figure out why `svg` images don't show immediately when uploading as avatar

[Beamanator]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. In main, add 'svg' to the list of allowed avatar extensions in const AVATAR_ALLOWED_EXTENSIONS: ['jpg', 'jpeg', 'png', 'gif', 'bmp'],
2. On Android or iOS App, navigate to Profile page
3. Click avatar, choose "Upload photo"
4. Select a svg image, then continue through the crop modal to upload the image

Expected Result:

User can immediately see the avatar image that they just uploaded

Actual Result:

• Image doesn't show up until it uploads to the server and the URL is returned
• User needs to refresh to see the image

Workaround:

None needed

Platform:

Where is this issue occurring?

• All

Version Number: Reproducible in staging?: Y Reproducible in production?: Y GH conversation: https://github.com/Expensify/App/pull/12549#issuecomment-1326579036

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01953030ea34ada712
• Upwork Job ID: 1622513437122138112
• Last Price Increase: 2023-02-06

[melvin-bot[bot]]

Triggered auto assignment to @arielgreen (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[Beamanator]

While this issue currently exists, I would prefer waiting till https://github.com/Expensify/App/pull/12549 gets merged before working on this (most likely can be external) since in that PR we're enabling other image formats to be uploaded

[Beamanator]

https://github.com/Expensify/App/pull/12549 just got merged! I thinkkkkk we should still wait for it to get to Production, just in case there's any regressions found

[JmillsExpensify]

Additionally, now that #12549 just hit production, we should remove the hold. Woo!

[JmillsExpensify]

cc @trjExpensify Since you're on the main tracking issue.

[trjExpensify]

Ah yeah, let me take this over for you @arielgreen. :)

[trjExpensify]

@Beamanator what's the plan? Are you taking this or are we going external? I agree it should come off hold now.

[trjExpensify]

@Beamanator, this issue will hit 4 weeks old after next week and it doesn't seem like we've made any further progress here yet to get to a resolution before then. What are the next steps?

[Beamanator]

True true, I believe next steps are to get this discussion moved to #expensify-open-source to make sure we're aligned on how to proceed

[Beamanator]

Posted here to try to get alignment: https://expensify.slack.com/archives/C01GTK53T8Q/p1671547890266159

[bondydaa]

Assigning myself since beaman is OOO. caught up on that thread and confirmed with Ioni next steps are:

1. confirm if a png is being converted to a jpg or not?
2. if it is converted to jpg then just make sure it's got the correct file extension. If it stays as a png then make it convert to jpg?
3. ensure all file formats listed already adhere to this https://expensify.slack.com/archives/C01GTK53T8Q/p1671558426183609?thread_ts=1671547931.029679&cid=C01GTK53T8Q
   1. Converting everything to JPG (in fact, why wouldn’t we also do that for PNG so that everything is in the same file format?) [note from bondy: except for case 1 if it's happening. converting to png to preserve transparency is a nice-to-have for now]
   2. Supporting BMP since it’s a very common file on Windows (and those computers far outnumber Macs)
   3. Not allowing gif animation in avatars, even if we support uploading that file type.

I'll dig into this tomorrow when I'm back online.

[JmillsExpensify]

My take is that we should add the NewFeature label to this issue. Here's my reasoning: The imagine improvements are like reliable notifications as well as the navigation reboot. They all collective solve "bugs" we've discovered in the app, but they are actually larger architectural plays that are more accurately part of N7. WAQ is simply about solving existing bugs older than 30 days, and this bug was essentially created in the process of adding new capabilities. So yes, it's a bug and we should fix it as part of the image improvements project. But I also don't think it's part of WAQ as we result.

[melvin-bot[bot]]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[melvin-bot[bot]]

Triggered auto assignment to @sakluger (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[JmillsExpensify]

I was chatting with @trjExpensify and he rightly pointed out that we should keep the bug label on this issue for clarity. Though in addition to that, I'm going to add [HOLD WAQ] in the title, because again, this is bug created by an implementation of new features in progress. So there's no way it should hold up WAQ. Adding the hold will help us stay focused on the issues that block N7 and prevent WAQ from being done.

[bondydaa]

okay unassigning myself then and looking for other bugs

[melvin-bot[bot]]

@Beamanator, @trjExpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

[Beamanator]

Ok so if this is on hold I shouldn't prioritize it @JmillsExpensify ? Or should I still make progress on it once i'm back (Monday)? Also if it's on hold I am thinking to drop the priority to Weekly

[trjExpensify]

Are there other bugs on the image improvements hit list that you're currently working on or is this next up?

[Beamanator]

I just submitted a PR for this one today: https://github.com/Expensify/App/issues/13957

So this one is up next!

[trjExpensify]

Dope, let's get it!

[Beamanator]

Part 1 which will convert all images (except pngs) to jpeg is here: https://github.com/Expensify/Web-Expensify/pull/35994

[Beamanator]

Once this is working fine, I'm planning to open this up to contributors to help fix these issues mentioned in the OP:

1. iOS throws errors when uploading BMP
2. iOS converts GIF to JPG in URL of uploaded file
3. Uploading GIF in Android throws console warning, isn't able to upload image

[trjExpensify]

Oh, are those three still going to be a problem when part 1 is done? 😕

[sketchydroide]

https://github.com/Expensify/App/issues/13910 should be fixed or made not an issue by this GH when it's done, closing the other one.

[trjExpensify]

Same question as here for @Beamanator, Melv.

[Beamanator]

Sorry y'all :D Good questions @trjExpensify - I need to do some testing to see if bmp or gif have problems when being uploaded in native devices - I don't know exactly when the errors appeared (before or after trying to upload them as avatars). If the errors happened before uploading, I think the issues could still arise

[trjExpensify]

I need to do some testing to see if bmp or gif have problems when being

Noice, let me know how you get on!

[Beamanator]

Ok well I was able to upload a GIF as a chat attachment, but not a profile photo! I can get to the "Edit photo" avatar crop screen, but then clicking "Save" does nothing - this is what we experienced before and mentioned at the top of the issue.

So that will need to be fixed.

iOS converts GIF to JPG in URL of uploaded file

This is still true, nothing wrong with it :D

iOS throws errors when uploading BMP

This is also still true, but I thinkkkk we can just not let iOS users upload BMP images instead of spending a lot of time and energy getting a "fix" - I believe it's very unlikely that an iOS user will have BMP images on their phone that they want to upload

[Beamanator]

One more thing is I need to test out enabling SVG avatar uploads - i'll work on that today

Although I'm running into this issue when trying to display an svg that has been saved in Onyx 🤔

[Beamanator]

Oh it looks like Image (React Native component)'s source prop doesn't support svg, this could be a problem for mobile - idk why it's not working for web though

[Beamanator]

GIF works no problem:

SVG no happy:

Failed to load resource blob:http://localhost:8080/c19adda9-969b-4303-ab41-bd3e19b7123e (404)

[Beamanator]

@trjExpensify I think I'm going to need someone else's help on this - it kinda looks like there might be a problem storing SVG image data locally (using URL.createObjectURL) the way we store everything else, though I couldn't find any evidence of other people hitting the same issue.

I also think it could be nice to wait for Georgia's PR https://github.com/Expensify/App/pull/13216 to be merged before we keep going with SVGs since there's a decent amount of overlap there, getting SVGs to load up in avatar icons

[JmillsExpensify]

@Beamanator @trjExpensify We should move the WAQ hold in my opinion in any case.

[Beamanator]

Georgia also brought up a good point about svgs:

Note: The user can still not upload an SVG as an avatar because SVGs must be supported on both old/new dot

[trjExpensify]

Is there a concern with allowing SVG avatar uploads on OldDot or something?

[trjExpensify]

Question from Friday still stands, Melv.

[Beamanator]

Oops sorry for the delay - at least displaying the SVG avatars in OldDot needs to work before we start supporting uploading in NewDot. @grgia it seems like in your default svg avatar PR, we're not even trying to support default svg avatars in OldDot right?

[grgia]

For the default avatar PR, since we want to use different default avatars on OldDot vs NewDot, I didn't end up needing to touch OldDot. But yes, to use SVGs for avatars we would need to support it on both platforms

[trjExpensify]

I'm a bit confused trying to piece this together:

1. we're saying that NewDot has a problem with storing SVG avatars locally
2. Members can't upload SVG avatars because of (1)
3. We're able to use SVG avatars in NewDot for the default avatars despite the limitation in (1)?
4. To do (2) we need to add support for SVG avatars in OldDot as well, that's a concern because of ??

We'd probably benefit from writing up a summary of where we're at with these moving parts, adding that to the OP, and starting a discussion on whatever decisions we need to make to go from her.

[grgia]

I think it's

1. I am under the impression that Old Dot does not support SVGs for avatars, and user-uploaded avatars are referenced from the same key in the user's personal details on both Old and NewDot. So if you uploaded an SVG in NewDot (say we store the function), it could work, but then your avatar would be broken when you log back into Old Dot.
2. Yes
3. Yes, the way that we were handling default avatars in New Dot was by hashing logins. So right now, it technically doesn't matter if you have a path to your default avatar saved or not as long as we know the login. Therefore, we were able to store all the default avatar SVGs locally and just grab the correct one whenever we hashing the login. I was following the patterns we already had in place while implementing that PR specifically. For instance, how we handle the SVG workspace avatars.

[Beamanator]

Ok yeah so here's a small recap from my side:

1. gif avatar uploads don't work on Android devices
   • How do we feel about disallowing this type of upload for now, vs trying to fix the issue in a 3rd party lib?
2. bmp avatar uploads don't work on iOS devices
   • How do we feel about disallowing this type of upload for now, vs trying to fix the issue in a 3rd party lib?
3. svg avatar uploads:
   1. Mobile: Looks like they're not supported in React Native (notes)
   2. Web:
      • When uploading, we first store the image data in Onyx (using URL.createObjectURL()).
      • Somehow this seems to not be working properly, as I keep seeing this error when we try to display that immediately after upload
      • Note: Once the SVG uploads, we actually do successfully convert it to a .jpeg so actually it should be a problem displaying these in OldDot, so I think that's my bad for leading us down the wrong path there 🙃

[JmillsExpensify]

gif How do we feel about disallowing this type of upload for now?

I think that's fine. These aren't a priority

bmp How do we feel about disallowing this type of upload for now?

I think that's fine. These aren't a priority

svg Once the SVG uploads, we actually do successfully convert it to a .jpeg so actually it should be a problem displaying these in OldDot

Did you mean shouldn't because we converted it to jpeg? I think you did but want to confirm.

[trjExpensify]

gif avatar uploads don't work on Android devices How do we feel about disallowing this type of upload for now, vs trying to fix the issue in a 3rd party lib?

Isn't it a bit extreme to not allow these file type uploads across all platforms, just because of a problem with the file format on one platform? (Same for bmp). Why are we discarding a path like this:

1. Open it up to a contributor
2. Explain what we want i.e to fix the root cause upstream
3. Have them submit a PR upstream
4. In the meantime, accept that gif avatars don't work on Android and bmp uploads don't work on iOS until (2).

[JmillsExpensify]

That's a fair suggestion, though perhaps we should even align on what doesn't work mean? Does that mean crash? Does that mean the user tries something and it doesn't work with no feedback. I'll admit, I was assuming something worse than just it doesn't work with no feedback.

[JmillsExpensify]

Then as for addressing it, to clear, I absolutely agree we should address it. My bigger concern is ensuring that default avatars have nothing in their way, though per my question above, it sounds like this isn't blocking it? Would love a confirmation from @Beamanator.

[Beamanator]

@JmillsExpensify I'm 99% sure there's nothing blocking default avatars here, just the uploading of new custom SVG avatars by users

[Beamanator]

Isn't it a bit extreme to not allow these file type uploads across all platforms, just because of a problem with the file format on one platform? (Same for bmp).

@trjExpensify just to make sure we're on the same page here, for bmp I'm suggesting we only disallow bmps from being uploaded on iOS, not on all devices (same for gif on Android only).

But yeah if we're ok with In the meantime, accept that gif avatars don't work on Android and bmp uploads don't work on iOS until (2)., then I like your plan of opening up an issue to get them fixed, at their own pace 👍

[JmillsExpensify]

Awesome that sounds like a plan to me too. Shall I create the separate issue for bmp (iOS) and gif (Android)?

[Beamanator]

Nice, yeah if you have time today that would be great 👍 👍 If not, I can get to it tomorrow or Friday