Description
Andreas Jonsson
2021-11-15 11:44:20 UTC
Created attachment 127636 [details] [review] Bug 29480: Add new systempreference PatronSelfRegistrationUseridGenerationMethod and add email as option. Test plan - Install patch and run koha-upgrade-schema <instance> - The systempreference PatronSelfRegistrationUseridGenerationMethod should be found in the adminstrative interface under OPAC -> Self registration and modification - Also make sure - Make sure that PatronSelfRegistration is set to Allow and PatronSelfRegistrationDefaultCategory is set to an existing borrower category - Make sure that PatronSelfModificationBorrowerUnwantedField doesn't prevent the input of an email address in the self registration form. (I.e., make sure "email" is NOT selected for exclusion.) - Go to opac and self register a patron and verify that the userid is on the form firstname.surname. - Change systempreference to 'email' - Go to OPAC and self register a patron and make sure to add a valid email address. - Verify the that the userid is the email adress. - Go to OPAC and self register a patron using the SAME valid email adress as above (to provoke a conflicting userid). - Verify that the userid is on the form firstname.surname. Andreas, can you set yourself as assignee? I might be wrong but I think sysprefs.sql needs updating too. Created attachment 127679 [details] [review] Bug 29480: Add new systempreference PatronSelfRegistrationUseridGenerationMethod and add email as option. Test plan - Install patch and run koha-upgrade-schema <instance> - The systempreference PatronSelfRegistrationUseridGenerationMethod should be found in the adminstrative interface under OPAC -> Self registration and modification - Also make sure - Make sure that PatronSelfRegistration is set to Allow and PatronSelfRegistrationDefaultCategory is set to an existing borrower category - Make sure that PatronSelfModificationBorrowerUnwantedField doesn't prevent the input of an email address in the self registration form. (I.e., make sure "email" is NOT selected for exclusion.) - Go to opac and self register a patron and verify that the userid is on the form firstname.surname. - Change systempreference to 'email' - Go to OPAC and self register a patron and make sure to add a valid email address. - Verify the that the userid is the email adress. - Go to OPAC and self register a patron using the SAME valid email adress as above (to provoke a conflicting userid). - Verify that the userid is on the form firstname.surname. Yes, I had missed to add the system preference to sysprefs.sql. Thank you David for pointing this out! Please provide tests. You also need to rename 'borrower' with 'patron'. The POD for generate_userid() should be updated. Created attachment 142133 [details] [review] Bug 29480: Add new systempreference PatronSelfRegistrationUseridGenerationMethod and add email as option. Test plan - Install patch and run koha-upgrade-schema <instance> - The systempreference PatronSelfRegistrationUseridGenerationMethod should be found in the adminstrative interface under OPAC -> Self registration and modification - Also make sure - Make sure that PatronSelfRegistration is set to Allow and PatronSelfRegistrationDefaultCategory is set to an existing borrower category - Make sure that PatronSelfModificationBorrowerUnwantedField doesn't prevent the input of an email address in the self registration form. (I.e., make sure "email" is NOT selected for exclusion.) - Go to opac and self register a patron and verify that the userid is on the form firstname.surname. - Change systempreference to 'email' - Go to OPAC and self register a patron and make sure to add a valid email address. - Verify the that the userid is the email adress. - Go to OPAC and self register a patron using the SAME valid email adress as above (to provoke a conflicting userid). - Verify that the userid is on the form firstname.surname. Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised. I will provide followups for the issues that have been pointed out. Created attachment 142134 [details] [review] Bug 29480: (follow up) Use "patron", not "borrower" Also changes "user name" to "userid". Created attachment 142135 [details] [review] Bug 29480: (follow up) Add tests to subtest generate_userid Created attachment 142136 [details] [review] Bug 29480: (follow up) Update POD for generate_userid() Add an explanation of how PatronSelfRegistrationUseridGenerationMethod will affect behaviour. Please use the new style for an atomic update: use Modern::Perl; return { bug_number => "BUG_NUMBER", description => "A single line description", up => sub { my ($args) = @_; my ($dbh, $out) = @$args{qw(dbh out)}; # Do you stuffs here $dbh->do(q{}); # Print useful stuff here say $out "Update is going well so far"; }, }; See skeleton file in the atomicupdate folder. Functional question about: +If PatronSelfRegistrationUseridGenerationMethod is set to 'email', the email +address will be used as the userid. If the patron being created has no email, +or the patron has an email address that is already in use by another patron, +the default way of creating the userid ($firstname.$surname) will be used. Is that really what you want? If my email address is already in the database, I probably dont want another account. One of the issues we face is actually that impatient people register 2 or 3 accounts. There is a security aspect too. You do not want to offer a way to check if an email address is present. So it might be safer to send a verification mail to the email address specified that either gives you a link to proceed or tells you that there is an account already. The OPAC could just say: check your mail, and not reveal the information. What do you think? (In reply to Marcel de Rooy from comment #13) > Functional question about: > > +If PatronSelfRegistrationUseridGenerationMethod is set to 'email', the email > +address will be used as the userid. If the patron being created has no > email, > +or the patron has an email address that is already in use by another patron, > +the default way of creating the userid ($firstname.$surname) will be used. > > Is that really what you want? If my email address is already in the > database, I probably dont want another account. > One of the issues we face is actually that impatient people register 2 or 3 > accounts. We see this for families where the email address of the parents is used in the children's accounts as well. Also sometimes for internal accounts where a library address might be used several times. Some academic libraries also do separate accounts for 'permanent loans'. Maybe display a warning or so instead of not allowing it? (In reply to Katrin Fischer from comment #14) > (In reply to Marcel de Rooy from comment #13) > > Functional question about: > > > > +If PatronSelfRegistrationUseridGenerationMethod is set to 'email', the email > > +address will be used as the userid. If the patron being created has no > > email, > > +or the patron has an email address that is already in use by another patron, > > +the default way of creating the userid ($firstname.$surname) will be used. > > > > Is that really what you want? If my email address is already in the > > database, I probably dont want another account. > > One of the issues we face is actually that impatient people register 2 or 3 > > accounts. > > We see this for families where the email address of the parents is used in > the children's accounts as well. Also sometimes for internal accounts where > a library address might be used several times. Some academic libraries also > do separate accounts for 'permanent loans'. > > Maybe display a warning or so instead of not allowing it? Maybe a lax or strict mode or so? Obviously you dont need to use email as userid, but if you do, it might be nice to choose between fallback to name based userid or not allowing it.. *** Bug 2237 has been marked as a duplicate of this bug. *** On bug 32426 I will give the plugin approach a try in getting this long standing enh req further. (In reply to Marcel de Rooy from comment #17) > On bug 32426 I will give the plugin approach a try in getting this long > standing enh req further. Submitted some patches on that report now. Please note that the plugin approach on bug 32426 has been pushed now. This allows you to move your code to a Koha plugin. |