Bug 12986 - Upgrade the DataTables jQuery plugin to the latest version
Summary: Upgrade the DataTables jQuery plugin to the latest version
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 12987 13288 13487
  Show dependency treegraph
 
Reported: 2014-09-24 15:04 UTC by Jonathan Druart
Modified: 2016-12-05 21:23 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 12986: Upgrade the DataTables jQuery plugin to the latest version (1.10.2) (291.71 KB, patch)
2014-09-24 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12986: Upgrade the DataTables jQuery plugin to the latest version (1.10.2) (292.21 KB, patch)
2014-10-06 18:50 UTC, Owen Leonard
Details | Diff | Splinter Review
[PASSED QA] Bug 12986: Upgrade the DataTables jQuery plugin to the latest version (1.10.2) (292.22 KB, patch)
2014-10-10 12:04 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2014-09-24 15:04:20 UTC
DataTables 1.10.2 is now available.

The footerCallback function does not seem to work correctly with our current version.
Comment 1 Jonathan Druart 2014-09-24 15:06:13 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2014-09-24 15:20:10 UTC
Upgrading to DataTables 1.10 from 1.9 (http://datatables.net/upgrade/1.10) lists some areas which might need particular scrutiny.
Comment 3 Zeno Tajoli 2014-10-01 09:25:58 UTC
Hi Jonathan,

we have problem to test this patch.
Could you tell us where footerCallback function doesn't work with present version
and it needs 1.10.2 of  DataTables jQuery plugin ?

About the change of 1.10 (reading http://datatables.net/upgrade/1.10):
-- Pagination (two_button), is present only in datatables.css,a css that came with the plugin. So is not used by us

-- function fnRender: we don't use it.

-- function bScrollInfinite: we don't use it

-- Could be problems with IE6/IE7: from 1.10.2 of  DataTables jQuery plugin
cookie based state saving has been replaced with localStorage based state saving.
This function is not availabe in IE6/IE7 there are same way to live with this
[The stateLoadCallbackDT and stateSaveCallbackDT parameters can be used to provide a suitable function if you wish to use cookies.]

-- Question: Do we use API plug-ins of ataTables jQuery plugin ?
A real question, I don't know the answer.
Comment 4 Jonathan Druart 2014-10-01 09:47:35 UTC
Zeno, you can try to apply patches from bug 12897, 12988 and 12989 without this one.
The behavior was broken without updating the DT plugin.
Comment 5 Owen Leonard 2014-10-01 12:02:39 UTC
Zeno are you finding errors when testing this patch or do you question whether we should be upgrading DataTables at all?

Is it continued support for IE6/7 which is at issue here?
Comment 6 Paola Rossi 2014-10-01 12:34:19 UTC
(In reply to Jonathan Druart from comment #4)

Zeno and I thank you, Jonathan.

> Zeno, you can try to apply patches from bug 12897, 12988 and 12989 without
> this one.

1) I think the first bug was 12987, wasn't it? 

2) Before trying to apply tha patch, I'm in doubt about 12987.

A) On the graph at <http://www.gliffy.com/go/publish/6160409> kindly prepared at bug-12825, the two bugs 12988 and 12989 depend on 12987, and 12987 depends on this current bug 12986.
So the "blocks" set of this bug 12986 should be limited to 12987.
And the bug 12988 would depend on bug 12987, instead of bug 12986 presently shown. 
And the bug 12989 would depend on bug 12987, instead of bug 12986 presently shown. 

B) Otherwise, in the graph the two bugs 12988 and 12989 would point directly to 12986, instead of pointing to 12987.
The "blocks" set of this bug 12986 should remain the same: 7,8 and 9.
And the bug 12988 keeps on depending on the bug 12986 presently shown. 
And the bug 12989 keeps on depending on the bug 12986 presently shown. 

Thank you in advance.
Comment 7 Zeno Tajoli 2014-10-01 12:45:13 UTC
(In reply to Owen Leonard from comment #5)
> Zeno are you finding errors when testing this patch or do you question
> whether we should be upgrading DataTables at all?

In fact the question is more "Should we upgrade DataTables".
- I don't have a PC with IE6/IE7 so I can't test if we have a problem with
version 10.x of DataTables

> Is it continued support for IE6/7 which is at issue here?

I think yes.

The other problem are any plugin that use  API of DataTables plugin.
Do you know our situation about this ?
Comment 8 Owen Leonard 2014-10-01 12:50:13 UTC
I haven't tested this patch at all so I don't know if the upgrade causes any problems. However, I think we should always try to keep up to date with the libraries Koha uses unless there is some specific reason not to.

In my opinion the possibility that it may cause problems for IE6/7 is not enough of an issue to block an upgrade to the plugin.
Comment 9 Jonathan Druart 2014-10-01 13:01:25 UTC
(In reply to Paola Rossi from comment #6)
> (In reply to Jonathan Druart from comment #4)
> 
> Zeno and I thank you, Jonathan.
> 
> > Zeno, you can try to apply patches from bug 12897, 12988 and 12989 without
> > this one.
> 
> 1) I think the first bug was 12987, wasn't it? 

Yes of course, sorry.


> 2) Before trying to apply tha patch, I'm in doubt about 12987.

Yes, the graph is correct!
I updated the dependencies on bugzilla.
Comment 10 Paola Rossi 2014-10-02 12:14:12 UTC
(In reply to Jonathan Druart from comment #4)
> [...] you can try to apply patches from bug 12897, 12988 and 12989 without
> this one.
> The behavior was broken without updating the DT plugin.

1) Looking for a feedback, I've applied 12987, 12955, 12988, 12984 and 12989 without leading 12986. And I saw no number in the last row at the bottom of the tables in:

ACQ home: /acqui/acqui-home.pl
All Funds di ADMIN: /admin/aqbudgets.pl

2) Then I've applied 12986, 12987, 12955, 12988, 12984 and 12989.
And I saw :
2A) still no numbers in the two tables
2B) The Showing broken.

So I had no feedback for 12986 until this step 2) !

3) So I've tried another test, and I've applied 12986, 12987, 12955, 12988, 12984 without the last 12989.
3.1) ACQ home was perfect: numbers ware present and OK, and Showing was OK too
3.2) Funds was KO: no numbers, broken Showing.

So 3.1 had given me a feedback for 12986, but only about acq-home.

4) So I've tried another test, and I've applied 12986, 12987, 12955, 12988 without the last 12984.
4.1) ACQ home was OK about numbers which were present,
4.2) Funds was KO: no numbers, broken Showing as 3.1

5) So I've tried a further test, and I've applied 12986, 12987, 12955 without the last 12988.
5.1) ACQ home was OK about numbers which were present,
5.2) finally in Funds I saw numbers (whose sums were not right about hide/show' filter) [and no Showing].

So I think that 12986 is able to be signed off, but I keep it in the "In Discussion" status.

The other linked patches are not yet perfect. In particular, if the sequence of applied patches I've written above is not wrong:
bug 12986, bug 12987, bug 12955, bug 12988, bug 12984, bug 12989, 
at least:
12988 ruins the bottom of the funds' table,
12989 ruins the bottom of the ACQ-home' table.
Comment 11 Jonathan Druart 2014-10-02 13:55:30 UTC
Hello Paola, I found 2 bugs.
I submitted 2 patches, 1 on bug 12987 (The new format_price include file should be include on using datatables) and 1 on bug 12989 (Fix JS error).
I hope that will fixed your issues.
Comment 12 Zeno Tajoli 2014-10-02 14:04:57 UTC
I have find the answer to this question:
>>Question: Do we use API plug-ins of DataTables jQuery plugin ?

We have those plugin of DataTables:
In  intranet-tmpl/lib/jquery/plugins/ 
-- jquery.dataTables.grouping.js
No clear if it works or not

-- jquery.dataTables.colvis.js
It is an official estension of DataTables, probably it works or an update to last version is enough

-- jquery.dataTables.columnFilter.js
In it is site speack about DataTables 1.9
http://jquery-datatables-column-filter.googlecode.com/svn/trunk/index.html

I send emails to authors to undestand compliance situation
Comment 13 Paola Rossi 2014-10-03 15:41:43 UTC
(In reply to Jonathan Druart from comment #11)
> Hello Paola, I found 2 bugs.
> I submitted 2 patches, 1 on bug 12987 (The new format_price include file
> should be include on using datatables) and 1 on bug 12989 (Fix JS error).
> I hope that will fixed your issues.

Thanks, Jonathan. 

Sorry, it's not yet OK:

I've applied these patches against master 3.17.00.025 :
12986, 12987 (2), 12955, 12988, 12984, 12989 (2)

but no numbers neither in the "Total" row of acqui-home, nor in the "Period allocated" row of funds in /admin/aqbudgets.pl.
Comment 14 Zeno Tajoli 2014-10-03 19:50:45 UTC
Hi to all,

I think to have finish the basic check asked comment #2.
There are two points that emerge as important:
-- Support of IE6/IE7
-- Interaction with plugins of this plugin.

a)Support of IE6/IE7
Discussions on chat and mailing list give as answer: we can drop support
of IE6/IE7; 
http://lists.katipo.co.nz/pipermail/koha/2014-October/040960.html,
http://lists.koha-community.org/pipermail/koha-devel/2014-October/040836.html,
http://irc.koha-community.org/koha/2014-10-01#i_1575703,
http://irc.koha-community.org/koha/2014-10-02#i_1575984

b)Interaction with plugins of this plugin.
There three plugin of DataTables in Koha, they are in  intranet-tmpl/lib/jquery/plugins/ 

1) jquery.dataTables.colvis.js
Written from the same firm that write DataTables, update to new version of DataTables.

2) jquery.dataTables.rowGrouping.js
We use the last version, there are same minir problems,the best option is to use old 1.9 API when you is plugin, see https://code.google.com/p/jquery-datatables-row-grouping/issues/detail?id=75&sort=-id

3) jquery.dataTables.columnFilter.js
We don't use the last version, there is a patch to improve compatibility with v.1.10, https://code.google.com/p/jquery-datatables-column-filter/issues/detail?id=158&sort=-id

So my idea is that:
-- We can update
-- We need to update also plugins.

So, now for me 'Patch doesn't apply' because you don't update plugin.
I think that a patch to test needs:

1) update DataTables to v 1.10
2) update query.dataTables.colvis.js
3) evalute if update or not query.dataTables.columnFilter.js and insert or not
the patch linked.

Bye
Comment 15 Jonathan Druart 2014-10-05 22:55:26 UTC
Hello Zeno, thanks for your tests.
This bug report only updates the DataTables plugin.
We could update others too if needed, but it is not the goal of this one, except if regressions are found.

Back to Needs Signoff.
Comment 16 Paola Rossi 2014-10-06 09:49:42 UTC
After having applied these patches :

12986, 12987 (2), 12955, 12988, 12984, 12989 (2)

I've tested what is required by 12987, 12988, 12989.

Using both the browsers Mozilla FF version 32.0.3 and IExplorer version 11.0. everything is OK excluding this case:

currencyformat pref set to FR,
fund's (=aqbudgets.pl) totals are all zeros (ERROR).

So I pass the patch to "Failed QA" status.

NB. To test the patches it might be that the browser should be closed and reopened.
Comment 17 Owen Leonard 2014-10-06 13:58:03 UTC
(In reply to Paola Rossi from comment #16)
> After having applied these patches :
> 
> 12986, 12987 (2), 12955, 12988, 12984, 12989 (2)

I don't think this patch should be failed based on testing those bugs since this report isn't designated as being dependent on them.
Comment 18 Jonathan Druart 2014-10-06 14:04:20 UTC
(In reply to Owen Leonard from comment #17)
> (In reply to Paola Rossi from comment #16)
> > After having applied these patches :
> > 
> > 12986, 12987 (2), 12955, 12988, 12984, 12989 (2)
> 
> I don't think this patch should be failed based on testing those bugs since
> this report isn't designated as being dependent on them.

Yes, please mark as failed qa bug 12988 if the problem only exists on the aqbudgets.pl page.
Comment 19 Owen Leonard 2014-10-06 18:50:41 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2014-10-10 12:04:11 UTC
Created attachment 32128 [details] [review]
[PASSED QA] Bug 12986: Upgrade the DataTables jQuery plugin to the latest version (1.10.2)

DataTables 1.10.2 is now available.

The footerCallback function does not seem to work correctly with our
current version.

Test plan:
Go on the maximum of pages where DataTables is used and try to catch a
bug/regression :)

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

I tested many many pages with DataTables, taking special care to find
pages with different DataTables configurations. I found no errors in the
OPAC or staff client.

Tested thoroughly in Firefox (latest) on Windows 7.
Tested less thoroughly in Chrome (latest) on Windows 7.
Tested briefly in Internet Explorer 7 in Vista and Internet Explorer 8
and found no bugs which were related to the DataTables upgrade.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Katrin Fischer 2014-10-27 22:37:24 UTC
Leaving bread crumbs... from here, go to bug 12987.
Comment 22 Tomás Cohen Arazi 2014-11-10 15:57:36 UTC
Patch pushed to master.

Thanks Jonathan!