Bug 24533 - Improved sorting in checkouts table
Summary: Improved sorting in checkouts table
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Martin Renvoize (ashimema)
QA Contact: Testopia
URL:
Keywords:
Depends on: 23493 25890
Blocks:
  Show dependency treegraph
 
Reported: 2020-01-29 16:22 UTC by Martin Renvoize (ashimema)
Modified: 2025-04-10 16:14 UTC (History)
13 users (show)

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


Attachments
Bug 24533: Switch to aDataSort for multi-column sorting (5.12 KB, patch)
2020-01-29 16:27 UTC, Martin Renvoize (ashimema)
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 (ashimema)
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 (ashimema)
Details | Diff | Splinter Review
Bug 24533: Switch to aDataSort for multi-column sorting (4.99 KB, patch)
2025-03-05 09:26 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 24533: Switch to aDataSort for multi-column sorting (5.14 KB, patch)
2025-04-01 13:36 UTC, Magnus Enger
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize (ashimema) 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 (ashimema) 2020-01-29 16:27:09 UTC Comment hidden (important, obsolete)
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 (ashimema) 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 Comment hidden (obsolete)
Comment 5 Martin Renvoize (ashimema) 2020-08-21 16:13:03 UTC Comment hidden (obsolete)
Comment 6 Martin Renvoize (ashimema) 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 (ashimema) 2020-11-05 14:51:59 UTC Comment hidden (obsolete)
Comment 8 Martin Renvoize (ashimema) 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
Comment 11 Koha collecto 2024-05-24 17:24:26 UTC
+1
Comment 12 Martin Renvoize (ashimema) 2025-03-05 09:26:42 UTC Comment hidden (obsolete)
Comment 13 Martin Renvoize (ashimema) 2025-03-05 09:27:46 UTC
Rebased
Comment 14 Martin Renvoize (ashimema) 2025-03-05 09:31:50 UTC
This is a bugfix for the functionality as it stands whilst dropping the row grouping I would say is more of a change request.  Can we work this one through and then if people want to remove functionality in a follow-up enhancement bug.
Comment 15 Magnus Enger 2025-04-01 13:36:29 UTC
Created attachment 180190 [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.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Works as advertised. When sorting on "Due date", the grouping by
"Previous/Today's checkouts" is disabled.
Comment 16 Lucas Gass (lukeg) 2025-04-02 09:00:06 UTC
With this patch applied something the sorting/offset seems to be off by 1 column. Due date works as described. Every other column seems to now sort on the column directly to its left, instead of the column clicked.