Last Comment Bug 585028 - QuickSearch: Advanced Shortcut for Priority (P1-5 as search word) broken (e.g. buglist.cgi?quicksearch=P1+:Bugzilla)
: QuickSearch: Advanced Shortcut for Priority (P1-5 as search word) broken (e.g...
Status: RESOLVED FIXED
: regression
Product: Bugzilla
Classification: Server Software
Component: Query/Bug List (show other bugs)
: 3.6.1
: All All
: -- normal (vote)
: Bugzilla 3.6
Assigned To: Frédéric Buclin
: default-qa
:
Mentors:
Depends on: 302511
Blocks: 584956 611129
  Show dependency treegraph
 
Reported: 2010-08-06 06:56 PDT by Thomas D. (needinfo?me)
Modified: 2010-11-10 14:37 PST (History)
2 users (show)
mkanat: approval+
mkanat: approval4.0+
LpSolit: blocking4.0+
mkanat: approval3.6+
mkanat: blocking3.6.3+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1 (2.03 KB, patch)
2010-08-23 15:37 PDT, Frédéric Buclin
mkanat: review-
Details | Diff | Splinter Review
patch, v2 (2.79 KB, patch)
2010-09-20 03:31 PDT, Frédéric Buclin
mkanat: review+
Details | Diff | Splinter Review

Description Thomas D. (needinfo?me) 2010-08-06 06:56:43 PDT
Shortcut for Priority (P1-P5 as search word) is broken:

STR

https://bugzilla.mozilla.org/buglist.cgi?quicksearch=P1+:Bugzilla

Actual result
- returns 2792 bugs that are in Bugzilla product or component (regardless of their value in Priority field)

Expected result
- should return all bugs with Priority=P1 for Bugzilla product or component
(according to https://bugzilla.mozilla.org/page.cgi?id=quicksearch.html#shortcuts)
Comment 1 Frédéric Buclin 2010-08-06 07:32:09 PDT
Hum, yeah, Bugzilla 3.6 and above seem to ignore P\d+.
Comment 2 Max Kanat-Alexander 2010-08-06 17:51:01 PDT
The code is certainly there:

    # P1-5 Syntax
    if ($word =~ m/^P(\d+)(?:-(\d+))?$/i) {

So we just have to figure out why it's not working.
Comment 3 Frédéric Buclin 2010-08-23 15:37:25 PDT
Created attachment 468501 [details] [diff] [review]
patch, v1

My patch fixes several problems:

- P1-5 is P1%2D5 when entering _special_field_syntax(), as we encoded minus signs to protect them from being seen as negation.

- It fixes a warning appearing in the error log about $2 being undefined when not using the P1-5 syntax (and so $2 - 1 is a problem).

- In case someone added (like me) P0 or removed e.g. P2, but the specified Pn (n being an integer) exists, then we use it. If it doesn't exist, then we fall back as being the nth priority of the list.

- Having $start > $end didn't work. I fixed that too.
Comment 4 Max Kanat-Alexander 2010-08-29 19:26:02 PDT
(In reply to comment #3)
> - P1-5 is P1%2D5 when entering _special_field_syntax(), as we encoded minus
> signs to protect them from being seen as negation.

  Hmm. That shouldn't be happening--we should only be encoding minus signs if they're inside quotes. So we should fix that as well, as part of this patch.
Comment 5 Max Kanat-Alexander 2010-09-07 02:37:54 PDT
Comment on attachment 468501 [details] [diff] [review]
patch, v1

>+    # P1-5 Syntax. %2D is the encoded form for "-", see splitString().
>+    if ($word =~ m/^P(\d+)(?:%2D(\d+))?$/i) {

  We're only supposed to be encoding minus signs if they're in quotes. And if the minus sign is in a quote, then we definitely would not want this behavior to be happening.

>+        my $end = defined $2 ? firstidx { $_ eq "P$2" } @$legal_priorities : undef;

  The precedence is unclear to me there. Perhaps making it "my $end; if (defined $2) {" would be better.

>         my $prios = $legal_priorities->[$start];

>-        if ($end) {
>+
>+        if (defined $end) {
>+            # If Pn doesn't exist explicitly, then we mean the nth priority.
>+            if ($end == -1) {
>+                $end = min(scalar(@$legal_priorities), $2) - 1;

  Perhaps you should put $2 into some other variable and use it, there?

  Also, if $2 is 0, then that could come out as -1, no?

>+            ($start, $end) = reverse($start, $end) if $end < $start;

  Could just do ($start, $end) = ($end, $start) if $end < $start.

  Does this work even if somebody tries to do "P1-P1"?


  Otherwise this is a good improvement generally--we definitely want this patch one way or another.
Comment 6 Frédéric Buclin 2010-09-07 07:15:46 PDT
(In reply to comment #5)
>   We're only supposed to be encoding minus signs if they're in quotes. And if
> the minus sign is in a quote, then we definitely would not want this behavior
> to be happening.

Well, we encoded the minus sign to fix bug 553884.
Comment 7 Max Kanat-Alexander 2010-09-07 18:32:59 PDT
(In reply to comment #6)
> Well, we encoded the minus sign to fix bug 553884.

  Right, but the minus sign should only be encoded *if it's inside of quotes*, because that's what bug 553884 was. So if minus signs *outside* of quotes are getting encoded, that's a bug.
Comment 8 Frédéric Buclin 2010-09-08 02:54:16 PDT
(In reply to comment #7)
> So if minus signs *outside* of quotes are getting encoded, that's a bug.

That's not a bug, AFAIK. Encoded characters are always fine, maybe not necessary, but harmless.
Comment 9 Max Kanat-Alexander 2010-09-08 15:37:54 PDT
(In reply to comment #8)
> That's not a bug, AFAIK. Encoded characters are always fine, maybe not
> necessary, but harmless.

  Okay, but it's a bug because we didn't intend it to happen, and it shouldn't be happening, so we should investigate why it's happening.
Comment 10 Frédéric Buclin 2010-09-20 03:31:50 PDT
Created attachment 476757 [details] [diff] [review]
patch, v2

OK, I fixed the incorrect encoding and everything else.
Comment 11 Frédéric Buclin 2010-09-20 03:33:37 PDT
(FYI, the correct syntax is e.g. P1-5, not P1-P5)
Comment 12 Max Kanat-Alexander 2010-09-20 15:51:57 PDT
Comment on attachment 476757 [details] [diff] [review]
patch, v2

Looks great! Thanks. :-)
Comment 13 Frédéric Buclin 2010-09-21 10:50:34 PDT
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Search/Quicksearch.pm
Committed revision 7485.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
modified Bugzilla/Search/Quicksearch.pm
Committed revision 7414.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/3.6/
modified Bugzilla/Search/Quicksearch.pm
Committed revision 7179.

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