Description
Owen Leonard
2016-02-23 13:26:25 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. By overwrite, I mean contents, of course. The name would remain doc-head-close.inc which should preserve the git history I think. (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. Created attachment 48457 [details] [review] Bug 15883 - Upgrade jQuery from v1.7.2 in the staff client This patch adds jQuery 1.12.0 and jQueryUI 1.11.4 to the staff client. It adds a new version of doc-head-close.inc, doc-head-close-jquery-1.12.0.inc, to be included on pages which have been tested with the new versions of jQuery and jQueryUI. This patch changes only one template to use the new include file, intranet-main.tt, as a proof of concept. To test, apply the patch and view the staff client home page. Any feature on the page which uses JavaScript should be tested: - Dropdown menus like "More" and the user menu. - Header search form tabs, and the preservation of search form text during tab switching. - The "Help" link popup. - Cart functionality -- Confirm that an clicking the "Cart" link when empty shows you the correct message. -- After adding titles to the Cart, return to the home page and confirm that the count of items in the Cart is correct. -- Confirm that clicking the "Cart" link triggers the Cart popup. -- In the Cart popup, click "Empty and close" and confirm that the "Cart" link now shows "(0)" as the count. (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. (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! 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? (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? 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? :) 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. Created attachment 50087 [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. Note that jQuery 2.2.3 does not support Internet Explorer 6, 7, or 8. (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 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 (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. 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 Created attachment 50098 [details] [review] Bug 15883: Use .prop() instead of .attr() for 'checked' According to https://jquery.com/upgrade-guide/1.9/#attr-versus-prop- .attr() is no longer correct to access the checked state of a checkbox. This patch do the following replacements: .attr('checked') => .prop('checked') .attr('checked, '') => .prop('checked', false) .attr('checked, 'checked') => .prop('checked', true) .attr('checked', boolValue) => .prop('checked', boolValue) .removeAttr('checked') => .prop('checked', false) .attr('checked') == 'checked' => .is(':checked') 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. (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. (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 on attachment 50098 [details] [review] Bug 15883: Use .prop() instead of .attr() for 'checked' Patch moved to bug 16238 Created attachment 50151 [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> Marc, could you detail what you have tested? (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 (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. (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/ Created attachment 50196 [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> Created attachment 50197 [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> 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> Just forgot twice to add my signoff... Pushed to Master - Should be in the May 2016 Release. Thanks! |