© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
73416ba
(Get the code!)
I have just a couple comments that I think would help improve the code.
First, I strongly recommend that the sort_order column be made NULLABLE and not get a default. There are at least 2 good reasons for this, one is practical future-proofing, and one is conceptual:
1) with a NULLABLE column, we can later refine the underlying query such that manually-ordered rows are displayed either first or last, separately from the sort order, by using the NULLS FIRST/LAST modifier to the ORDER BY component. This is important because there are two classes of sorted things -- manually sorted and automatically sorted -- and they should be segregated based on that fact, not flipped first/last based on ASC/DESC sort.
2) NULL better models the intent of the new feature -- we are saying "humans are not overriding the natural order, so the human-supplied order is undefined". That is quite different, conceptually, from "humans are not overriding the natural order, so the natural order assumes this row comes first".
Second, Ben identified the exact the spot in the code where copy ordering is applied for the TPAC (and serials display in the SC -- see the code comment above the sub containing the commit-linked code). As it turns out, bmp is already available in the FROM clause of that query, so adding the same order_by elements seen here* in between the call number label sort key and the copy number element should do just what we want. That is, sort first aou.name, then on call number, then human-part-order, then natural part order, then copy number, then barcode.
Jason's cleanup of the upgrade script is also important, thanks for pointing that out.
Overall, this patch has good code economy and does the right things in the right places. Solid work, Dan.
* http://