Bug 10951 - Make the NoLoginInstructions a customizable field through preferences
Summary: Make the NoLoginInstructions a customizable field through preferences
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Blou
QA Contact: Jonathan Druart
URL:
Keywords:
: 6864 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-09-25 20:42 UTC by Blou
Modified: 2016-12-05 21:22 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Enhancement adding system preference NoLoginInstructions to be used when wrong credential are used in OPAC login (3.40 KB, patch)
2013-09-25 20:55 UTC, Blou
Details | Diff | Splinter Review
Bug 10951 - Adding NoLoginInstructions to system preferences (5.33 KB, patch)
2013-09-27 14:04 UTC, Blou
Details | Diff | Splinter Review
Second patch to the bug, contains sysprefs.sql (1.67 KB, patch)
2013-09-27 15:14 UTC, Blou
Details | Diff | Splinter Review
Bug 10951 - Second patch to the bug, contains sysprefs.sql (1.67 KB, patch)
2013-09-27 15:15 UTC, Blou
Details | Diff | Splinter Review
Adding NoLoginInstructions to system preferences (6.80 KB, patch)
2013-09-27 17:46 UTC, Blou
Details | Diff | Splinter Review
Adding NoLoginInstructions to system preferences (6.80 KB, patch)
2013-09-27 20:39 UTC, Blou
Details | Diff | Splinter Review
Neglected to escape the single quotes in the sql command. This corrects it. (1.73 KB, patch)
2013-10-04 12:35 UTC, Blou
Details | Diff | Splinter Review
Bug 10951 - Adding NoLoginInstructions to system preferences (6.83 KB, patch)
2013-10-04 14:55 UTC, Blou
Details | Diff | Splinter Review
Corrected entry which was on two lines. (1.80 KB, patch)
2013-10-04 15:15 UTC, Blou
Details | Diff | Splinter Review
Corrected entry which was on two lines. (1.80 KB, patch)
2013-10-04 15:48 UTC, Blou
Details | Diff | Splinter Review
changed escape \' to '' (and \? to ?) (1.79 KB, patch)
2013-10-04 15:49 UTC, Blou
Details | Diff | Splinter Review
Adding NoLoginInstructions to system preferences (6.79 KB, patch)
2013-10-04 18:04 UTC, Blou
Details | Diff | Splinter Review
Bug 10951 - Adding NoLoginInstructions to system preferences (6.79 KB, patch)
2013-10-04 19:37 UTC, Blou
Details | Diff | Splinter Review
[SIGNED OFF] Bug 10951 - Adding NoLoginInstructions to system preferences (6.81 KB, patch)
2013-10-04 19:46 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 10951 - Extra Patch to fix next sentence. (1.55 KB, patch)
2013-10-07 15:12 UTC, Blou
Details | Diff | Splinter Review
The change was causing issue with the next sentence. Adjusted the start (uppercase) and ending (dot) on self-registration invitation. (1.65 KB, patch)
2013-10-07 17:25 UTC, Mark Tompsett
Details | Diff | Splinter Review
[ SIGNED OFF] Bug 10951 - Tweaked opac-auth.tt sentence. (1.63 KB, patch)
2013-10-07 17:28 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 10951 - Adding NoLoginInstructions to system preferences (6.90 KB, patch)
2013-11-04 14:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10951 - Tweaked opac-auth.tt sentence. (1.67 KB, patch)
2013-11-04 14:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10951: The new pref NoLoginInstructions should be empty by default. (4.46 KB, patch)
2013-11-04 14:21 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 Blou 2013-09-25 20:42:20 UTC
On a failed login, the default message is harcorded into opac-auth.tt.

It would be preferable to allow for a preference to override that message (for example: ...Please bring an ID to the front desk located on 3rd floor...)

The changes modify
-opac-auth.tt to allow for custom value
-admin/preferences/opac.pref to add it to the preferences with a description
-C4/Auth.pm for the loading of the preference

TESTING
1) in OPAC, logged out, try login in by entering no or wrong credentials.  Acknowledge the "Don't have a passord yet?" message
2) Apply the patch
3) Regression Test: Redo step 1.  Same (default) message should appear.
4) Log in to intranet, 
   - select NoLoginInstructions in system preferences.
   - Enter new (xml) message.  Possible: 
     <h5>Welcome to Koha, please bring your passport to the front office</h5>
   - and save
5) refresh the OPAC, try login again with invalid credentials.  The new message should appear.
Comment 1 Blou 2013-09-25 20:55:58 UTC Comment hidden (obsolete)
Comment 2 Galen Charlton 2013-09-26 20:26:07 UTC
Thanks for the patch.  I should point out that nowadays it is very common for this sort of minor tweak to be handled using jQuery entered in (in this case) the OPACUserJS system preference.  Here is a specific example on the wiki for changing the login help, which accomplishes the intent of this patch without requiring that a new system preference be added.

http://wiki.koha-community.org/wiki/JQuery_Library#Change_Login_Help
Comment 3 Mark Tompsett 2013-09-26 20:46:43 UTC
See http://wiki.koha-community.org/wiki/System_Preferences for information on adding system preferences. You forgot to modify installer/data/mysql/updatedatabase.pl and installer/data/mysql/kohastructure.sql, but you did get the appropriate .pref file. :)
Comment 4 Robin Sheat 2013-09-26 22:55:03 UTC
I think having a syspref for this is better, as not everyone runs javascript. My only issue with the idea is that it makes it harder to be language aware.
Comment 5 Owen Leonard 2013-09-26 23:18:36 UTC
A more long-term and internationalization-friendly (and complicated) solution would be to build a generalized framework for entering multiple text blocks in a Koha News-style interface tied to available languages.
Comment 6 Katrin Fischer 2013-09-27 06:01:45 UTC
We've had a longer discussion about this on IRC last night. I agree with both Owen and Robin. We actually should find a better way than sysprefs - one that allows you to offer the ocntent in multiple languages. But as long as we don't have that this system preference could be really helpful. I know a lot of people who turn off Javascript on websites - a system preference allows to always display a text and you can toggle it with jQuery between languages then (I am using different div blocks with a language class). It's not perfect, but a little better.
Comment 7 Blou 2013-09-27 14:04:12 UTC Comment hidden (obsolete)
Comment 8 Mark Tompsett 2013-09-27 14:12:22 UTC
Thank you for noting the updatedatabase.pl missing part, but you still forgot installer/data/mysql/kohastructure.sql which is important for fresh installs.
Comment 9 Katrin Fischer 2013-09-27 14:16:55 UTC
I think the file Mark means is syspref.sql - if you had changed the database structure it would go in kohastructure.sql, but for a system preference we only need an entry in sysprefs.sql :)
Comment 10 Blou 2013-09-27 15:14:20 UTC Comment hidden (obsolete)
Comment 11 Blou 2013-09-27 15:15:33 UTC Comment hidden (obsolete)
Comment 12 Blou 2013-09-27 15:19:22 UTC
Live and learn.  I now have to do that for my other patches.

If there's a standard/preference when naming a second patch, please let me know.
git-bz throws me error so I'm doing it all manually.
Comment 13 Blou 2013-09-27 17:46:29 UTC Comment hidden (obsolete)
Comment 14 Blou 2013-09-27 20:39:09 UTC Comment hidden (obsolete)
Comment 15 Mark Tompsett 2013-09-28 02:52:09 UTC
This looks good, Blou, but I haven't tested it. I just eyeballed it. I should be able to sign this off pretty quickly later. :)
Comment 16 I'm just a bot 2013-09-29 04:44:35 UTC
Patch applied cleanly, go forth and signoff
Comment 17 Mark Tompsett 2013-10-04 00:01:09 UTC
Comment on attachment 21524 [details] [review]
Adding NoLoginInstructions to system preferences

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

::: installer/data/mysql/updatedatabase.pl
@@ +7157,5 @@
>  
> +$DBversion ="3.13.00.XXX";
> +if ( CheckVersion($DBversion) ) {
> +    $dbh->do("INSERT IGNORE INTO systempreferences (variable,value,options,explanation,type) 
> +    VALUES ('NoLoginInstructions', '<h5>Don't have a password yet?</h5> <p> If you don't have a password yet, stop by the circulation desk the next time you're in the library. We'll happily set one up for you.</p> <h5>Don't have a library card?</h5> <p> If you don't have a library card, stop by your local library to sign up.  </p>', '60|10', 'Instructions when a person is not connected', 'Textarea')");

You forgot to escape the single quotes in the message and you then typed contractions like "Don't" and "We'll" in the message. Escape the single quotes, and this works fine.
Comment 18 Blou 2013-10-04 12:35:57 UTC Comment hidden (obsolete)
Comment 19 Blou 2013-10-04 12:38:25 UTC
Thank you!  I can't believe i had done the escaping for its sister file (sysprefs.sql) and not this one.

This new patch patches the big previous patch.

My apologies.  I'll be more careful next time.

> You forgot to escape the single quotes in the message and you then typed
> contractions like "Don't" and "We'll" in the message. Escape the single
> quotes, and this works fine.
Comment 20 Mark Tompsett 2013-10-04 13:12:07 UTC
Oops. I forgot to mention escaping the question mark too. :) Could you patch that up too?
Comment 21 Mark Tompsett 2013-10-04 13:34:45 UTC
Comment on attachment 21524 [details] [review]
Adding NoLoginInstructions to system preferences

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

::: installer/data/mysql/sysprefs.sql
@@ +418,5 @@
>  ('XSLTResultsDisplay','default','','Enable XSL stylesheet control over results page display on intranet','Free'),
>  ('yuipath','local','local|http://yui.yahooapis.com/2.5.1/build','Insert the path to YUI libraries, choose local if you use koha offline','Choice'),
>  ('z3950AuthorAuthFields','701,702,700',NULL,'Define the MARC biblio fields for Personal Name Authorities to fill biblio.author','free'),
> +('z3950NormalizeAuthor','0','','If ON, Personal Name Authorities will replace authors in biblio.author','YesNo'),
> +('NoLoginInstructions', '<h5>Don\'t have a password yet?</h5> <p> If you don\'t have a password yet, stop by the circulation desk the next time you\'re in the library. We\'ll happily set one up for you.</p> <h5>Don\'t have a library card?</h5> <p> If you don\'t have a library card, stop by your local library to sign up.  </p>', '60|10', 'Instructions when a person is not connected', 'Textarea')

This should probably be in the alphabetical order location. This question mark didn't need escaping, because it isn't wrapped in perl code. :)
Comment 22 Blou 2013-10-04 14:55:08 UTC Comment hidden (obsolete)
Comment 23 Mark Tompsett 2013-10-04 14:58:59 UTC
Comment on attachment 21524 [details] [review]
Adding NoLoginInstructions to system preferences

Last patch posted replaces this.
Comment 24 Blou 2013-10-04 15:15:56 UTC Comment hidden (obsolete)
Comment 25 Blou 2013-10-04 15:48:56 UTC Comment hidden (obsolete)
Comment 26 Blou 2013-10-04 15:49:57 UTC Comment hidden (obsolete)
Comment 27 Blou 2013-10-04 18:04:00 UTC Comment hidden (obsolete)
Comment 28 Blou 2013-10-04 19:37:14 UTC Comment hidden (obsolete)
Comment 29 Mark Tompsett 2013-10-04 19:38:00 UTC
Comment on attachment 21814 [details] [review]
Adding NoLoginInstructions to system preferences

This is obsolete.
Comment 30 Mark Tompsett 2013-10-04 19:46:37 UTC Comment hidden (obsolete)
Comment 31 Mark Tompsett 2013-10-04 19:47:24 UTC
Comment on attachment 21815 [details] [review]
Bug 10951 - Adding NoLoginInstructions to system preferences

Obsolete (unsigned) version.
Comment 32 Mark Tompsett 2013-10-04 19:50:19 UTC
test plan:

- drop kohadatabase, create blank one
- go to staff client and do web install
- confirm the nologin-ish system preference was created.
- restore only koha database
- go to staff client and do web upgrade
- confirm the nologin-ish system preference was created.

play with values of the system preference
- does the message appear on failed log in? YES.
- blank the system preference, refresh OPAC, gives old default message? YES.
- set system preference to 'blah', refresh OPAC, gives 'blah'? YES.

Looks great! Signed off. :)
Comment 33 Blou 2013-10-07 15:12:29 UTC Comment hidden (obsolete)
Comment 34 Mark Tompsett 2013-10-07 17:25:42 UTC Comment hidden (obsolete)
Comment 35 Mark Tompsett 2013-10-07 17:28:03 UTC Comment hidden (obsolete)
Comment 36 Mark Tompsett 2013-10-07 17:35:54 UTC
The patch was such a minor tweak. Unless someone wants to complain about a few pixels shifting up between the default message and the one that is preloaded, I see no reason for QA to fail this. Though, optimization-wise, perhaps a blank setting and not filling in the system preference is better? It would certainly have saved us the escaping headache. :)
Comment 37 Blou 2013-10-07 17:54:05 UTC
Well, not sure I should say that, but the patch would have worked perfectly without messing the preference, indeed.  There's a default value in the .tt that would have done the job.

I can't tell you how we got to this.  But if I can ship you some Tylenol (for the headache I caused), I'll be happy to do.

I made sure to reuse the lessons on my other patches.  They should be a lot easier on the signee, I promess.

Thanks again and again!
Comment 38 Jonathan Druart 2013-11-04 14:19:24 UTC
QA comment:

The syspref makes sense for me. It works as described.
The pref should not be filled (syspref.sql and updatedb): it will be displayed in English for everybody. Since a default message is defined in the template, the pref value should be empty.
I will provide a followup for that and for 4 tab characters.

Marked as Passed QA.
Comment 39 Jonathan Druart 2013-11-04 14:21:05 UTC
Created attachment 22711 [details] [review]
Bug 10951 - Adding NoLoginInstructions to system preferences

On a failed login, the default message is harcorded into opac-auth.tt.

     It would be preferable to allow for a preference to override that message (for example: ...Please bring an ID to t
     The changes modify
         -opac-auth.tt to allow for custom value
         -admin/preferences/opac.pref to add it to the preferences with a description
         -C4/Auth.pm for the loading of the preference
         -sysprefs.sql
         -updatedatabase.pl

     TESTING
         1) in OPAC, logged out, try login in by entering no or wrong credentials.  Acknowledge the "Don't have a p
         2) Apply the patch
         3) Regression Test: Redo step 1.  Same (default) message should appear.
         4) Log in to intranet,
             - select NoLoginInstructions in system preferences.
             - Enter new (xml) message.  Possible:
             <h5>Welcome to Koha, please bring your passport to the front office</h5>
            - and save
         5) refresh the OPAC, try login again with invalid credentials.  The new message should appear.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 40 Jonathan Druart 2013-11-04 14:21:13 UTC
Created attachment 22712 [details] [review]
Bug 10951 - Tweaked opac-auth.tt sentence.

The change was causing issue with the next sentence. Adjusted
the start (uppercase) and ending (dot) on self-registration
invitation.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 41 Jonathan Druart 2013-11-04 14:21:18 UTC
Created attachment 22713 [details] [review]
Bug 10951: The new pref NoLoginInstructions should be empty by default.

We don't want to display an English message by default for everybody.
The default message is in the template, so the pref should be empty.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 42 Jonathan Druart 2013-11-04 14:22:07 UTC
Note for RM: maybe "patron" is better than "person" in the pref description.
Comment 43 Galen Charlton 2014-04-19 17:08:31 UTC
Pushed to master, along with various follow-ups, including one adding support for the Bootstrap theme.

Thanks, Phillipe!
Comment 44 Fridolin Somers 2014-07-11 07:16:28 UTC
Pushed to 3.14.x, will be in 3.14.09

(very nice, thanks all)
Comment 45 Fridolin Somers 2015-08-11 07:59:28 UTC
*** Bug 6864 has been marked as a duplicate of this bug. ***