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

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

Closed
isagoico opened this issue Nov 18, 2021 · 30 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@isagoico
Copy link

isagoico commented Nov 18, 2021

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:

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

Expected Result:

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

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

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

image

Expensify/Expensify Issue URL:

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

View all open jobs on GitHub

@MelvinBot
Copy link

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

@parasharrajat
Copy link
Member

Proposal

  1. To do this we have to set the prioritizeDefaultRoomsInSearch: false for getSearchOptions.
    prioritizeDefaultRoomsInSearch: true,

@mountiny
Copy link
Contributor

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

Jag96 commented Nov 23, 2021

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

  1. Person Name
  2. Room Name
  3. 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.

@MelvinBot MelvinBot removed the Overdue label Nov 23, 2021
@trjExpensify
Copy link
Contributor

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

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

@Jag96
Copy link
Contributor

Jag96 commented Nov 29, 2021

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!

@MelvinBot MelvinBot removed the Overdue label Nov 29, 2021
@mountiny
Copy link
Contributor

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

@trjExpensify
Copy link
Contributor

That was it from my POV at this point! 👍

@Jag96 Jag96 added the External Added to denote the issue can be worked on by a contributor label Nov 29, 2021
@MelvinBot
Copy link

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

@Jag96
Copy link
Contributor

Jag96 commented Nov 29, 2021

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

Ok. Great. Based on the expected behaviour #6359 (comment).
It can be fixed as follows:

  1. Set the prioritizeDefaultRoomsInSearch: false for getSearchOptions.
    prioritizeDefaultRoomsInSearch: true,
  2. 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
Copy link
Contributor

Jag96 commented Nov 30, 2021

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

@MelvinBot
Copy link

📣 @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 📖

@Jag96 Jag96 added Reviewing Has a PR in review Exported labels Dec 1, 2021
@botify botify removed the Daily KSv2 label Dec 1, 2021
@MelvinBot MelvinBot added the Weekly KSv2 label Dec 1, 2021
@MelvinBot
Copy link

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

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 1, 2021
@MelvinBot
Copy link

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

@Jag96 Jag96 removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 1, 2021
@Jag96
Copy link
Contributor

Jag96 commented Dec 1, 2021

@parasharrajat I created the job and sent an offer!

@trjExpensify
Copy link
Contributor

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

Jag96 commented Dec 1, 2021

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

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

Jag96 commented Dec 3, 2021

So the logic should be:

  1. Always prioritize 1:1 chats and group chats over rooms (e.g. #announce)
  2. 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
Copy link
Contributor

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

Jag96 commented Dec 3, 2021

Good idea! Dropped a message in slack for discussion

@Jag96
Copy link
Contributor

Jag96 commented Dec 3, 2021

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 botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Dec 8, 2021
@botify botify changed the title Search - Make users appear first in the result list over group chats or rooms [HOLD for payment 2021-12-15] Search - Make users appear first in the result list over group chats or rooms Dec 8, 2021
@botify
Copy link

botify commented Dec 8, 2021

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

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

@parasharrajat
Copy link
Member

Ping for
image

@jboniface

@jboniface
Copy link

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

@mallenexpensify mallenexpensify self-assigned this Dec 18, 2021
@mosfiend
Copy link

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:1 chat/group chats /recent
  2. 1:1 chat/group chats /oldest
  3. rooms /recent
  4. 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
Copy link
Contributor

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

Paid @parasharrajat

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests