Closed
Bug 179451
Opened 22 years ago
Closed 20 years ago
Move order-by generation from buglist.cgi into search.pm
Categories
(Bugzilla :: Query/Bug List, defect, P3)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.20
People
(Reporter: bugreport, Assigned: mkanat)
References
Details
Attachments
(1 file, 3 obsolete files)
7.56 KB,
patch
|
Details | Diff | Splinter Review |
Current approach to generation of order-by is a hack. See bug 179260.
buglist should pass its desired ordering to search.pm and have the query written
at once.
Assignee | ||
Comment 1•20 years ago
|
||
I very much want to do this, because it actually blocks sorting enumerations
correctly once we move those enumerations to tables. Feel free to take it back
if you are actually working on it.
Assignee: bbaetz → mkanat
Blocks: bz-enums
Assignee | ||
Comment 2•20 years ago
|
||
My proposal is this:
Search.pm should take an array of hashes, called @order. The array is an
in-order list of fields that should be sorted by ('field'), and an optional sort
order for each of them ('direction').
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•20 years ago
|
||
I've been working on this; here's the evidence. :-)
Right now this code seems to work very well. It's missing the ability to
correctly deal with the LEFT JOIN that bugs.target_milestone needs (and which
many other things will need, soon), so I'm not asking for review yet, but I'm
putting this patch up here because it works, and I thought it would be good to
track some progress and let other people see it if they want.
Assignee | ||
Comment 4•20 years ago
|
||
OK, this patch is ready. I've tested it, and it works. I think the comments
basically explain how things work and why they work that way.
Attachment #164512 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #164669 -
Flags: review?(myk)
Reporter | ||
Updated•20 years ago
|
Attachment #164669 -
Flags: review?(bugreport)
Assignee | ||
Comment 5•20 years ago
|
||
Comment on attachment 164669 [details] [diff] [review]
Version 2
Joel says that really only he and justdave touch this file, nowadays.
Attachment #164669 -
Flags: review?(myk)
Reporter | ||
Comment 6•20 years ago
|
||
Comment on attachment 164669 [details] [diff] [review]
Version 2
I haven't tested this yet, but....
In searchorder, having a field called "desc" that is 1 for descending and 0 for
ascending is rather confusing. I had to jump all over the place to understand
what this was doing.
There is a bunch of gymnastics to get orderlist recast from order scatterered
pretty far apart. Please do this all in one place.
There are a bunch of places where the code syling conventions are broken.
+ foreach my $orderitem (@orderlist) {
+ if($specialorderjoin{$orderitem->{'field'}}) {
+ push(@supptables, $specialorderjoin{$orderitem->{'field'}});
+ }
+ }
for example, no space after the if, and incorrect indentation.
I presume that having buglist.cgi generate the sort order list (and please do
call it a SORT order, not a SEARCH order) from a string is just for
compatibility until we can fix buglist, right?
It would be best if you kept the list of serach fields and their directions
seperate until final assembly into the SQL statement rather than performing
the strng concatenation within AddOrderByItemTo(). That will help when
DB-Compat folks go to add it to the GROUP BY statement.
Please do somthing about the subroutine names. I am not sure if
AddOrderByItemTo() is a name up with which I will put. ;-) How about
BuildItemOrder() ??
When we break up $db_order into strings, how sure are we that there will always
be exactly one space? Should we split on \s+ in the first place? That will
also eliminate the need to strip whitespace.
I'd also like to see someone run a whole bunch of tests on this (after these
changes) and report back. That can be done between the r+ and the a?
Attachment #164669 -
Flags: review?(bugreport) → review-
Assignee | ||
Comment 7•20 years ago
|
||
(In reply to comment #6)
> (From update of attachment 164669 [details] [diff] [review])
> In searchorder, having a field called "desc" that is 1 for descending and 0
> for ascending is rather confusing. I had to jump all over the place to
> understand what this was doing.
Yeah. I'll just get rid of that entirely, and just add ASC or DESC to the
field name itself; it's really easy to parse.
> There is a bunch of gymnastics to get orderlist recast from order scatterered
> pretty far apart. Please do this all in one place.
OK. I was following the current Search.pm convention, where the field is
initialized at the top, but then things aren't actually done until later. I can
move it all around so that it's in the same place.
> There are a bunch of places where the code syling conventions are broken.
Yeah, my apologies; this is my first real patch for upstream Bugzilla.
> I presume that having buglist.cgi generate the sort order list (and please do
> call it a SORT order, not a SEARCH order) from a string is just for
> compatibility until we can fix buglist, right?
Yes, *definitely*. Right now there's *so much* complexity in creating the
$db_order in buglist.cgi, that it's easier to parse it as-is, but then after I
write this I'm going to work on making buglist.cgi create the list in the new,
easier, more logical way.
> It would be best if you kept the list of serach fields and their directions
> seperate until final assembly into the SQL statement rather than performing
> the strng concatenation within AddOrderByItemTo(). That will help when
> DB-Compat folks go to add it to the GROUP BY statement.
Oh, good idea. I'll make it do that, then.
> Please do somthing about the subroutine names. I am not sure if
> AddOrderByItemTo() is a name up with which I will put. ;-) How about
> BuildItemOrder() ??
Yeah, that's much better. I actually didn't like the name either; I'll change
it. :-)
> When we break up $db_order into strings, how sure are we that there will
> always be exactly one space? Should we split on \s+ in the first place? That
> will also eliminate the need to strip whitespace.
Yes, we should just split on \s+, I think you're right.
> I'd also like to see someone run a whole bunch of tests on this (after these
> changes) and report back. That can be done between the r+ and the a?
OK. I've run some tests on it, and it works for normal sort from the buglist,
and for Target Milestone sorting, the way that things work, now. I'm pretty sure
that the recursion is correct for BuildItemOrder(), as I also tested that in a
few ways.
It would probably be good to get some more extensive tests done as well.
Thankfully, we don't change any really complex stuff--namely, we keep
buglist.cgi mostly as-is.
Assignee | ||
Comment 8•20 years ago
|
||
> It would be best if you kept the list of serach fields and their directions
> seperate until final assembly into the SQL statement rather than performing
> the strng concatenation within AddOrderByItemTo()
Oh, I just realized that this requires me to at some point separate the
fields... but if ONLY the GROUP BY folks (who will probably be *me*, actually)
will need to do that, I think it will be easier to just let them do the string
parsing inside whatever function they want to use. It's easy string parsing; a
few lines inside of a foreach that I now get to take out of buglist.cgi.
Assignee | ||
Comment 9•20 years ago
|
||
OK, here's a new version that takes into account all of the comments, above. I
think that overall this new version is much simpler, especially for the caller.
I also added a mechanism for something like "target_milestone DESC" to work
properly.
Attachment #164669 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #165504 -
Flags: review?(bugreport)
Reporter | ||
Comment 10•20 years ago
|
||
Comment on attachment 165504 [details] [diff] [review]
Version 3 (Simpler, Fixed)
Prior to checkin, I'd like to confirm that the use of "our" here is ok and also
understand the role of "$reverseorder" in this.
Incidentally, I'm not sure if I have any good ideas for how to handle the UI to
specify a descending sort on a certain column.
Attachment #165504 -
Flags: review?(bugreport) → review+
Comment 11•20 years ago
|
||
> Incidentally, I'm not sure if I have any good ideas for how to handle the UI to
> specify a descending sort on a certain column.
An idea would be this:
If a column in unsorted, by clicking it we should make it sorted ascending and
display a small arrow next to it. Clicking it again would make it sorted
descending, and the arrow should be mirrored so it points in the other way.
Reporter | ||
Comment 12•20 years ago
|
||
I like that idea, as long as we only do that for the last column the user
clicked. Otherwise, it makes it very tricky when you want to specify first one
order then another, then want to put the first one back.
For example, I sort by Priority, then owner by clicking on owner, then priority.
After I do this, I realize that I wanted bug nuber last, so I click Bug number,
owner, then priority. I would want all 3 of these to be Ascending.
So, if we want reverse priority, then owner, then bug number, I click on number,
owner, then priority, then priority again. That would work.
In any case, that is another bug.
Severity: normal → minor
Flags: approval?
Priority: -- → P3
Target Milestone: --- → Bugzilla 2.22
Assignee | ||
Comment 13•20 years ago
|
||
The role of $reverseorder is to handle things like sorting by "target_milestone
DESC".
Let's say that we had a field A that normally translates to a sort order of "B
ASC, C DESC". If we sort by "A DESC", what we really then mean is "B DESC, C
ASC". So $reverseorder is only used if we call BuildOrderBy recursively, to let
it know that we're "reversing" the order. That is, that we wanted "A DESC", not "A".
I also agree about the UI -- I think that's a good idea. Has anybody filed that
bug, yet?
Assignee | ||
Comment 14•20 years ago
|
||
OK, here's a version that's basically identical to the above patch, but it's
got a few more nice comments in there. Also, it actually applies to the tip.
Attachment #165504 -
Attachment is obsolete: true
Comment 15•20 years ago
|
||
Targeting bug to 2.20 since the 2.20 feature freeze was canceled.
Target Milestone: Bugzilla 2.22 → Bugzilla 2.20
Updated•20 years ago
|
Flags: approval? → approval+
Comment 16•20 years ago
|
||
Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v <-- buglist.cgi
new revision: 1.272; previous revision: 1.271
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v <-- Search.pm
new revision: 1.74; previous revision: 1.73
done
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•