Bug 14266 - Replace trim() with $.trim() in opac-shareshelf.tt
Summary: Replace trim() with $.trim() in opac-shareshelf.tt
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-25 04:06 UTC by David Cook
Modified: 2016-12-05 21:23 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Replace trim() with $.trim() in opac-shareshelf.tt (1.78 KB, patch)
2015-05-25 04:12 UTC, David Cook
Details | Diff | Splinter Review
Replace trim() with $.trim() in opac-shareshelf.tt (1.88 KB, patch)
2015-05-25 23:45 UTC, Indranil Das Gupta
Details | Diff | Splinter Review
[COUNTER-PATCH] Bug 14266: Trim the email address in the pl script (2.58 KB, patch)
2015-05-26 11:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14266: Trim the email address in the pl script (2.70 KB, patch)
2015-05-28 19:16 UTC, Indranil Das Gupta
Details | Diff | Splinter Review
[PASSED QA] Bug 14266: Trim the email address in the pl script (2.77 KB, patch)
2015-05-29 16:01 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 David Cook 2015-05-25 04:06:29 UTC
At the moment, we use both jQuery's $.trim() and the regular Javascript trim() in our templates.

You can find trim() in:
- koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shareshelf.tt
- koha-tmpl/intranet-tmpl/prog/en/modules/admin/categorie.tt

You can find $.trim() in:
- koha-tmpl/intranet-tmpl/prog/en/modules/admin/transport-cost-matrix.tt

The jQuery $.trim() appears perfectly cross-platform, but the Javascript trim() isn't supported in IE8 or earlier. It was only introduced in IE9.

Since we already use jQuery everywhere, I don't see why we can't use $.trim() instead of trim(). 

At the moment, people using IE8 in the staff client (and I'm sure there are many of them) can't save patron categories. Javascript errors will prevent the submission of the page.

--

I know we occasionally debate supporting older versions of IE, but this is a pretty easy win.
Comment 1 David Cook 2015-05-25 04:12:47 UTC Comment hidden (obsolete)
Comment 2 Indranil Das Gupta 2015-05-25 13:32:22 UTC
(In reply to David Cook from comment #1)
> Created attachment 39470 [details] [review] [review]
<snipped>
> 5) Type in an email address and click "Send"
> 6) Note an error preventing the submission of the page (you can
> also see this in the console log)

Strangely I couldn't reproduce the error on IE 8.0.6001.18702
There was no problem with form submission.
Comment 3 David Cook 2015-05-25 23:40:40 UTC
(In reply to Indranil Das Gupta from comment #2)
> Strangely I couldn't reproduce the error on IE 8.0.6001.18702
> There was no problem with form submission.

Ah, my bad with the test plan. The form will still submit. It'll just produce a Javascript error along the way.
Comment 4 Indranil Das Gupta 2015-05-25 23:45:06 UTC
Created attachment 39497 [details] [review]
Replace trim() with $.trim() in opac-shareshelf.tt

This patch replaces trim() with $.trim() which is supported
in versions of IE older than IE9.

Revised test plan
=================

Before applying patch:

0) Use IE 8 or Document Mode 8 in a newer IE using F12 Developer Tools
1) Set OpacAllowSharingPrivateLists to "Allow" in Global System Preferences
2) Create a private list in the OPAC
3) Add a record to the private list
4) Click "Share" or "Share list" on one of the list screens
5) Type in an email address and click "Send"
6) Note the error in the console log
7) The page should submit

Apply the patch:

7) Hold shift + refresh the browser to update any Javascript cache
8) Try to "Share" the list again
9) Note that the form submit after clicking "Send" and
that there are no errors in the console log

http://bugs.koha-community.org/show_bug.cgi?id=14266

Signed-off-by: Indranil Das Gupta <indradg@gmail.com>
Remarks: Works as per revised test plan
Comment 5 Jonathan Druart 2015-05-26 11:09:50 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2015-05-26 11:10:12 UTC
David, could you please have a look at my counter patch?
Comment 7 David Cook 2015-05-28 00:10:30 UTC
(In reply to Jonathan Druart from comment #6)
> David, could you please have a look at my counter patch?

Looks good to me :). I'm not attached to trim on the client side.

I'm not really working on Koha for the rest of the week, but I could test this next week if no one does before then.

Indranil, you could get another sign off in ;)
Comment 8 Indranil Das Gupta 2015-05-28 19:16:11 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2015-05-29 16:01:29 UTC
Created attachment 39701 [details] [review]
[PASSED QA] Bug 14266: Trim the email address in the pl script

The original concern of bug 14266 was to provide a compatibility for
<IE9.
But actually we don't need to trim the email address template side.
It will even better to trim it in the perl script, so that the email
will be trimed even if JS is disabled.

Test plan:
1/ Share a list and does not provide any email address
2/ Submit
=> The form is not submited, no alert/message is displayed (same as
before this patch).
3/ Share a list and provide an email address with spaces before and
after
4/ Submit
=> You should receive the email

Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com>
Test output compliant with expected test plan outcome.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Kyle M Hall 2015-05-29 16:05:56 UTC
Having trouble attaching first patch. Consider both to have passed qa. It just gets overwritten anyway.
Comment 11 Tomás Cohen Arazi 2015-06-01 17:20:19 UTC
Patches pushed to master.

Thankas David!
Comment 12 Chris Cormack 2015-06-03 08:03:22 UTC
Pushed to 3.20.x will be in 3.20.1
Comment 13 Liz Rea 2015-06-22 22:39:52 UTC
Patch pushed to 3.18.x will be in 3.18.08
Comment 14 Mason James 2015-07-25 09:52:32 UTC
Pushed to 3.16.x, will be in 3.16.13