Bug 28349 - Date sorting incorrect in some tables
Summary: Date sorting incorrect in some tables
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: master
Hardware: All All
: P5 - low normal with 12 votes (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 26234
Blocks: 29218
  Show dependency treegraph
 
Reported: 2021-05-13 14:17 UTC by David Roberts
Modified: 2021-10-29 15:50 UTC (History)
7 users (show)

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


Attachments
Bug 28349: (bug 26234 follow-up) Correct wrong array merge (1.91 KB, patch)
2021-10-07 10:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28349: (bug 26234 follow-up) Correct wrong array merge (2.12 KB, patch)
2021-10-07 13:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28349: [DO NOT PUSH] Recreate the problem (2.30 KB, patch)
2021-10-13 12:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28349: (bug 26234 follow-up) Correct wrong array merge (2.18 KB, patch)
2021-10-28 08:24 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 David Roberts 2021-05-13 14:17:05 UTC
When using the en-GB translation, column sorting of saved reports (../cgi-bin/koha/reports/guided_reports.pl?phase=Use%20saved) doesn't work properly when sorting by date. For example, sorting by date created could result in the following order:

02/04/2019
02/04/2020
02/08/2018
03/04/2019

This is presumably because the UK date format is dd/mm/yyyy, not mm/dd/yyyy. It works correctly in the en translation. Sorting by non-date columns in en-GB also works correctly.
Comment 1 Fridolin Somers 2021-10-04 22:33:10 UTC
Looks like it is fixed by Bug 27994
Comment 2 Fridolin Somers 2021-10-04 23:27:34 UTC
I see it is Bug 26234 that removed :
  { "sType": "title-string", "aTargets" : [ "title-string" ] }
Comment 3 Jonathan Druart 2021-10-05 07:01:29 UTC
Hi David, that seems to work correctly for me on master, with en-GB and dateformat=dd/mm/yyyy

On which version of Koha do you see that problem? Can you recreate on a sandbox?
Comment 4 Jonathan Druart 2021-10-05 07:02:02 UTC
(In reply to Fridolin Somers from comment #2)
> I see it is Bug 26234 that removed :
>   { "sType": "title-string", "aTargets" : [ "title-string" ] }

Not related and not a bug, title-string has been moved to columns_settings.inc (not removed).
Comment 5 Fridolin Somers 2021-10-05 20:25:41 UTC
I reproduce this bug on our demo actually 20.11.09 :
https://intranet-demo.biblibre.com/
Comment 6 Jonathan Druart 2021-10-07 10:25:51 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2021-10-07 10:27:12 UTC
Problem and fix can be easily tested on v20.11.09 (sql reports), but this patch is for master and 21.05 as well.
Comment 8 Jonathan Druart 2021-10-07 13:07:48 UTC
Created attachment 125875 [details] [review]
Bug 28349: (bug 26234 follow-up) Correct wrong array merge

Bug 26234 allowed to remove the different classes we use in aoColumnDefs
for sorting columns of table using DT.

However there is a terrible mistake, the merge of existing aoColumnDefs
array with the default one is totally wrong:
  $.extend(true, new_parameters, default_column_defs);
When we actually wanted to do:
  $.extend(true, new_parameters["aoColumnDefs"], default_column_defs);

But it's still wrong, extend is doing a deep copy and the array will be
replaced by the other one, whereas we want to append.

We want to merge default_column_defs with the existing aocolumnDefs,
this patch is doing it explicitely.

This bug only exists when there is an existing aocolumnDefs.

See commit d3f3a55e0b1544ebcd3166d141e126a4324baab1
It happens on:
 * cataloguing/z3950_search.tt
 * reports/guided_reports_start.tt
 * serials/subscription-detail.tt
 * opac-course-reserves.tt
 * opac-detail.tt

However this bug has been hidden as we have in master related bug from
bug 27945, which removes title-string.
Comment 9 Jonathan Druart 2021-10-13 12:10:13 UTC
Created attachment 126179 [details] [review]
Bug 28349: [DO NOT PUSH] Recreate the problem

You can apply this patch to recreate the problem.
NoSort is removed from the default DT settings but we are passing it
from the KohaTable constructor. We are then expecting the first column
(the checkbox) to not be orderable.
Without the fix, the parameter is not taken into account (ie. not
correctly merged into the DT default parameters).
Comment 10 Martin Renvoize 2021-10-28 08:24:18 UTC
Created attachment 127028 [details] [review]
Bug 28349: (bug 26234 follow-up) Correct wrong array merge

Bug 26234 allowed to remove the different classes we use in aoColumnDefs
for sorting columns of table using DT.

However there is a terrible mistake, the merge of existing aoColumnDefs
array with the default one is totally wrong:
  $.extend(true, new_parameters, default_column_defs);
When we actually wanted to do:
  $.extend(true, new_parameters["aoColumnDefs"], default_column_defs);

But it's still wrong, extend is doing a deep copy and the array will be
replaced by the other one, whereas we want to append.

We want to merge default_column_defs with the existing aocolumnDefs,
this patch is doing it explicitely.

This bug only exists when there is an existing aocolumnDefs.

See commit d3f3a55e0b1544ebcd3166d141e126a4324baab1
It happens on:
 * cataloguing/z3950_search.tt
 * reports/guided_reports_start.tt
 * serials/subscription-detail.tt
 * opac-course-reserves.tt
 * opac-detail.tt

However this bug has been hidden as we have in master related bug from
bug 27945, which removes title-string.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2021-10-28 08:25:33 UTC
That was somewhat fun to reproduce and test.. I'm going to be bold and go for straight QA here.. it's a clear fix, with no regressions, once I wrapped my head around it.

Passing QA
Comment 12 Jonathan Druart 2021-10-28 10:29:47 UTC
Pushed to master for 21.11, thanks to everybody involved!