302511 - Remove QuickSearch's dependency on default priority values

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, enhancement, P4)

Description

[Marc Schumann [:Wurblzap]]

QuickSearch offers a feature to restrict the result set to a range of
priorities. This relies on each of the legal priority values to match
/[pP][1-5]/
(http://lxr.mozilla.org/mozilla/source/webtools/bugzilla/quicksearch.js#533).
This feature stops working if you modify the list of legal priority values away
from the default.

It should be possible to use the same QuickSearch syntax to address priorities
ordered by sortkey, so that p1 matches the priority value with the lowest
sortkey, p2 matches the next one and so on.

Comment 1

[Marc Schumann [:Wurblzap]]

The offending code has moved to http://lxr.mozilla.org/mozilla/source/webtools/bugzilla/Bugzilla/Search/Quicksearch.pm#291 in the meantime.

Comment 2

[Max Kanat-Alexander]

Attachment: v1

Pretty simple fix.

Comment 3

[Marc Schumann [:Wurblzap]]

Comment on attachment 373408 [details] [diff] [review]
v1

This patch breaks searching for priority ranges. It's good to add the priority match, but the "p" syntax should stay supported. See comment 0.

Comment 4

[Max Kanat-Alexander]

Comment on attachment 373408 [details] [diff] [review]
v1

Yeah, I'd rather just remove that functionality now that the default priorities won't be P1 - P5 and it won't make sense (nor will it be discoverable).

Comment 5

[Frédéric Buclin]

I'm not allowed to override Marc's review. So even if I granted review, Marc's r- would still stand.

Comment 6

[Marc Schumann [:Wurblzap]]

Please leave the "p" syntax in for backwards compatibility. Removing this would annoy people on legacy installations who use p1-2 routinely.

This is already discoverable on page.cgi?id=quicksearchhack.html.

Comment 7

[Frédéric Buclin]

Comment on attachment 373408 [details] [diff] [review]
v1

Hum yes, Marc is right. Old installations will still use P1-P5 by default.

Comment 8

[Max Kanat-Alexander]

(In reply to comment #7)
> (From update of attachment 373408 [details] [diff] [review])
> Hum yes, Marc is right. Old installations will still use P1-P5 by default.

  That's not the point Marc was making.

Comment 9

[Max Kanat-Alexander]

Attachment: v2

Okay, this preserves the syntax. Tested on landfill.

Comment 10

[Marc Schumann [:Wurblzap]]

Comment on attachment 373547 [details] [diff] [review]
v2

Yeah, this appears to be exactly what's needed. The new regexp looks strange to me (?:) at first glance; I'll take a closer look.

Comment 11

[Max Kanat-Alexander]

(?:) means "don't capture this, these parentheses are purely to delimit some group of characters that we are then going to say something about" (in this case, that the group is optional).

Comment 12

[Marc Schumann [:Wurblzap]]

Comment on attachment 373547 [details] [diff] [review]
v2

If I ask for p7-9, I get a cryptic error message.

The code must check for legal start and end values. If one of them is out of range, addChart() shouldn't be called.

Similarly, it shouldn't be called if the upper limit is lower than the lower limit.

In addition, both \d should be replaced by \d+.

Comment 13

[Max Kanat-Alexander]

Attachment: v3

Okay, fixed it.

Comment 14

[Max Kanat-Alexander]

Attachment: v4

There was a slight error in the last patch.

Also, I don't see a problem with the first item being higher than the second. It seems to still work--it just doesn't include the second item, and that's clear when you do the search, since Bugzilla now tells you what you searched for. I don't see a Perl warning or anything caused by it.

Comment 15

[Marc Schumann [:Wurblzap]]

Comment on attachment 374980 [details] [diff] [review]
v4

Yeah, this is good.

I don't know what I've seen with reversed limits.

Comment 16

[Max Kanat-Alexander]

Checking in Bugzilla/Search/Quicksearch.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search/Quicksearch.pm,v  <--  Quicksearch.pm
new revision: 1.24; previous revision: 1.23
done