Bug 24533 - Improved sorting in checkouts table
Summary: Improved sorting in checkouts table
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on: 23493 25890
Blocks:
  Show dependency treegraph
 
Reported: 2020-01-29 16:22 UTC by Martin Renvoize
Modified: 2021-09-14 17:46 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 24533: Switch to aDataSort for multi-column sorting (5.12 KB, patch)
2020-01-29 16:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
sorting screenshot (106.84 KB, image/png)
2020-02-28 14:42 UTC, Andrew Fuerste-Henry
Details
Bug 24533: Switch to aDataSort for multi-column sorting (5.17 KB, patch)
2020-08-21 16:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24533: Switch to aDataSort for multi-column sorting (5.27 KB, patch)
2020-11-05 14:51 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2020-01-29 16:22:53 UTC
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.
Comment 1 Martin Renvoize 2020-01-29 16:27:09 UTC
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.
Comment 2 Owen Leonard 2020-02-14 16:53:15 UTC
Sorry but with this patch applied the sorting is completely broken!
Comment 3 Martin Renvoize 2020-02-24 11:32:49 UTC
Could you elaborate a bit for me Owen.. broken in what way.. for me sorting is working on all fields with this patch.
Comment 4 Andrew Fuerste-Henry 2020-02-28 14:42:15 UTC
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.
Comment 5 Martin Renvoize 2020-08-21 16:13:03 UTC
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.
Comment 6 Martin Renvoize 2020-08-21 16:14:17 UTC
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.
Comment 7 Martin Renvoize 2020-11-05 14:51:59 UTC
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.
Comment 8 Martin Renvoize 2020-11-05 14:52:11 UTC
Rebased.
Comment 9 Henry Bolshaw 2021-03-17 15:48:57 UTC
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.
Comment 10 Marjorie Barry-Vila 2021-09-14 15:06:24 UTC
(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