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)

2.17.1
All
Other

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: bugreport, Assigned: mkanat)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
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
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
Attached patch Version 1 (obsolete) — Splinter Review
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.
Attached patch Version 2 (obsolete) — Splinter Review
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
Attachment #164669 - Flags: review?(myk)
Attachment #164669 - Flags: review?(bugreport)
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)
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-
(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.
> 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.
Attached patch Version 3 (Simpler, Fixed) (obsolete) — Splinter Review
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
Attachment #165504 - Flags: review?(bugreport)
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+
> 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.
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
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?
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
Targeting bug to 2.20 since the 2.20 feature freeze was canceled.
Target Milestone: Bugzilla 2.22 → Bugzilla 2.20
Flags: approval? → approval+
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
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: