Bug 28349

Summary: Date sorting incorrect in some tables
Product: Koha Reporter: David Roberts <david.roberts>
Component: ReportsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, hc, janet.mcgowan, jonathan.druart, kyle, lucy.vaux-harvey, martin.renvoize
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27994
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00
Bug Depends on: 26234    
Bug Blocks: 29218    
Attachments: Bug 28349: (bug 26234 follow-up) Correct wrong array merge
Bug 28349: (bug 26234 follow-up) Correct wrong array merge
Bug 28349: [DO NOT PUSH] Recreate the problem
Bug 28349: (bug 26234 follow-up) Correct wrong array merge

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!