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

RESOLVED FIXED in Bugzilla 3.6

Status

()

Bugzilla
Query/Bug List
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Thomas D. (currently busy elsewhere; needinfo?me), Assigned: Frédéric Buclin)

Tracking

({regression})

3.6.1
Bugzilla 3.6
regression
Dependency tree / graph
Bug Flags:
approval +
approval4.0 +
blocking4.0 +
approval3.6 +
blocking3.6.3 +

Details

Attachments

(1 attachment, 1 obsolete attachment)

2.79 KB, patch
Max Kanat-Alexander
: review+
Details | Diff | Splinter Review
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)
(Assignee)

Comment 1

7 years ago
Hum, yeah, Bugzilla 3.6 and above seem to ignore P\d+.
Keywords: regression
OS: Windows XP → All
Hardware: x86 → All
(Assignee)

Updated

7 years ago
Target Milestone: --- → Bugzilla 3.6

Comment 2

7 years ago
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.
Flags: blocking3.6.3+
(Assignee)

Comment 3

7 years ago
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.
Assignee: query-and-buglist → LpSolit
Status: NEW → ASSIGNED
Attachment #468501 - Flags: review?(mkanat)
(Assignee)

Updated

7 years ago
Depends on: 302511

Comment 4

7 years ago
(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

7 years ago
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.
Attachment #468501 - Flags: review?(mkanat) → review-
(Assignee)

Comment 6

7 years ago
(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

7 years ago
(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.
(Assignee)

Comment 8

7 years ago
(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

7 years ago
(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.
(Assignee)

Comment 10

7 years ago
Created attachment 476757 [details] [diff] [review]
patch, v2

OK, I fixed the incorrect encoding and everything else.
Attachment #468501 - Attachment is obsolete: true
Attachment #476757 - Flags: review?(mkanat)
(Assignee)

Updated

7 years ago
Flags: blocking4.0+
(Assignee)

Comment 11

7 years ago
(FYI, the correct syntax is e.g. P1-5, not P1-P5)
Summary: QuickSearch: Advanced Shortcut for Priority (P1-P5 as search word) broken (e.g. buglist.cgi?quicksearch=P1+:Bugzilla) → QuickSearch: Advanced Shortcut for Priority (P1-5 as search word) broken (e.g. buglist.cgi?quicksearch=P1+:Bugzilla)

Comment 12

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

Looks great! Thanks. :-)
Attachment #476757 - Flags: review?(mkanat) → review+

Updated

7 years ago
Flags: approval4.0+
Flags: approval3.6+
Flags: approval+
(Assignee)

Comment 13

7 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Blocks: 584956
(Assignee)

Updated

7 years ago
Blocks: 611129
You need to log in before you can comment on or make changes to this bug.