search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
2.97k stars 2.48k forks source link

[HOLD for payment 2024-05-06] [HOLD for payment 2024-05-03] [$250] Mention - Avatar is not displaying on Concierge in mention list #41014

Open lanitochka17 opened 1 week ago

lanitochka17 commented 1 week ago

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

Version Number: 1.4.66-0 Reproducible in staging?: Y Reproducible in production?: N If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4516979&group_by=cases:section_id&group_order=asc&group_id=306201&group_by=cases:section_id&group_order=asc&group_id=306201 Email or phone of affected tester (no customers): shussain+accoun1@applausemail.com Issue reported by: Applause - Internal Team

Action Performed:

  1. Open a chat
  2. Press @ and check Concierge

Expected Result:

Avatar should be displayed on Concierge in mention list

Actual Result:

Avatar is not displaying on Concierge in mention list

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/dc9b332d-a255-4240-a01e-a035a0beeaf5

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~015c5e79bf2b62e7b7
  • Upwork Job ID: 1783566739983851520
  • Last Price Increase: 2024-04-25
melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @neil-marcellini (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

github-actions[bot] commented 1 week ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.
lanitochka17 commented 1 week ago

@neil-marcellini FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

neil-marcellini commented 1 week ago

Ok, taking a look. In general we can add external and the contributors can help us figure out if it's a backend problem.

melvin-bot[bot] commented 1 week ago

Job added to Upwork: https://www.upwork.com/jobs/~015c5e79bf2b62e7b7

melvin-bot[bot] commented 1 week ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @thesahindia (External)

neil-marcellini commented 1 week ago

I finally confirmed that the personal detail for concierge has the correct avatar link https://d2k5nsl2zxldvw.cloudfront.net/images/icons/concierge_2022.png. On production it's the same value, so I don't think it's a problem with the data.

ZhenjaHorbach commented 1 week ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Mention - Avatar is not displaying on Concierge in mention list

What is the root cause of that problem?

The problem is that when we try to get the default avatar, we use accountID, which in the case of suggestionList , we do not pass

https://github.com/Expensify/App/blob/6060e6ffd5387975e65339344828141d83c9d642/src/libs/UserUtils.ts#L85-L107

What changes do you think we should make in order to solve the problem?

To fix this issue we can update this code and add accountID: detail?.accountID,

https://github.com/Expensify/App/blob/6060e6ffd5387975e65339344828141d83c9d642/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx#L238-L250

And also pass accountID for Avatar

https://github.com/Expensify/App/blob/6060e6ffd5387975e65339344828141d83c9d642/src/components/MentionSuggestions.tsx#L82-L89

What alternative solutions did you explore? (Optional)

As alternative we can update getUserMentionOptions and pass accountID only in case CONCIERGE, NOTIFICATIONS and so on

neil-marcellini commented 1 week ago

Thanks @ZhenjaHorbach. Can you explain the root cause a bit more please? Why is it that we don't pass the accountID when creating the mention? It looks to me like we do.

ZhenjaHorbach commented 1 week ago

Thanks @ZhenjaHorbach. Can you explain the root cause a bit more please? Why is it that we don't pass the accountID when creating the mention? It looks to me like we do.

In this PR we added accountID for Avatar and added additional checking which use accountID inside getAvatar But since we didn’t pass this ID for mentionsList It turns out that this function does not work correctly

https://github.com/Expensify/App/pull/39229/files#diff-244cdd9be92eb59eaa9aa7b7d3e310ab0b3845111e3de28746ec3ea59ae970b4R89-R90

And here we can't get ConciergeAvatar without accountID

https://github.com/Expensify/App/blob/6060e6ffd5387975e65339344828141d83c9d642/src/libs/UserUtils.ts#L86-L88

Actually in this case I think we need to check all places where we use Avatar

neil-marcellini commented 1 week ago

Ok @ZhenjaHorbach I think I finally understand your proposal. I would say the root cause is this.

  1. When we build the suggestions the source is set to this https://github.com/Expensify/App/blob/c6117097e247a8ca7c51b5d59d9d08a901b75162/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx#L245 Previously it was this, and was changed here. https://github.com/Expensify/App/blob/45cd559d2a4af67b8610a87bb472840da087cf80/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx#L241

  2. MentionSuggestions renders the Avatar here and doesn't pass an accountID https://github.com/Expensify/App/blob/c6117097e247a8ca7c51b5d59d9d08a901b75162/src/components/MentionSuggestions.tsx#L82-L89

  3. The avatar source is set here using UserUtils.getAvatar. The accountID is undefined https://github.com/Expensify/App/blob/c6117097e247a8ca7c51b5d59d9d08a901b75162/src/components/Avatar.tsx#L89

  4. That calls getDefaultAvatar https://github.com/Expensify/App/blob/c6117097e247a8ca7c51b5d59d9d08a901b75162/src/libs/UserUtils.ts#L151

  5. Since there's not accountID, it incorrectly returns a default avatar https://github.com/Expensify/App/blob/c6117097e247a8ca7c51b5d59d9d08a901b75162/src/libs/UserUtils.ts#L106

I'm wondering how we can test this. I can't reproduce on dev because the avatar is always blank for me. I'll try against the staging api instead of our dev api.

Or we could just revert Use fallback user avatar in cases where the user is unknown to us. @grgia what do you think since you were involved there?

neil-marcellini commented 1 week ago

@ZhenjaHorbach I put up a PR based loosely on your proposal. I hope that's cool with you and I really appreciate you pointing me in the right direction. I would have approved your proposal earlier, but I didn't really understand the root cause until I investigate it thoroughly myself.

ZhenjaHorbach commented 1 week ago

Fix concierge avatar in mention suggestion #41034

Happy to help )

grgia commented 1 week ago

@Kicu could you take a look at this?

grgia commented 1 week ago

@neil-marcellini I doubt we will need to revert, I'm fairly certain this will be an easy fix

Kicu commented 1 week ago

hey I'm the author of https://github.com/Expensify/App/pull/39229 and I took a look at this, few points from me in random order

Because of this I believe the best fix for this is to actually do pass accountID prop correctly. This PR seems to do just that (https://github.com/Expensify/App/pull/41034).

I tested locally on my dev the same fix, and it looks good

before (main)

after passing accountID

In general we only need accountID to be able to display the "pretty" svg versions of: default avatars, ConciergeAvatar and NotificationsAvatar. Otherwise .avatar from user details is always correct. In addition I can supply a separate PR that adds the new accountID prop in a few places where Avatar is used, because it seems I might miss them.

@grgia

grgia commented 1 week ago

@Kicu are you able to open a PR fix for this as a followup in that case?

Kicu commented 1 week ago

@grgia we can use this fix by @neil-marcellini I believe - it's already created: https://github.com/Expensify/App/pull/41034

allroundexperts commented 1 week ago

@neil-marcellini Can you please assign this to me since I reviewed the PR?

mountiny commented 1 week ago

Ended up reverting the PR to unblock the deploy, assign @Kicu to address this in the second iteration

Kicu commented 1 week ago

I will fix this as part of trying to bring back the fallback avatars fix

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

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

If no regressions arise, payment will be issued on 2024-05-03. :confetti_ball:

For reference, here are some details about the assignees on this issue:

melvin-bot[bot] commented 1 week ago

⚠️ 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.

allroundexperts commented 6 days ago

The PR was closed and the offending PR was reverted. As such, we don't need to wait for the 7 days for the payment.

melvin-bot[bot] commented 5 days ago

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

If no regressions arise, payment will be issued on 2024-05-06. :confetti_ball:

For reference, here are some details about the assignees on this issue: