Bug 26233

Summary: Edit item date sort still does not sort correctly
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: CatalogingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Nick Clemens <nick>
Severity: normal    
Priority: P5 - low CC: aleisha, andrewfh, ivan.dziuba, lucas, m.de.rooy, oleonard, sudrland, victor
Version: 20.05   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26234
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.04, 19.11.10, 19.05.15
Bug Depends on: 25553    
Bug Blocks:    
Attachments: Bug 26233: (bug 25553 follow-up) Make date columns sortable on the edit items table
Add data-order attr to <td> that include dates
Bug 26233: (bug 25553 follow-up) Make date columns sortable on the edit items table
Bug 26233: (bug 25553 follow-up) Make date columns sortable on the edit items table

Description Jonathan Druart 2020-08-18 08:53:48 UTC
Follow-up report for 
Bug 25553 - Edit item date sort does not sort correctly
Comment 1 Jonathan Druart 2020-08-18 08:56:17 UTC
Created attachment 108437 [details] [review]
Bug 26233: (bug 25553 follow-up) Make date columns sortable on the edit items table

We were missing to pass the title-string to datatables.

Test plan:
Retest bug 26233, with appropriate dates
Comment 2 Lucas Gass 2020-08-18 14:41:20 UTC
Jonathan,

I applied this patch and the itemst still doesn't sort dates correctly for me on additem.tt
Comment 3 Lucas Gass 2020-08-18 14:51:55 UTC
Created attachment 108522 [details] [review]
Add data-order attr to <td> that include dates

This patch adds the special dataTable attr 'data-order' in order to correctly sort the dates
To test:
Retest Bug 23233, with appropriate dates
Comment 4 Lucas Gass 2020-08-18 14:55:24 UTC
This data-order patch works with or without Jonathan's patch so I left his alone.
Comment 5 Jonathan Druart 2020-08-18 15:01:07 UTC
I will retest but I am pretty sure it works when I tested it this morning.

I don't think we should use data-order, but stick to the way we usually order dates (title-string) instead.
Comment 6 Lucas Gass 2020-08-18 15:12:52 UTC
Jonathan,

I am testing items with these dates and the sorting of dateaccessioned still seems wrong:

MariaDB [koha_kohadev]> select itemnumber, dateaccessioned from items where biblionumber = '4';
+------------+-----------------+
| itemnumber | dateaccessioned |
+------------+-----------------+
|         12 | 2015-01-28      |
|         13 | 2015-04-08      |
|         14 | 2014-05-08      |
|         18 | 2015-11-28      |
+------------+-----------------+


Thanks!
Comment 7 Katrin Fischer 2020-08-18 18:32:34 UTC
I think we've already used data-order in at least one spot.
Comment 8 Owen Leonard 2020-08-18 20:05:00 UTC
data-order wasn't available in DataTables when we first started using the plugin. I think it is preferable to use data-order going forward because it's not dependent on our own addon.
Comment 9 Jonathan Druart 2020-08-19 06:05:09 UTC
(In reply to Lucas Gass from comment #6)
> Jonathan,
> 
> I am testing items with these dates and the sorting of dateaccessioned still
> seems wrong:
> 
> MariaDB [koha_kohadev]> select itemnumber, dateaccessioned from items where
> biblionumber = '4';
> +------------+-----------------+
> | itemnumber | dateaccessioned |
> +------------+-----------------+
> |         12 | 2015-01-28      |
> |         13 | 2015-04-08      |
> |         14 | 2014-05-08      |
> |         18 | 2015-11-28      |
> +------------+-----------------+


Looks ok for me: https://snipboard.io/2MTgn6.jpg
Comment 10 Jonathan Druart 2020-08-19 06:07:41 UTC
(In reply to Owen Leonard from comment #8)
> data-order wasn't available in DataTables when we first started using the
> plugin. I think it is preferable to use data-order going forward because
> it's not dependent on our own addon.

The "span title" needs to be removed in that case :)
If we go with data-order I think we should do a big move and have only 1 way of doing things. I am willing to help anybody who would like to work on that.
Comment 11 Ivan Dziuba 2020-08-19 15:33:12 UTC
I don't install this patch, but I have already patch for bug 25553 and it works 
+------------+-----------------+
| itemnumber | dateaccessioned |
+------------+-----------------+
|      42365 | 2013-12-30      |
|      42366 | 2015-06-11      |
|      84707 | 2019-03-14      |
+------------+-----------------+

https://prnt.sc/u2b7od
Comment 12 Jonathan Druart 2020-08-19 16:06:08 UTC
Hi Ivan, please try with the 4 dates from comment 6 and with the system preferences dateformat set to "us"
Comment 13 Ivan Dziuba 2020-08-19 17:44:41 UTC
(In reply to Jonathan Druart from comment #12)
> Hi Ivan, please try with the 4 dates from comment 6 and with the system
> preferences dateformat set to "us"

Hi Jonathan, sorry I don't have biblionumber = '4', but I have more than 4 dates for biblionumber = '66343' and it works nice...

T-SQL:
select itemnumber, dateaccessioned from items where biblionumber = '66343';

+------------+-----------------+
| itemnumber | dateaccessioned |
+------------+-----------------+
|      75554 | 2014-11-25      |
|      75555 | 2014-12-17      |
|      75556 | 2014-12-17      |
|      75557 | 2015-01-28      |
|      75558 | 2015-02-21      |
|      75559 | 2015-03-17      |
|      75560 | 2015-04-14      |
|      75561 | 2015-05-21      |
|      75562 | 2015-06-11      |
|      75563 | 2015-08-28      |
|      75564 | 2015-10-13      |
|      75565 | 2015-11-19      |
|      75566 | 2016-01-06      |
|      75567 | 2016-02-11      |
|      75568 | 2016-03-15      |
|      75569 | 2016-04-13      |
|      75570 | 2016-05-07      |
|      75571 | 2016-06-15      |
|      75572 | 2016-08-19      |
|      75573 | 2016-09-21      |
|      75574 | 2016-11-23      |
+------------+-----------------+

https://prnt.sc/u2dabp

Thanks!
Comment 14 Lucas Gass 2020-08-19 20:24:23 UTC
Jonathan,

Because you pushed 26234 to master this patch no longer cleanly applies.
Comment 15 Michael Sutherland 2020-08-19 21:15:42 UTC
You can easily run a report and get the same results. I have hundred of holdings and can show that it doesn't work properly. Let's take Der Spiegel for example. I have 630 holdings in my test server running 19.11.08.  

https://vtlib-eds-files.s3.amazonaws.com/Screen+Shot+2020-08-19+at+5.13.10+PM.png
Comment 16 Jonathan Druart 2020-08-20 07:44:52 UTC
(In reply to Lucas Gass from comment #14)
> Jonathan,
> 
> Because you pushed 26234 to master this patch no longer cleanly applies.

Ho right, I understand now!

So this patch is no longer needed in master, that's certainly why it was working for Ivan!
Comment 17 Lucas Gass 2020-08-20 14:13:26 UTC
Yes, this issue is now fixed (in my testing) because of Bug 26234, marking this one resolved. Thanks everyone!
Comment 18 Lucas Gass 2020-08-20 14:41:55 UTC
> The "span title" needs to be removed in that case :)
> If we go with data-order I think we should do a big move and have only 1 way
> of doing things. I am willing to help anybody who would like to work on that.

If we would like to go the data-order route moving forward I would be happy to work on that.
Comment 19 Jonathan Druart 2020-08-20 14:56:14 UTC
It needs to be backported Lucas.
Comment 20 Lucas Gass 2020-08-25 15:45:09 UTC
I've applied this patch to 20.05.x and these dates still do not sort correctly for me. I used the same sample data that I mentioned in Comment 6.

I go to /cgi-bin/koha/cataloguing/additem.pl

And my sorting looks like this:

https://snipboard.io/MX5B2c.jpg
https://snipboard.io/0AHUva.jpg
Comment 21 Jonathan Druart 2020-08-26 08:14:02 UTC
Lucas, it looks of for me:

07a4da6ae5 (HEAD) Bug 26233: (bug 25553 follow-up) Make date columns sortable on the edit items table
fdc76815ba (origin/20.05.x) Bug 25348: (QA follow-up) Silence irrelevant warnings

% restart_all

/cgi-bin/koha/cataloguing/additem.pl?biblionumber=4
Edit the 4 items.
Before sorting: https://snipboard.io/9a1Q64.jpg
After: https://snipboard.io/UzcnIG.jpg

I confirm it's broken *without* this patch.
Comment 22 Andrew Fuerste-Henry 2020-08-27 12:59:09 UTC
Without the patch, my items do not sort correctly by dateacquired or datelastseen on 20.05.x: https://snipboard.io/ONZvwk.jpg

With the patch, they do sort correctly: https://snipboard.io/OJ4dpu.jpg
Comment 23 Tomás Cohen Arazi 2020-08-27 13:03:55 UTC
Created attachment 109224 [details] [review]
Bug 26233: (bug 25553 follow-up) Make date columns sortable on the edit items table

We were missing to pass the title-string to datatables.

Test plan:
Retest bug 26233, with appropriate dates

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Nick Clemens 2020-08-28 10:44:56 UTC
Created attachment 109278 [details] [review]
Bug 26233: (bug 25553 follow-up) Make date columns sortable on the edit items table

We were missing to pass the title-string to datatables.

Test plan:
Retest bug 26233, with appropriate dates

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 25 Katrin Fischer 2020-08-29 11:02:49 UTC
Nick, can you please finish here once you got the feedback from Lucas?
Comment 26 Jonathan Druart 2020-08-31 08:49:50 UTC
Sending to 20.05 queue.
Comment 27 Lucas Gass 2020-09-04 15:50:28 UTC
pushed to 20.05.x for 20.05.04
Comment 28 Aleisha Amohia 2020-09-10 03:47:52 UTC
backported to 19.11.x for 19.11.10
Comment 29 Victor Grousset/tuxayo 2020-09-20 18:15:00 UTC
Backported to 19.05.x branch for 19.05.15