Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
kbecciv opened this issue Sep 16, 2022 · 40 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review

Comments

@kbecciv
Copy link

kbecciv commented Sep 16, 2022

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. Go to URL https://staging.new.expensify.com/
    and login with any account
  2. Go to Settings->Profile->Upload photo
  3. Select an image with a white background and click save

Attachment:
1f98b (2)

Expected Result:

The background on the avatar should remain white

Actual Result:

The white background changes to black

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.2.1.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): any

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5734779_Recording__1914.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

@melvin-bot
Copy link

melvin-bot bot commented Sep 16, 2022

Triggered auto assignment to @tgolen (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@thesahindia
Copy link
Member

thesahindia commented Sep 16, 2022

It's not an issue, it happens because the background is transparent. The result will be the same if we try it on other platforms (e.g. Slack, Whatsapp)

@tgolen
Copy link
Contributor

tgolen commented Sep 16, 2022

Shouldn't the transparent background remain transparent though? Is this because it's being converted to a JPEG at some point (and transparency isn't supported)?

@parasharrajat
Copy link
Member

It seems like the backend is removing the transparency after processing the image.

@varshamb
Copy link
Contributor

Yes, while saving, image is converting jpeg and jpeg doesn't support transparency.

@melvin-bot melvin-bot bot added the Overdue label Sep 19, 2022
@tgolen
Copy link
Contributor

tgolen commented Sep 19, 2022

All right, sounds like this would be an internal issue for us to fix then on the server. It would be nice to support images with transparent backgrounds.

@melvin-bot melvin-bot bot removed the Overdue label Sep 19, 2022
@tgolen tgolen removed their assignment Sep 19, 2022
@tgolen tgolen added the Improvement Item broken or needs improvement. label Sep 19, 2022
@jasperhuangg jasperhuangg added the Internal Requires API changes or must be handled by Expensify staff label Sep 19, 2022
@flodnv flodnv added Weekly KSv2 and removed Daily KSv2 labels Sep 21, 2022
@Beamanator
Copy link
Contributor

Putting this issue on HOLD and tracking in Image-related feature request and improvements

@Beamanator Beamanator changed the title Web - Profile - The white background in the image changes to black [HOLD] Web - Profile - The white background in the image changes to black Oct 6, 2022
@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@melvin-bot melvin-bot bot added the Overdue label Oct 18, 2022
@JmillsExpensify JmillsExpensify added the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Oct 18, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 18, 2022

Triggered auto assignment to @MitchExpensify (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@melvin-bot melvin-bot bot added Daily KSv2 and removed AutoAssignerTriage Auto assign issues for triage to an available triage team member Weekly KSv2 Overdue labels Oct 18, 2022
@puneetlath puneetlath added the Bug Something is broken. Auto assigns a BugZero manager. label Oct 19, 2022
@MitchExpensify
Copy link
Contributor

I am unable to reproduce this

@mvtglobally
Copy link

Issue not reproducible during KI retests. (Third week) Are we ok to close this one?

@trjExpensify
Copy link
Contributor

Oh, interesting. @Beamanator but your App PR from this list isn't merged yet? 😅

@Beamanator
Copy link
Contributor

Haha I'm still able to reproduce this issue with a transparent PNG :D Hoping to continue the App PR today!!

@Beamanator
Copy link
Contributor

Beamanator commented Nov 25, 2022

Found some annoying issues on Mobile where Android & iOS Native Apps don't properly handle bmp & gif file uploads (with the help of @Santhosh-Sellavel 's very thorough tests 👍 ) 🤷 so I'm planning to make a follow-up issue (#13038) that's external where others can help investigate possible solutions in the 3rd-party library we use, @oguzhnatly/react-native-image-manipulator

Note: We "could" consider this a feature request which would put that follow-up on hold? Or, since it's very closely related to this current issue which we consider WAQ, maybe no hold needed 🤔 Thoughts @JmillsExpensify @trjExpensify ?

@JmillsExpensify
Copy link

Great question. This is a bit of a grey area, since we recently added the ability to support all file types, which pre-dates WAQ. So based on that, given that we now support all file types and we have bugs related to that, I think we should solve them now as part of WAQ.

@Beamanator
Copy link
Contributor

That's fair, hopefully the PR to support all file types (for web / desktop / mobile web) will get merged soon, then I'll take the issue to fix bmp / gif images off HOLD, and we can get to work on it (probably from contractors since it seems like a big refactor that'll require updating at least 1 3rd-party lib)

@JmillsExpensify
Copy link

Ok cool. That sounds like a plan to me. I think we should put this issue on HOLD in the meantime in any case.

@JmillsExpensify JmillsExpensify changed the title [Image] Web - Profile - The white background in the image changes to black [HOLD #12549] [Image] Web - Profile - The white background in the image changes to black Nov 29, 2022
@Beamanator
Copy link
Contributor

Wait wait taking this off hold b/c #12549 is the PR to fix this issue :D (or am I missing something @JmillsExpensify )?

@Beamanator Beamanator changed the title [HOLD #12549] [Image] Web - Profile - The white background in the image changes to black [Image] Web - Profile - The white background in the image changes to black Nov 30, 2022
@JmillsExpensify
Copy link

Ah, haha thanks. I'm with you now.

@Beamanator
Copy link
Contributor

Have one last thing to figure out before being able to merge (iOS mWeb, .png file format specifically) - calling in @jasperhuangg & @Santhosh-Sellavel to do this last test, being 100% sure we're uploading files of that format

@trjExpensify
Copy link
Contributor

Looks like we're merged here! 🎉

On this follow-up #13038, do we want to add it to the Image tracker?

@Beamanator
Copy link
Contributor

HECK ya we are.

And yes good idea I'll add that ^ follow-up to the tracker! Still won't take it off HOLD till the PR for this one gets to Production (to make sure there's no weird regressions)

@trjExpensify
Copy link
Contributor

Cool, makes sense!

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Dec 8, 2022
@melvin-bot melvin-bot bot changed the title [Image] Web - Profile - The white background in the image changes to black [HOLD for payment 2022-12-15] [Image] Web - Profile - The white background in the image changes to black Dec 8, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 8, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.36-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2022-12-15. 🎊

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot
Copy link

melvin-bot bot commented Dec 8, 2022

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@Beamanator] The PR that introduced the bug has been identified. Link to the PR: (none, this wasn't a regression)
  • [@Beamanator] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: (none, this wasn't a regression)
  • [@Beamanator] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: (none, this wasn't a regression)
  • [@trjExpensify / @mallenexpensify] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here: Adding a profile avatar is covered already here.

@Santhosh-Sellavel
Copy link
Collaborator

Please assign me this for tracking payment thanks!

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Dec 14, 2022
@trjExpensify
Copy link
Contributor

Sent you a contract, @Santhosh-Sellavel.

@trjExpensify
Copy link
Contributor

Cool, settled!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests