585028 - QuickSearch: Advanced Shortcut for Priority (P1-5 as search word) broken (e.g. buglist.cgi?quicksearch=P1+:Bugzilla)

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Thomas D. (:thomas8)]

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]

Hum, yeah, Bugzilla 3.6 and above seem to ignore P\d+.

Comment 2

[Max Kanat-Alexander]

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]

Attachment: 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]

(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]

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]

(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]

(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]

(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]

(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]

Attachment: patch, v2

OK, I fixed the incorrect encoding and everything else.

Comment 11

[Frédéric Buclin]

(FYI, the correct syntax is e.g. P1-5, not P1-P5)

Comment 12

[Max Kanat-Alexander]

Comment on attachment 476757 [details] [diff] [review]
patch, v2

Looks great! Thanks. :-)

Comment 13

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Search/Quicksearch.pm
Committed revision 7485.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.0/
modified Bugzilla/Search/Quicksearch.pm
Committed revision 7414.

Committing to: bzr+ssh://[e-mail]/bugzilla/3.6/
modified Bugzilla/Search/Quicksearch.pm
Committed revision 7179.