[$500] Bug: You can't search a room with names like "x.y" reported by @Puneet-here

[kavimuru]

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

---

Action Performed:

1. Create a workspace > Rename it to "x.y"
2. Search the room by typing "x.y"

Expected Result:

You should see the room

Actual Result:

You won't get any results

Workaround:

unknown

Platform:

Where is this issue occurring?

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

Version Number: v1.2.18-2 Reproducible in staging?: y Reproducible in production?: y Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/196831239-02bffcff-2d10-4c5b-a9cc-e63dfdc7cc41.mov

https://user-images.githubusercontent.com/43996225/196831202-373ab346-8920-41e5-a55e-484c10fe6ef7.mp4

Expensify/Expensify Issue URL: Issue reported by: @Puneet-here Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1666216269896569

View all open jobs on GitHub

[melvin-bot[bot]]

Triggered auto assignment to @laurenreidexpensify (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[melvin-bot[bot]]

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

[laurenreidexpensify]

@cristipaval looks like a solid external to me.

[cristipaval]

Hey @laurenreidexpensify, yes, I had a glance locally on my machine and seems that the search is performed on the client, no api call. We can mark it as external.

[melvin-bot[bot]]

Current assignee @laurenreidexpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane (External)

[melvin-bot[bot]]

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

[laurenreidexpensify]

https://www.upwork.com/jobs/~0105f9e9709c5f9cc1

[Ollyws]

Proposal

OptionsListUtils removes full stops from the search string here: https://github.com/Expensify/App/blob/b2acb8f48e6ca05cbce64771e9499f85c4872018/src/libs/OptionsListUtils.js#L375-L382 If we don't want this functionality we can simply remove it:

const searchWords = _.map(
        searchValue
-           .replace(/\./g, '')
            .replace(/,/g, ' ')
            .split(' '),
        word => word.trim(),

[0xmiroslav]

Proposal

This is caused by https://github.com/Expensify/App/pull/8375 which fixes https://github.com/Expensify/App/issues/8007

Solution: .replace(/\./g, '') is only for email search, so we can remove that only when search for chat room names

https://github.com/Expensify/App/blob/b2acb8f48e6ca05cbce64771e9499f85c4872018/src/libs/OptionsListUtils.js#L375-L382

    const searchWords = _.map(
        isChatRoom ? searchValue.replace(/,/g, ' ').split(' ') : searchValue.replace(/\./g, '').replace(/,/g, ' ').split(' '),
        word => word.trim(),
    );

This fixes both https://github.com/Expensify/App/issues/12026 and https://github.com/Expensify/App/issues/8007

[rushatgabhane]

The bug makes sense to me. Workspace names could have the company's domain name in it.

@roryabraham I like @0xmiroslav's proposal. 🎀 👀 🎀 C+ reviewed

[laurenreidexpensify]

@roryabraham bump :)

[roryabraham]

I'm not sure ... why don't we just remove periods from both the searchValue and the searchWords? Also, why do we just remove periods and replace commas with a space?

[roryabraham]

Long-term, I think we need a more generic, performant, and robust text searching algorithm. We can treat that as out-of-scope for now, but I don't want to add a bunch of domain-specific search rule exceptions, like "ignore periods when looking for DMs, but don't ignore them when looking for chat rooms"

[roryabraham]

Overall I'm not confident in any of the proposals so far, sorry

[melvin-bot[bot]]

@rushatgabhane, @roryabraham, @laurenreidexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot[bot]]

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

[laurenreidexpensify]

Price doubled

[laurenreidexpensify]

Price now $1000, bumped thread https://expensify.slack.com/archives/C01GTK53T8Q/p1667826951304139?thread_ts=1666216269.896569&cid=C01GTK53T8Q

@Puneet-here do you have any solutions to propose?

[s77rt]

Proposal

diff --git a/src/libs/OptionsListUtils.js b/src/libs/OptionsListUtils.js
index d23a3e119..b420e878c 100644
--- a/src/libs/OptionsListUtils.js
+++ b/src/libs/OptionsListUtils.js
@@ -189,7 +189,7 @@ function getSearchText(report, reportName, personalDetailList, isChatRoomOrPolic
     if (!isChatRoomOrPolicyExpenseChat) {
         for (let i = 0; i < personalDetailList.length; i++) {
             const personalDetail = personalDetailList[i];
-            searchTerms = searchTerms.concat([personalDetail.displayName, personalDetail.login.replace(/\./g, '')]);
+            searchTerms = searchTerms.concat([personalDetail.displayName, personalDetail.login, personalDetail.login.replace(/\./g, '')]);
         }
     }
     if (report) {
@@ -389,14 +389,11 @@ function createOption(logins, personalDetails, report, reportActions = {}, {
  * @returns {Boolean}
  */
 function isSearchStringMatch(searchValue, searchText, participantNames = new Set(), isChatRoom = false) {
-    const searchWords = _.map(
-        searchValue
-            .replace(/\./g, '')
-            .replace(/,/g, ' ')
-            .split(' '),
-        word => word.trim(),
-    );
-    return _.every(searchWords, (word) => {
+    const searchWords = _.compact(_.uniq([
+        searchValue,
+        ..._.map(searchValue.replace(/,/g, ' ').split(' '), word => word.trim()),
+    ]));
+    return _.some(searchWords, (word) => {
         const matchRegex = new RegExp(Str.escapeForRegExp(word), 'i');
         const valueToSearch = searchText && searchText.replace(new RegExp(/&nbsp;/g), '');
         return matchRegex.test(valueToSearch) || (!isChatRoom && participantNames.has(word));

Explanation

The main problem with the previous solutions and the existing one is that the original searchValue is being overwritten and completely lost. The better approach is to add more searchValues and keeping the original.

Following my proposal, in the isSearchStringMatch function, searchWords contains the original searchValue and other derived search values. those derived values are used for two reasons: 1. get more relevant search results and 2. multiple search (see below).

As for the #8007 issue, following the same approach, searching for a member will be based on his display name, his login/email and his filtered login/email (the filter is to remove the periods) as seen on getSearchText function

Also, there is another issue, which is the use of _every instead of _some. The use of _every was introduced by #1523 to fix #1283 However, the old proposal only returns fully matched results. using _some will return both partially and fully matched results; meaning if you search for "test1 test2" you will get messages with user test1, test2, test1&test2. Besides that the use of _some is required here nevertheless.

Results:

1. Searching for "x.y" returns "x.y" (fixes this issues)
2. Searching for "abcxyz" or "abc.xyz" returns emails that contains "abcxyz" and/or "abc.xyz" (fixes #8007)
3. Searching for "test1 test2" or "test1, test2" returns results containing "test1", "test2" and results that contains both "test1" and "test2" (related #1283)

https://user-images.githubusercontent.com/16493223/200965219-3736327a-3d17-4dca-82b5-c827b0bbe59f.mp4

[laurenreidexpensify]

@rushatgabhane what you think?

[rushatgabhane]

reviewing tonight

[laurenreidexpensify]

@rushatgabhane are we 👍 or 👎 for this proposal? Thanks

[rushatgabhane]

Searching for "test1 test2" or "test1, test2" returns results containing "test1", "test2" and results that contains both "test1" and "test2

One good thing about this would be that let's say you're searching for a group containing "alice, bob, claire", but only a group of "alice" and "bob" exists. We'll be showing alice and bob's group vs returning no search results at all.

But I'm not sure about this change because it's gonna show anything and everything related to "alice", "bob" and "claire"

[s77rt]

that's correct, this is more a matter of design whether this should be the expected behaviour or not. Let me know if I should update my proposal given the expected behaviour

[rushatgabhane]

@s77rt so from what I understand your proposal makes two changes only

1. return power set of the comma seperated search terms
2. (1) will allow us to remove the . hack used for searching emails

[rushatgabhane]

@s77rt bug: searching for expensify.com doesn't show marc's report.

https://user-images.githubusercontent.com/29673073/201958907-2975cc0c-53a7-4b0b-aae9-56b2db9618f0.mov

[s77rt]

@rushatgabhane I can't reproduce the bug. Can you give more details?

https://user-images.githubusercontent.com/16493223/201962311-32cff3b4-ca81-4022-a654-5b1f4e8ecf1e.mp4

[rushatgabhane]

@s77rt im not sure about the steps or why it's happening.

But could it be this? -

1. Have 10+ contacts from a particular domain eg: test.com
2. Search for test.com and make sure all contacts with that domain are visible.

[s77rt]

@rushatgabhane Still no bug

https://user-images.githubusercontent.com/16493223/201983792-a4d7cbc7-2bc8-4e4d-bf9f-4044be90a31e.mp4

Can you try with the latest version (main branch) then apply the patch