Last Comment Bug 870282 - Searching by "Priority" is missing an "Isn't" operator
: Searching by "Priority" is missing an "Isn't" operator
Status: RESOLVED FIXED
[good first bug][mentor=aceman][lang=...
:
Product: MailNews Core
Classification: Components
Component: Search (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 24.0
Assigned To: Suyash Agarwal (:sshagarwal)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-09 01:52 PDT by :aceman
Modified: 2013-05-14 05:34 PDT (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (7.39 KB, patch)
2013-05-10 07:26 PDT, Suyash Agarwal (:sshagarwal)
no flags Details | Diff | Splinter Review
Patch v2 (10.71 KB, patch)
2013-05-10 14:20 PDT, Suyash Agarwal (:sshagarwal)
rkent: review+
acelists: feedback+
Details | Diff | Splinter Review
Patch v2 (revised) (10.39 KB, patch)
2013-05-14 03:41 PDT, Suyash Agarwal (:sshagarwal)
syshagarwal: review+
acelists: feedback+
Details | Diff | Splinter Review

Description :aceman 2013-05-09 01:52:41 PDT
If searching messages by the Priority field, the available operators are "Is", "Is Lower than", "Is Higher than". There is no "Isn't" operator.

The use-case could be:
Search all messages where Priority isn't Normal. It is currently possible to simulate that via "(Priority, is higher than, Normal) OR (Priority, is lower than, Normal)", but that forces the OR operator on all other search terms in the same search (or filter).

So this operator needs to be added to:
nsMsgSearchTerm.cpp > nsMsgSearchTerm::MatchPriority

and:
mailnews/base/search/src/nsMsgImapSearch.cpp (in the lines like: "SetAvailable (nsMsgSearchAttrib::Priority, nsMsgSearchOp::Is, 1);") . It seems the UI picks it up from there.
Comment 1 Suyash Agarwal (:sshagarwal) 2013-05-10 07:26:41 PDT
Created attachment 747970 [details] [diff] [review]
Patch

Sir,

I have tried to make the changes as per the description.
Comment 2 Kent James (:rkent) 2013-05-10 08:48:23 PDT
Could you add a test for this new attribute in mailnews/base/test/unit/test_search.js?
Comment 3 Suyash Agarwal (:sshagarwal) 2013-05-10 14:20:06 PDT
Created attachment 748229 [details] [diff] [review]
Patch v2

Sir,

I have added the test.
Comment 4 :aceman 2013-05-10 15:19:57 PDT
Comment on attachment 748229 [details] [diff] [review]
Patch v2

The change and the test works great! Thanks.
Comment 5 Kent James (:rkent) 2013-05-13 10:10:40 PDT
Comment on attachment 748229 [details] [diff] [review]
Patch v2

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

Overall, looks good and great job. Please just fix the one nit.

::: mailnews/base/test/unit/test_search.js
@@ +207,5 @@
>        customHeader: "withspace",
>        count: 1},
> +    
> +    //test for priority
> +    { testString: Components.interfaces.nsMsgPriority.lowest,

Nit: In xpcshell tests, standard abbreviations are predefined, and should be used. So please change these five instances of Components.interfaces. to Ci.
Comment 6 Suyash Agarwal (:sshagarwal) 2013-05-14 03:41:18 PDT
Created attachment 749207 [details] [diff] [review]
Patch v2 (revised)
Comment 7 Suyash Agarwal (:sshagarwal) 2013-05-14 03:45:52 PDT
Sir,
Thanks for your review and feedback.
I have made the changes.
Carrying over review from rkent.
Comment 8 :aceman 2013-05-14 04:03:51 PDT
Comment on attachment 749207 [details] [diff] [review]
Patch v2 (revised)

Yes, that's it.
Comment 9 :aceman 2013-05-14 04:05:17 PDT
Congratulations to your first user visible change in TB and Seamonkey:)
Comment 10 Ryan VanderMeulen [:RyanVM] 2013-05-14 05:34:49 PDT
https://hg.mozilla.org/comm-central/rev/e232ca630f82

Note You need to log in before you can comment on or make changes to this bug.