Closed Bug 1091675 Opened 7 years ago Closed 7 years ago

Implement quoting to disable multi-word search in addressbook

Categories

(Thunderbird :: Address Book, enhancement)

enhancement
Not set
normal

Tracking

(thunderbird36 fixed)

RESOLVED FIXED
Thunderbird 36.0
Tracking Status
thunderbird36 --- fixed

People

(Reporter: aceman, Assigned: sshagarwal)

References

Details

(Keywords: ux-efficiency, Whiteboard: [needs documentation])

Attachments

(1 file, 7 obsolete files)

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)
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #8516820 - Flags: feedback?(acelists)
Oh this patch is to be applied over the patch from bug 1000775.
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 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.
Attachment #8516820 - Flags: feedback?(acelists) → feedback+
Attached patch Patch v2 (obsolete) — Splinter Review
Fixed the nits.

Thanks.
Attachment #8516820 - Attachment is obsolete: true
Attachment #8517255 - Flags: feedback?(acelists)
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"' ?
Attachment #8517255 - Flags: feedback?(acelists) → feedback+
(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 :)
Attached patch Patch v3 (obsolete) — Splinter Review
(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.
Attachment #8517255 - Attachment is obsolete: true
Attachment #8517886 - Flags: review?(mkmelin+mozilla)
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?
Attachment #8517886 - Flags: review?(mkmelin+mozilla) → review-
Attached patch Patch v4 (obsolete) — Splinter Review
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?
Attachment #8517886 - Attachment is obsolete: true
Attachment #8522400 - Flags: review?(mkmelin+mozilla)
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 on attachment 8522400 [details] [diff] [review]
Patch v4

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

So I think I'm happy w/ this part.
Attachment #8522400 - Flags: review?(mkmelin+mozilla) → review+
Attached patch Patch v5 (obsolete) — Splinter Review
(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.
Attachment #8522400 - Attachment is obsolete: true
Attachment #8523386 - Flags: review+
Attachment #8523386 - Flags: feedback?(acelists)
(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) <foo@example.org>"
Attached patch Patch v5 upgraded (obsolete) — Splinter Review
Okay, made the suggested changes.
Also, if this patch has any copyright violations or
can face any legal actions, pleas let me know :)

Thanks.
Attachment #8523386 - Attachment is obsolete: true
Attachment #8523386 - Flags: feedback?(acelists)
Attachment #8523440 - Flags: review+
Attachment #8523440 - Flags: feedback?(acelists)
Attached patch Patch for check-in (obsolete) — Splinter Review
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.
Attachment #8523440 - Attachment is obsolete: true
Attachment #8523440 - Flags: feedback?(acelists)
Attachment #8523487 - Flags: review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
Whiteboard: [needs documentation]
https://hg.mozilla.org/comm-central/rev/93f4e62465fd -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 36.0
Blocks: 1100801
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
My fault. I ran all the xpcshell tests and not the mozmills.
Apologies. I'll fix this asap.

Thanks.
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.
Attachment #8523487 - Attachment is obsolete: true
Attachment #8526184 - Flags: review?(mkmelin+mozilla)
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
Attachment #8526184 - Flags: review?(mkmelin+mozilla) → review+
https://hg.mozilla.org/comm-central/rev/4dccd9337159 -> FIXED again
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1140218
You need to log in before you can comment on or make changes to this bug.