Bug 12663 - SCOUserCSS and SCOUserJS ignored on selfcheck login page
Summary: SCOUserCSS and SCOUserJS ignored on selfcheck login page
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Self checkout (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-07-25 12:00 UTC by Nicole C. Engard
Modified: 2017-12-07 22:16 UTC (History)
10 users (show)

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


Attachments
Bug 12663 - SCOUserCSS and SCOUserJS ignored on selfcheck login page (18.06 KB, patch)
2016-01-15 21:05 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 12663 - SCOUserCSS and SCOUserJS ignored on selfcheck login page (17.53 KB, patch)
2016-01-25 16:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 12663 - Prevent OPACUserJS and OPACUserCSS from loading (1.76 KB, patch)
2016-01-26 19:44 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12663 - SCOUserCSS and SCOUserJS ignored on selfcheck login page (17.59 KB, patch)
2016-01-26 20:25 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12663 - Prevent OPACUserJS and OPACUserCSS from loading (1.82 KB, patch)
2016-01-26 20:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 12663 - SCOUserCSS and SCOUserJS ignored on selfcheck login page (5.75 KB, patch)
2016-03-08 22:08 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 12663 - SCOUserCSS and SCOUserJS ignored on selfcheck login page (6.16 KB, patch)
2016-03-25 17:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 12663 - SCOUserCSS and SCOUserJS ignored on selfcheck login page (6.45 KB, patch)
2016-03-26 08:52 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12663: (QA followup) (2.38 KB, patch)
2016-03-31 19:50 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 12663 - SCOUserCSS and SCOUserJS ignored on selfcheck login page (6.50 KB, patch)
2016-04-04 15:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 12663: (QA followup) (2.38 KB, patch)
2016-04-04 15:29 UTC, Nick Clemens
Details | Diff | Splinter Review
[PASSED QA] Bug 12663 - SCOUserCSS and SCOUserJS ignored on selfcheck login page (6.57 KB, patch)
2016-04-04 21:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 12663: (QA followup) (2.45 KB, patch)
2016-04-04 21:23 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2014-07-25 12:00:55 UTC
The two preferences: SCOUserCSS and SCOUserJS --- Do not apply to the selfcheck login page.

To reproduce:
* Make changes using above prefs
* GOTO selfcheck module:
  http://YOURCATALOG/cgi-bin/koha/sco/sco-main.pl

If you have not logged in, your changes will not be visible.
Comment 1 Nick Clemens 2016-01-15 21:05:58 UTC Comment hidden (obsolete)
Comment 2 M. Tompsett 2016-01-23 19:36:31 UTC
Comment on attachment 46736 [details] [review]
Bug 12663 - SCOUserCSS and SCOUserJS ignored on selfcheck login page

Review of attachment 46736 [details] [review]:
-----------------------------------------------------------------

::: C4/Auth.pm
@@ +1250,5 @@
>          PatronSelfRegistrationDefaultCategory => C4::Context->preference("PatronSelfRegistrationDefaultCategory"),
>          persona                               => C4::Context->preference("Persona"),
>          opac_css_override                     => $ENV{'OPAC_CSS_OVERRIDE'},
> +        SCOUserCSS                            => C4::Context->preference("SCOUserCSS"),
> +        SCOUserJS                             => C4::Context->preference("SCOUserJS"),

There is no need to pass system preferences. Use Koha.Preference calls in the Template Toolkit files.
Comment 3 Nick Clemens 2016-01-25 16:45:16 UTC Comment hidden (obsolete)
Comment 4 Nick Clemens 2016-01-25 16:47:25 UTC
(In reply to M. Tompsett from comment #2)
> There is no need to pass system preferences. Use Koha.Preference calls in
> the Template Toolkit files.
Fixed the calls I added, thanks Mark!
Comment 5 Nick Clemens 2016-01-26 19:44:08 UTC Comment hidden (obsolete)
Comment 6 Nick Clemens 2016-01-26 20:25:34 UTC Comment hidden (obsolete)
Comment 7 Nick Clemens 2016-01-26 20:25:41 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2016-01-27 12:34:07 UTC
Heather, did you win the bonus points?
Comment 9 Jonathan Druart 2016-02-09 09:04:36 UTC
Heather, Have you tested the CAS and Shibboleth authentication methods?
Comment 10 Nick Clemens 2016-02-09 12:59:15 UTC
I don't believe so as they don't use either at NEKLS
Comment 11 Jonathan Druart 2016-03-07 10:23:37 UTC
I don't like the new "type" these patches introduced, and I don't understand the complexity of the patches:
The SCO user should only be used for the SCO module. From this assumption why don't you just test if the logged in user is the one defined in the  AutoSelfCheckID  pref?
I may missed something and it could be a naive approach, but so easy :)
Comment 12 HB-NEKLS 2016-03-07 13:51:07 UTC
Jonathan, we're needing to use Koha's Self Check at multiple branches, so have to use multiple logins; we can't save those to the AutoSelfCheckID sys pef as a result. Hence the development need. I'll can't answer the complexity question :) I'll let Nick do that.
Comment 13 Jonathan Druart 2016-03-08 08:23:54 UTC
(In reply to Heather Braum from comment #12)
> Jonathan, we're needing to use Koha's Self Check at multiple branches, so
> have to use multiple logins; we can't save those to the AutoSelfCheckID sys
> pef as a result. Hence the development need. I'll can't answer the
> complexity question :) I'll let Nick do that.

You are overriding the pref values
Comment 14 Jonathan Druart 2016-03-08 08:25:47 UTC
(In reply to Jonathan Druart from comment #13)
> (In reply to Heather Braum from comment #12)
> > Jonathan, we're needing to use Koha's Self Check at multiple branches, so
> > have to use multiple logins; we can't save those to the AutoSelfCheckID sys
> > pef as a result. Hence the development need. I'll can't answer the
> > complexity question :) I'll let Nick do that.

(previous comment left too fast!)
You are overriding the pref values using the ENV var, that's what you mean?
If yes, it should not change anything to what I suggested :)
Comment 15 Nick Clemens 2016-03-08 16:15:25 UTC
You are correct Joubu, there are easier methods, patch to follow later
Comment 16 Nick Clemens 2016-03-08 22:08:50 UTC Comment hidden (obsolete)
Comment 17 Marc Véron 2016-03-24 10:18:31 UTC
I can not reproduce the issue on current master.
Comment 18 Owen Leonard 2016-03-24 13:49:20 UTC
(In reply to Marc Véron from comment #17)
> I can not reproduce the issue on current master.

I though I couldn't either, but I figured it out: This bug is about the login page which is shown when the administrative user signs in, not the individual self check user.

So if you have AutoSelfCheckAllowed set to "Don't allow," a staff user must sign in at the self check machine before patrons can use it. It's this login page (the regular OPAC's login form) which is affected by this patch. This detail is missing from the test plan.

As far as SCOUserJS and SCOUserCSS are concerned this patch works as described. However patron logins to self check are now broken. I can sign in successfully as the administrative user, but when I sign in as the patron I'm redirected to the administrative sign-in page.
Comment 19 Nick Clemens 2016-03-25 17:44:29 UTC Comment hidden (obsolete)
Comment 20 Marc Véron 2016-03-26 08:52:01 UTC Comment hidden (obsolete)
Comment 21 Marc Véron 2016-03-26 08:53:58 UTC
Oh, it helps to read the bug tile, of course it is intended  :-)
Comment 22 Katrin Fischer 2016-03-29 22:17:39 UTC
I am not happy about the change of behaviour here. 

By using an if-else-construct where OPACUserCSS and OpacUserJS are in the else branch this removes all the styling done to the login page per these preferences. As the SCO login page looks like a normal OPAC page, this doesn't feel right.
Comment 23 Nick Clemens 2016-03-30 21:11:54 UTC
(In reply to Katrin Fischer from comment #22)
> By using an if-else-construct where OPACUserCSS and OpacUserJS are in the
> else branch this removes all the styling done to the login page per these
> preferences. As the SCO login page looks like a normal OPAC page, this
> doesn't feel right.

The intention here is to allow the library to make this page look different, removing the css and js that is present on the opac is intended to help that.

My thought would be to add another IF/ELSE block and if this is an SCO login change "Log in to your account" to 'Staff log in to self checkout station" and maybe a link back to the opac login. Would that work for you?
Comment 24 Katrin Fischer 2016-03-30 21:28:48 UTC
We can get more opinions on this - but maybe the page needs a rework? It doesn't appear as part of the self check sites at the moment, so that's why I am not sure about changing the current behavour. It shows the header and footer as all of the normal OPAC sites.

If the SCOUserCSS is after the normal OpacUSerCSS you will still be able to overwrite everything that you don't like, without introducing a change in behaviour. Could that work?
Comment 25 Nick Clemens 2016-03-31 19:50:15 UTC Comment hidden (obsolete)
Comment 26 Katrin Fischer 2016-04-03 13:09:39 UTC
Please rebase!

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 12663 - SCOUserCSS and SCOUserJS ignored on selfcheck login page
Using index info to reconstruct a base tree...
M	C4/Auth.pm
M	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth.tt
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth.tt
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth.tt
Auto-merging C4/Auth.pm
Failed to merge in the changes.
Comment 27 Nick Clemens 2016-04-04 15:29:24 UTC Comment hidden (obsolete)
Comment 28 Nick Clemens 2016-04-04 15:29:29 UTC Comment hidden (obsolete)
Comment 29 Katrin Fischer 2016-04-04 21:22:20 UTC
Created attachment 49879 [details] [review]
[PASSED QA] Bug 12663 - SCOUserCSS and SCOUserJS ignored on selfcheck login page

Currently if not logged in when browsing to
http://YOURCATALOG/cgi-bin/koha/sco/sco-main.pl
You are redirected to opac-auth.tt and SCOUserCSS and SCOUserJS are not
loaded. This page passes through a parameter to the template to indicate
this is an SCO login and appropriate CSS and JS should be loaded.

Additionally this patch ensure that when loggin in using the form you
are redirected to the sco-main.pl instead of the patron account page for
the user.

To test:
1 - Verify that normal login works on both staff and opac
2 - Verify that SCO link goes to login page if AutoSelfCheckAllowed is
set to "Don't allow"
3 - Enter changes into SCOUserJS and SCOUserCSS and observe these are
present on SCO log in page with AutoSelfCheck disabled
4 - Verify that a logged in opac user without permissions cannot access
the self-checkout module
5 - Verify that AutoSelfCheckAllowed and associated system preferences
function as expected
6 - Verify the AutoSelfCheck user is logged out if they attempt to visit
another page

Followed test plan.
If I go to http://YOURCATALOG/cgi-bin/koha/sco/sco-main.pl, CSS and JS trigger already on
the login form, I suppose that is intended.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 30 Katrin Fischer 2016-04-04 21:23:13 UTC
Created attachment 49880 [details] [review]
[PASSED QA] Bug 12663: (QA followup)

    Insert SCOUserCSS/JS 'after' OPACUserCSS/JS rather than 'instead of'
    i.e. Remove IF/ELSE and use 2 IF

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 31 Brendan Gallagher 2016-04-22 00:58:19 UTC
Pushed to Master - Should be in the May 2016 release!  Thanks!
Comment 32 Julian Maurice 2016-05-09 08:40:04 UTC
Patch pushed to 3.22.x, will be in 3.22.7