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

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: bugzilla2007, Assigned: LpSolit)

Tracking

({regression})

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

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

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

9 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

9 years ago
Target Milestone: --- → Bugzilla 3.6
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

9 years ago
Posted patch patch, v1 (obsolete) — Splinter Review
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

9 years ago
Depends on: 302511
(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 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

9 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.
(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

9 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.
(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

9 years ago
Posted patch patch, v2Splinter Review
OK, I fixed the incorrect encoding and everything else.
Attachment #468501 - Attachment is obsolete: true
Attachment #476757 - Flags: review?(mkanat)
Assignee

Updated

9 years ago
Flags: blocking4.0+
Assignee

Comment 11

9 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 on attachment 476757 [details] [diff] [review]
patch, v2

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

Updated

9 years ago
Flags: approval4.0+
Flags: approval3.6+
Flags: approval+
Assignee

Comment 13

9 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
Closed: 9 years ago
Resolution: --- → FIXED
Assignee

Updated

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