[HOLD for payment 2023-03-31] Display avatars & avatar thumbnails correctly in app

[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. Inspect avatar in LHN (in chrome dev tools)

Expected Result:

1. The avatar URL should be the thumbnail avatar, which is the URL ending in _128.

Actual Result:

1. Notice the avatar URL doesn't have _128. at the end, indicating it's not the thumbnail URL

Workaround:

Yes

Platform:

Where is this issue occurring?

• Web
• iOS (probably)
• Android (probably)
• Desktop App
• Mobile Web

Version Number: Latest
Reproducible in staging?: Y
Reproducible in production?: Y
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1670258536848779?thread_ts=1669089889.220939&cid=C049HHMV9SM

View all open jobs on GitHub

[melvin-bot]

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

[Beamanator]

WIP PR is here: #13366

[isabelastisser]

Hey @Beamanator, is this an internal issue? I noticed the Engineering label, and I'm not sure what are my next steps in the process.

[Beamanator]

@isabelastisser yes! I'm working on the fix so I just marked it internal :D

[Beamanator]

Ok problem number one: There's a few places in the server & OldDot that need to be updated to get avatar thumbnail URL no matter what the URL is (now that we're storing different avatar URLs like png, jpg, gif, bmp [update: only should be storing JPEG & PNG soon])

Problem number two: For some reason I only see avatar stored in Onyx, not avatarThumbnail, which comes back from the server and is stored in Onyx here: https://github.com/Expensify/Web-Expensify/blob/da84c28aa29364b043189faf240af69683079be4/lib/UserAPI.php#L2768-L2769

• This happens b/c on sign in we only send avatar for some reason
• Regression from [No QA] Remove unused code in deprecated API #13053, I believe - we used to set the avatar thumbnail in the front end (via formatPersonalDetails) but we deleted that

[Beamanator]

Dropping to weekly b/c I can't get to this immediately & it's not super high priority (I have a WIP PR: https://github.com/Expensify/Web-Expensify/pull/35828)

[JmillsExpensify]

Just my two cents: I think the scope of this issue surpasses the scope of WAQ. It's absolutely a bug and we should fix it. However, it involves updating OldDot and other scope which is part of the broader image improvements. I'd added HOLD WAQ to this issue title as a result, and also filed this issue in N7. More context on that in this Slack thread.

[Beamanator]

Groovy thanks for the update @JmillsExpensify 👍 Since there's not many WAQ bugs left I will still try to prioritize this week, unless some other WAQ bugs fall my way

[JmillsExpensify]

Awesome agreed! I mainly wanted to clarify where I see the priority of the issue falling, we should fix it regardless. 🙌

[Beamanator]

Totally makes sense 👍

[Beamanator]

Alright looking into where we show avatars, here's the current status (✅ = correct avatar is showing, ❌ = incorrect avatar showing):

Should show full avatar:

1. Another user's full profile image (chat report user details -> Full avatar view) ✅
2. Initial settings avatar preview ✅
3. Profile page avatar preview ✅

Should show avatar thumbnail:

1. LHN profile in top right corner ❌
2. LHN chats ✅
3. Chat Report messages ✅
4. Chat report header ✅
5. Chat report user details screen ✅

[Beamanator]

Assigning @rushatgabhane here since you're the C+ on the PR :D 👍

[MelvinBot]

@Beamanator, @rushatgabhane, @isabelastisser Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[Beamanator]

PR has been approved by 1, need 1 more approval!

[MelvinBot]

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

[MelvinBot]

@Beamanator, @rushatgabhane, @isabelastisser Huh... This is 4 days overdue. Who can take care of this?

[Beamanator]

Welp PR was merged to staging 2 days ago (here) but caused this regression: #16354 (see approved proposal)

[MelvinBot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[MelvinBot]

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

• Show user's thumbnail avatar in the LHN (in Settings button at the top) #15680

If no regressions arise, payment will be issued on 2023-03-31. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• 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

[MelvinBot]

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:

• [@rushatgabhane / @Beamanator] The PR that introduced the bug has been identified. Link to the PR:
• [@rushatgabhane / @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:
• [@rushatgabhane / @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:
• [@isabelastisser] Determine if we should create a regression test for this bug.
• [@rushatgabhane] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@isabelastisser] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[MelvinBot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[isabelastisser]

@Beamanator I just noticed the title of this issue payment and wanted to confirm that since this is internal, I don't need to hire contributors in Upwork and process payments, correct?

What are the next steps here before this can be closed?

Thanks!

[Beamanator]

@isabelastisser Actually we do need to pay out @rushatgabhane as the C+ since he reviewed my internal PR 👍 😬

[isabelastisser]

I created the job in Upwork and hired @rushatgabhane for the C+ review.

[isabelastisser]

Payment issued in Upwork, all set!

@Beamanator can we close the issue?

[Beamanator]

Awesome sauce! Yep we can close now 👍