| Summary: | Improved sorting in checkouts table | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Martin Renvoize (ashimema) <martin.renvoize> |
| Component: | Circulation | Assignee: | Martin Renvoize (ashimema) <martin.renvoize> |
| Status: | Pushed to stable --- | QA Contact: | Jan Kissig <bibliothek> |
| Severity: | normal | ||
| Priority: | P5 - low | CC: | amit.gupta, amitddng135, andrew, bibliothek, blawlor, bolshawh, fridolin.somers, gmcharlt, kebliss, koha, kyle, lucas, marjorie.barry-vila, oleonard, patrick.robitaille |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| See Also: | https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24456 | ||
| GIT URL: | Initiative type: | --- | |
| Sponsorship status: | --- | Comma delimited list of Sponsors: | |
| Crowdfunding goal: | 0 | Patch complexity: | Trivial patch |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: |
This update fixes and improves how sorting works in the Checkouts table of the staff interface.
## What was wrong before
When checkouts were grouped into “Today’s checkouts” and “Previous checkouts,” sorting by a column (like title or branch) would break the grouping and show everything in one long list.
Some columns also sorted incorrectly because they were linked to the wrong data.
## What’s changed
Sorting now keeps the Today/Previous grouping, making the list easier to understand.
The Due date column is the exception — sorting by due date now gives you a clean list ordered by due date only, which is what most staff expect.
All columns now sort the correct data.
|
Version(s) released in: |
25.11.00,25.05.06
|
| Circulation function: | |||
| Bug Depends on: | 23493, 25890 | ||
| Bug Blocks: | |||
| Attachments: |
Bug 24533: Switch to aDataSort for multi-column sorting
sorting screenshot Bug 24533: Switch to aDataSort for multi-column sorting Bug 24533: Switch to aDataSort for multi-column sorting Bug 24533: Switch to aDataSort for multi-column sorting Bug 24533: Switch to aDataSort for multi-column sorting Bug 24533: Switch to aDataSort for multi-column sorting Bug 24533: (follow-up) Fix incorrect orderData column indexes Bug 24533: Switch to aDataSort for multi-column sorting Bug 24533: (follow-up) Fix incorrect orderData column indexes |
||
|
Description
Martin Renvoize (ashimema)
2020-01-29 16:22:53 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. 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 Created attachment 178953 [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 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. 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. 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. Created attachment 188872 [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. Created attachment 188873 [details] [review] Bug 24533: (follow-up) Fix incorrect orderData column indexes The previous commit added orderData configurations to maintain grouping when sorting, but the column indexes were off by one. Each orderData should reference its own column index as the secondary sort criterion, not the previous column's index. Corrected mappings: - Title (index 4): [1,3] -> [1,4] - Record Type (index 5): [1,4] -> [1,5] - Item Type (index 6): [1,5] -> [1,6] - Collection (index 7): [1,6] -> [1,7] - Location (index 8): [1,7] -> [1,8] - Home Branch (index 9): [1,8] -> [1,9] - Current Branch (index 12): [1,11] -> [1,12] - Call Number (index 13): [1,12] -> [1,13] - Copy Number (index 14): [1,13] -> [1,14] - Charge (index 15): [1,14] -> [1,15] - Fine (index 16): [1,15] -> [1,16] - Price (index 17): [1,16] -> [1,17] This ensures that when clicking a column header to sort, DataTables will: 1. Sort by column 1 (group header) first to maintain grouping 2. Sort by the clicked column (using correct index) as secondary sort Follow-up corrects the indexes again given Lucas's comment.. pretty sure an additional field got added some time and and hadn't been properly accounted for in the rebases. Created attachment 189629 [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. Signed-off-by: Jan Kissig <bibliothek@th-wildau.de> Created attachment 189630 [details] [review] Bug 24533: (follow-up) Fix incorrect orderData column indexes The previous commit added orderData configurations to maintain grouping when sorting, but the column indexes were off by one. Each orderData should reference its own column index as the secondary sort criterion, not the previous column's index. Corrected mappings: - Title (index 4): [1,3] -> [1,4] - Record Type (index 5): [1,4] -> [1,5] - Item Type (index 6): [1,5] -> [1,6] - Collection (index 7): [1,6] -> [1,7] - Location (index 8): [1,7] -> [1,8] - Home Branch (index 9): [1,8] -> [1,9] - Current Branch (index 12): [1,11] -> [1,12] - Call Number (index 13): [1,12] -> [1,13] - Copy Number (index 14): [1,13] -> [1,14] - Charge (index 15): [1,14] -> [1,15] - Fine (index 16): [1,15] -> [1,16] - Price (index 17): [1,16] -> [1,17] This ensures that when clicking a column header to sort, DataTables will: 1. Sort by column 1 (group header) first to maintain grouping 2. Sort by the clicked column (using correct index) as secondary sort Signed-off-by: Jan Kissig <bibliothek@th-wildau.de> (In reply to Martin Renvoize (ashimema) from comment #14) > 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. Though I am not totally happy how grouping and "not grouping" are handled here (I would prefer marking a "today's checkouts" row f.e. by an icon or class and always sort them together with the other items), the patch works as described. Nice work everyone! Pushed to main for 25.11 please add release notes Nice work everyone! Pushed to 25.05.x I prefer not backport to 24.11.x LTS for UI stability |