Bug 26233 - Edit item date sort still does not sort correctly
Summary: Edit item date sort still does not sort correctly
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: 20.05
Hardware: All All
: P5 - low normal
Assignee: Jonathan Druart
QA Contact: Nick Clemens (kidclamp)
URL:
Keywords:
Depends on: 25553
Blocks:
  Show dependency treegraph
 
Reported: 2020-08-18 08:53 UTC by Jonathan Druart
Modified: 2021-06-14 21:31 UTC (History)
8 users (show)

See Also:
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
Circulation function:


Attachments
Bug 26233: (bug 25553 follow-up) Make date columns sortable on the edit items table (1.08 KB, patch)
2020-08-18 08:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Add data-order attr to <td> that include dates (1.41 KB, patch)
2020-08-18 14:51 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 26233: (bug 25553 follow-up) Make date columns sortable on the edit items table (1.13 KB, patch)
2020-08-27 13:03 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 26233: (bug 25553 follow-up) Make date columns sortable on the edit items table (1.19 KB, patch)
2020-08-28 10:44 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 (lukeg) 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 (lukeg) 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 (lukeg) 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 (lukeg) 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 (lukeg) 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 (lukeg) 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 (lukeg) 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 (lukeg) 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 (tcohen) 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 (kidclamp) 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 (lukeg) 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