The sorting in the checkouts table regressed with bug 23493. Before the patch: * Initial table load would group the results by "Today's checkouts" and "Previous checkouts". * Any subsequent column sort would disable the grouping entirely. After the patch: * Initial table load groups the results by "Today's checkouts" and "Previous checkouts". * Any subsequent column sort will sort by that column, but leave the group titles intact. As such, sorting by title for example can end up with multiple "Today's checkouts" header rows which is confusing. I propose that for all column sorts, with the exception of due date, we perform a primary sort on the grouping field followed by a secondary sort on the column in question. This way we maintain the grouping and just sort within the groups. For the due date column sort I propose sorting directly on due date and removing the grouping header rows entirely.
Created attachment 98088 [details] [review] Bug 24533: Switch to aDataSort for multi-column sorting This patch adds aDataSort configurations for each sortable column such that we always maintain the group sorting primarily and apply a secondary sort using the column in question. When sorting by due date you are much more likely to want the true date sorting without forcing a primary sort on the grouping column. We achieve that by removing the primary sort on checkout date and disable rowGrouping in such a case.
Sorry but with this patch applied the sorting is completely broken!
Could you elaborate a bit for me Owen.. broken in what way.. for me sorting is working on all fields with this patch.
Created attachment 99765 [details] sorting screenshot This did get a little weird on me. I checked out four things and then updated issuedate and date_due for some of them, ending up with these values: select issue_id, issuedate, date_due from issues order by issuedate; +----------+---------------------+---------------------+ | issue_id | issuedate | date_due | +----------+---------------------+---------------------+ | 4 | 2020-02-19 14:17:00 | 2020-03-05 23:59:00 | | 1 | 2020-02-21 14:17:00 | 2020-03-04 23:59:00 | | 2 | 2020-02-28 14:17:44 | 2020-03-24 23:59:00 | | 3 | 2020-02-28 14:17:50 | 2020-04-29 23:59:00 | +----------+---------------------+---------------------+ Sorting looked ok to me except for when I tried to sort by Checked Out On (see attached screenshot). It looks like it might just be sorting on issue_id rather than date? +----------+---------------------+---------------------+ | issue_id | issuedate | date_due | +----------+---------------------+---------------------+ | 1 | 2020-02-21 14:17:00 | 2020-03-04 23:59:00 | | 2 | 2020-02-28 14:17:44 | 2020-03-24 23:59:00 | | 3 | 2020-02-28 14:17:50 | 2020-04-29 23:59:00 | | 4 | 2020-02-19 14:17:00 | 2020-03-05 23:59:00 | +----------+---------------------+---------------------+ I suppose in the real world sort by issue_id should be a reliable way to sort on checkout order, but it does open up some possible confusion if libraries are out there updating things from the database.
Created attachment 108836 [details] [review] Bug 24533: Switch to aDataSort for multi-column sorting This patch adds aDataSort configurations for each sortable column such that we always maintain the group sorting primarily and apply a secondary sort using the column in question. When sorting by due date you are much more likely to want the true date sorting without forcing a primary sort on the grouping column. We achieve that by removing the primary sort on checkout date and disable rowGrouping in such a case.
Rebased and was about to dig into the errors you reported Andrew when I realised that I think they're already fixed by the bug that conflicted with this one causing the rebase requirement: bug 25890 Could you test again and let me know if you find any further issues.. all seems well to me now.
Created attachment 113120 [details] [review] Bug 24533: Switch to aDataSort for multi-column sorting This patch adds aDataSort configurations for each sortable column such that we always maintain the group sorting primarily and apply a secondary sort using the column in question. When sorting by due date you are much more likely to want the true date sorting without forcing a primary sort on the grouping column. We achieve that by removing the primary sort on checkout date and disable rowGrouping in such a case.
Rebased.
The patch works as expected as far as I can tell, but I preferred the original behaviour prior to bug 23493: "* Initial table load would group the results by "Today's checkouts" and "Previous checkouts". * Any subsequent column sort would disable the grouping entirely." I like the way the "Today's checkouts" and "Previous checkouts" groupings are removed when sorting by date and would prefer the table to do the same when sorting by Title, branch etc. I can see that library staff might want to see the "Today's checkouts" and "Previous checkouts" groupings. However, if people wanted to keep the today/previous grouping, I think it would make most sense to keep this grouping when sorting by date and remove it for all other columns i.e. the opposite of what this patch does.
(In reply to Henry Bolshaw from comment #9) > The patch works as expected as far as I can tell, but I preferred the > original behaviour prior to bug 23493: > > "* Initial table load would group the results by "Today's checkouts" and > "Previous checkouts". > * Any subsequent column sort would disable the grouping entirely." > > I like the way the "Today's checkouts" and "Previous checkouts" groupings > are removed when sorting by date and would prefer the table to do the same > when sorting by Title, branch etc. > > I can see that library staff might want to see the "Today's checkouts" and > "Previous checkouts" groupings. However, if people wanted to keep the > today/previous grouping, I think it would make most sense to keep this > grouping when sorting by date and remove it for all other columns i.e. the > opposite of what this patch does. +1
+1