179451 - Move order-by generation from buglist.cgi into search.pm

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect, P3)

Description

[Joel Peshkin]

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.

Comment 1

[Max Kanat-Alexander]

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.

Comment 2

[Max Kanat-Alexander]

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').

Comment 3

[Max Kanat-Alexander]

Attachment: Version 1

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.

Comment 4

[Max Kanat-Alexander]

Attachment: Version 2

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.

Comment 5

[Max Kanat-Alexander]

Comment on attachment 164669 [details] [diff] [review]
Version 2

Joel says that really only he and justdave touch this file, nowadays.

Comment 6

[Joel Peshkin]

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?

Comment 7

[Max Kanat-Alexander]

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

Comment 8

[Max Kanat-Alexander]

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

Comment 9

[Max Kanat-Alexander]

Attachment: Version 3 (Simpler, Fixed)

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.

Comment 10

[Joel Peshkin]

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.

Comment 11

[Vlad Dascalu]

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

Comment 12

[Joel Peshkin]

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.

Comment 13

[Max Kanat-Alexander]

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?

Comment 14

[Max Kanat-Alexander]

Attachment: Version 3.1 (Un-bitrotted, more docs)

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.

Comment 15

[Vlad Dascalu]

Targeting bug to 2.20 since the 2.20 feature freeze was canceled.

Comment 16

[Shane H. W. Travis]

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