Bug 15883

Summary: Upgrade jQuery from v1.7.2 in the staff client
Product: Koha Reporter: Owen Leonard <oleonard>
Component: TemplatesAssignee: Owen Leonard <oleonard>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: bgkriegel, brendan, cnighswonger, dcook, jdemuth, jonathan.druart, josef.moravec, julian.maurice, mtj, veron
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16218
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 14546, 16238, 16239, 16241, 16321, 16553, 29155    
Attachments: Bug 15883 - Upgrade jQuery from v1.7.2 in the staff client
Bug 15883 - Upgrade jQuery from v1.7.2 in the staff client
Bug 15883: Use .prop() instead of .attr() for 'checked'
Bug 15883 - Upgrade jQuery from v1.7.2 in the staff client
Bug 15883 - Upgrade jQuery from v1.7.2 in the staff client
Bug 15883 - Upgrade jQuery from v1.7.2 in the staff client
Bug 15883 - Upgrade jQuery from v1.7.2 in the staff client

Description Owen Leonard 2016-02-23 13:26:25 UTC
In order to upgrade Bootstrap in the staff client we first need to upgrade jQuery.

After discussing it some in IRC (http://irc.koha-community.org/koha/2016-02-22#i_1791968), this is the plan I'm working on right now:

An initial patch will start by adding the basis for upgrades:

1. Add a new version of doc-head-close.inc which includes new version of jQuery and jQueryUI. That include file will be named doc-head-close-jquery.1.12.0.inc to reflect the version of jQuery it uses.

2. Add the 1.12.0 version of jQuery, naming the file jquery-1.12.0.min.js

3. Add the latest version of jQueryUI, 1.11.4, naming the files jquery-ui-1.11.4.js and jquery-ui-1.11.4.min.css.

Subsequent patches can take a page-by-page approach to the upgrade, dropping in the new include file and making any upgrades necessary to Javascript on that page. As compatibility issues are discovered, affected JS files can be re-created with updated file names to reflect the jQuery version, as in "basket-jquery-1.12.0.js"

Disadvantages to this method:

1. It's possible some patches will have to touch both doc-head-close.inc and doc-head-close-jquery.1.12.0.inc. However that file changes fairly seldom.

2. git history getting lost between files. Can it be done so that the new include file keeps the history of doc-head-close.inc and doc-head-close.inc is created as if it is the new file? Eventually it could be removed.

I would like to hear opinions and suggestions about this.
Comment 1 Chris Nighswonger 2016-02-23 23:15:14 UTC
(In reply to Owen Leonard from comment #0)
> An initial patch will start by adding the basis for upgrades:
> 
> 1. Add a new version of doc-head-close.inc which includes new version of
> jQuery and jQueryUI. That include file will be named
> doc-head-close-jquery.1.12.0.inc to reflect the version of jQuery it uses.
> 

Just for clarification: Is this nomenclature intended to be permanent or is this just temporary until everything is upgraded?

> 
> 2. git history getting lost between files. Can it be done so that the new
> include file keeps the history of doc-head-close.inc and doc-head-close.inc
> is created as if it is the new file? Eventually it could be removed.

Assuming we will eventually return to the doc-head-close.inc nomenclature, why wouldn't we just overwrite the original doc-head-close.inc with the new doc-head-close-jquery.1.12.0.inc at the appropriate time and let git handle is as one big diff? This would preserve the history and it should be obvious in the log and patch what was done.
Comment 2 Chris Nighswonger 2016-02-23 23:16:52 UTC
By overwrite, I mean contents, of course. The name would remain doc-head-close.inc which should preserve the git history I think.
Comment 3 Owen Leonard 2016-02-23 23:49:02 UTC
(In reply to Chris Nighswonger from comment #1)
> Just for clarification: Is this nomenclature intended to be permanent or is
> this just temporary until everything is upgraded?

Theoretically temporary I guess.
Comment 4 Owen Leonard 2016-02-29 14:38:57 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2016-03-11 14:31:35 UTC
(In reply to Owen Leonard from comment #3)
> (In reply to Chris Nighswonger from comment #1)
> > Just for clarification: Is this nomenclature intended to be permanent or is
> > this just temporary until everything is upgraded?
> 
> Theoretically temporary I guess.

I'd prefer the contrary: update all existing occurrences of [% INCLUDE 'doc-head-close.inc' %] with [% INCLUDE 'doc-head-close.old.inc' %] # s/old/whatever
I can provide a follow-up if you agree.
Comment 6 Owen Leonard 2016-03-11 14:53:07 UTC
(In reply to Jonathan Druart from comment #5)

> I'd prefer the contrary: update all existing occurrences of [% INCLUDE
> 'doc-head-close.inc' %] with [% INCLUDE 'doc-head-close.old.inc' %] #
> s/old/whatever

That's fine with me. I wonder if the name of the "old" include should reflect the version? I'm concerned that we are going to end up with doc-head-close.old.inc, doc-head-close.older.inc, and doc-head-close.even-older.inc if newer jQuery upgrades start getting added before the last upgrade is complete!
Comment 7 Bernardo Gonzalez Kriegel 2016-04-04 19:49:07 UTC
There is a small conflict

Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt
Failed to merge in the changes.

Solving that we got: 
Software error:
Template process failed: file error - searchbox-keep-text.inc: not found at /home/bgkriegel/kohaclone/C4/Templates.pm line 119

Where is 'searchbox-keep-text.inc'?

BTW, have you an agreement on file names, i.e. jquery-ver vs. old/older?
Comment 8 Owen Leonard 2016-04-06 16:43:20 UTC
(In reply to Jonathan Druart from comment #5)

> I'd prefer the contrary: update all existing occurrences of [% INCLUDE
> 'doc-head-close.inc' %] with [% INCLUDE 'doc-head-close.old.inc' %] #
> s/old/whatever

Looking at this again... Part of the intention of my original plan was to not have to modify hundreds of files at once. That's going to create lots of problems for pending patches isn't it?
Comment 9 Jonathan Druart 2016-04-07 10:24:59 UTC
I'd prefer to keep the jquery.js pointing on the newest version.
Do you know how big is the work to move to jQuery v1.12?
What if we brutally update all the templates to use the new version and fix them when we find bugs? :)
Comment 10 Owen Leonard 2016-04-07 13:50:25 UTC
I think there is no benefit to having a file called "jquery.js" which is of unknown (or at least non-obvious) version. History has shown that we are bad at keeping jQuery updated, so it would not be long before it wasn't the newest version.
Comment 11 Owen Leonard 2016-04-08 16:01:59 UTC Comment hidden (obsolete)
Comment 12 Owen Leonard 2016-04-08 16:10:55 UTC
Note that jQuery 2.2.3 does not support Internet Explorer 6, 7, or 8.
Comment 13 Julian Maurice 2016-04-08 17:12:03 UTC
(In reply to Owen Leonard from comment #12)
> Note that jQuery 2.2.3 does not support Internet Explorer 6, 7, or 8.

As well as Microsoft: https://www.microsoft.com/en-us/WindowsForBusiness/End-of-IE-support, so not a big issue I think.
In fact, only IE 11 is supported by Microsoft
Comment 14 Julian Maurice 2016-04-08 17:52:37 UTC
I applied the patch and went to several places in the staff client (search, circulation, patrons, serials, admin, reports, tools, ...).
No issues so far, not even a warning from jQuery Migrate.

I'll try to do more testing when I have time
Comment 15 Owen Leonard 2016-04-08 18:18:32 UTC
(In reply to Julian Maurice from comment #14)
> No issues so far, not even a warning from jQuery Migrate.

Thanks for testing. I would expect you to get console log warnings from jQuery Migrate on every page if only from Bootstrap's js. Did you switch from using the production (minified) to the development (unminified) version?

I'd love to be able to build in an easy switch for doing that but I don't know how.
Comment 16 Julian Maurice 2016-04-09 04:41:50 UTC
Okay, when using development version of jQuery Migrate I can see some warnings.
Maybe we should use that by default, it's only 4K more than the production version, and will force us to fix errors as we see them
Comment 17 Julian Maurice 2016-04-09 06:20:55 UTC Comment hidden (obsolete)
Comment 18 Julian Maurice 2016-04-09 06:23:15 UTC
Note that there is still a call to .attr('checked', false) in jquery.dataTables.columnFilter.js
I am not sure how we should handle that.
Comment 19 Owen Leonard 2016-04-09 12:55:05 UTC
(In reply to Julian Maurice from comment #16)
> Okay, when using development version of jQuery Migrate I can see some
> warnings.
> Maybe we should use that by default

My concern is that the large quantity of warnings might make it difficult for those who are working on their own JavaScript code to debug their work.

If my patch seems to work, with the migrate plugin working to prevent compatibility errors, I suggest we work to get that pushed and submit separate follow-up bugs which depend on this one.
Comment 20 Julian Maurice 2016-04-11 08:04:12 UTC
(In reply to Owen Leonard from comment #19)
> My concern is that the large quantity of warnings might make it difficult
> for those who are working on their own JavaScript code to debug their work.
Yes, this will be annoying, thus forcing us to fix the errors :)

> If my patch seems to work, with the migrate plugin working to prevent
> compatibility errors, I suggest we work to get that pushed and submit
> separate follow-up bugs which depend on this one.
Okay I'll create a separate bug for my patch
Comment 21 Julian Maurice 2016-04-11 08:09:25 UTC
Comment on attachment 50098 [details] [review]
Bug 15883: Use .prop() instead of .attr() for 'checked'

Patch moved to bug 16238
Comment 22 Marc Véron 2016-04-12 14:48:14 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2016-04-13 08:57:03 UTC
Marc, could you detail what you have tested?
Comment 24 Mason James 2016-04-13 12:58:24 UTC
(In reply to Julian Maurice from comment #18)
> Note that there is still a call to .attr('checked', false) in
> jquery.dataTables.columnFilter.js
> I am not sure how we should handle that.

Datatables looks to have a version that is built for jQuery 2.x
Perhaps upgrading DT will fix this problem

https://datatables.net/download/index
Comment 25 Marc Véron 2016-04-13 13:08:42 UTC
(In reply to Jonathan Druart from comment #23)
> Marc, could you detail what you have tested?

First I used the test plan in comment #4 to verify that the new jQuery version works.
Then I went through staff client for ca. 45 min., with JavaScript error console open (in FF). I focused on pages where jQuery is involved, especially pages with tables / column settings etc.
After that I added some code to IntranetUserJS (similar to Bug 16218) to display the jQuery version, because I wanted to verify that the new version was included on all pages I opened, and I went again through staff client.
Comment 26 Mason James 2016-04-13 13:20:17 UTC
(In reply to Mason James from comment #24)
> (In reply to Julian Maurice from comment #18)
> > Note that there is still a call to .attr('checked', false) in
> > jquery.dataTables.columnFilter.js
> > I am not sure how we should handle that.
> 
> Datatables looks to have a version that is built for jQuery 2.x
> Perhaps upgrading DT will fix this problem
> 
> https://datatables.net/download/index

oops, ignore my suggestion.  i see 'jquery.dataTables.columnFilter.js' is a plugin

 https://code.google.com/archive/p/jquery-datatables-column-filter/
Comment 27 Jonathan Druart 2016-04-13 13:58:34 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2016-04-13 14:01:56 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2016-04-13 14:02:18 UTC
Created attachment 50198 [details] [review]
Bug 15883 - Upgrade jQuery from v1.7.2 in the staff client

This patch updates the staff client version of jQuery to 2.2.3. In order
to maintain compatibility with existing scripts, the jQuery migrate
plugin has also been added.

Included is the development version of the jQuery migrate plugin.
Developers could modify doc-head-close.inc to use this version if they
wanted to take advantage of its upgrade warning log.

To test, apply the patch and test JavaScript interactions everywhere in
the staff client.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 30 Jonathan Druart 2016-04-13 14:02:45 UTC
Just forgot twice to add my signoff...
Comment 31 Brendan Gallagher 2016-04-20 20:33:09 UTC
Pushed to Master - Should be in the May 2016 Release.  Thanks!