When entering an email (or arguably, a userid) to recover the password, a user should not have to know or remember the casing of the given email. Quick exercise for a quick solution led to no results. $search_results = [ Koha::Patrons->search( { -or => { email => $email, emailpro => $email, B_email => $email } } ) ]; Using search_like is not available, and using email => { 'like', $email} does not lead to the intended result. Here's hoping a similarly simple fix exists.
Created attachment 79037 [details] [review] Bug 20023: Password recovery using email should be case insensitive When entering an email to recover the password, a user should not have to know or remember the casing of the given email. Test 0) enable OpacResetPassword 1) On the OPAC, click "Forgot your password" 2) Enter your email address as in your user account BUT WITH A DIFFERENT CASING 3) Submit. This will fail. 4) Apply the patch, redo with success.
I added a small modif to clean some warnings in the logs, while at it.
Created attachment 79070 [details] Internal server error at opac-password-recovery.pl after applying patch After applying the patch, opac-password-recovery.pl resolves at a screen complaining of an internal server error. Restarting plack, apache and memcached does not resolve this.
Created attachment 79086 [details] [review] Bug 20023: Password recovery using email should be case insensitive When entering an email to recover the password, a user should not have to know or remember the casing of the given email. Test 0) enable OpacResetPassword 1) On the OPAC, click "Forgot your password" 2) Enter your email address as in your user account BUT WITH A DIFFERENT CASING 3) Submit. This will fail. 4) Apply the patch, redo with success.
Sorry about that. :( Now fixed.
Created attachment 79125 [details] [review] Bug 20023: Password recovery using email should be case insensitive When entering an email to recover the password, a user should not have to know or remember the casing of the given email. Test 0) enable OpacResetPassword 1) On the OPAC, click "Forgot your password" 2) Enter your email address as in your user account BUT WITH A DIFFERENT CASING 3) Submit. This will fail. 4) Apply the patch, redo with success. Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 79201 [details] [review] Bug 20023: Password recovery using email should be case insensitive When entering an email to recover the password, a user should not have to know or remember the casing of the given email. Test 0) enable OpacResetPassword 1) On the OPAC, click "Forgot your password" 2) Enter your email address as in your user account BUT WITH A DIFFERENT CASING 3) Submit. This will fail. 4) Apply the patch, redo with success. Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 79202 [details] [review] Bug 20023: (QA follow-up) Tiny simplification Remove empty emails from the list rightaway (incl. all spaces). Would be tempted to lc the params->{email} at the start btw..
Created attachment 79203 [details] [review] Bug 20023: Password recovery using email should be case insensitive When entering an email to recover the password, a user should not have to know or remember the casing of the given email. Test 0) enable OpacResetPassword 1) On the OPAC, click "Forgot your password" 2) Enter your email address as in your user account BUT WITH A DIFFERENT CASING 3) Submit. This will fail. 4) Apply the patch, redo with success. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 79204 [details] [review] Bug 20023: (QA follow-up) Tiny simplification Remove empty emails from the list rightaway (incl. all spaces). Would be tempted to lc the params->{email} at the start btw.. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment on attachment 79204 [details] [review] Bug 20023: (QA follow-up) Tiny simplification Review of attachment 79204 [details] [review]: ----------------------------------------------------------------- ::: opac/opac-password-recovery.pl @@ +73,4 @@ > $errMultipleAccountsForEmail = 1; > } > elsif ( $borrower = $search_results->next() ) { # One matching borrower > + my @emails = grep { $_ && !/^\s+$/ } ( $borrower->email, $borrower->emailpro, $borrower->B_email ); Not sure this is simplification :) @@ +77,2 @@ > > + my $firstNonEmptyEmail = @emails ? $emails[0] : undef; So simplify it, really :) my $firstNonEmptyEmail = shift @emails;
Staying sharp, Joubu. Great. I removed the regex for spaces. It wasn't there before and you can't save three spaces as an email address anymore. Using the shift too. So simple :)
Created attachment 79270 [details] [review] Bug 20023: (QA follow-up) Tiny simplification Remove empty emails from the list rightaway (incl. all spaces). Would be tempted to lc the params->{email} at the start btw.. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 79271 [details] [review] Bug 20023: (QA follow-up) Tiny simplification Remove empty emails from the list rightaway. Would be tempted to lc the params->{email} at the start btw.. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Awesome work all! Pushed to master for 18.11
Pushed to 18.05.x for 18.05.05
Pushed to 17.11.x for 17.11.11
The tiny follow-up broke password recovery on 17.11 (haven't tested on other versions yet. Reverting fixes the problem. The problem: No matter what you enter, no matching account will be found, even if the email address and all data are entered correctly. The simplification here is almost as long as the original patch, I feel it was not good to push this as a QA follow-up without more testing. (Sorry)
(In reply to Katrin Fischer from comment #18) > The tiny follow-up broke password recovery on 17.11 (haven't tested on other > versions yet. Reverting fixes the problem. > > The problem: No matter what you enter, no matching account will be found, > even if the email address and all data are entered correctly. > > The simplification here is almost as long as the original patch, I feel it > was not good to push this as a QA follow-up without more testing. (Sorry) Will have a look
(In reply to Katrin Fischer from comment #18) > The tiny follow-up broke password recovery on 17.11 (haven't tested on other > versions yet. Reverting fixes the problem. Works on current master. Looking further.
(In reply to Marcel de Rooy from comment #20) > (In reply to Katrin Fischer from comment #18) > > The tiny follow-up broke password recovery on 17.11 (haven't tested on other > > versions yet. Reverting fixes the problem. > > Works on current master. Looking further. Hmm. Userid works but email might not..
We were testing with email.
Found the cause. How stupid!
Not to blame anyone. But replacing my $firstNonEmptyEmail = @emails ? $emails[0] : undef; by the shift @emails obviously was not so smart after all. Because we still test @emails later on !
Patch forthcoming
Hi Marcel, can you attach the fix on bug 21771? It's already a bug in released versions, so it will be more clear for release notes.
Created attachment 81985 [details] [review] Bug 20023: (Follow-up) Revert the shift statement Obviously you should never shift an items from an array if you need that item later on :) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested with entering userid as well as entering email..
Attention RM and Rmaints : Please pick LAST follow-up.
(In reply to Katrin Fischer from comment #26) > Hi Marcel, can you attach the fix on bug 21771? It's already a bug in > released versions, so it will be more clear for release notes. OK
Comment on attachment 81985 [details] [review] Bug 20023: (Follow-up) Revert the shift statement Moved to 21771
Sorry for all confusion people. But fixed ;)