Bug 16238 - Upgrade jQuery in staff client: use .prop() instead of .attr()
Summary: Upgrade jQuery in staff client: use .prop() instead of .attr()
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Keywords:
Depends on: 15883
Blocks: 16394
  Show dependency treegraph
 
Reported: 2016-04-11 08:06 UTC by Julian Maurice
Modified: 2016-12-05 21:27 UTC (History)
4 users (show)

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


Attachments
Bug 16238: Use .prop() instead of .attr() for 'checked' (46.29 KB, patch)
2016-04-11 08:08 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 16238: QA fix: remove tab characters (3.55 KB, patch)
2016-04-13 08:31 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 16238: Use .prop() instead of .attr() for 'checked' (46.48 KB, patch)
2016-04-13 12:38 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 16238: QA fix: remove tab characters (3.69 KB, patch)
2016-04-13 12:39 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 16238: Use .prop() instead of .attr() for 'disabled' (24.73 KB, patch)
2016-04-13 18:29 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 16238: Use .prop() instead of .attr() for 'readonly' (1.98 KB, patch)
2016-04-13 18:51 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 16238: Use .prop() instead of .attr() for 'checked' (46.42 KB, patch)
2016-04-22 07:15 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 16238: QA fix: remove tab characters (3.68 KB, patch)
2016-04-22 07:15 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 16238: Use .prop() instead of .attr() for 'disabled' (25.05 KB, patch)
2016-04-22 07:15 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 16238: Use .prop() instead of .attr() for 'readonly' (1.98 KB, patch)
2016-04-22 07:15 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16238: Use .prop() instead of .attr() for 'checked' (46.53 KB, patch)
2016-04-22 12:57 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16238: QA fix: remove tab characters (3.79 KB, patch)
2016-04-22 12:57 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16238: Use .prop() instead of .attr() for 'disabled' (25.16 KB, patch)
2016-04-22 12:57 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16238: Use .prop() instead of .attr() for 'readonly' (2.08 KB, patch)
2016-04-22 12:57 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16238 [Follow-up] Upgrade jQuery in staff client: use .prop() instead of .attr() (2.53 KB, patch)
2016-04-22 13:03 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16238 [Follow-up] Upgrade jQuery in staff client: use .prop() instead of .attr() (2.59 KB, patch)
2016-04-26 17:01 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 16238: Use .prop() instead of .attr() for 'checked' (46.54 KB, patch)
2016-04-26 17:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16238: QA fix: remove tab characters (3.80 KB, patch)
2016-04-26 17:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16238: Use .prop() instead of .attr() for 'disabled' (25.17 KB, patch)
2016-04-26 17:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16238: Use .prop() instead of .attr() for 'readonly' (2.10 KB, patch)
2016-04-26 17:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16238 [Follow-up] Upgrade jQuery in staff client: use .prop() instead of .attr() (2.65 KB, patch)
2016-04-26 17:52 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 Julian Maurice 2016-04-11 08:06:09 UTC
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.
Comment 1 Julian Maurice 2016-04-11 08:08:50 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2016-04-12 15:05:08 UTC
QA tools complain at 2 places:


koha-tmpl/intranet-tmpl/prog/en/includes/messaging-preference-form.inc
forbidden pattern: tab char (line 6)

koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
forbidden pattern: tab char (line 169)
Comment 3 Julian Maurice 2016-04-13 08:31:30 UTC Comment hidden (obsolete)
Comment 4 Marc Véron 2016-04-13 12:38:35 UTC Comment hidden (obsolete)
Comment 5 Marc Véron 2016-04-13 12:39:33 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2016-04-13 13:51:02 UTC
What about the others occurrences?
I have found some others at the OPAC:
   git grep '\.attr' **/*.tt|grep "'checked'"
Comment 7 Julian Maurice 2016-04-13 14:06:19 UTC
(In reply to Jonathan Druart from comment #6)
> What about the others occurrences?
> I have found some others at the OPAC:
>    git grep '\.attr' **/*.tt|grep "'checked'"
I don't know how I could have missed so many of them. I'll provide a followup
Comment 8 Julian Maurice 2016-04-13 14:09:17 UTC
(In reply to Julian Maurice from comment #7)
> (In reply to Jonathan Druart from comment #6)
> > What about the others occurrences?
> > I have found some others at the OPAC:
> >    git grep '\.attr' **/*.tt|grep "'checked'"
> I don't know how I could have missed so many of them. I'll provide a followup
In fact, it's completely normal, bug 15883 only upgrades jQuery for staff interface, so no changes required in OPAC
Comment 9 Jonathan Druart 2016-04-13 14:12:05 UTC
What do you only fix checked?
There are also disabled and readonly.
Comment 10 Jonathan Druart 2016-04-13 14:12:24 UTC
s/What/Why
Comment 11 Julian Maurice 2016-04-13 14:24:35 UTC
(In reply to Jonathan Druart from comment #9)
> Why do you only fix checked?
> There are also disabled and readonly.

No particular reason. I suppose this should be done as well
Comment 12 Julian Maurice 2016-04-13 18:29:08 UTC Comment hidden (obsolete)
Comment 13 Julian Maurice 2016-04-13 18:51:49 UTC Comment hidden (obsolete)
Comment 14 Marc Véron 2016-04-21 14:01:31 UTC
Sorry, does not apply:

Applying: Bug 16238: Use .prop() instead of .attr() for 'checked'
Applying: Bug 16238: QA fix: remove tab characters
Applying: Bug 16238: Use .prop() instead of .attr() for 'disabled'
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/itemsearch.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/itemsearch.tt
Comment 15 Owen Leonard 2016-04-21 18:24:03 UTC
That merge problem isn't very serious. I was able to correct it while testing, but I'm still going through all the changes before I can sign off.
Comment 16 Julian Maurice 2016-04-22 07:15:31 UTC Comment hidden (obsolete)
Comment 17 Julian Maurice 2016-04-22 07:15:33 UTC Comment hidden (obsolete)
Comment 18 Julian Maurice 2016-04-22 07:15:36 UTC Comment hidden (obsolete)
Comment 19 Julian Maurice 2016-04-22 07:15:39 UTC Comment hidden (obsolete)
Comment 20 Julian Maurice 2016-04-22 07:16:05 UTC
Patches rebased on master
Comment 21 Owen Leonard 2016-04-22 12:57:12 UTC Comment hidden (obsolete)
Comment 22 Owen Leonard 2016-04-22 12:57:19 UTC Comment hidden (obsolete)
Comment 23 Owen Leonard 2016-04-22 12:57:25 UTC Comment hidden (obsolete)
Comment 24 Owen Leonard 2016-04-22 12:57:32 UTC Comment hidden (obsolete)
Comment 25 Owen Leonard 2016-04-22 13:03:21 UTC Comment hidden (obsolete)
Comment 26 Bernardo Gonzalez Kriegel 2016-04-26 17:01:26 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2016-04-26 17:52:32 UTC
Created attachment 50737 [details] [review]
Bug 16238: 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')

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 28 Jonathan Druart 2016-04-26 17:52:36 UTC
Created attachment 50738 [details] [review]
Bug 16238: QA fix: remove tab characters

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 29 Jonathan Druart 2016-04-26 17:52:40 UTC
Created attachment 50739 [details] [review]
Bug 16238: Use .prop() instead of .attr() for 'disabled'

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 30 Jonathan Druart 2016-04-26 17:52:44 UTC
Created attachment 50740 [details] [review]
Bug 16238: Use .prop() instead of .attr() for 'readonly'

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 31 Jonathan Druart 2016-04-26 17:52:48 UTC
Created attachment 50741 [details] [review]
Bug 16238 [Follow-up] Upgrade jQuery in staff client: use .prop() instead of .attr()

This patch corrects some tabs which should have been replaced with
spaces.

To test, diff against the previous commit with the '-w' flag. There
should be no visible changes.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Space changes, no errors.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 32 Kyle M Hall 2016-04-29 14:32:00 UTC
Pushed to master for the 16.05 release. Thanks Julian!