Bug 36640 - Upgrade DataTables from 1.13.6 to 2.x
Summary: Upgrade DataTables from 1.13.6 to 2.x
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 33484 37238 34913 38113
Blocks: 13965 26553 30113 33729 38185 38255 38259 38295 38296 38310 38311 38388 38436 38461 38543 38596 38958 39080 39169 38114 38456 38484
  Show dependency treegraph
 
Reported: 2024-04-18 11:34 UTC by Owen Leonard
Modified: 2025-02-19 20:02 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00
Circulation function:


Attachments
Bug 36640: Fix Asset.t (1.15 KB, patch)
2024-11-15 13:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36640: (follow-up) Fix itemsearch (1.02 KB, patch)
2024-11-19 10:40 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2024-04-18 11:34:36 UTC
DataTables is now in 2.x releases, so we have more upgrading to do.
Comment 1 Jonathan Druart 2024-10-16 13:19:49 UTC
Pushed a remote branch with the WIP work.

It's built on top of bug 33484.
Comment 2 Jonathan Druart 2024-10-16 13:41:42 UTC
https://gitlab.com/joubu/Koha/-/commits/bug_36640
Comment 3 Jonathan Druart 2024-10-24 09:08:20 UTC
For discussion

Behaviour change:
https://datatables.net/upgrade/2#Order-sequence

"""
When ordering columns by clicking on their header element, in v1 it would simply toggle between ascending and descending ordering for the columns (controllable with the columns.orderSequence option). DataTables 2 introduces a new option to this parameter, an empty string value which represents no ordering. The default has been updated to make use of this and the ordering sequence that is applied when clicking the header cell is now:

    Ascending
    Descending
    No order

It will repeat that sequence indefinitely as the column header is clicked. As before, it can be controlled using the columns.orderSequence option, and if you wish to restore the default DataTables 1.x behaviour you can do so by setting the default:
	
DataTable.defaults.column.orderSequence = ['asc', 'desc'];
"""
Comment 4 David Cook 2024-10-25 00:42:24 UTC
(In reply to Jonathan Druart from comment #3)
> It will repeat that sequence indefinitely as the column header is clicked.
> As before, it can be controlled using the columns.orderSequence option, and
> if you wish to restore the default DataTables 1.x behaviour you can do so by
> setting the default:
> 	
> DataTable.defaults.column.orderSequence = ['asc', 'desc'];

I reckon we keep the 1.x behaviour, yeah?
Comment 5 Jonathan Druart 2024-10-31 09:51:10 UTC
(In reply to David Cook from comment #4)
> (In reply to Jonathan Druart from comment #3)
> > It will repeat that sequence indefinitely as the column header is clicked.
> > As before, it can be controlled using the columns.orderSequence option, and
> > if you wish to restore the default DataTables 1.x behaviour you can do so by
> > setting the default:
> > 	
> > DataTable.defaults.column.orderSequence = ['asc', 'desc'];
> 
> I reckon we keep the 1.x behaviour, yeah?

Done in commit "Bug 36640: Restore previous order sequence behaviour"
Comment 6 Jonathan Druart 2024-10-31 10:10:37 UTC
This is ready for testing.

There is a "TODO" commit at the top of the branch with the known to be done.
Comment 7 Jonathan Druart 2024-10-31 13:19:53 UTC
Style of the selects in the column filter has been reverted inadvertently on the item search.

Trying to summarize the findings here:

The selector is `#itemsearchform select, input[type="text"] {`, which is wrong. It is supposed to apply to the item search *form* only, but we apply to all the inputs.

There is another class we have lost: .form-control
This is what was used by the selects. Now we are using the filters added from datatables.js and the class no longer exist.

There is a class already on the selects: "dt-select-filter", we should use this one, and add one to the inputs: "dt-input-filter". And add a style for them, that will be applied to ALL columns filters of all our tables using them.
Comment 8 Lisette Scheer 2024-11-08 22:40:47 UTC
Testing in my devbox comparing to main on a bywatersandbox:
-Sorting works correctly
-Column toggle works correctly 
-Export works correctly
-Copy sharable link works, (after I changed the base url setting)
-Configure no longer jumps to the correct table on the configure page, just the section. (also on main) 
-The font size is 11.7px (computed) rather than 13px and the columns are less clearly delineated in the dropdown. 

Tables tested:
-Issues (on checkouts and more members)
-Patron search results
 -Filter search no longer works for typing library in the main filter box, the dropdown does work
-Returns
-Holds Queue
-Bookings to collect
-holds to pull
(there was an error in both main and devbox when trying hold ratios)

The biggest concern I have is the change in font size making it harder to read and than when you zoom the columns toggle moves off the screen after a couple zooms. That's an accessibility issue for sure. Otherwise everything I tested worked well except the filter search in library working in main and not this. 

Those could be a problem but I'm not sure if they're big enough to block it or if followups would be better.
Comment 9 Katrin Fischer 2024-11-11 14:51:34 UTC
(In reply to Lisette Scheer from comment #8)
> Testing in my devbox comparing to main on a bywatersandbox:
> -Sorting works correctly
> -Column toggle works correctly 
> -Export works correctly
> -Copy sharable link works, (after I changed the base url setting)
> -Configure no longer jumps to the correct table on the configure page, just
> the section. (also on main) 
> -The font size is 11.7px (computed) rather than 13px and the columns are
> less clearly delineated in the dropdown. 
> 
> Tables tested:
> -Issues (on checkouts and more members)
> -Patron search results
>  -Filter search no longer works for typing library in the main filter box,
> the dropdown does work
> -Returns
> -Holds Queue
> -Bookings to collect
> -holds to pull
> (there was an error in both main and devbox when trying hold ratios)
> 
> The biggest concern I have is the change in font size making it harder to
> read and than when you zoom the columns toggle moves off the screen after a
> couple zooms. That's an accessibility issue for sure. Otherwise everything I
> tested worked well except the filter search in library working in main and
> not this. 
> 
> Those could be a problem but I'm not sure if they're big enough to block it
> or if followups would be better.

I think font-size would hopefully be an easy fix and we should file the problem with the configure link - I noticed it too on main.

But it doesn't sound as we have a blocker here currently. Lisette, would you agree? Would you be OK signing off? and Thanks for all the testing!
Comment 10 Jake Deery 2024-11-12 14:51:15 UTC
See https://koha-hedgedoc.servers.llownd.net/s/Comments_on_the_DataTables_2.x_switch

I have yet to find an issue. If a patch were attached, I would gladly sign-off :-)
Comment 12 Owen Leonard 2024-11-12 15:30:48 UTC
Found an issue: Acquisitions -> Invoices

Checking the "Show only subscriptions" checkbox triggers an error in the console:

Uncaught TypeError: $(...).dataTable().columns is not a function
invoices.pl:1977:49

I don't know if it makes a difference that I don't have any invoices related to subscriptions.
Comment 13 Lisette Scheer 2024-11-12 15:36:56 UTC
(In reply to Katrin Fischer from comment #9)

> But it doesn't sound as we have a blocker here currently. Lisette, would you
> agree? Would you be OK signing off? and Thanks for all the testing!

Yes!
Comment 14 Owen Leonard 2024-11-12 16:49:10 UTC
The "Clear filter" button isn't getting added in some situations... I've noticed it on the suggestions page and the Z39.50/SRU servers page.'

I pushed a small fix for the alignment of the clear button here: https://gitlab.com/koha-dev/koha-dev/-/commits/bug_36640. It can be tested with libraries administration, cities, and SMTP servers just to name a few.
Comment 15 Jonathan Druart 2024-11-13 11:31:01 UTC
(In reply to Lisette Scheer from comment #8)
> -Patron search results
>  -Filter search no longer works for typing library in the main filter box,
> the dropdown does work

Because bug 38130 was not on the branch. Done now.

(In reply to Owen Leonard from comment #14)
> The "Clear filter" button isn't getting added in some situations... I've
> noticed it on the suggestions page and the Z39.50/SRU servers page.'

Fixed on "Bug 36640: (follow-up) Remove fnFilter - fix acqui/invoices"

> I pushed a small fix for the alignment of the clear button here:
> https://gitlab.com/koha-dev/koha-dev/-/commits/bug_36640. It can be tested
> with libraries administration, cities, and SMTP servers just to name a few.

cherry-picked, thanks!


Added Jake and Lisette's signed-off-by lines, thanks!
Comment 16 Jonathan Druart 2024-11-13 11:48:12 UTC
The configure link behaviour will be fixed by the follow-up on bug 38069. Waiting for RM to pick it.
Comment 17 Owen Leonard 2024-11-13 12:08:42 UTC
> (In reply to Owen Leonard from comment #14)
> > The "Clear filter" button isn't getting added in some situations... I've
> > noticed it on the suggestions page and the Z39.50/SRU servers page.'
> 
> Fixed on "Bug 36640: (follow-up) Remove fnFilter - fix acqui/invoices"

That commit only touches acqui/invoices.tt. Are you thinking of a different fix? I still see the problem today after a fresh pull from your branch.

It looks like table which use KohaTable() are affected.
Comment 18 Katrin Fischer 2024-11-13 12:09:49 UTC
Where are we at here? Will you attach the patches to bugzilla or should I pick from remote?
Comment 19 Jonathan Druart 2024-11-13 13:59:05 UTC
Quotes editor fixed on "Bug 36640: Fix quotes editor"
Comment 20 Owen Leonard 2024-11-13 16:26:39 UTC
(In reply to Lisette Scheer from comment #8)
> -The font size is 11.7px (computed) rather than 13px and the columns are
> less clearly delineated in the dropdown. 

In my test the font size is the same before and after the upgrade, so I suspect it's an optical illusion related to the fact that the new version doesn't have the same borders and padding. I left the dropdowns looking like the default in DataTables 2 rather than applying the custom style we had before. Perhaps that was a mistake?
Comment 21 Katrin Fischer 2024-11-13 16:26:57 UTC
(In reply to Jonathan Druart from comment #16)
> The configure link behaviour will be fixed by the follow-up on bug 38069.
> Waiting for RM to pick it.

Picked.
Comment 22 Jonathan Druart 2024-11-14 08:29:55 UTC
Trying to correct a confusion.

(In reply to Jonathan Druart from comment #15)
> (In reply to Owen Leonard from comment #14)
> > The "Clear filter" button isn't getting added in some situations... I've
> > noticed it on the suggestions page and the Z39.50/SRU servers page.'
> 
> Fixed on "Bug 36640: (follow-up) Remove fnFilter - fix acqui/invoices"

This patch was not for the "clear filter" I quoted, but for this comment:

(In reply to Owen Leonard from comment #14)
> The "Clear filter" button isn't getting added in some situations... I've
> noticed it on the suggestions page and the Z39.50/SRU servers page.'
Comment 23 Jonathan Druart 2024-11-14 10:01:07 UTC
"Clear filter" should be fixed on "Bug 36640: Fix display of 'Clear filter'"
Comment 24 Jonathan Druart 2024-11-14 10:16:56 UTC
New commit: "Bug 36640: Fix behaviour of 'Clear filter'"
Comment 25 Jake Deery 2024-11-14 10:23:22 UTC
Just to update, I've tested Joubu's latest commits and they're now working.

Jake.
Comment 26 Jonathan Druart 2024-11-15 13:08:10 UTC
Created attachment 174575 [details] [review]
Bug 36640: Fix Asset.t
Comment 27 Katrin Fischer 2024-11-15 13:14:25 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 28 Jonathan Druart 2024-11-19 10:40:16 UTC
Created attachment 174783 [details] [review]
Bug 36640: (follow-up) Fix itemsearch

We replaced 'columnFilter', and it's no longer there.
Comment 29 Katrin Fischer 2024-11-19 15:39:52 UTC
Please switch status back to Passed QA when attaching a follow-up. It's hard to make sure I catch them all with the continuous big amount of bug email.
Comment 30 Katrin Fischer 2024-11-19 15:40:49 UTC
Picked "(follow-up) Fix itemsearch" for main.