[HOLD for payment 2021-12-15] Search - Make users appear first in the result list over group chats or rooms

isagoico commented 2 years 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!

Action Performed:

Log in with an account that it's in a workspace and has rooms available
Search for a user

Expected Result:

User should be prioritized over group chats or rooms. The prioritization should be:

Person name (e.g. Ann)
Room name (e.g. Ann, Joe or Ann's Room)
Room Description (e.g. the image below)

Actual Result:

Room is being prioritized over the user

Workaround:

No need.

Platform:

Where is this issue occurring?

Web
iOS
Android
Desktop App
Mobile Web

Version Number: 1.1.15-1

Reproducible in staging?: Yes Reproducible in production?: Yes

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:

Issue reported by: @mountiny Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1637012485218100

View all open jobs on GitHub

MelvinBot commented 2 years ago

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

parasharrajat commented 2 years ago

Proposal

To do this we have to set the prioritizeDefaultRoomsInSearch: false for getSearchOptions. https://github.com/Expensify/App/blob/d2d17181727f2a4d264618986438df2733f107d0/src/libs/OptionsListUtils.js#L581

mountiny commented 2 years ago

I think Expected behaviour might be more complicated, I guess we should distinguish between the name and description.

Let's consider scenario where we search an and then we would want the #announce roome to be first before any person. Here in this case the #announce room is should because of the limited in the description.

We should first make sure we have the expected behaviour thought through before making this external. Thoughts about the behaviour @TomatoToaster and @trjExpensify?

Jag96 commented 2 years ago

From the thread it sounds like we want to prioritize in this order:

Person Name
Room Name
Room Description (Policy/Domain name in this case)

Let's consider scenario where we search an and then we would want the #announce room to be first before any person.

All rooms have the # at the beginning, right? So I'd think if you typed an you'd want to show results for people first, and if you typed #an you'd show the room results.

trjExpensify commented 2 years ago

All rooms have the # at the beginning, right? So I'd think if you typed an you'd want to show results for people first, and if you typed #an you'd show the room results.

I agree with that! 👍

Separately, do we need to consider the logic for GroupDMs versus DMs? As in, if you search Ted, you'd expect to return Ted first ahead of Ted, Joe wouldn't you?

MelvinBot commented 2 years ago

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

Jag96 commented 2 years ago

Separately, do we need to consider the logic for GroupDMs versus DMs? As in, if you search Ted, you'd expect to return Ted first ahead of Ted, Joe wouldn't you?

That makes sense to me 👍

Any other considerations to make here @vitHoracek @trjExpensify? If not we can make this external!

mountiny commented 2 years ago

That also seems reasonable to me! I think we settled down on a good solution :)

trjExpensify commented 2 years ago

That was it from my POV at this point! 👍

MelvinBot commented 2 years ago

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

Jag96 commented 2 years ago

@parasharrajat I tried testing w/ your proposal and it looks good for single users vs rooms, but it looks like you can still see a group chat above a single user chat. Can you update your proposal so that single users are shown above group chats?

parasharrajat commented 2 years ago

Ok. Great. Based on the expected behaviour https://github.com/Expensify/App/issues/6359#issuecomment-976021754. It can be fixed as follows:

Set the prioritizeDefaultRoomsInSearch: false for getSearchOptions. https://github.com/Expensify/App/blob/d2d17181727f2a4d264618986438df2733f107d0/src/libs/OptionsListUtils.js#L581

Add a new flag prioritizeOneToOneReportsInSearch. mark it true for getSearchOptions. Then add the following code after line OptionsListUtils.js#L510

// If we are prioritizing 1:1 chats in search, do it only once we started searching
if (prioritizeOneToOneReportsInSearch && searchValue !== '') {
    const [oneToOneReports, otherReports] = _.partition(recentReportOptions, option => !!option.login);
    recentReportOptions = oneToOneReports.concat(otherReports);
}

It will satisfy all three.

Jag96 commented 2 years ago

@parasharrajat that sounds good to me! @jboniface let's hire @parasharrajat for this one

MelvinBot commented 2 years ago

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

MelvinBot commented 2 years ago

Current assignee @parasharrajat is eligible for the Exported assigner, not assigning anyone new.

MelvinBot commented 2 years ago

Current assignee @Jag96 is eligible for the Exported assigner, not assigning anyone new.

Jag96 commented 2 years ago

@parasharrajat I created the job and sent an offer!

trjExpensify commented 2 years ago

@parasharrajat I tried testing w/ your proposal and it looks good for single users vs rooms, but it looks like you can still see a group chat above a single user chat. Can you update your proposal so that single users are shown above group chats?

Ah, so one thing here @parasharrajat. I was just chatting to @Jag96, but I think this was the logic for most recent when there’s a tiebreaker in the DMs match?

As in, if we have Joe and Joe, Ted. I just sent a message in Joe, Ted and I search for Joe it would show Joe, Ted before Tom.

Does everyone agree we should reinstate that if it was removed? If so, happy to add a milestone on to this Upwork contract to account for it 👍

Jag96 commented 2 years ago

Does everyone agree we should reinstate that if it was removed? If so, happy to add a milestone on to this Upwork contract to account for it 👍

Yup, I forgot about this sorry. I think adding a milestone to the Upwork contract is fine.

parasharrajat commented 2 years ago

Ok. understood but @Jag96 could you please lay down all the update cases once again so that I can decide what to do to cover all?

Jag96 commented 2 years ago

So the logic should be:

Always prioritize 1:1 chats and group chats over rooms (e.g. #announce)
Prioritize 1:1 chats over group chats based on which chat had the most recent message

So if I search for Tom and I haven't chatted w/ Tom 1:1 but we have a group chat, the results would show Tom, Joe, Rajat first, then Tom, then any rooms (e.g. #announce). Then, if I chat w/ Tom in a 1:1 and search Tom, the 1:1 should show up before the Tom, Joe, Rajat group, and then the #announce room would show at the bottom.

@trjExpensify any other additions there?

puneetlath commented 2 years ago

Prioritize 1:1 chats over group chats based on which chat had the most recent message

Hm, I dunno. I kind of feel like we should always prioritize the DM if only one name has been typed in. Perhaps we can discuss in the #open-source channel to make sure we have agreement on this real quick before going forward.

It has been discussed a few times before I remember (for example here), so I think it'd be good to get some broader visiblity on this decision.

Jag96 commented 2 years ago

Good idea! Dropped a message in slack for discussion

Jag96 commented 2 years ago

Based on the discussion it sounds like there isn't any additional work to be done here! I will update this GH if anything changes when others come online next week, but for now, we can leave this as is

botify commented 2 years ago

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

https://github.com/Expensify/App/pull/6544

If no regressions arise, payment will be issued on 2021-12-15. :confetti_ball:

parasharrajat commented 2 years ago

Ping for

@jboniface

jboniface commented 2 years ago

@parasharrajat sorry this slipped by me, job is here https://www.upwork.com/jobs/~01d07f9c95161deca4 i invited you

mosfiend commented 2 years ago

Is the search is mapping over the filtered array of objects? If that's the case, wouldn't the following solution work in theory? If it doesn't can you help me understand why?

1:1 chat/group chats /recent
1:1 chat/group chats /oldest
rooms /recent
rooms /oldest

Each chat object should have

a date property defaulting to null whose value is the date() of the last message
an isRoom property

searchOutput
            .filter(chat=>!chat.isRoom)
            .sort((chatA,chatB)=> chatA.date-chatB.date)
            .concat(searchOutput
                                    .filter(chat=>chat.isRoom)
                                    .sort((chatA,chatB)=> chatA.date-chatB.date)
)

Apologies if that's beside the point, still glossing over the codebase

mallenexpensify commented 2 years ago

@parasharrajat apologies for the delay here, I hired you, can you accept the offer so I can pay?

@mosfiend the best place to look for jobs is here https://github.com/Expensify/App/issues?q=is%3Aopen+is%3Aissue+label%3A%22Help+Wanted%22 it's all jobs with the Help Wanted label, the one's we're hiring for

mallenexpensify commented 2 years ago

Paid @parasharrajat

Expensify / App