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

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

Closed
5 of 8 tasks
IuliiaHerets opened this issue Oct 19, 2024 · 32 comments
Closed
5 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Reviewing Has a PR in review Weekly KSv2

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Oct 19, 2024

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: #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:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6639700_1729346685510.bandicam_2024-10-19_22-01-20-118.mp4

View all open jobs on GitHub

@IuliiaHerets IuliiaHerets added DeployBlockerCash This issue or pull request should block deployment Bug Something is broken. Auto assigns a BugZero manager. labels Oct 19, 2024
Copy link

melvin-bot bot commented Oct 19, 2024

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.

Copy link

melvin-bot bot commented Oct 19, 2024

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

Copy link

melvin-bot bot commented Oct 19, 2024

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

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Oct 19, 2024
Copy link
Contributor

👋 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
Copy link
Contributor

hannojg commented Oct 21, 2024

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

@hannojg
Copy link
Contributor

hannojg commented Oct 21, 2024

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

Screen.Recording.2024-10-21.at.10.18.47.mov

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
Copy link
Contributor

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
Copy link
Contributor

Retest requested. Just making sure before moving forward.

@izarutskaya
Copy link

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

Recording.2867.mp4
Recording.2868.mp4

@MarioExpensify
Copy link
Contributor

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
Copy link
Contributor

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

@yuwenmemon yuwenmemon added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Oct 21, 2024
@melvin-bot melvin-bot bot added the Overdue label Oct 24, 2024
@melvin-bot melvin-bot bot added the Overdue label Nov 1, 2024
Copy link

melvin-bot bot commented Nov 2, 2024

@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
Copy link
Contributor

hannojg commented Nov 4, 2024

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
Copy link
Contributor

hannojg commented Nov 5, 2024

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.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Nov 5, 2024
@isabelastisser
Copy link
Contributor

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

@hannojg
Copy link
Contributor

hannojg commented Nov 6, 2024

@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
Copy link
Contributor

commenting for assignment :)

@isabelastisser
Copy link
Contributor

hey @chrispader, I just assigned you. Thanks!

@marcaaron
Copy link
Contributor

@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.

@marcaaron marcaaron removed the Reviewing Has a PR in review label Nov 23, 2024
@isabelastisser
Copy link
Contributor

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

@chrispader
Copy link
Contributor

chrispader commented Nov 25, 2024

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

  • yonghongkok2@gmail.com: (same as in the repro video)

Screenshot 2024-11-25 at 19 44 11

  • hanno@margelo.com:

Screenshot 2024-11-25 at 19 43 49

  • Onyx DB personalDetailsList_ key:

Screenshot 2024-11-25 at 19 59 25

@chrispader
Copy link
Contributor

chrispader commented Nov 25, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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.

@melvin-bot melvin-bot bot added the Overdue label Dec 5, 2024
@isabelastisser
Copy link
Contributor

Closing this!

@melvin-bot melvin-bot bot removed the Overdue label Dec 5, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants