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
3.58k stars 2.92k forks source link

Search-Search result shows fallback avatar and email instead of custom avatar and display name #51123

Open IuliiaHerets opened 1 month ago

IuliiaHerets commented 1 month 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: 9.0.51-1 Reproducible in staging?: Y Reproducible in production?: N Issue was found when executing this PR: https://github.com/Expensify/App/pull/48652 Email or phone of affected tester (no customers): applausetester+kh081006@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Click on the search icon in any report.
  3. Enter an email address that has custom avatar and display name.

Expected Result:

The search result will show user with custom avatar and display name.

Actual Result:

The search result shows user with fallback avatar and email address as display name instead of custom avatar and custom display name.

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/b94533a2-09b9-4c17-87fe-b327d9673e97

View all open jobs on GitHub

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @isabelastisser (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

💬 A slack conversation has been started in #expensify-open-source

github-actions[bot] commented 1 month 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.
hannojg commented 1 month ago

Hey, I was responsible for the accused PR. Feel free to assign this to me I can look into it today!

hannojg commented 1 month ago

Hm, i can neither reproduce this on staging or latest dev commit:

https://github.com/user-attachments/assets/4722535e-fcc7-4ff9-9ad7-d40b48934b6d

The PR hasn't changed anything about how we display search results, it just changed how we filter for them. So i am not really sure if the PR could've caused such behaviour.

MarioExpensify commented 1 month ago

I also cannot reproduce here, either from the Desktop or the WebApp, both react to the search the same as Production App. Maybe we could get this retested?

image

MarioExpensify commented 1 month ago

Retest requested. Just making sure before moving forward.

izarutskaya commented 1 month ago

Still reproducible on staging Build v9.0.51-2 And different behavior in production

https://github.com/user-attachments/assets/8a1fe034-45dd-4771-909a-2b7862f7fe10

https://github.com/user-attachments/assets/0d899ea6-1a1e-4724-a1b3-0d22929e287c

MarioExpensify commented 1 month ago

Hey @hannojg, mind taking another look? I looked through other PRs and did not find any other that may be the cause for this issue. Feel free to reach me out if you think it is unrelated to your latest changes.

marcaaron commented 1 month ago

That PR was reverted it seems. @MarioExpensify assigning myself since I merged that PR, but feel free to stay assigned.

melvin-bot[bot] commented 1 month ago

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

isabelastisser commented 1 month ago

Not overdue; it looks like we are still working on the PR.

isabelastisser commented 1 month ago

@hannojg, can you please share an update? Thanks!

isabelastisser commented 1 month ago

Bump @hannojg. Thanks!

isabelastisser commented 4 weeks ago

Heads up, I will be OOO from Oct 31 until Nov 4, so please re-apply the Bug label and assign a new member if needed. Thanks!

melvin-bot[bot] commented 3 weeks ago

@hannojg @isabelastisser @marcaaron this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

hannojg commented 3 weeks ago

Guys sorry, this completely slipped past me! I will take another look at that one this week! (If this is related to the mentioned PR, it currently shouldn't be reproducible on either staging on prod, as the PR has been reverted. I am working on a new PR, and will keep this bug in mind this time.)

hannojg commented 3 weeks ago

Okay, so after investigating this, this isn't a problem with the PR originally mentioned:

The problem seems to be that sometimes you get a 407 error for the search query, thus we are not getting any data from the backend:

CleanShot 2024-11-04 at 17 57 16

We can see from the screenshot that the network command for the search was fired before the user got re-authenticated.

I originally got assigned to this issue because it was suspected that my PR caused this issue - which it didn't. Our team (margelo) can look into the issue if you like, or you can make it external i think.

isabelastisser commented 3 weeks ago

@hannojg, thanks for the update! Can you please continue to work on this issue? Thanks!

hannojg commented 3 weeks ago

@chrispader can you comment here? Chris from the Margelo team will work on a fix for this ticket (as i am very busy with other tasks rn).

chrispader commented 3 weeks ago

commenting for assignment :)

isabelastisser commented 3 weeks ago

hey @chrispader, I just assigned you. Thanks!

marcaaron commented 6 days ago

@chrispader do you have an update for this one?

My understanding is that the Reviewing label should not have been applied and nobody is working on this yet.

isabelastisser commented 3 days ago

Hey @chrispader, can you please follow up on the questions above? Thanks!

chrispader commented 3 days ago

Hey so sorry for the massive delay! I was totally busy on another PR and should have looked into this way earlier! 🫠

I investigated the problem and can clearly reproduce the problem for a particular set of accounts/email addresses.

Problem

The problem essentially is that the response received from the backend for SearchForReports sometimes returns a login (email) property in the fetched (to-be merged) personalDetails, whereas sometimes not.

If no login property is provided in the response from the backend, the personal details option received from useOptionsList() will not have a login property and therefore the logic in OptionsListUtils.filterOptions() is not able to match it to the email that the user types in the search field.

Because the logic couldn't find any personal details item, we will just add a "blank" options list item to the options list and therefore the default avatar is used.

Solution

Because of this i would argue that this is a backend issue and the SearchForReports should always return a login property for the Onyx key personalDetailsList_. Looking into the local Onyx DB, we also don't really have any other data to match the (typed in) email address to the personal details, except for the name (could be ambiguous) or the account ID, where we would have to do a very complex and costly lookup.

Backend responses for different accounts/email addresses and Onyx DB screenshot

Screenshot 2024-11-25 at 19 44 11

Screenshot 2024-11-25 at 19 43 49

Screenshot 2024-11-25 at 19 59 25

chrispader commented 3 days ago

I don't think it is on purpose, that the backend doesn't send a login property for some accounts, right?

Or am i missing something here?

cc @marcaaron @isabelastisser

marcaaron commented 3 days ago

AFAICT there's really only one case where we would not return a login in the personalDetail.

There is a property called isKnownUser and if it's false then no login is returned (actually a few other fields will not be returned either pronouns, timezone, phoneNumber and validated). Internal code here.

I think this feature is working as intended. @puneetlath worked on this and can give a second opinion. But my guess for this issue based on the evidence provided is that we didn't make the connection to isKnownUser feature.

puneetlath commented 2 days ago

That sounds correct to me. You should get the full personal details, including login, when you "know" someone. You should get the partial personal details when you don't.

chrispader commented 2 days ago

That sounds correct to me. You should get the full personal details, including login, when you "know" someone. You should get the partial personal details when you don't.

Ok i understand. Does that mean, that since we don't receive these infos, we also don't want to show things like the avatar?

Without the email, address we cannot really match it with the search input... e.g. when i'm searching for an email address of a user i "don't know", e.g. "yonghongkok2@gmail.com", we receive the missing data like avatar and name from the SearchForReports query, but cannot match it with the mail address.

marcaaron commented 2 days ago

I think what is being reported is probably fine and we should close this out (but seems like it could reopen again as a suspected bug). We could maybe improve the UX so that "not known" users show up with a ? avatar or something, but I don't feel strongly about prioritizing this right now.