-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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 2023-01-05] [HOLD #13392] [$1000] Android - Avatar is not displayed on push notification #12967
Comments
Triggered auto assignment to @arielgreen ( |
@arielgreen Eep! 4 days overdue now. Issues have feelings too... |
Bug0 Triage ChecklistNote: see this SO for more information.
|
Current assignee @arielgreen is eligible for the External assigner, not assigning anyone new. |
Job added to Upwork: https://www.upwork.com/jobs/~0171d7d835c1969527 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat ( |
Triggered auto assignment to @robertjchen ( |
I could reproduce the issue as well on the debug build and started to debug the issue. The following error occurred when fetching the icon and appears to occur on Android O (onwards):
To resolve the issue, we could convert the avatar to the desired bitmap (as used by the canvas we create in getCroppedBitmap) // android/app/src/main/java/com/expensify/chat/customairshipextender/CustomNotificationProvider.java
public Bitmap getCroppedBitmap(Bitmap bitmap) {
// Convert to ARGB_8888
bitmap = bitmap.copy(Config.ARGB_8888, true);
Bitmap output = Bitmap.createBitmap(bitmap.getWidth(),
bitmap.getHeight(), Config.ARGB_8888);
Canvas canvas = new Canvas(output);
// ..
} That would resolve the bug as seen here. |
@StefanNemeth Thanks for the proposal, it looks like this is indeed the source of the issue! Just out of curiosity, is there a reason why we don't use |
According to the documentation PixelCopy allows for copy operations from Surface to Bitmap (e.g. for screenshots). We are dealing with bitmaps here so bitmap.copy is the proper way to do it afaik. |
Got it, that makes sense. @parasharrajat What are your thoughts on the above proposal? 🙏 |
I am not an Android native guy but can we just use I don't see where we create the Bitmap with |
Practically it will have the same effect, yes. However, I chose to use the immutable copy option as it is usually bad practice to alter input objects in a function like Edit: Actually it won't work with |
Ok, Thanks for the explanation. I am good with both. I have asked internally to review it before we proceed. |
Ok @StefanNemeth 's proposal looks good to me. Cc: @robertjchen 🎀 👀 🎀 C+ reviewed |
@StefanNemeth offer sent! @parasharrajat please apply when you get a moment 😄 |
ah, @robertjchen looks like @arielgreen is the BZ member for this 😂 |
@dylanexpensify Ah, my bad! I was getting these issues mixed up 😵 |
PR is on HOLD for #13392. |
@robertjchen, @StefanNemeth, @parasharrajat, @arielgreen Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
Having issues signing in the app. #13392 is done so we can remove the HOLD. |
@robertjchen, @StefanNemeth, @parasharrajat, @arielgreen Whoops! This issue is 2 days overdue. Let's get this updated quick! |
This is no more on hold and is ready for merge. Someone, please remove the HOLD from the title. |
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.45-0 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 2023-01-05. 🎊 After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
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:
|
Am I eligible for the 50% bonus as the PR created within a few hours after assignment was held? |
@StefanNemeth yes, I think that is fair. Paid. @parasharrajat sending offer now. |
[@parasharrajat / @robertjchen] The PR that introduced the bug has been identified. Link to the PR: [@parasharrajat / @robertjchen] 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: [@parasharrajat / @robertjchen] 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: https://expensify.slack.com/archives/C049HHMV9SM/p1673432839280069 @robertjchen Please update the checklist at #12967 (comment) |
it's not clear to me what exactly caused the regression in the linked PR. I recall that notification profile images have been broken for weeks/months now. Perhaps they were recently fixed? But I can't recall. |
Ahhh, I see. That PR was January 2022, not 2023 😄 |
All payments have been issued. |
Just need to look into whether we can create a regression test and then this will be good to close out. |
Hey @arielgreen I think this should be added as a new regression test, as it ensures we test for custom profile pics as well as the default icons: Ensure rounded profile images are displayed for notifications
|
Discussing regression test here. |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Issue found when executing PR #12540
Action Performed:
Expected Result:
Avatar image of the web user is displayed after received a push notification in Android app
Actual Result:
Avatar image of the web user isn't displayed after received a push notification in Android app
Workaround:
Unknown
Platform:
Where is this issue occurring?
Version Number: 1.2.30.0
Reproducible in staging?: Yes
Reproducible in production?: Yes
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Screen_Recording_20221123-115704_New.Expensify.1.mp4
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: