1091675 - Implement quoting to disable multi-word search in addressbook

Status: RESOLVED FIXED

(Thunderbird :: Address Book, enhancement)

Description

[:aceman]

Currently, in the compose autocomplete and in addressbook searches we implement multi-word search, meaning:
Typing in "word1 word2" (without quotes) will search for cards having "word1" in some field AND "word2" in some field (may be different fieds).

We could implement a mechanism (e.g. quoting) to search for whole string as typed, including spaces.
So that "word1 word2" matches all cards what have "word1 word2" in one of their fields.

I see request for this feature e.g. in bug 970456 comment 43.
Bug 970456 prioritizes the matches in the result set of the search which may still be too large.
The bug here should produce a smaller result set with more relevant results if the user knows better what he is looking for and will decide to use the quotes.

Some context and specification:

Bug 1000775 comment 16

Thomas D. 2014-10-30 12:07:05 CET
(In reply to :aceman from comment #15)
> Just for info, this will affect patch in bug 118624.
> 
> I remember complaints that some users do not like the multiword searches.

I'm not aware of such complaints... (users are more concerned about startsWith vs. contains) multiword definitely wins over single-string because multiword can still find single-string, but single-string fails for a lot of legitimate queries and unreasonably limits the search to exact multiword strings which kinda defeats the purpose of searching because you already have to know the exact string. That said...

> Would you mind adding a way to search for the whole string, e.g. if it is
> inside quotes? If there isn't a bug already I can create it.

Yes, quoted strings should definitely be observed as single strings, which is useful and legitimate functionality for certain scenarios.

"foo bar" --> search for single string *"foo bar"*
"foo bar" baz --> search for *"foo bar"* AND *baz*
foo bar --> search for *foo* AND *bar*

Not sure how to deal with
"foo bar (missing trailing quote)

I think in quick search we automatically parse that same as
"foo bar" (treat as if trailing quote at end of search string was present)
which looks acceptable to me without deeper consideration (single word with quote seems less likely)

Comment 1

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1

We (myself and mconley) discussed about the duplication of the
helper method in both mailnews/ and mail/, so, we need a jsm
for removing this duplication (and others that already exist).
That calls for another follow-up.

Thanks.

Comment 2

[Suyash Agarwal (:sshagarwal)]

Oh this patch is to be applied over the patch from bug 1000775.

Comment 3

[:aceman]

Yeah, I have since noticed abCommon.js is not accessible from autocomplete. The autocomplete is a strange beast and a jsm module would solve the problem of duplication.
So if you create the module in some bug, please make it block bug 118624 so that we can then use it to move bug 118624 forward.

Comment 4

[:aceman]

Comment on attachment 8516820 [details] [diff] [review]
Patch v1

Review of attachment 8516820 [details] [diff] [review]:
-----------------------------------------------------------------

This seems to work for me, thanks!
I just found a small problem. A string like this does not produce 3 'searchwords': "ace man" xxx "exp and test". It only looks for "ace man" and "exp and test" .

::: mail/components/addrbook/content/abCommon.js
@@ +590,5 @@
>      return gDirectoryTreeView.getDirectoryAtIndex(gDirTree.currentIndex).URI;
>    }
>  }
>  
> +function buildUpSearchQuery(aSearchQuery, aModelQuery)

Please add a description of the function /* ... */ as it seems to have 2 different modes of operation (if aModelQuery is null). Also I would not call both arguments query. One is a search string and the other is a database query.

@@ +594,5 @@
> +function buildUpSearchQuery(aSearchQuery, aModelQuery)
> +{
> +  let queryURI = "";
> +
> +  if (aSearchQuery != "") {

Maybe an early return here?

::: mail/components/addrbook/content/addressbook.js
@@ +502,5 @@
>    */
>    var searchInput = document.getElementById("peopleSearchInput");
> +  // Use helper method to split up search query to multi-word search
> +  // query against multiple fields.
> +  searchURI += buildUpSearchQuery(searchInput.value, gQueryURIFormat);

You lost the check if searchInput actually exists.

::: mailnews/addrbook/src/nsAbAutoCompleteSearch.js
@@ +396,5 @@
>  function encodeABTermValue(aString) {
>    return encodeURIComponent(aString).replace(/\(/g, "%28").replace(/\)/g, "%29");
>  }
>  
> +function buildUpSearchQuery(aSearchQuery, aModelQuery)

Please add a comment that we need to merge these 2 functions with the ones in abCommon.js.

Comment 5

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

Fixed the nits.

Thanks.

Comment 6

[:aceman]

Comment on attachment 8517255 [details] [diff] [review]
Patch v2

Review of attachment 8517255 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks better now.

::: mail/components/addrbook/content/abCommon.js
@@ +609,5 @@
> +  if (aSearchString == "")
> +    return aSearchString;
> +
> +  let quotedTerms = [];
> +  let searchString = aSearchString.trim();

The return after trim. However I see now you return an array once, and if you have modelquery then a string. Please add it to the comment above.
I wonder if you also should return [] (empty array) if aSearchString is "". Or maybe null? How does the callers behave if buildUpSearchQuery returns "" ? What if we send the AB database an URI without any query string? Even no '?'... In the AB searches, it probably returns whole AB (all cards). But what does it do in autocomplete? We would not want all cards (better none). Of course, it is possible there is a check that a string of "" never gets to this place in the code.

@@ +632,5 @@
> +  if (searchString.length) {
> +    searchWords = quotedTerms.concat(searchString.trim().split(/\s+/));
> +  } else {
> +    searchWords = quotedTerms;
> +  }

I see no change in this algorithm between the 2 patches. Have you fixed the behaviour when search string is '"ace man" xxx "exp and test"' ?

Comment 7

[Suyash Agarwal (:sshagarwal)]

(In reply to :aceman from comment #6)
> 
> I see no change in this algorithm between the 2 patches. Have you fixed the
> behaviour when search string is '"ace man" xxx "exp and test"' ?

Ya, fixed :)

Comment 8

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3

(In reply to :aceman from comment #6)
> Comment on attachment 8517255 [details] [diff] [review]
> Patch v2
> 
> ::: mail/components/addrbook/content/abCommon.js
> @@ +609,5 @@
> > +  if (aSearchString == "")
> > +    return aSearchString;
> > +
> > +  let quotedTerms = [];
> > +  let searchString = aSearchString.trim();
> 
> The return after trim. However I see now you return an array once, and if
> you have modelquery then a string. Please add it to the comment above.
> I wonder if you also should return [] (empty array) if aSearchString is "".
> Or maybe null? How does the callers behave if buildUpSearchQuery returns ""
> ? What if we send the AB database an URI without any query string? Even no
> '?'... In the AB searches, it probably returns whole AB (all cards). But
> what does it do in autocomplete? We would not want all cards (better none).
> Of course, it is possible there is a check that a string of "" never gets to
> this place in the code.
Ya, [1] is where this is being handled.

[1] http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsAbAutoCompleteSearch.js#296

Thanks.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 8517886 [details] [diff] [review]
Patch v3

Review of attachment 8517886 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/addrbook/content/abCommon.js
@@ +601,5 @@
> + *
> + * @return
> + * if aModelQuery is null,     searchWords an array of separated search
> + *                             terms from the full search string.
> + * if aModelQuery is not null, queryURI.

Functions should return one thing, not two completeely different things. You need to refactor this.

@@ +625,5 @@
> +    quotedTerms.push(searchString.substring(startIndex + 1, endIndex));
> +    query = searchString.substring(0, startIndex) +
> +            ((endIndex < searchString.length) ?
> +             searchString.substr(endIndex + 1) : "");
> +    searchString = query.trim();

clearer to do this with an if clause

@@ +629,5 @@
> +    searchString = query.trim();
> +  }
> +
> +  let searchWords = [];
> +  if (searchString.length) {

I'd prefer length checking comparisons to a number

@@ +630,5 @@
> +  }
> +
> +  let searchWords = [];
> +  if (searchString.length) {
> +    searchWords = quotedTerms.concat(searchString.trim().split(/\s+/));

searchString is already trimmed
but also, doesn't this duplicate words?

Comment 10

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v4

I've tried to re-factor the proposed method.
(In reply to Magnus Melin from comment #9)
> Comment on attachment 8517886 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 8517886 [details] [diff] [review]:
> -----------------------------------------------------------------
> > +  let searchWords = [];
> > +  if (searchString.length) {
> > +    searchWords = quotedTerms.concat(searchString.trim().split(/\s+/));
> 
> searchString is already trimmed
> but also, doesn't this duplicate words?
Duplicate words? How?

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 8522400 [details] [diff] [review]
Patch v4

Review of attachment 8522400 [details] [diff] [review]:
-----------------------------------------------------------------

Way better, thx! 
This probably breaks some tests? And it should also have some test.

::: mail/components/addrbook/content/abCommon.js
@@ +613,5 @@
> + * against multiple fields of the addressbook cards.
> + *
> + * @param aSearchString The full search string entered by the user.
> + *
> + * @return  searchWords an array of separated search

@return an array
(the searchWords is not needed and confusing).

it may fit on one row then also

@@ +628,5 @@
> +  // Split up multiple search words to create a *foo* and *bar* search against
> +  // search fields, using the OR-search template from modelQuery for each word.
> +  // If the search query has quoted terms as "foo bar", extract them as is.
> +  let startIndex;
> +  let query;

only used inside the while loop. declare it there instead

@@ +664,5 @@
> + */
> +function generateQueryURI(aModelQuery, aSearchWords)
> +{
> +  if (!aModelQuery || !aSearchWords || aSearchWords.length == 0)
> +    return "";

Is it really worth handling !aModelQuery?
If it's not really meant to ever happen, maybe just let things blow up if it does?

::: mailnews/addrbook/src/nsAbAutoCompleteSearch.js
@@ +401,5 @@
>  }
>  
> +/**
> + * This is an exact replica of the method in abCommon.js and needs to
> + * be merged to remove this duplication.

Put a note in the other file too. Is there a bug already? If not, file one and mention the bug number

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 8522400 [details] [diff] [review]
Patch v4

Review of attachment 8522400 [details] [diff] [review]:
-----------------------------------------------------------------

So I think I'm happy w/ this part.

Comment 13

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v5

(In reply to Magnus Melin from comment #11)
> Comment on attachment 8522400 [details] [diff] [review]
> Patch v4
> 
> Review of attachment 8522400 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Way better, thx! 
> This probably breaks some tests? And it should also have some test.
Luckily it doesn't break any existing addressbook/ tests.

> @@ +664,5 @@
> > + */
> > +function generateQueryURI(aModelQuery, aSearchWords)
> > +{
> > +  if (!aModelQuery || !aSearchWords || aSearchWords.length == 0)
> > +    return "";
> 
> Is it really worth handling !aModelQuery?
> If it's not really meant to ever happen, maybe just let things blow up if it
> does?
I think we should leave this in case, things blow up? Is it expensive?
If yes, please let me know I'll strip it off.
 
> @@ +401,5 @@
> Put a note in the other file too. Is there a bug already? If not, file one
> and mention the bug number

Will do.

I couldn't figure out better test cases. Can you please confirm if this suffices else please suggest a few test cases.

Thanks.

Comment 14

[Magnus Melin [:mkmelin]]

(In reply to Suyash Agarwal (:sshagarwal) from comment #13)
> > Is it really worth handling !aModelQuery?
> > If it's not really meant to ever happen, maybe just let things blow up if it
> > does?
> I think we should leave this in case, things blow up? Is it expensive?

It's not so much about expensive or not, it just makes for very hard code to work with (and debug) if functions return seemingly ok return values even if you're on a failure path

> I couldn't figure out better test cases. Can you please confirm if this
> suffices else please suggest a few test cases.

Could you add one case where you test some none-trivial case, like
 "firstname lastname" mr "example dev" 
 to match people like "mr firstname lastname (example dev) <[e-mail]>"

Comment 15

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v5 upgraded

Okay, made the suggested changes.
Also, if this patch has any copyright violations or
can face any legal actions, pleas let me know :)

Thanks.

Comment 16

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch for check-in

Okay so approved we can use such test cases.
If there's anything that needs alterations please do let me know.
Carrying over review from mkmelin.

Thanks.

Comment 17

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/93f4e62465fd -> FIXED

Comment 18

[Magnus Melin [:mkmelin]]

Backed out for oranges: https://hg.mozilla.org/comm-central/rev/d22eeb1a685a

https://tbpl.mozilla.org/php/getParsedLog.php?id=52828867&tree=Thunderbird-Trunk&full=1

TEST-UNEXPECTED-FAIL | /builds/slave/test/build/mozmill/addrbook/test-address-book.js | test-address-book.js::test_deleting_contact_causes_confirm_prompt
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/mozmill/addrbook/test-address-book.js | test-address-book.js::test_deleting_contacts_causes_confirm_prompt
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/mozmill/addrbook/test-address-book.js | test-address-book.js::test_deleting_mailing_lists
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/mozmill/addrbook/test-update-mailing-list.js | test-update-mailing-list.js::test_contact_in_mailing_list_updated


Probably caused by

JavaScript error: chrome://messenger/content/addressbook/abCommon.js, line 669: TypeError: aSearchWords.forEach is not a function

Comment 19

[Suyash Agarwal (:sshagarwal)]

My fault. I ran all the xpcshell tests and not the mozmills.
Apologies. I'll fix this asap.

Thanks.

Comment 20

[Suyash Agarwal (:sshagarwal)]

Let's see what this has to say:
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=6b35a7967843

Comment 21

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch with fix for test failure

So the issue was, as a fallback I was returning an empty string
instead of an empty array which was posing issues with "forEach".
Fixed now.

All the xpcshell and mozmill tests pass on my system. Trunk is
currently busted so a try run cannot be done.

Thanks.

Comment 22

[Magnus Melin [:mkmelin]]

Comment on attachment 8526184 [details] [diff] [review]
Patch with fix for test failure

Review of attachment 8526184 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thx Suyash! r=mkmelin

Comment 23

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/4dccd9337159 -> FIXED again