Bug 24803 - Clicking "Log in to your account" throws fatal Javascript error
Summary: Clicking "Log in to your account" throws fatal Javascript error
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low blocker (vote)
Assignee: David Cook
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-03-04 23:29 UTC by David Cook
Modified: 2020-04-09 19:57 UTC (History)
6 users (show)

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


Attachments
Bug 24803: Remove JS error by following Bootstrap instructions (2.58 KB, patch)
2020-03-05 00:04 UTC, David Cook
Details | Diff | Splinter Review
Bug 24803: [Alternate patch] bypass Bootstrap click handler by using "#" (1.97 KB, patch)
2020-03-05 00:04 UTC, David Cook
Details | Diff | Splinter Review
Bug 24803: Remove JS error by following Bootstrap instructions (2.69 KB, patch)
2020-03-05 00:55 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 24803: [Alternate patch] bypass Bootstrap click handler by using "#" (2.08 KB, patch)
2020-03-05 00:58 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 24803: [Alternate] Clicking "Log in to your account" throws fatal Javascript error (2.05 KB, patch)
2020-03-05 13:08 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24803: [Alternate] Clicking "Log in to your account" throws fatal Javascript error (2.10 KB, patch)
2020-03-05 23:46 UTC, David Cook
Details | Diff | Splinter Review
Bug 24803: [PASSED_QA] Clicking "Log in to your account" throws fatal Javascript error (2.20 KB, patch)
2020-03-06 06:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24803: Remove JS error when clicking "Log in to your account" (2.21 KB, patch)
2020-03-06 08:54 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 David Cook 2020-03-04 23:29:48 UTC
Clicking "Log in to your account" throws a fatal Javascript error, because the link has a href of "/cgi-bin/koha/opac-user.pl" and Bootstrap passes the href to Jquery as if it were a CSS ID selector.

In the past, Jquery didn't throw a fatal error for this mistake, so it was fine. But now Jquery appears to be stricter with the upgrade to 3.4.1.

This error causes Javascript to fail and could lead to unexpected behaviour. 

(I have an idea for a patch that I'll be trying out very soon.)
Comment 1 David Cook 2020-03-04 23:47:29 UTC
My first idea is to replace "/cgi-bin/koha/opac-user.pl" with #loginModal (as per https://www.w3schools.com/Bootstrap/bootstrap_ref_js_modal.asp), and that works (after disabling our Koha click handler for showing/hiding the modal).

But... now I'm wondering if we had the "href" as "/cgi-bin/koha/opac-user.pl" for a reason. Maybe so the link would still work if Javascript wasn't working?

So I'm going to work on an alternative idea suggested by Jonathan too...
Comment 2 David Cook 2020-03-04 23:49:46 UTC
(In reply to David Cook from comment #1)
> So I'm going to work on an alternative idea suggested by Jonathan too...

Oh except the alternative idea won't retain a usable link either... well I might provide it anyway, and let the community decide which they prefer.
Comment 3 David Cook 2020-03-05 00:04:25 UTC Comment hidden (obsolete)
Comment 4 David Cook 2020-03-05 00:04:44 UTC Comment hidden (obsolete)
Comment 5 Bernardo Gonzalez Kriegel 2020-03-05 00:55:54 UTC
Created attachment 100159 [details] [review]
Bug 24803: Remove JS error by following Bootstrap instructions

This patch patches #lodinModal to Bootstrap's click.modal.data-api event handler
for the '[data-toggle="modal"] selector, so Bootstrap can handle the
hide/show behaviour of the login modal.

Test plan:
1. Apply patch
2. Open F12 dev tools
3. Click "Log in to your account" on OPAC
4. Observe no Javascript errors in console

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
JS error verified, and corrected. No qa errors.
Comment 6 Bernardo Gonzalez Kriegel 2020-03-05 00:58:09 UTC
Created attachment 100160 [details] [review]
Bug 24803: [Alternate patch] bypass Bootstrap click handler by using "#"

This patch bypasses Bootstrap's click.modal.data-api event handler
for the '[data-toggle="modal"] selector by using a "#" as the "href"

Test plan:
1. Apply patch
2. Open F12 dev tools
3. Click "Log in to your account" on OPAC
4. Observe no Javascript errors in console

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
JS error verified, and corrected. No qa errors.
Comment 7 Bernardo Gonzalez Kriegel 2020-03-05 00:59:09 UTC
(In reply to David Cook from comment #2)
> (In reply to David Cook from comment #1)
> > So I'm going to work on an alternative idea suggested by Jonathan too...
> 
> Oh except the alternative idea won't retain a usable link either... well I
> might provide it anyway, and let the community decide which they prefer.

Both works :)
Comment 8 David Cook 2020-03-05 02:01:55 UTC
Thanks, Bernardo!

I'm curious to see what QA and RM think about the best approach.
Comment 9 Owen Leonard 2020-03-05 13:08:10 UTC
Created attachment 100191 [details] [review]
Bug 24803: [Alternate] Clicking "Log in to your account" throws fatal Javascript error

This patch removes the "data-toggle" attribute from the login link in
order to avoid a JavaScript error. Since we have a custom click handler
to trigger the modal we don't need Bootstrap's "automatic" modal
handling.

Test plan:
1. Apply patch
2. Open F12 dev tools
3. Click "Log in to your account" on OPAC
4. Observe no Javascript errors in console
Comment 10 David Cook 2020-03-05 23:46:09 UTC
Created attachment 100208 [details] [review]
Bug 24803: [Alternate] Clicking "Log in to your account" throws fatal Javascript error

This patch removes the "data-toggle" attribute from the login link in
order to avoid a JavaScript error. Since we have a custom click handler
to trigger the modal we don't need Bootstrap's "automatic" modal
handling.

Test plan:
1. Apply patch
2. Open F12 dev tools
3. Click "Log in to your account" on OPAC
4. Observe no Javascript errors in console

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 11 David Cook 2020-03-05 23:47:52 UTC
Nice one, Owen. 

I think I prefer Owen's patch best, since it keeps a usable link.

But will leave it up to QA/RM.

(Also, as Jonathan noted on koha-devel, GoogleOpenIDConnect will be affected by the same issue, so whatever approach we choose here should be applied there as well.)
Comment 12 Marcel de Rooy 2020-03-06 06:57:33 UTC
Created attachment 100209 [details] [review]
Bug 24803: [PASSED_QA] Clicking "Log in to your account" throws fatal Javascript error

This patch removes the "data-toggle" attribute from the login link in
order to avoid a JavaScript error. Since we have a custom click handler
to trigger the modal we don't need Bootstrap's "automatic" modal
handling.

Test plan:
1. Apply patch
2. Open F12 dev tools
3. Click "Log in to your account" on OPAC
4. Observe no Javascript errors in console

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2020-03-06 06:58:42 UTC
RM: I prefer Owen's patch too, marked as PASSED_QA. If you agree, obsolete the other two.
Comment 14 Jonathan Druart 2020-03-06 08:54:37 UTC
Created attachment 100217 [details] [review]
Bug 24803: Remove JS error when clicking "Log in to your account"

This patch removes the "data-toggle" attribute from the login link in
order to avoid a JavaScript error. Since we have a custom click handler
to trigger the modal we don't need Bootstrap's "automatic" modal
handling.

Test plan:
1. Apply patch
2. Open F12 dev tools
3. Click "Log in to your account" on OPAC
4. Observe no Javascript errors in console

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Jonathan Druart 2020-03-06 08:55:40 UTC
(In reply to Marcel de Rooy from comment #13)
> RM: I prefer Owen's patch too, marked as PASSED_QA. If you agree, obsolete
> the other two.

I am not RM, but everybody prefers Owen's patch ;)
I planned to QA those patches this morning, done!
Comment 16 Martin Renvoize 2020-03-06 10:05:14 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 17 Joy Nelson 2020-04-01 22:25:44 UTC
Backported to 19.11.x for 19.11.05
Comment 18 Lucas Gass 2020-04-09 19:57:09 UTC
cannot recreate in 19.05.x, no backport