Search - Some users are not prioritized over chats or rooms

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Issue was found when executing #6544

Action Performed:

1. Open app and login
2. Open the Search page.
3. Search for something.
4. verify search results

Expected Result:

Users appear first in the result list over group chats or rooms

Actual Result:

Some users appear after group chats or rooms

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.1.18.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Bug5362931_Pr_6544.mp4

Expensify/Expensify Issue URL:
Issue reported by: Applause
Slack conversation:

View all open jobs on GitHub
Issue was found when executing #6544

[OSBotify]

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

[MelvinBot]

Triggered auto assignment to @marcochavezf (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[Jag96]

cc @parasharrajat

[parasharrajat]

I am not able to reproduce this on staging. From the vid, it seems the search is not even working.

[marcochavezf]

I think I was able to reproduce the issue, the users [email protected] and [email protected] are appearing below the group chats:

There's no error in the console log of staging, but when I test locally I see these errors (not sure if it's related):

I will continue investigating.

[parasharrajat]

It may be only causing issues for only email entries.

[marcochavezf]

It may be only causing issues for only email entries.

ah are you referring to the console errors or the users that are appearing below the group chats?
Oh I see, the users that appear below are the email entries (no report chat created for those users). So seems the users that are not prioritized in the video are email entries, thus this is not an issue introduced by a recent change.

👋🏽 @Jag96 what could be the next action for this issue? do we remove the deploy blocker label?

[isagoico]

I think that the users that appear below the group chats are user that don't have a existing conversation with the logged in user.

[Jag96]

👋🏽 @Jag96 what could be the next action for this issue? do we remove the deploy blocker label?

@marcochavezf I think we can remove the deploy blocker label here since this seems like a case we missed in #6544 instead of a regression. I've posted in slack to double-check how we want to handle this case, but I think we can just make this a follow up to fix the missed case.

Removing the deploy blocker label and updating to daily and leaving assigned to me for now, but @isagoico I think we can check off #6544 if there aren't any regressions with search.

[Jag96]

@parasharrajat could you please update the logic from #6544 to include a fix for this?

[MelvinBot]

📣 @parasharrajat You have been assigned to this job by @Jag96!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[parasharrajat]

@isagoico Could you please share test account credentials with me to test this issue? I can't reproduce reliably on my testing account. You can slack me if that is the way. Thanks

[parasharrajat]

@Jag96
Ok, so I have found the cause of this. First I would like to confirm the implementation.

1. We need to put the personalDetails array in the correct position in the search item list. Currently we append this which is why this issue is happening.
   I think it should begetOptionsresponsibility to give the correct sorted list instead of merging thepersonalDetailsin therecentReports` ourself in the component. But this requires a minor refactor of the getOptions component.

I will merge the personalDetails array in the recentReports before sorting. Now personalDetails would always me [] and they would be merged in recentReports in the correct sorted Order. This require dev attention every time someone turn sortByReportTypeInSearch on.

    return {
        personalDetails: personalDetailsOptions,
        recentReports: recentReportOptions,
        userToInvite,
    };

Second, we can divide the recentreports into two sections (first for 1:1 chat and second for other) and place the third section of personalDetails after the first section. So that order will be maintained in SearchPage.

let me know. Which one do you like?

Both of these approaches will fix the issue of the duplicate rooms as I said here #5699 (comment) Due to the reason that keys are duplicated in the section list and thus old items with the same keys are never removed.

At last, I think this should be added as a milestone of the existing contract as this case was missed in the main issue and it is an extension to the old code or not caused by the old PR's code.

[parasharrajat]

@Jag96 What do you think? I am ready to send the PR.

[roryabraham]

@parasharrajat @Jag96's going to be ooo for a few days, I'm going to temporarily take over as CME here. Just catching up now...

[roryabraham]

We need to put the personalDetails array in the correct position in the search item list. Currently we append this which is why this issue is happening.

So just to clarify, this code is what you're referring to, right?

I think it should be getOptions responsibility to give the correct sorted list instead of merging the personalDetails in the recentReports ourself in the component.

I agree

I will merge the personalDetails array in the recentReports before sorting.

Makes sense

Now personalDetails would always me [] and they would be merged in recentReports in the correct sorted Order. This require dev attention every time someone turn sortByReportTypeInSearch on.

Unfortunately I got a bit confused at this point. Can you explain what you meant by this?

[parasharrajat]

I'm going to temporarily take over as CME here. Just catching up now.

Thanks.

So just to clarify, this code is what you're referring to, right?

Exactly.

Now personalDetails would always me [] and they would be merged in recentReports in the correct sorted Order. This require dev attention every time someone turn sortByReportTypeInSearch on.

When we will merge the personalDetails in the main recentChats while sortByReportTypeInSearch is true. There is no use of returning the duplicate personalDetails array. It will become confusing and error prone. So I suggest returning []. Now Someone using this flag should be cautious that personalDetails would be merged in the recentReports and personalDetails would be empty. It could be confusing why personalDetails is empty.

[parasharrajat]

PR Up... Testing it.

[roryabraham]

Sorry for the delay here @parasharrajat, reviewed the PR earlier today

[parasharrajat]

From #6627 (comment)

At last, I think this should be added as a milestone of the existing contract as this case was missed in the main issue and it is an extension to the old code or not caused by the old PR's code.

Still waiting for the Upwork job to be created.

[MelvinBot]

Triggered auto assignment to @adelekennedy (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[roryabraham]

Sorry your payment got lost in the shuffle here, @parasharrajat! @adelekennedy Can we create an Upwork job for this, then hire @parasharrajat? The PR is currently on staging.

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[adelekennedy]

Hired!

[parasharrajat]

Thanks, @adelekennedy

[roryabraham]

The fix for this was deployed to production 6 days ago. There were some comments on the PR but it sounds like an unrelated bug. So @adelekennedy I think we should be good to pay this out tomorrow.

[adelekennedy]

Noted! Have it on my calendar to pay out on 1/25 - then I will close the job

[mvtglobally]

@adelekennedy @roryabraham Can you check below video. We seem to be able to repro the issue.

6627.mp4

Also, we noticed that exact search match is not always displayed on Top. Not sure if its related to this PR

[adelekennedy]

Double checking here @roryabraham the job is closed and paid out - but do we need to reopen the job?

[roryabraham]

Honestly, I can't keep track of all the expected behavior in the labyrinth that is OptionsListUtils. I think that we should close this and add a new issue to:

1. First, clearly and thoroughly define the results ordering, accounting for:
   1. DM's, group chats, policy default rooms, workspace chats, user-created rooms
   2. How the order might change for the New Chat or New Group pages, and for users that don't yet have Expensify accounts.
2. Create unit tests to cover all those expected results.
3. Attempt to DRY up and generalize all the logic we have now.
4. Patch logic as necessary to make tests pass.

Thoughts? cc @parasharrajat

[parasharrajat]

Ok, nice thoughts @roryabraham. We should certainly do that. But #6627 (comment) is a different issue.

it is not the order of options. I think QA is saying that the search is not working properly. The exact match should come first.

So the search should also look into the personalDetails (contacts) with respect to Recent reports(chats). This should be a separate issue and thus we can close this and create a new one. Also, plan the order properly (based on your thoughts) and then fix it.

[roryabraham]

Created a follow-up: #7582