Bug 28171

Summary: Serial enumeration / chronology sorting is broken in biblio page
Product: Koha Reporter: Joonas Kylmälä <joonas.kylmala>
Component: CatalogingAssignee: Joonas Kylmälä <joonas.kylmala>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: christian.nelson, fridolin.somers, jonathan.druart+koha, kyle, lucas, m.de.rooy, nugged, oleonard
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.01
Bug Depends on: 27980, 13420    
Bug Blocks:    
Attachments: Bug 28171: Serial enumeration / chronology sorting is broken in biblio page
Bug 28171: Serial enumeration / chronology sorting is broken in biblio page
Bug 28171: Revert "Bug 27980: Remove itemdata_publisheddate from controller"
Bug 28171: Restore previous sorting behaviour for Serial enumchron column
Bug 28171: Revert "Bug 27980: Remove itemdata_publisheddate from controller"
Bug 28171: Restore previous sorting behaviour for Serial enumchron column
Bug 28171: Revert "Bug 27980: Remove itemdata_publisheddate from controller"
Bug 28171: Restore previous sorting behaviour for Serial enumchron column
Bug 28171: Revert "Bug 27980: Remove itemdata_publisheddate from controller"
Bug 28171: Restore previous sorting behaviour for Serial enumchron column
Bug 28171: Revert "Bug 27980: Remove itemdata_publisheddate from controller"
Bug 28171: Restore previous sorting behaviour for Serial enumchron column
Bug 28171: Restore comment to remove ambiguity

Description Joonas Kylmälä 2021-04-19 14:09:58 UTC
In catalogue/detail.pl page the "Serial enumeration / chronology" column sorting is broken after the changes in bug 27980. To reproduce problem add 2 items, one with "A" and one with "B" value in "h - Serial Enumeration / chronology item" field. Then go to catalogue/detail.pl and try to sort by clicking on the "Serial enumeration / chronology" column.
Comment 1 Owen Leonard 2021-04-19 18:43:15 UTC
Created attachment 119869 [details] [review]
Bug 28171: Serial enumeration / chronology sorting is broken in biblio page

This patch updates the bibliographic detail page so that the "Serial
enumeration/chronology" column is sorted by published date only if a
published date is present.

To test, apply the patch and locate a bibliograhpic record for a
subscription that has multiple items.

On the bibliographic detail page, confirm that the "Serial
enumeration/chronology" column sorts correctly.
Comment 2 Peter Vashchuk 2021-04-20 09:23:29 UTC
Created attachment 119896 [details] [review]
Bug 28171: Serial enumeration / chronology sorting is broken in biblio page

This patch updates the bibliographic detail page so that the "Serial
enumeration/chronology" column is sorted by published date only if a
published date is present.

To test, apply the patch and locate a bibliograhpic record for a
subscription that has multiple items.

On the bibliographic detail page, confirm that the "Serial
enumeration/chronology" column sorts correctly.

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Comment 3 Joonas Kylmälä 2021-04-20 13:53:48 UTC
Hmm, the proposed solution here is not like it was before bug 27980. Before we sorted either with publisheddate alone if one of the items had it at least, but now with this we would sort both of the values. This seems okay because how this seems to work now is that the ones with the data-order attribute set (so publisheddate) are sorted as one group and then the ones without data-order are sorted as another group. So both get sorted, but separately. Owen, do you think this change in sorting order is okay? Previously the ones without publisheddate didn't get sorted so this might actually be improvement but want to hear your opinion as well.

Also I didn't find any documentation for this in DataTables manual so we might also want to consider if we should depend on this and whether the sorting order will possibly change in future if DataTables changes some code because it seems undocumented feature.
Comment 4 Owen Leonard 2021-04-20 14:21:09 UTC
I'm glad to have your detailed feedback, because I don't have good serials data to test with and I wasn't sure I was mocking it up correctly. I don't know the answer to whether the grouped sorting is better, so I defer to your opinion.

What feature do you mean when you say it isn't documented? The data-order sorting option is documented here: https://datatables.net/examples/advanced_init/html5-data-attributes.html
Comment 5 Joonas Kylmälä 2021-04-21 14:16:37 UTC
(In reply to Owen Leonard from comment #4)
> I'm glad to have your detailed feedback, because I don't have good serials
> data to test with and I wasn't sure I was mocking it up correctly. I don't
> know the answer to whether the grouped sorting is better, so I defer to your
> opinion.
> 
> What feature do you mean when you say it isn't documented? The data-order
> sorting option is documented here:
> https://datatables.net/examples/advanced_init/html5-data-attributes.html

The feature of Datatables sorting separately the rows with order-by attribute and the ones without and then concatenating them. There is no mention of that in the documentation so it probably could have been implemented so that it would sort everything as one group.

I think the undocumented sorting functionality here is good but only doubt is that if it breaks in the future and our users will expect us to fix that it might not be possible.
Comment 6 Joonas Kylmälä 2021-04-21 14:20:06 UTC
In the previous message with "order-by" I wanted to say instead "data-order"
Comment 7 Joonas Kylmälä 2021-04-21 15:19:02 UTC
From the DataTables website I found this:

"In order for the HTML 5 data-* attribute detection and processing to work correctly, all cells in a column must have the same attribute available. Without this, DataTables will give a warning."

(https://datatables.net/manual/data/orthogonal-data)

So unless I'm misunderstanding we probably shouldn't even depend on this, but go back to the old behaviour where we had all the columns having the data-order value set even though for some it was empty.

However, I didn't get this warning message when testing.
Comment 8 Joonas Kylmälä 2021-04-21 15:21:24 UTC
Owen, so my recommendation would be that in this bug we restore the old behaviour before bug 27980 and if somebody wants to modify that let's open a new bug for this. The "Bug 27980: Remove itemdata_publisheddate from controller" needs to be reverted to restore the old behaviour.
Comment 9 Joonas Kylmälä 2021-04-21 16:23:31 UTC
Created attachment 119960 [details] [review]
Bug 28171: Revert "Bug 27980: Remove itemdata_publisheddate from controller"

This reverts commit c7fef23763754ad65c5a59438ed370d53aeec4b6.

The code is needed back to fix broken sorting.
Comment 10 Joonas Kylmälä 2021-04-21 16:23:36 UTC
Created attachment 119961 [details] [review]
Bug 28171: Restore previous sorting behaviour for Serial enumchron column

Before Bug 27980 the sorting was done either by item's publisheddate
if it existed in at least one item. If it didn't exist then the
sorting was done using "h - Serial Enumeration / chronology"
field. This restores this sorting functionality in catalogue/detail.pl
page.

To test:
 1. Create a serial subscription for a biblio, make sure to enable the
    option "Create an item record when receiving this serial"
 2. Receive two serials, so that they have different received dates
    and check that the items are now created to the biblio
 3. Add now two additional items with one having "A" and other having
    "B" in the "h - Serial Enumeration / chronology" field.
 4. Try sorting and notice the items with publisheddate are being
    sorted and ones without stay in the order they happened to be
 5. Remove the items with publisheddate and notice that now items with
    "A" and "B" get sorted correctly
Comment 11 Joonas Kylmälä 2021-04-21 16:28:36 UTC
Attached solution that should restore exactly the previous behaviour. Let me know what you think.
Comment 12 Lucas Gass 2021-05-11 16:49:00 UTC
Hi Joonas,

Your latest patch fixes the issue, so I am wondering if it is still necessary to revert c7fef23763754ad65c5a59438ed370d53aeec4b6 ?
Comment 13 Joonas Kylmälä 2021-05-12 08:33:18 UTC
(In reply to Lucas Gass from comment #12)
> Hi Joonas,
> 
> Your latest patch fixes the issue, so I am wondering if it is still
> necessary to revert c7fef23763754ad65c5a59438ed370d53aeec4b6 ?

It is necessary, my code uses the code that c7fef23763754ad65c5a59438ed370d53aeec4b6 removed so it needs to be restored by reverting the commit.
Comment 14 PTFS Europe Sandboxes 2021-05-20 06:55:20 UTC
Created attachment 121210 [details] [review]
Bug 28171: Revert "Bug 27980: Remove itemdata_publisheddate from controller"

This reverts commit c7fef23763754ad65c5a59438ed370d53aeec4b6.

The code is needed back to fix broken sorting.

Signed-off-by: cnelson <christian.nelson@tritonia.fi>
Comment 15 PTFS Europe Sandboxes 2021-05-20 06:55:24 UTC
Created attachment 121211 [details] [review]
Bug 28171: Restore previous sorting behaviour for Serial enumchron column

Before Bug 27980 the sorting was done either by item's publisheddate
if it existed in at least one item. If it didn't exist then the
sorting was done using "h - Serial Enumeration / chronology"
field. This restores this sorting functionality in catalogue/detail.pl
page.

To test:
 1. Create a serial subscription for a biblio, make sure to enable the
    option "Create an item record when receiving this serial"
 2. Receive two serials, so that they have different received dates
    and check that the items are now created to the biblio
 3. Add now two additional items with one having "A" and other having
    "B" in the "h - Serial Enumeration / chronology" field.
 4. Try sorting and notice the items with publisheddate are being
    sorted and ones without stay in the order they happened to be
 5. Remove the items with publisheddate and notice that now items with
    "A" and "B" get sorted correctly

Signed-off-by: cnelson <christian.nelson@tritonia.fi>
Comment 16 PTFS Europe Sandboxes 2021-05-20 07:04:11 UTC Comment hidden (obsolete)
Comment 17 PTFS Europe Sandboxes 2021-05-20 07:04:14 UTC Comment hidden (obsolete)
Comment 18 Joonas Kylmälä 2021-05-20 07:09:54 UTC
Created attachment 121214 [details] [review]
Bug 28171: Revert "Bug 27980: Remove itemdata_publisheddate from controller"

This reverts commit c7fef23763754ad65c5a59438ed370d53aeec4b6.

The code is needed back to fix broken sorting.

Signed-off-by: Christian Nelson <christian.nelson@tritonia.fi>
Comment 19 Joonas Kylmälä 2021-05-20 07:09:57 UTC
Created attachment 121215 [details] [review]
Bug 28171: Restore previous sorting behaviour for Serial enumchron column

Before Bug 27980 the sorting was done either by item's publisheddate
if it existed in at least one item. If it didn't exist then the
sorting was done using "h - Serial Enumeration / chronology"
field. This restores this sorting functionality in catalogue/detail.pl
page.

To test:
 1. Create a serial subscription for a biblio, make sure to enable the
    option "Create an item record when receiving this serial"
 2. Receive two serials, so that they have different received dates
    and check that the items are now created to the biblio
 3. Add now two additional items with one having "A" and other having
    "B" in the "h - Serial Enumeration / chronology" field.
 4. Try sorting and notice the items with publisheddate are being
    sorted and ones without stay in the order they happened to be
 5. Remove the items with publisheddate and notice that now items with
    "A" and "B" get sorted correctly

Signed-off-by: Christian Nelson <christian.nelson@tritonia.fi>
Comment 20 Joonas Kylmälä 2021-05-20 07:12:44 UTC
As Christian was signing-off the patches via Koha Sandbox the signed-off line didn't update to the correct name. Fixed it now on behalf of Christian in the latest patches attached.

Ready for QA.
Comment 21 Kyle M Hall 2021-05-28 13:53:24 UTC
Created attachment 121483 [details] [review]
Bug 28171: Revert "Bug 27980: Remove itemdata_publisheddate from controller"

This reverts commit c7fef23763754ad65c5a59438ed370d53aeec4b6.

The code is needed back to fix broken sorting.

Signed-off-by: Christian Nelson <christian.nelson@tritonia.fi>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Kyle M Hall 2021-05-28 13:54:04 UTC
Created attachment 121484 [details] [review]
Bug 28171: Restore previous sorting behaviour for Serial enumchron column

Before Bug 27980 the sorting was done either by item's publisheddate
if it existed in at least one item. If it didn't exist then the
sorting was done using "h - Serial Enumeration / chronology"
field. This restores this sorting functionality in catalogue/detail.pl
page.

To test:
 1. Create a serial subscription for a biblio, make sure to enable the
    option "Create an item record when receiving this serial"
 2. Receive two serials, so that they have different received dates
    and check that the items are now created to the biblio
 3. Add now two additional items with one having "A" and other having
    "B" in the "h - Serial Enumeration / chronology" field.
 4. Try sorting and notice the items with publisheddate are being
    sorted and ones without stay in the order they happened to be
 5. Remove the items with publisheddate and notice that now items with
    "A" and "B" get sorted correctly

Signed-off-by: Christian Nelson <christian.nelson@tritonia.fi>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Jonathan Druart 2021-06-03 13:32:05 UTC
(In reply to Joonas Kylmälä from comment #11)
> Attached solution that should restore exactly the previous behaviour. Let me
> know what you think.

Regarding bug 13420 it seems that we need to restore the previous behaviour, and not pick Owen's patch (what I was thinking first).


commit 0ecf7694eac802f29ea4786502b7bffaaad3a6bf
Bug 13420: Fallback to the previous behavior if published date is not used

                [% IF volinfo %]
                    [% IF itemdate_publisheddate #If there is at least one published date, use it for sorting%]
                        <th class="title-string">Serial enumeration / chronology</th>
                    [% ELSE %]
                        <th>Serial enumeration / chronology</th>
                    [% END %]
                [% END %]

Maybe we should restore this comment.
Comment 24 Jonathan Druart 2021-06-03 13:38:25 UTC
Created attachment 121593 [details] [review]
Bug 28171: Restore comment to remove ambiguity
Comment 25 Joonas Kylmälä 2021-06-04 07:30:14 UTC
(In reply to Jonathan Druart from comment #23)
> Maybe we should restore this comment.

Yeah, good idea. :)
Comment 26 Jonathan Druart 2021-06-14 14:22:01 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2021-06-14 14:22:39 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 28 Kyle M Hall 2021-06-18 11:08:53 UTC
Pushed to 21.05.x for 21.05.01
Comment 29 Fridolin Somers 2021-06-21 12:28:47 UTC
Depends on Bug 27980 not in 20.11.x