Remove QuickSearch's dependency on default priority values

RESOLVED FIXED in Bugzilla 3.6

Status

()

Bugzilla
Query/Bug List
P4
enhancement
RESOLVED FIXED
13 years ago
7 years ago

People

(Reporter: Wurblzap, Assigned: Max Kanat-Alexander)

Tracking

2.21
Bugzilla 3.6
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

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

Comment 1

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

Updated

11 years ago
Priority: -- → P4
(Assignee)

Updated

9 years ago
Assignee: query-and-buglist → mkanat
Blocks: 488931
Status: NEW → ASSIGNED
Target Milestone: --- → Bugzilla 3.6
(Assignee)

Comment 2

9 years ago
Created attachment 373408 [details] [diff] [review]
v1

Pretty simple fix.
Attachment #373408 - Flags: review?(LpSolit)
(Reporter)

Comment 3

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

Comment 4

9 years ago
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).
Attachment #373408 - Flags: review?(LpSolit)

Comment 5

9 years ago
I'm not allowed to override Marc's review. So even if I granted review, Marc's r- would still stand.
(Reporter)

Comment 6

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

Updated

9 years ago
Attachment #373408 - Flags: review?(LpSolit)

Comment 7

9 years ago
Comment on attachment 373408 [details] [diff] [review]
v1

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

Comment 8

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

Comment 9

9 years ago
Created attachment 373547 [details] [diff] [review]
v2

Okay, this preserves the syntax. Tested on landfill.
Attachment #373408 - Attachment is obsolete: true
Attachment #373547 - Flags: review?(wurblzap)
(Reporter)

Comment 10

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

Comment 11

9 years ago
(?:) 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).
(Reporter)

Updated

9 years ago
Attachment #373547 - Flags: review?(wurblzap) → review-
(Reporter)

Comment 12

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

Comment 13

9 years ago
Created attachment 374979 [details] [diff] [review]
v3

Okay, fixed it.
Attachment #373547 - Attachment is obsolete: true
Attachment #374979 - Flags: review?(wurblzap)
(Assignee)

Comment 14

9 years ago
Created attachment 374980 [details] [diff] [review]
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.
Attachment #374979 - Attachment is obsolete: true
Attachment #374980 - Flags: review?(wurblzap)
Attachment #374979 - Flags: review?(wurblzap)
(Reporter)

Comment 15

9 years ago
Comment on attachment 374980 [details] [diff] [review]
v4

Yeah, this is good.

I don't know what I've seen with reversed limits.
Attachment #374980 - Flags: review?(wurblzap) → review+
(Assignee)

Updated

9 years ago
Flags: approval+
(Assignee)

Comment 16

9 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

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