Bug 8753 - Add forgot password link to OPAC
Summary: Add forgot password link to OPAC
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P1 - high enhancement (vote)
Assignee: Charles Farmer
QA Contact: Marcel de Rooy
URL:
Keywords:
: 3059 (view as bug list)
Depends on: 12633
Blocks: 15585 16471 16707 18616 18653 26941
  Show dependency treegraph
 
Reported: 2012-09-11 04:12 UTC by Amit Gupta
Modified: 2020-11-05 14:47 UTC (History)
38 users (show)

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


Attachments
Add forgot password link to OPAC (22.46 KB, patch)
2013-09-26 18:34 UTC, Blou
Details | Diff | Splinter Review
Add forgot password link to OPAC (23.37 KB, patch)
2013-09-27 14:23 UTC, Blou
Details | Diff | Splinter Review
Bug 8753 - Add forgot password link to OPAC (3.10 KB, patch)
2013-09-27 17:19 UTC, Blou
Details | Diff | Splinter Review
Add forgot password link to OPAC (27.48 KB, patch)
2013-10-07 13:45 UTC, Blou
Details | Diff | Splinter Review
Rebased patch (27.63 KB, patch)
2013-10-21 19:26 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8753 - Add forgot password link to OPAC (2.56 KB, patch)
2013-10-25 18:23 UTC, Blou
Details | Diff | Splinter Review
Bug 8753 - Add forgot password link to OPAC (38.09 KB, patch)
2014-01-20 22:09 UTC, Blou
Details | Diff | Splinter Review
Fixes all comments specified by mtompsett. (37.65 KB, patch)
2014-01-27 20:55 UTC, Blou
Details | Diff | Splinter Review
Bug 8753 - Add forgot password link to OPAC (38.38 KB, patch)
2014-01-27 20:57 UTC, Blou
Details | Diff | Splinter Review
Bug 8753 - Add forgot password link to OPAC (37.78 KB, patch)
2014-01-28 16:18 UTC, Blou
Details | Diff | Splinter Review
Removed unnecessary references in template calls. (1.48 KB, patch)
2014-01-28 19:14 UTC, Blou
Details | Diff | Splinter Review
Removed unnecessary argument in call to SendPasswordRecoveryEmail. Improved var naming (2.25 KB, patch)
2014-01-28 20:24 UTC, Blou
Details | Diff | Splinter Review
Bug 8753 - Add forgot password link to OPAC (38.35 KB, patch)
2014-01-28 21:13 UTC, Blou
Details | Diff | Splinter Review
Bug 8753 - Add forgot password link to OPAC (37.12 KB, patch)
2014-01-29 14:39 UTC, Blou
Details | Diff | Splinter Review
Improved UGLY code (sic). (3.03 KB, patch)
2014-01-30 21:32 UTC, Blou
Details | Diff | Splinter Review
hash vs hashref issue (1.37 KB, patch)
2014-01-30 21:41 UTC, Blou
Details | Diff | Splinter Review
Bug 8753 - Add forgot password link to OPAC (36.57 KB, patch)
2014-01-30 22:09 UTC, Blou
Details | Diff | Splinter Review
Bug 8753 - Add forgot password link to OPAC (patch) (2.50 KB, patch)
2014-02-04 19:28 UTC, Blou
Details | Diff | Splinter Review
Bug 8753 - Add forgot password link to OPAC (36.80 KB, patch)
2014-02-04 21:11 UTC, Blou
Details | Diff | Splinter Review
Delete the exprired links after (any) successful reset (979 bytes, patch)
2014-02-05 13:09 UTC, Blou
Details | Diff | Splinter Review
Bug 8753 - Add forgot password link to OPAC (24.61 KB, patch)
2014-06-20 15:44 UTC, Blou
Details | Diff | Splinter Review
Bug 8753 - Add forgot password link to OPAC - Updated (2.60 KB, patch)
2014-11-03 18:10 UTC, simith.doliveira
Details | Diff | Splinter Review
Bug 8753 - Add forgot password link to OPAC - DBIx Schema Added (1.91 KB, patch)
2014-11-03 18:13 UTC, simith.doliveira
Details | Diff | Splinter Review
SQL code in the .pl files removed. New .pm and .t files created. (12.98 KB, patch)
2014-12-16 18:28 UTC, simith.doliveira
Details | Diff | Splinter Review
Bug 8753 - Add forgot password link to OPAC (27.94 KB, patch)
2015-03-11 17:58 UTC, simith.doliveira
Details | Diff | Splinter Review
Bug 8753 - Add forgot password link to OPAC (28.63 KB, patch)
2015-04-03 19:15 UTC, Blou
Details | Diff | Splinter Review
Bug 8753 - Add forgot password link to OPAC (31.43 KB, patch)
2015-05-11 13:01 UTC, Maxime Beaulieu
Details | Diff | Splinter Review
[Signed-off] Bug 8753 - Add forgot password link to OPAC (31.62 KB, patch)
2015-05-13 15:53 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 8753 - Add forgot password link to OPAC (31.94 KB, patch)
2015-05-13 22:18 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 8753 - Followup - change value text on syspref (1.61 KB, patch)
2015-05-13 22:33 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 8753 - followup - update text for link to match common UI paradigms (4.79 KB, patch)
2015-05-13 22:56 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 8753 - followup - update text for link to match common UI paradigms, fixes OpacPublic disabled view (5.02 KB, patch)
2015-05-13 23:00 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 8753 - Fixed the forbidden pattern error in opac-auth.tt (1.42 KB, patch)
2015-06-03 20:37 UTC, Rémi Mayrand-Provencher
Details | Diff | Splinter Review
Bug 8753 - Use Koha::Borrowers instead of C4::Members (6.07 KB, patch)
2015-07-24 20:20 UTC, Maxime Beaulieu
Details | Diff | Splinter Review
Stray text appears above header (35.20 KB, image/png)
2015-07-26 09:54 UTC, Marc Véron
Details
Bug 8753 - [followup] fix the title on opac-password-recovery.tt (1.23 KB, patch)
2015-07-26 22:11 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 8753 - [followup] fix error when no information is provided (1.46 KB, patch)
2015-08-12 04:28 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 8753 - [followup} Correcting spelling mistakes (1.92 KB, patch)
2015-08-19 22:25 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 8753 - [followup] fix error when no information is provided (1.48 KB, patch)
2015-08-19 22:45 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 8753 - [followup] POD cleanup (1016 bytes, patch)
2015-08-19 23:20 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 8753 - [followup] POD cleanup (1.22 KB, patch)
2015-08-20 10:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8753 - Smartmatch substitute, Math::Random::Secure, Perltidy, Passwordrecovery.t (20.80 KB, patch)
2015-10-01 15:58 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8753 - Various little things - removing new dependency, changes to errors, textual updates (9.67 KB, patch)
2015-10-07 03:39 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 8753 - Add forgot password link to OPAC (31.62 KB, patch)
2015-10-18 20:30 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 8753 - Add forgot password link to OPAC (31.62 KB, patch)
2015-11-03 00:46 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 8753 - Followup - change value text on syspref (9.27 KB, patch)
2015-11-03 00:46 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 8753 - Fixed the forbidden pattern error in opac-auth.tt (1.42 KB, patch)
2015-11-03 00:46 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 8753 - Use Koha::Borrowers instead of C4::Members (6.00 KB, patch)
2015-11-03 00:47 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 8753 - [followup] POD cleanup (1.22 KB, patch)
2015-11-03 00:47 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 8753 - Smartmatch substitute, Math::Random::Secure, Perltidy, Passwordrecovery.t (20.80 KB, patch)
2015-11-03 00:47 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 8753 - Various little things - removing new dependency, changes to errors, textual updates (9.67 KB, patch)
2015-11-03 00:48 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 8753 - Add forgot password link to OPAC (31.72 KB, patch)
2015-11-03 11:04 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 8753 - Followup - change value text on syspref (9.40 KB, patch)
2015-11-03 11:06 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 8753 - Fixed the forbidden pattern error in opac-auth.tt (1.56 KB, patch)
2015-11-03 11:07 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 8753 - Use Koha::Borrowers instead of C4::Members (6.14 KB, patch)
2015-11-03 11:08 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 8753 - [followup] POD cleanup (1.36 KB, patch)
2015-11-03 11:09 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 8753 - Smartmatch substitute, Math::Random::Secure, Perltidy, Passwordrecovery.t (21.01 KB, patch)
2015-11-03 11:21 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 8753 - Various little things - removing new dependency, changes to errors, textual updates (9.81 KB, patch)
2015-11-03 11:22 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 8753 - Add forgot password link to OPAC (31.28 KB, patch)
2015-11-27 14:26 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8753 - Followup - change value text on syspref (9.40 KB, patch)
2015-11-27 14:28 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8753 - Fixed the forbidden pattern error in opac-auth.tt (1.55 KB, patch)
2015-11-27 14:29 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8753 - Use Koha::Borrowers instead of C4::Members (6.13 KB, patch)
2015-11-27 14:29 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8753 - [followup] POD cleanup (1.36 KB, patch)
2015-11-27 14:29 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8753 - Smartmatch substitute, Math::Random::Secure, Perltidy, Passwordrecovery.t (21.01 KB, patch)
2015-11-27 14:29 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8753 - Various little things - removing new dependency, changes to errors, textual updates (9.80 KB, patch)
2015-11-27 14:29 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8753 - Add forgot password link to OPAC (31.28 KB, patch)
2015-11-30 14:26 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8753 - Followup - change value text on syspref (9.40 KB, patch)
2015-11-30 14:26 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8753 - Fixed the forbidden pattern error in opac-auth.tt (1.55 KB, patch)
2015-11-30 14:27 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8753 - Use Koha::Borrowers instead of C4::Members (6.13 KB, patch)
2015-11-30 14:27 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8753 - [followup] POD cleanup (1.36 KB, patch)
2015-11-30 14:27 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8753 - Smartmatch substitute, Math::Random::Secure, Perltidy, Passwordrecovery.t (21.01 KB, patch)
2015-11-30 14:27 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8753 - Various little things - removing new dependency, changes to errors, textual updates (9.80 KB, patch)
2015-11-30 14:27 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8753 - propagating the changes to kohastructure.sql and sysprefs.sql (3.20 KB, patch)
2015-11-30 14:27 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8753 - propagating the changes to kohastructure.sql and sysprefs.sql (3.27 KB, patch)
2016-01-07 02:18 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 8753 - Add forgot password link to OPAC (31.29 KB, patch)
2016-01-22 09:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8753 - Followup - change value text on syspref (9.42 KB, patch)
2016-01-22 09:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8753 - Use Koha::Borrowers instead of C4::Members (6.15 KB, patch)
2016-01-22 09:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8753 - [followup] POD cleanup (1.37 KB, patch)
2016-01-22 09:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8753 - Smartmatch substitute, Math::Random::Secure, Perltidy, Passwordrecovery.t (21.02 KB, patch)
2016-01-22 09:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8753 - Various little things - removing new dependency, changes to errors, textual updates (9.81 KB, patch)
2016-01-22 09:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8753 - propagating the changes to kohastructure.sql and sysprefs.sql (3.38 KB, patch)
2016-01-22 09:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8753: [QA Follow-up] Primary key and collation (4.68 KB, patch)
2016-01-22 09:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8753 [QA Followup] - Add notice to translations (10.12 KB, patch)
2016-01-27 18:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 8753 [QA Followup] - Add notice to translations (10.19 KB, patch)
2016-01-27 22:56 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 Amit Gupta 2012-09-11 04:12:07 UTC
If user forgot there password once click on add forgot password link to opac then system generate a new password and send to patron email address.
Comment 1 Eric Bégin 2013-04-19 18:36:45 UTC
A better way to handle this is to send a uniquely generated link to the user which will allow him to reset its password.
Comment 2 Eric Bégin 2013-04-19 18:38:04 UTC
*** Bug 3059 has been marked as a duplicate of this bug. ***
Comment 3 Chris Cormack 2013-04-21 00:33:42 UTC
(In reply to comment #1)
> A better way to handle this is to send a uniquely generated link to the user
> which will allow him to reset its password.

I agree, this is the common best practice, we should never be sending passwords to users (unless they are onetime use only) ... i'd like to remove the code that already does this in staff, rather than adding more.
Comment 4 Nicole C. Engard 2013-08-26 14:26:52 UTC
This says it's assigned - is someone actually working on this?

Nicole
Comment 5 Eric Bégin 2013-08-26 17:46:47 UTC
We actually have this code running on our 3.10.x version.

It's juste a matter of finding time to merge on the main branch...
Comment 6 Blou 2013-09-26 18:34:52 UTC Comment hidden (obsolete)
Comment 7 Blou 2013-09-26 19:33:01 UTC
TEST PLAN:

1) apply the patch
2) go to system preferences OPAC>>Privacy and set 'OpacResetPassword' to ON.  That will cause the link 'Forgot your password' to show up on the welcome page, below connection box.
3) refresh front page, click on 'Forgot your password' and enter a VALID address (one that is associated to an entry in borrowers.email or borrowers.email_pro.
3b) Also try an INVALID address (valid yet not in your koha db).  An error message will show up.
4) An email should be received at that address with a link.
5) Follow the link in the mail to fill the new password.
   Until a satisfactory new password is entered, the old password is not reset.
6) Go to main page try the new password.
Comment 8 Blou 2013-09-27 14:23:24 UTC Comment hidden (obsolete)
Comment 9 Owen Leonard 2013-09-27 15:05:30 UTC
Looks like this requires an update to the database, but I don't see any changes in updatedatabase.pl.
Comment 10 Blou 2013-09-27 17:19:10 UTC Comment hidden (obsolete)
Comment 11 Blou 2013-09-27 17:20:28 UTC
The second patch "Bug 8753 - Add forgot password link to OPAC" contains the db scripts.
Comment 12 I'm just a bot 2013-09-29 06:23:50 UTC
Patch applied cleanly, go forth and signoff
Comment 13 Blou 2013-10-07 13:45:19 UTC Comment hidden (obsolete)
Comment 14 JD@TeTakere 2013-10-21 19:18:06 UTC
Patch didn't apply one the sandboxes. There was a conflict in updatedatabase.pl
Comment 15 Chris Cormack 2013-10-21 19:26:43 UTC Comment hidden (obsolete)
Comment 16 Blou 2013-10-21 19:34:11 UTC
Thank you sir!  I barely had time to notice the issue email then set up my environment to rebase, that you had already solved it.
Comment 17 Blou 2013-10-22 20:35:38 UTC
Changing the status from "Patch doesn't apply" to "Need signoff".
Not sure if that was needed since rebaser knows the business, please let me if otherwise.
Comment 18 Kyle M Hall 2013-10-25 14:30:48 UTC
Looks like the table borrower_password_recovery was left out of the database update by accident.
Comment 19 Blou 2013-10-25 18:23:24 UTC Comment hidden (obsolete)
Comment 20 Blou 2013-10-25 18:25:28 UTC
Well, seems I'm fully forgot updatedatabase.pl in first patch.
Rebased and fixed.
Comment 21 Jonathan Druart 2013-11-28 14:51:49 UTC
Early QA comment:
1/ new routine needs unit tests
2/ remove tab characters
3/ perltidy new pl script
4/ use Modern::Perl; (you only use strict)
5/ only 1 syspref is added to sysprefs.sql
6/ why don't you sent the email using utf8?
7/ on generating the new password char, maybe should you use the alphabet syspref (+ numbers)
8/ why don't you use koha opac header for opac-send-password-recovery template file?
9/ use the tt plugin to access to syspref value (Koha.Preference("mypref")).
10/ what is the 'UNAUTHORIZ' check (in opac-password-recovery.pl)?

10 is a good number.
Comment 22 Blou 2014-01-20 22:09:42 UTC Comment hidden (obsolete)
Comment 23 Mark Tompsett 2014-01-23 21:27:12 UTC
Comment on attachment 24586 [details] [review]
Bug 8753 - Add forgot password link to OPAC

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

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

No longer necessary.

::: C4/Members.pm
@@ +2273,5 @@
> +    my $uuid_str;
> +    $uuid_str .= $chars[rand @chars] for 1..32;
> +
> +    #insert into database
> +    my $sth = $dbh->prepare( 'INSERT INTO borrower_password_recovery VALUES (? ,? , ADDDATE(NOW(), INTERVAL 2 DAY) )' );

Is this ANSI SQL?

@@ +2281,5 @@
> +    #define to/from emails
> +    my $kohaEmail = C4::Context->preference( 'KohaAdminEmailAddress' );
> +
> +    #create link
> +    my $uuidLink = "http://" . C4::Context->preference( 'OPACBaseURL' ) . "/cgi-bin/koha/opac-password-recovery.pl?uniqueKey=$uuid_str";

Does someone have that funky code to determine http vs https in a URL handy? Please don't hard code http.

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
@@ +556,5 @@
> +              default: 1
> +              choices:
> +                  yes: "On"
> +                  no: "Off"
> +            - ". If On, the user can reset his password on OPAC."

There are prettier ways to format this.
        -
            - "The user "
            - pref: OpacResetPassword
              default: 1
              choices:
                  yes: "can reset"
                  no: "can not reset"
            - " their password on OPAC."

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-password-recovery.tt
@@ +48,5 @@
> +            More than one account has been found for the email address: "<strong>[% email %]</strong>"
> +            <br/>Try yo use your alternative email if you have another.
> +        [% ELSIF (errAlreadyStartRecovery) %]
> +            The process of password recovery has already started for this account ("<strong>[% email %]</strong>")
> +            <br/>Check your emails; you should recieve the link to reset your password.

receive typo.

@@ +74,5 @@
> +
> +[% ELSIF (new_password) %]
> +    [% IF (errLinkNotValid) %]
> +        <span class="TxtErreur"><h6>
> +        We could not authentify you as the account owner.

authenticate

::: koha-tmpl/opac-tmpl/prog/en/modules/opac-auth.tt
@@ +86,5 @@
>  
>  <input type="submit" value="Log In" class="submit" />
> +[% IF Koha.Preference('OpacPasswordChange') && Koha.Preference('OpacResetPassword') %]
> +    <p><a href="/cgi-bin/koha/opac-password-recovery.pl">Forgot your password?</a></p>
> +[% END %]

Could this be in the nologininstructions div?

::: koha-tmpl/opac-tmpl/prog/en/modules/opac-password-recovery.tt
@@ +46,5 @@
> +            More than one account has been found for the email address: "<strong>[% email %]</strong>"
> +            <br/>Try yo use your alternative email if you have another.
> +        [% ELSIF (errAlreadyStartRecovery) %]
> +            The process of password recovery has already started for this account ("<strong>[% email %]</strong>")
> +            <br/>Check your emails; you should recieve the link to reset your password.

receive typo.

@@ +72,5 @@
> +
> +[% ELSIF (new_password) %]
> +    [% IF (errLinkNotValid) %]
> +        <span class="TxtErreur"><h6>
> +        We could not authentify you as the account owner.

authenticate

::: opac/opac-main.pl
@@ +58,4 @@
>      koha_news_count     => $koha_news_count,
>      display_daily_quote => C4::Context->preference('QuoteOfTheDay'),
>      daily_quote         => $quote,
> +    OpacResetPassword   => C4::Context->preference('OpacResetPassword'),

You tweaked opac-main.tt to use the Koha.Preference method of grabbing system preferences in the template. There's no need to modify this file.

::: opac/opac-password-recovery.pl
@@ +48,5 @@
> +my $errPassTooShort;
> +
> +my $dbh = C4::Context->dbh;
> +
> +$template->param( OpacResetPassword => C4::Context->preference("OpacResetPassword") );

This is not needed.
Comment 24 Mark Tompsett 2014-01-23 21:30:08 UTC
Please look at my eyeball comments, and deal with this:
 FAIL   koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth.tt
   FAIL   forbidden patterns
                forbidden pattern: tab char (line 98)
                forbidden pattern: tab char (line 100)
                forbidden pattern: tab char (line 99)
   OK     tt_valid
   OK     valid_template
Comment 25 Mark Tompsett 2014-01-24 16:32:20 UTC
Comment on attachment 24586 [details] [review]
Bug 8753 - Add forgot password link to OPAC

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

::: opac/opac-password-recovery.pl
@@ +102,5 @@
> +            email                   => HTML::Entities::encode($email),
> +        );
> +    }
> +    else {
> +        #generate uuid and send recovery email

Bug 11575 has:
    ($in->{'query'}->https() ? "https://" : "http://")
$query->https() may be what you need. Some way of passing that to SendPasswordRecoveryEmail, so you don't hardcode http or https.
Comment 26 Blou 2014-01-27 20:55:23 UTC Comment hidden (obsolete)
Comment 27 Blou 2014-01-27 20:57:12 UTC Comment hidden (obsolete)
Comment 28 Chris Cormack 2014-01-27 23:20:32 UTC
Comment on attachment 24781 [details] [review]
Bug 8753 - Add forgot password link to OPAC

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

::: opac/opac-password-recovery.pl
@@ +135,5 @@
> +    if (   ($borrower_number)
> +        && ( $password eq $repeatPassword )
> +        && ( length($password) >= $minPassLength ) )
> +    {  #apply changes
> +        changepassword( $username, $borrower_number, md5_base64($password) );

We no longer md5 hash the password in Koha, you should do 

use Koha::AuthUtils;

hash_password($password) instead.  

(It's using bcrypt with a salt). Old passwords still work, we check both, but anytime people change their password it should be hashed with the much more secure bcrypt
Comment 29 Blou 2014-01-28 16:18:33 UTC Comment hidden (obsolete)
Comment 30 Mark Tompsett 2014-01-28 18:53:08 UTC
Comment on attachment 24846 [details] [review]
Bug 8753 - Add forgot password link to OPAC

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

::: C4/Auth.pm
@@ +1068,4 @@
>          PatronSelfRegistrationDefaultCategory => C4::Context->preference("PatronSelfRegistrationDefaultCategory"),
>          persona            => C4::Context->preference("Persona"),
>          opac_css_override => $ENV{'OPAC_CSS_OVERRIDE'},
> +        OpacResetPassword => C4::Context->preference("OpacResetPassword"),

You added OpacResetPassword, and all the templates use the Koha system preference grabbing plugin. There's no need for this.

@@ +1068,5 @@
>          PatronSelfRegistrationDefaultCategory => C4::Context->preference("PatronSelfRegistrationDefaultCategory"),
>          persona            => C4::Context->preference("Persona"),
>          opac_css_override => $ENV{'OPAC_CSS_OVERRIDE'},
> +        OpacResetPassword => C4::Context->preference("OpacResetPassword"),
> +        OpacPasswordChange => C4::Context->preference("OpacPasswordChange"),

This wasn't here, so I don't see a reason to add it, since all the template changes you made use the Koha system preference grabbing plugin.

::: opac/opac-main.pl
@@ +58,4 @@
>      koha_news_count     => $koha_news_count,
>      display_daily_quote => C4::Context->preference('QuoteOfTheDay'),
>      daily_quote         => $quote,
> +    OpacResetPassword   => C4::Context->preference('OpacResetPassword'),

There's no point to have this.
Comment 31 Blou 2014-01-28 19:14:14 UTC Comment hidden (obsolete)
Comment 32 Mark Tompsett 2014-01-28 19:57:18 UTC
Comment on attachment 24846 [details] [review]
Bug 8753 - Add forgot password link to OPAC

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

::: C4/Members.pm
@@ +2266,5 @@
> +
> +sub SendPasswordRecoveryEmail {
> +    my $borrowernumber = shift;
> +    my $email = shift;
> +    my $query = shift;

Perhaps with the code later you were trying to handle:
    my $query = shift // CGI->new();

@@ +2285,5 @@
> +    #define to/from emails
> +    my $kohaEmail = C4::Context->preference( 'KohaAdminEmailAddress' );
> +
> +    #create link
> +    my $https = $query->https() ? "https://" : "http://";

I believe I have seen others call this protocol, but changing this isn't necessary.

@@ +2290,5 @@
> +    my $uuidLink = $https . C4::Context->preference( 'OPACBaseURL' ) . "/cgi-bin/koha/opac-password-recovery.pl?uniqueKey=$uuid_str";
> +    #warn $uuidLink;
> +
> +    #build email content
> +    my $query = new CGI;

Why do you need this?! You have line 2270.
Comment 33 Blou 2014-01-28 20:24:09 UTC Comment hidden (obsolete)
Comment 34 Blou 2014-01-28 21:13:10 UTC Comment hidden (obsolete)
Comment 35 Mark Tompsett 2014-01-28 21:33:44 UTC
Comment on attachment 24855 [details] [review]
Bug 8753 - Add forgot password link to OPAC

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

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

They're back. :(

::: opac/opac-main.pl
@@ +58,4 @@
>      koha_news_count     => $koha_news_count,
>      display_daily_quote => C4::Context->preference('QuoteOfTheDay'),
>      daily_quote         => $quote,
> +    OpacResetPassword   => C4::Context->preference('OpacResetPassword'),

Oops.
Comment 36 Blou 2014-01-29 14:39:33 UTC Comment hidden (obsolete)
Comment 37 Mark Tompsett 2014-01-29 21:07:09 UTC
You forget to delete the password reset entry after resetting the password.
Comment 38 Mark Tompsett 2014-01-29 22:36:33 UTC
Please test with /cgi-bin/koha/opac-password-recovery.pl?uniqueKey=OneThatDoesNotExist

404.pl is not the correct behaviour, in my opinion.
Comment 39 Blou 2014-01-30 19:43:10 UTC
(In reply to M. Tompsett from comment #37)
> You forget to delete the password reset entry after resetting the password.

You mean from table borrower_password_recovery ?  I validated it does delete the entry.

And I can't reproduce the 404 of your next comment.  When I append your line to my opac base url, it sends me to a message:

"
We could not authenticate you as the account owner.
Be sure to use the link you received in your email. 
"

Are you sure the patch was correctly applied?

Thanks again for your time!
Comment 40 Blou 2014-01-30 21:32:21 UTC Comment hidden (obsolete)
Comment 41 Blou 2014-01-30 21:41:32 UTC Comment hidden (obsolete)
Comment 42 Blou 2014-01-30 22:09:18 UTC Comment hidden (obsolete)
Comment 43 Mark Tompsett 2014-01-31 20:46:49 UTC
The generate a new link process can create multiple links (potentially huge number) for a single user. It should overwrite the existing one. No more than one entry per borrowernumber.
Comment 44 Blou 2014-02-04 19:28:31 UTC Comment hidden (obsolete)
Comment 45 Blou 2014-02-04 21:11:19 UTC Comment hidden (obsolete)
Comment 46 Blou 2014-02-05 13:09:22 UTC Comment hidden (obsolete)
Comment 47 Mark Tompsett 2014-02-05 15:13:51 UTC
Vast improvement on the DELETE.
Sadly, still have two problems.

"Framing Problem"
Click forget password link
Type in username and password to log in to OPAC
Click link in email. (screen 1)
Log out of OPAC
Click link in email. (screen 2)
Both screens are framed differently.

"Email Problem"
http://irc.koha-community.org/koha/2014-02-04#i_1460899
Comment 48 Owen Leonard 2014-02-05 17:51:28 UTC
I notice this feature is enabled by default. I think it should be disabled by default.

There are many problems with the template which I'll work on a follow-up for.
Comment 49 Owen Leonard 2014-02-05 18:59:32 UTC
For what it's worth, I tested this and didn't get a password recovery email. I noticed in trying to understand how the emails should be sent that the feature doesn't use notice templates or the message queue. Should we be adding a feature that uses yet another method for handling emails?
Comment 50 Mark Tompsett 2014-02-05 21:54:52 UTC
My Ubuntu VM has email set up in a manner similar to
https://help.ubuntu.com/community/Postfix

This uses the Mail::SendMail library directly, which is already part of the dependencies for Koha. Checkout the code in C4/Members.pm around line 2314 after applying the patch.
The "if ( sendmail %mail )" is what actually does the emailing.

If you don't have postfix configured, then nullmailer is installed. Hopefully, that is configured correctly. I have yet to read sensible and useful documentation on it.

Some mail hosts delay sending/receiving when mail is relayed. Patience may be required while testing this.
Comment 51 David Cook 2014-02-05 22:11:52 UTC
(In reply to M. Tompsett from comment #50)
> My Ubuntu VM has email set up in a manner similar to
> https://help.ubuntu.com/community/Postfix
> 
> This uses the Mail::SendMail library directly, which is already part of the
> dependencies for Koha. Checkout the code in C4/Members.pm around line 2314
> after applying the patch.
> The "if ( sendmail %mail )" is what actually does the emailing.
> 
> If you don't have postfix configured, then nullmailer is installed.
> Hopefully, that is configured correctly. I have yet to read sensible and
> useful documentation on it.
> 
> Some mail hosts delay sending/receiving when mail is relayed. Patience may
> be required while testing this.

While other parts of Koha do rely on Sendmail directly, I think Owen has a very good point. We should definitely be using the notice templates, and we should probably be using the message queue.

The notice templates allow libraries to provide their own wording/translations to the message. I think that's a *must*. 

Using the message queue also helps Koha administrators to track the messages going out, since they might not have direct access to the MTA or mail server. 

Owen: I'm not sure what your testing scenario is but I noticed recently that my Koha VM's MTA is configured out of the box not to send email outside of its local domain. I don't know if that's relevant for you though.
Comment 52 HB-NEKLS 2014-02-23 18:03:10 UTC
I came into Koha today looking to see if there was any work on a patron being able to reset a forgotten password, and was happy to see this bug in progress. 

I want to +1 on the use of the existing notice templates and built-in messaging system in Koha for this feature. As the manager of a large, multi-branch system, that just gained access to the ability to finally have ALL notices sent from the branch email not the admin email (thank you developers for bugfix #5544!), I would hate to see password reset messages separately again come from the admin email address. 

Using the message queue will also be able to help us troubleshoot and work with patrons who repeatedly say "I reset my password but it didn't work." 

I hope this patch is eventually successful!
Comment 53 Blou 2014-06-20 15:44:11 UTC Comment hidden (obsolete)
Comment 54 Mark Tompsett 2014-07-24 22:05:50 UTC
With the moved to DBIx, the Schema file needs to be created as well. Hence, the change back to ASSIGNED. Sorry. Wasn't me that changed the underlying technologies in mid-stream. :(
Comment 55 simith.doliveira 2014-11-03 18:10:55 UTC Comment hidden (obsolete)
Comment 56 simith.doliveira 2014-11-03 18:13:23 UTC Comment hidden (obsolete)
Comment 57 Marcel de Rooy 2014-11-03 21:06:10 UTC
Interesting! Just some thoughts: 
The SQL code in the opac-recovery script will not make it pass QA. Please move it to module level (in DBIx?). 
Can you unit test SendPasswordRecoveryEmail?

I would not mind a mail with a library password; other info is more sensitive. If you can read/intercept the password from the mail, you can also read the unique userid for the reset password form. Same result: a hacked account. This approach is fine with me, feels more safe but is not per se safer imo.

Thinking out loud: if you do not include sensitive keywords as "password" in your mail (so rename your script?), would that be little more safe? Would not attract attention?
Comment 58 Chris Cormack 2014-11-03 21:11:25 UTC
(In reply to M. de Rooy from comment #57)
> Interesting! Just some thoughts: 
> The SQL code in the opac-recovery script will not make it pass QA. Please
> move it to module level (in DBIx?). 
> Can you unit test SendPasswordRecoveryEmail?
> 
> I would not mind a mail with a library password; other info is more
> sensitive. If you can read/intercept the password from the mail, you can
> also read the unique userid for the reset password form. Same result: a
> hacked account. This approach is fine with me, feels more safe but is not
> per se safer imo.

I disagree, a token should be time based, and will require the person to set a password. 
This password only they will know. 

It's true that if someone intercepts the token and uses it to set your password for you, that is still a problem. However when you go to use the token and find out it has been used you will know this. If they intercept your password, they could use it for a bunch of time without you ever knowing.

Sending passwords in the clear is a bad idea. Please do not do it.
Comment 59 Katrin Fischer 2014-11-03 21:15:58 UTC
I agree with Chris, I think. Also we have a similar approach for the patron verification - sending a link to a page where you learn about your password. It seems a good way to do this.
Comment 60 Eric Bégin 2014-11-03 21:21:52 UTC
The token sent to the user is valid only for 2 days or if the link is clicked.

Most of the time, a use who requests a password will click the link in the following minutes (if not seconds).

Of course, this doesn't prevent your email to be intercepted. As far as I know, this feature was implemented the way most of the « Forgot your password ? » links are implemented.
Comment 61 Liz Rea 2014-11-03 21:28:10 UTC
Also, if you send the password in the clear, if it is intercepted once, and you are one of (more people than we'd like to admit) those people who uses the same password in multiple accounts, it's not only your library account that is now possibly hacked. We should not ever, ever, ever show a password in the clear. Ever. 

I am glad this works the way it does, it is the proper way.

Liz
Comment 62 Eric Bégin 2014-11-03 21:32:02 UTC
Juste wondering how this clear text password came in the discussion.

That was never the case with this development.
Comment 63 Liz Rea 2014-11-03 21:34:58 UTC
Comment 57, 2nd paragraph - when someone doing QA suggests mailing a cleartext password might be ok, it seems right to emphatically tell them "no, that's a bad idea."

I for one appreciate the fact that it was never part of this dev, thanks for doing it properly. :)

Liz
Comment 64 Mark Tompsett 2014-11-04 00:14:03 UTC
(In reply to M. de Rooy from comment #57)
> Interesting! Just some thoughts: 
> The SQL code in the opac-recovery script will not make it pass QA. Please
> move it to module level (in DBIx?). 
> Can you unit test SendPasswordRecoveryEmail?

Though, I would make sure this comment didn't get missed in the midst of everyone discussing password logic.

M. de Rooy, are you talking about just the SELECT? What about the DELETE? Could you clarify which bits you mean by commenting on a splinter review?
Comment 65 Katrin Fischer 2014-11-04 06:01:24 UTC
We don't want any SQL code in the .pl files - so I'd say both.
Comment 66 Marcel de Rooy 2014-11-05 12:29:36 UTC
(In reply to Liz Rea from comment #63)
> Comment 57, 2nd paragraph - when someone doing QA suggests mailing a
> cleartext password might be ok, it seems right to emphatically tell them
> "no, that's a bad idea."

Please keep it in context. Hope I do not intercept any undertone about QA work here? 

For completeness: I completely agree that sending password by mail is theoretically very bad. However, we probably do not have a 100% safe solution for this. So therefore in a pragmatic view the sensitivity argument etc. 
And to add: As long as many OPACs are going over HTTP, we send the password clear text all over the place..

But again, also comment 57 This approach is fine with me!
Comment 67 David Cook 2014-11-05 22:23:49 UTC
(In reply to M. de Rooy from comment #66)
> (In reply to Liz Rea from comment #63)
> > Comment 57, 2nd paragraph - when someone doing QA suggests mailing a
> > cleartext password might be ok, it seems right to emphatically tell them
> > "no, that's a bad idea."
> 
> Please keep it in context. Hope I do not intercept any undertone about QA
> work here? 
> 
> For completeness: I completely agree that sending password by mail is
> theoretically very bad. However, we probably do not have a 100% safe
> solution for this. So therefore in a pragmatic view the sensitivity argument
> etc. 
> And to add: As long as many OPACs are going over HTTP, we send the password
> clear text all over the place..
> 
> But again, also comment 57 This approach is fine with me!

When it comes to security/safety, there is never a 100% guarantee. However, there are better and worse ways of doing things. It sounds like the approach used in these patches is a reasonable one. As Eric mentioned, it's the "standard" way of doing it.

As for sending passwords in the clear when using HTTP, that's not an issue with Koha's code. That's an end-user configuration issue. If people are using HTTP instead of HTTPS, that's their responsibility.
Comment 68 Owen Leonard 2014-11-06 18:50:48 UTC
If you use the same password recover link twice you get an error which is inaccurate:

"We could not authenticate you as the account owner

Be sure to use the link you received in your email."

It might make sense to retain entries in the borrower_password_recovery table for a certain amount of time after they have been "used" so that we can accurately tell the user that they have followed a link which is out of date.
Comment 69 Owen Leonard 2014-11-06 19:10:53 UTC
opac/opac-password-recovery.pl line 163:

warn "INLIBRO username $username";

The script contains a lot of SQL, which is a violation of coding guidelines:

http://wiki.koha-community.org/wiki/Coding_Guidelines#SQL8:_SQL_code_in_.pl_scripts

The template contains a check for the variable "errTooManyEmailFound" but I don't see where that variable is created. In fact there doesn't seems to be any handling of multiple identical email addresses. This is a problem for my library where parents often put their email addresses on their kids' accounts.
Comment 70 simith.doliveira 2014-12-16 18:28:39 UTC Comment hidden (obsolete)
Comment 71 Marc Véron 2015-01-18 16:07:43 UTC
Does not apply

Applying: Bug 8753 - Add forgot password link to OPAC
Using index info to reconstruct a base tree...
<stdin>:471: trailing whitespace.
    
<stdin>:500: trailing whitespace.
    );  
<stdin>:511: trailing whitespace.
    } );     
<stdin>:25: new blank line at EOF.
+
<stdin>:126: new blank line at EOF.
+
warning: squelched 1 whitespace error
warning: 6 lines add whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth.tt
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/includes/masthead.inc
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql
Auto-merging installer/data/mysql/kohastructure.sql
Failed to merge in the changes.
Patch failed at 0001 Bug 8753 - Add forgot password link to OPAC
Comment 72 Mark Tompsett 2015-01-20 13:36:22 UTC
simith.doliveira@inlibro.com, could you squash your patches together too? Just my preference. :)
Comment 73 Owen Leonard 2015-01-20 13:45:11 UTC
(In reply to M. Tompsett from comment #72)
> Just my preference. :)

Last I heard it was not the RM's preference, but that may have been a previous RM. Perhaps this is something we should add to the coding guidelines?
Comment 74 Mark Tompsett 2015-01-20 13:56:24 UTC
The patches were never signed off. I figure a squash isn't so bad at this stage. :)
Comment 75 simith.doliveira 2015-03-11 17:58:17 UTC Comment hidden (obsolete)
Comment 76 simith.doliveira 2015-03-11 17:59:02 UTC
Comment on attachment 34472 [details] [review]
SQL code in the .pl files removed. New .pm and .t files created.

>From b775b68292a4c9757f7d0b93d2675d9165c18cc4 Mon Sep 17 00:00:00 2001
>From: simith <simith@inlibro.com>
>Date: Tue, 16 Dec 2014 13:22:26 -0500
>Subject: [PATCH] SQL code in the .pl files removed. New .pm and .t files
> created.
>
>http://bugs.koha-community.org/show_bug.cgi?id=8753
>---
> C4/Passwordrecovery.pm                             | 159 +++++++++++++++++++++
> .../data/mysql/en/mandatory/sample_notices.sql     |   2 +-
> opac/opac-password-recovery.pl                     | 100 ++-----------
> 3 files changed, 173 insertions(+), 88 deletions(-)
> create mode 100644 C4/Passwordrecovery.pm
>
>diff --git a/C4/Passwordrecovery.pm b/C4/Passwordrecovery.pm
>new file mode 100644
>index 0000000..f1b26e9
>--- /dev/null
>+++ b/C4/Passwordrecovery.pm
>@@ -0,0 +1,159 @@
>+package C4::Passwordrecovery;
>+
>+# Copyright 2014 PTFS Europe
>+#
>+# This file is part of Koha.
>+#
>+# Koha is free software; you can redistribute it and/or modify it
>+# under the terms of the GNU General Public License as published by
>+# the Free Software Foundation; either version 3 of the License, or
>+# (at your option) any later version.
>+#
>+# Koha is distributed in the hope that it will be useful, but
>+# WITHOUT ANY WARRANTY; without even the implied warranty of
>+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>+# GNU General Public License for more details.
>+#
>+# You should have received a copy of the GNU General Public License
>+# along with Koha; if not, see <http://www.gnu.org/licenses>.
>+
>+use Modern::Perl;
>+use C4::Context;
>+
>+use vars qw($VERSION @ISA @EXPORT);
>+
>+BEGIN {
>+    # set the version for version checking
>+    $VERSION = 3.07.00.049;
>+    require Exporter;
>+    @ISA    = qw(Exporter);
>+    push @EXPORT, qw(
>+        &ValidateBorrowernumber
>+        &SendPasswordRecoveryEmail
>+        &GetValidLinkInfo
>+    );
>+}
>+
>+=head1 NAME
>+
>+C4::Passwordrecovery - Koha password recovery module
>+
>+=head1 SYNOPSIS
>+
>+use C4::Passwordrecovery;
>+
>+=head1 FUNCTIONS
>+
>+=head2 ValidateBorrowernumber
>+
>+$alread = ValidateBorrowernumber( $borrower_number );
>+
>+Check if the system already start recovery
>+
>+Returns true false
>+
>+=cut
>+
>+sub ValidateBorrowernumber {
>+    my ($borrower_number) = @_;
>+    my $schema = Koha::Database->new->schema;
>+
>+    my $rs = $schema->resultset('BorrowerPasswordRecovery')->search(
>+    {
>+       borrowernumber => $borrower_number,
>+       valid_until => \'> NOW()'
>+    }, {
>+        columns => 'borrowernumber'
>+    });
>+
>+    if ($rs->next){
>+        return 1;
>+    }
>+
>+    return 0;
>+}
>+
>+=head2 GetValidLinkInfo
>+
>+    Check if the link is still valid and return some info.
>+
>+=cut
>+
>+sub GetValidLinkInfo {
>+    my ($uniqueKey) = @_;
>+    my $dbh = C4::Context->dbh;
>+    my $query = '
>+    SELECT borrower_password_recovery.borrowernumber, userid
>+    FROM borrower_password_recovery, borrowers
>+    WHERE borrowers.borrowernumber = borrower_password_recovery.borrowernumber
>+    AND NOW() < valid_until
>+    AND uuid = ?
>+    ';
>+    my $sth = $dbh->prepare($query);
>+    $sth->execute($uniqueKey);
>+    return $sth->fetchrow;
>+}
>+
>+=head2 SendPasswordRecoveryEmail
>+
>+ It creates an email using the templates and send it to the user, using the specified email
>+
>+=cut
>+
>+sub SendPasswordRecoveryEmail {
>+    my $borrower = shift; # from GetMember
>+    my $userEmail = shift; #to_address (the one specified in the request)
>+    my $protocol = shift; #only required to determine if 'http' or 'https'
>+    my $update = shift;
>+
>+    my $schema = Koha::Database->new->schema;
>+
>+    # generate UUID
>+    my @chars = ("A".."Z", "a".."z", "0".."9");
>+    my $uuid_str;
>+    $uuid_str .= $chars[rand @chars] for 1..32;
>+
>+    # insert into database
>+    my $expirydate = DateTime->now(time_zone => C4::Context->tz())->add( days => 2 );
>+    if($update){
>+        my $rs = $schema->resultset('BorrowerPasswordRecovery')->search(
>+        {
>+            borrowernumber => $borrower->{'borrowernumber'},
>+        });
>+        $rs->update({uuid => $uuid_str, valid_until => $expirydate->datetime()});
>+    } else {
>+         my $rs = $schema->resultset('BorrowerPasswordRecovery')->create({
>+            borrowernumber=>$borrower->{'borrowernumber'},
>+            uuid => $uuid_str,
>+            valid_until=> $expirydate->datetime()
>+         });
>+    }
>+
>+    # create link
>+    my $uuidLink = $protocol . C4::Context->preference( 'OPACBaseURL' ) . "/cgi-bin/koha/opac-password-recovery.pl?uniqueKey=$uuid_str";
>+
>+    # prepare the email
>+    my $letter = C4::Letters::GetPreparedLetter (
>+        module => 'members',
>+        letter_code => 'PASSWORD_RESET',
>+        branchcode => $borrower->{branchcode},
>+        substitute => {passwordreseturl => $uuidLink, user => $borrower->{userid} },
>+    );
>+
>+    # define to/from emails
>+    my $kohaEmail = C4::Context->preference( 'KohaAdminEmailAddress' ); # from
>+
>+    C4::Letters::EnqueueLetter( {
>+         letter => $letter,
>+         borrowernumber => $borrower->{borrowernumber},
>+         to_address => $userEmail,
>+         from_address => $kohaEmail,
>+         message_transport_type => 'email',
>+    } );
>+
>+    return 1;
>+}
>+
>+END { }    # module clean-up code here (global destructor)
>+
>+1;
>\ No newline at end of file
>diff --git a/installer/data/mysql/en/mandatory/sample_notices.sql b/installer/data/mysql/en/mandatory/sample_notices.sql
>index 37e9d9f..67c8b34 100644
>--- a/installer/data/mysql/en/mandatory/sample_notices.sql
>+++ b/installer/data/mysql/en/mandatory/sample_notices.sql
>@@ -144,5 +144,5 @@ Your library.'
> );
> 
> INSERT INTO `letter` (module, code, branchcode, name, is_html, title, content, message_transport_type)
>-VALUES ('members','PASSWORD_RESET','','Online password reset',1,'Koha password recovery','<html>\r\n<p>This email has been sent in response to your password recovery request for the account <strong><< borrowers.userid>></strong>.\r\n</p>\r\n<p>\r\nYou can now create your new password using the following link:\r\n<br/><a href=\"<<passwordreseturl>>\"><<passwordreseturl>></a>\r\n</p>\r\n<p>This link will be valid for 2 days from this email\'s reception, then you must reapply if you do not change your password.</p>\r\n<p>Thank you.</p>\r\n</html>\r\n','email'
>+VALUES ('members','PASSWORD_RESET','','Online password reset',1,'Koha password recovery','<html>\r\n<p>This email has been sent in response to your password recovery request for the account <strong><<user>></strong>.\r\n</p>\r\n<p>\r\nYou can now create your new password using the following link:\r\n<br/><a href=\"<<passwordreseturl>>\"><<passwordreseturl>></a>\r\n</p>\r\n<p>This link will be valid for 2 days from this email\'s reception, then you must reapply if you do not change your password.</p>\r\n<p>Thank you.</p>\r\n</html>\r\n','email'
> );
>diff --git a/opac/opac-password-recovery.pl b/opac/opac-password-recovery.pl
>index 27c8ea6..b5cd2c2 100755
>--- a/opac/opac-password-recovery.pl
>+++ b/opac/opac-password-recovery.pl
>@@ -9,6 +9,7 @@ use C4::Koha;
> use C4::Members qw(changepassword GetMember GetMemberDetails );
> use C4::Output;
> use C4::Context;
>+use C4::Passwordrecovery qw(SendPasswordRecoveryEmail ValidateBorrowernumber GetValidLinkInfo);
> use Koha::AuthUtils qw(hash_password);
> my $query = new CGI;
> use HTML::Entities;
>@@ -44,11 +45,8 @@ my $errLinkNotValid;
> my $errPassNotMatch;
> my $errPassTooShort;
> 
>-my $dbh = C4::Context->dbh;
>-
> if ( $query->param('sendEmail') || $query->param('resendEmail') ) {
>-    #send mail + confirmation
>-
>+    my $protocol = $query->https() ? "https://" : "http://";
>     #try with the main email
>     $email ||= ''; # avoid undef
>     my $borrower_infos = GetMember( email => $email );
>@@ -63,11 +61,9 @@ if ( $query->param('sendEmail') || $query->param('resendEmail') ) {
>         $errNoEmailFound = 1;
>     }
>     elsif ( !$query->param('resendEmail') ) {
>-        my $sth = $dbh->prepare(
>-"SELECT borrowernumber FROM borrower_password_recovery WHERE NOW() < valid_until AND borrowernumber = ?"
>-        );
>-        $sth->execute($borrower_number);
>-        if ( my $already = $sth->fetchrow ) {
>+        my $already = ValidateBorrowernumber( $borrower_number );
>+
>+        if ( $already ) {
>             $hasError                = 1;
>             $errAlreadyStartRecovery = 1;
>         }
>@@ -82,7 +78,7 @@ if ( $query->param('sendEmail') || $query->param('resendEmail') ) {
>             email                   => HTML::Entities::encode($email),
>         );
>     }
>-    elsif ( SendPasswordRecoveryEmail( $borrower_infos, $email, $query, $query->param('resendEmail') ) ) {#generate uuid and send recovery email
>+    elsif ( SendPasswordRecoveryEmail( $borrower_infos, $email, $protocol, $query->param('resendEmail') ) ) {#generate uuid and send recovery email
>         $template->param(
>             mail_sent => 1,
>             email     => $email
>@@ -96,18 +92,7 @@ if ( $query->param('sendEmail') || $query->param('resendEmail') ) {
>     }
> }
> elsif ( $query->param('passwordReset') ) {
>-    #new password form
>-    #check if the link is still valid
>-    my $sth = $dbh->prepare(
>-        "SELECT borrower_password_recovery.borrowernumber, userid
>-                              FROM borrower_password_recovery, borrowers
>-                              WHERE borrowers.borrowernumber = borrower_password_recovery.borrowernumber
>-                              AND NOW() < valid_until
>-                              AND uuid = ?"
>-    );
>-    $sth->execute($uniqueKey);
>-    ( $borrower_number, $username ) = $sth->fetchrow;
>-
>+    ( $borrower_number, $username ) = GetValidLinkInfo($uniqueKey);
>     #validate password length & match
>     if (   ($borrower_number)
>         && ( $password eq $repeatPassword )
>@@ -116,8 +101,9 @@ elsif ( $query->param('passwordReset') ) {
>         changepassword( $username, $borrower_number, hash_password($password) );
> 
>         #remove entry
>-        my $sth = $dbh->prepare("DELETE FROM borrower_password_recovery WHERE uuid = ? or NOW() > valid_until");
>-        $sth->execute($uniqueKey);
>+        my $schema = Koha::Database->new->schema;
>+        my $rs = $schema->resultset('BorrowerPasswordRecovery')->search({-or => [uuid => $uniqueKey, valid_until => \'< NOW()']});
>+        $rs->delete;
> 
>         $template->param(
>             password_reset_done => 1,
>@@ -148,19 +134,12 @@ elsif ( $query->param('passwordReset') ) {
> }
> elsif ($uniqueKey) {  #reset password form
>     #check if the link is valid
>-    my $sth = $dbh->prepare(
>-        "SELECT borrower_password_recovery.borrowernumber, userid
>-                              FROM borrower_password_recovery, borrowers
>-                              WHERE borrowers.borrowernumber = borrower_password_recovery.borrowernumber
>-                              AND NOW() < valid_until
>-                              AND uuid = ?"
>-    );
>-    $sth->execute($uniqueKey);
>-    ( $borrower_number, $username ) = $sth->fetchrow;
>+    ( $borrower_number, $username ) = GetValidLinkInfo($uniqueKey);
>+
>     if ( !$borrower_number ) {
>         $errLinkNotValid = 1;
>     }
>-warn "INLIBRO username $username";
>+
>     $template->param(
>         new_password    => 1,
>         minPassLength   => $minPassLength,
>@@ -175,56 +154,3 @@ else { #password recovery form (to send email)
> }
> 
> output_html_with_http_headers $query, $cookie, $template->output;
>-
>-#
>-# It creates an email using the templates and send it to the user, using the specified email
>-#
>-sub SendPasswordRecoveryEmail {
>-    my $borrower = shift; # from GetMember
>-    my $userEmail = shift; #to_address (the one specified in the request)
>-    my $query = shift; #only required to determine if 'http' or 'https'
>-    my $update = shift;
>-
>-    my $dbh = C4::Context->dbh;
>-
>-    # generate UUID
>-    my @chars = ("A".."Z", "a".."z", "0".."9");
>-    my $uuid_str;
>-    $uuid_str .= $chars[rand @chars] for 1..32;
>-
>-    # insert into database
>-    my $expirydate = DateTime->now(time_zone => C4::Context->tz())->add( days => 2 );
>-    if($update){
>-       my $sth = $dbh->prepare( 'UPDATE borrower_password_recovery set uuid=?, valid_until=? where borrowernumber=? ');
>-       $sth->execute($uuid_str, $expirydate->datetime(), $borrower->{'borrowernumber'});
>-    } else {
>-       my $sth = $dbh->prepare( 'INSERT INTO borrower_password_recovery VALUES (?, ?, ?)');
>-       $sth->execute($borrower->{'borrowernumber'}, $uuid_str, $expirydate->datetime());
>-    }
>-
>-    # create link
>-    my $protocol = $query->https() ? "https://" : "http://";
>-    my $uuidLink = $protocol . C4::Context->preference( 'OPACBaseURL' ) . "/cgi-bin/koha/opac-password-recovery.pl?uniqueKey=$uuid_str";
>-
>-    # prepare the email
>-    my $letter = C4::Letters::GetPreparedLetter (
>-        module => 'members',
>-        letter_code => 'PASSWORD_RESET',
>-        branchcode => $borrower->{branchcode},
>-        tables => {borrowers => $borrower},
>-        substitute => {passwordreseturl => $uuidLink},
>-    );
>-
>-    # define to/from emails
>-    my $kohaEmail = C4::Context->preference( 'KohaAdminEmailAddress' ); # from
>-
>-    C4::Letters::EnqueueLetter( {
>-         letter => $letter,
>-         borrowernumber => $borrower->{'borrowernumber'},
>-         to_address => $userEmail,
>-         from_address => $kohaEmail,
>-         message_transport_type => 'email',
>-    } );
>-
>-    return 1;
>-}
>-- 
>1.9.1
Comment 77 simith.doliveira 2015-03-11 18:02:53 UTC
Sorry for #76

Conflit fixed and patches squashed
Comment 78 Jason Burds 2015-03-30 15:36:51 UTC
The sandbox you've requested is not ready.
Some problems occurred applying patches from bug 8753:
<h1>Something went wrong !</h1>Applying: Bug 8753 - Add forgot password link to OPAC Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/includes/masthead.inc
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging installer/data/mysql/en/mandatory/sample_notices.sql
CONFLICT (content): Merge conflict in installer/data/mysql/en/mandatory/sample_notices.sql
Failed to merge in the changes.
Patch failed at 0001 Bug 8753 - Add forgot password link to OPAC

When you have resolved this problem run git bz apply --continue.
If you would prefer to skip this patch, instead run git bz apply --skip.
To restore the original branch and stop patching run git bz apply --abort.
Bug 8753 - Add forgot password link to OPAC

36819 - Bug 8753 - Add forgot password link to OPAC

Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/Bug-8753---Add-forgot-password-link-to-OPAC-VCLXC4.patch .
Comment 79 Blou 2015-04-03 19:15:31 UTC Comment hidden (obsolete)
Comment 80 Arturo 2015-04-13 16:59:43 UTC
Hi, all. My library recently switched to Koha, and this is my first attempt at testing a patch, but I am not sure if I am doing this correctly or if I've truly run into an issue.

I followed the instructions at http://wiki.koha-community.org/wiki/Sandboxes to test this patch, bug 8753. I set up Sandbox 6 from BibLibre succesfully, logged in correctly, and did not see any error messages in the 'news' area, so it seems the patch was applied successfully.

Before I could follow the test plan listed in this thread, I randomly selected a  patron (I chose Colin Campbell), set a password for him, and set his e-mail address to my e-mail address so that I'd receive the e-mail notifications. Then I followed the test plan. Everything seemed OK except that I never received the e-mail to reset the pw. I then remembered that by default, Koha installations do not have e-mail enabled. Could that be the reason? Or would it be safe to assume that e-mail is enabled in the BibLibre sandboxes? I couldn't find that information anywhere.

Thanks for any assistance anyonce can provide!
Comment 81 Jonathan Druart 2015-04-14 07:52:28 UTC
(In reply to Arturo from comment #80)
> Hi, all. My library recently switched to Koha, and this is my first attempt
> at testing a patch, but I am not sure if I am doing this correctly or if
> I've truly run into an issue.
> 
> I followed the instructions at http://wiki.koha-community.org/wiki/Sandboxes
> to test this patch, bug 8753. I set up Sandbox 6 from BibLibre succesfully,
> logged in correctly, and did not see any error messages in the 'news' area,
> so it seems the patch was applied successfully.

I have just tested to apply this patch on sandbox 6, I received an email within 5 minutes:
  sandbox is not ready !

There is a conflict with the patch.

Did you see the pref OpacResetPassword?
Comment 82 Arturo 2015-04-14 13:06:39 UTC
(In reply to Jonathan Druart from comment #81)
> (In reply to Arturo from comment #80)
> > Hi, all. My library recently switched to Koha, and this is my first attempt
> > at testing a patch, but I am not sure if I am doing this correctly or if
> > I've truly run into an issue.
> > 
> > I followed the instructions at http://wiki.koha-community.org/wiki/Sandboxes
> > to test this patch, bug 8753. I set up Sandbox 6 from BibLibre succesfully,
> > logged in correctly, and did not see any error messages in the 'news' area,
> > so it seems the patch was applied successfully.
> 
> I have just tested to apply this patch on sandbox 6, I received an email
> within 5 minutes:
>   sandbox is not ready !
> 
> There is a conflict with the patch.
> 
> Did you see the pref OpacResetPassword?

Yes, I did see the OpacResetPassword preference, turned it on along with the pref to allow patrons to change their passwords, and went to the homepage and saw the "Forgot my password" link. Below is the result I got when I set up the sandbox. It does seem to start off with a warning, perhaps ("Use of uninitialized value"), but the message does say that the patch for bug 8753 were applied successfully.

The sandbox you've requested is now ready.
The patches attached to bugzilla 8753 have been applied, and the result is:

Updatedatabase:
Use of uninitialized value  in substitution (s///) at /home/koha/src/installer/data/mysql/updatedatabase.pl line 9676.
Upgrade to 3.19.00.004 done (Bug 13346: OpacExportOptions is now multiple) Upgrade to 3.19.00.005 done (Bug 13379 - Modify authorised_values.category to varchar(32)) Upgrade to 3.19.00.006 done (Bug 11944 - Convert DB tables to utf8_unicode_ci) Upgrade to 3.19.00.007 done (Bug 12905: Check budget integrity: OK) Upgrade to 3.19.00.008 done (Bug 12601 - Add new foreign key aqorders.budget_id) Upgrade to 3.19.00.009 done (Bug 13007 - Add new foreign key suggestions.budgetid) Upgrade to 3.19.00.010 done (Bug 5511 - SessionRestrictionByIP) Upgrade to 3.19.00.011 done (Bug 13417: Add permission to delete public lists) Upgrade to 3.19.00.012 done (Bug 13523 - Remove NOT NULL restriction on field marcxml due to mysql STRICT_TRANS_TABLES) Upgrade to 3.19.00.013 done (Bug 11395: Add permission tools_records_batchmod) Upgrade to 3.19.00.014 done (Bug 12648: Add letter ACQ_NOTIF_ON_RECEIV ) Upgrade to 3.19.00.015 done (Bug 11430 - Add primary key for search_history) Upgrade to 3.19.00.016 done (Bug 13380: Add the ORDER_CANCELLATION_REASON authorised value) Upgrade to 3.19.00.017 done (Bug 5786 - Move AllowOnShelfHolds to circulation matrix; Move OPACItemHolds system preference to circulation matrix) Upgrade to 3.19.00.018 done (Bug 10328: Rename opaccolorstylesheet to OpacAdditionalStylesheet Upgrade to 3.19.00.019 done (Bug 9580: Cover image from Coce, a remote image URL cache) Upgrade to XXX done (Bug 8753: Add forgot password link to OPAC) If you use this system for the 1st time, you may find usefull to reach the wiki page http://wiki.koha-community.org/wiki/Sandboxes to have some information about those results, to check if everything went well & if you can test the patch properly
Comment 83 Fridolin Somers 2015-04-17 12:45:50 UTC
I test on sandbox 13, I have a conflict on installer/data/mysql/updatedatabase.pl
But i solved it manually
Comment 84 Fridolin Somers 2015-04-17 12:59:56 UTC
I've found :
opac-sendbasket.pl: Error sending mail: Bad or missing From address: ''

You need to specify an address in syspref KohaAdminEmailAddress before testing sending emails.
In UNIMARC database at least.

Works for me with a cart.
Comment 85 Fridolin Somers 2015-04-17 13:07:29 UTC
(In reply to Fridolin SOMERS from comment #84)
> You need to specify an address in syspref KohaAdminEmailAddress before
> testing sending emails.
> In UNIMARC database at least.
Same for MARC21 database
Comment 86 David Kuhn 2015-04-22 15:59:21 UTC
Patch not loading on sandbox:

The sandbox you've requested is not ready.
Some problems occurred applying patches from bug 8753:
<h1>Something went wrong !</h1>Applying: Bug 8753 - Add forgot password link to OPAC
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql
Auto-merging installer/data/mysql/kohastructure.sql
Failed to merge in the changes.
Patch failed at 0001 Bug 8753 - Add forgot password link to OPAC

When you have resolved this problem run git bz apply --continue.
If you would prefer to skip this patch, instead run git bz apply --skip.
To restore the original branch and stop patching run git bz apply --abort.
Bug 8753 - Add forgot password link to OPAC

37492 - Bug 8753 - Add forgot password link to OPAC

Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/Bug-8753---Add-forgot-password-link-to-OPAC-QS82WH.patch .
Comment 87 Liz Rea 2015-05-07 04:14:31 UTC
Hi,

I had a chance to take a look at this today. My comments are not code-level, they are "I applied this and had a look at the actual function" level. This is definitely a desirable feature, please don't give up. :)

I saw several issues with this when I was testing:

1. The patch won't apply, it has easily resolved conflicts in database structure and update database. I fixed them for my testing purposes, just wanted you to be aware.

1a. As noted elsewhere, the feature should go into Koha turned off, instead of turned on. We don't want surprises for our users.

2. In the bootstrap theme, we will want to have the "Forgot password" link on the home page beneath the login, on /cgi-bin/koha/opac-user.pl, as well as in the modal box. This can be a follow up.

3. The markup on the opac-password-recovery pages is not consistent with the rest of the OPAC in the Bootstrap theme. This will need to be fixed.

4. On the "reset password" phase ("Password recovery form") - I don't understand why the first field is "Surname in lower case" and the second field is "Confirm password" - the standard behaviour for functions like this is to have something like "Type password" and "repeat password." The function works, but it wasn't clear to me when testing that the "surname" field was actually supposed to be the initial go at the password. It may be a relic from the implementation for your client, but I don't think we want it to be default Koha behaviour for it to suggest the surname as a password. Please fix.

5. *Good thing* submitting the form, (when I realised that both fields are for passwords) works just fine, and the password is reset, and I was able to log in with no issue.

6. Having a mismatch in the passwords on the reset form seems to pop up a box asking me to confirm which user I am changing the password for - this doesn't seem right to me, please check. It doesn't seem to actually *work* but it's a bit scary to see, as it suggests that I could possibly change the password for a user that is not "me."

7. This functionality does not work when you have more than one borrower with the same email address, it presents "We chould not authenticate you as the account owner." I think we may need to use a combination of userid and email address to authenticate users. Userid's do have to be unique, maybe use that instead and pop an error if the user does not have a registered email address. 

8. *Good thing* the email seems to work fine. Maybe minor updates to the text from a native English speaker are needed, but the message and the link are fine. Any changes can come in a follow up.

I understand this is a lot of things to deal with - I think everyone in the community wants this patch/function to be successful, so please say if you need help or if you have any questions.

If you feel like you would rather not continue on with this development, please say so that someone else will feel better about picking up where you left off. 

Cheers,
Liz Rea
Comment 88 Liz Rea 2015-05-07 04:45:21 UTC
A couple of other things that I saw just after I posted the last message - 

- Please check the copyright - should it be InLibro or PTFS Europe?
- It occurs to me that by default, the message queue sends things every 15 minutes and that's a pretty long time in password-reset land.
Comment 89 Maxime Beaulieu 2015-05-11 13:01:30 UTC Comment hidden (obsolete)
Comment 90 Liz Rea 2015-05-11 21:16:27 UTC
Hi,

Spectacular, thanks I'll have another look. :)

Cheers,
Liz
Comment 91 Marc Véron 2015-05-13 15:53:11 UTC Comment hidden (obsolete)
Comment 92 Liz Rea 2015-05-13 22:17:47 UTC
Hi,
I tested this with opacpublic enabled and disabled - we probably need to modify the templates to not include the opacnav blocks if opacpublic is disabled. This can be a followup.

I'm not super keen on the syspref text, I may send a followup with text that more clearly differentiates the purpose of this new syspref from OpacPasswordChange.

Otherwise, thank you very much for the updated patch!

Liz
Comment 93 Liz Rea 2015-05-13 22:18:31 UTC Comment hidden (obsolete)
Comment 94 Liz Rea 2015-05-13 22:33:43 UTC Comment hidden (obsolete)
Comment 95 Liz Rea 2015-05-13 22:56:26 UTC Comment hidden (obsolete)
Comment 96 Liz Rea 2015-05-13 23:00:04 UTC Comment hidden (obsolete)
Comment 97 Jonathan Druart 2015-05-26 14:24:18 UTC
 FAIL   C4/Passwordrecovery.pm
   FAIL   pod
                *** ERROR: Spurious text after =cut  in file C4/Passwordrecovery.pm
*** ERROR: Spurious =cut command  in file C4/Passwordrecovery.pm


 FAIL   opac/opac-password-recovery.pl
   FAIL   valid
                shift on reference is experimental 
                Smartmatch is experimental 


 FAIL   koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth.tt
   FAIL   forbidden patterns
                forbidden pattern: tab char (line 155)
Comment 98 Rémi Mayrand-Provencher 2015-06-03 20:37:32 UTC Comment hidden (obsolete)
Comment 99 David Kuhn 2015-06-04 20:29:49 UTC
Following test plan, entered a valid email address and received this software error:

Undefined subroutine &C4::Members::Search called at /home/koha/src/opac/opac-password-recovery.pl line 63.

The same happens with a valid user name.
Comment 100 Liz Rea 2015-06-07 21:36:55 UTC
I get this error as well now too. I believe it was introduced by the pushing of bug 12633, which removed C4::Members::Search.

Cheers,
Liz
Comment 101 Liz Rea 2015-07-14 00:09:14 UTC
Hi,

Just wondering if you all at inLibro were going to come back to this bug?

Cheers,
Liz
Comment 102 Blou 2015-07-14 12:28:53 UTC
I think this bug's history shows we are ALWAYS coming back.

We shall fight on the beach,
we shall fight on the tests,
we shall fight on the bugs,
we shall never surrender.

(bur our resources might be stuck on another beachhead right now)
Comment 103 Liz Rea 2015-07-14 21:02:20 UTC
(In reply to Blou from comment #102)
> I think this bug's history shows we are ALWAYS coming back.
> 
> We shall fight on the beach,
> we shall fight on the tests,
> we shall fight on the bugs,
> we shall never surrender.
> 
> (bur our resources might be stuck on another beachhead right now)

This is possibly the best bug reply ever.

:D
Comment 104 Maxime Beaulieu 2015-07-24 20:20:18 UTC Comment hidden (obsolete)
Comment 105 Marc Véron 2015-07-26 09:54:34 UTC Comment hidden (obsolete)
Comment 106 Liz Rea 2015-07-26 22:11:10 UTC Comment hidden (obsolete)
Comment 107 Tim Hannah 2015-08-05 19:32:07 UTC
Had a look at this, first time using kohadevbox and git bz, so could be something I did or didn't do. Looked good, no stray text, but when I tried resetting a password I got error message 

DBIx::Class::ResultSet::next(): Table koha_kohadev.borrower_password_recovery' doesn't exist at /home/vagrant/kohaclone/C4/Passwordrecovery.pm line 70

Any clues?
Comment 108 Katrin Fischer 2015-08-09 22:45:55 UTC
Hi Tim, not sure if that's the reason - but did you run the database update? you can run updatedatabase.pl from the command line or change kohaversion.pl to +1 and then after the update is run change it back to the number it had before.
Comment 109 Katrin Fischer 2015-08-09 22:58:11 UTC
... that should read 'add 1 to the version number at the end...' or similar :)
Comment 110 Aleisha Amohia 2015-08-10 04:50:46 UTC
The system breaks for me if I don't provide an email or username (leave both spaces empty) to send the reset link to.
Get this error: opac-password-recovery.pl: Can't use an undefined value as an ARRAY reference at /home/vagrant/kohaclone/opac/opac-password-recovery.pl line 66
Comment 111 Aleisha Amohia 2015-08-12 02:40:12 UTC
Also could be a good idea to fix the title in the metadata - at the moment it says <title>Waitaki District Libraries catalog
Comment 112 Liz Rea 2015-08-12 04:28:56 UTC Comment hidden (obsolete)
Comment 113 Liz Rea 2015-08-12 04:31:09 UTC
I wasn't able to replicate Aleisha's 2nd issue, it may be a local issue to her environment.

Liz
Comment 114 Aleisha Amohia 2015-08-12 06:35:31 UTC
Signing off! Works for me
Comment 115 Jonathan Druart 2015-08-19 15:51:13 UTC
 FAIL   opac/opac-password-recovery.pl
   FAIL   spelling
                 doesnt  ==> doesn't
   FAIL   valid
                defined(@array) is deprecated 
                Smartmatch is experimental 
                shift on reference is experimental 
                        (Maybe you should just omit the defined()?)

 FAIL   C4/Passwordrecovery.pm
   FAIL   pod
*** ERROR: Spurious text after =cut  in file C4/Passwordrecovery.pm
*** ERROR: Spurious =cut command  in file C4/Passwordrecovery.pm

 FAIL   koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-password-recovery.tt
   FAIL   spelling
                 occured  ==> occurred
Comment 116 Liz Rea 2015-08-19 22:25:11 UTC Comment hidden (obsolete)
Comment 117 Liz Rea 2015-08-19 22:45:55 UTC Comment hidden (obsolete)
Comment 118 Liz Rea 2015-08-19 23:20:06 UTC Comment hidden (obsolete)
Comment 119 Jonathan Druart 2015-08-20 10:05:00 UTC
Does the status should be Signoff?

I would say we should not provide any information if the user does not fill correct login or email.
The message should be generic "Bad info" instead of "Bad login" or "Bad email".
Comment 120 Jonathan Druart 2015-08-20 10:06:10 UTC
From perldoc rand:

rand() is not cryptographically secure. You should not rely on it in security-sensitive situations. As of this writing, a number of third-party CPAN modules offer random number generators intended by their authors to be cryptographically secure, including: Data::Entropy, Crypt::Random, Math::Random::Secure, and Math::TrulyRandom.
Comment 121 Jonathan Druart 2015-08-20 10:06:50 UTC Comment hidden (obsolete)
Comment 122 Jonathan Druart 2015-08-20 10:11:47 UTC
1/ Receive the email
2/ Click on the link
3/ Change the pwd
4/ Click again on the link
5/ Change the pwd
6/ You get an error

The error should appear at step 5
Comment 123 Jonathan Druart 2015-08-20 10:13:31 UTC
comment 21 did not get an answer. For instance tests are still missing.
Comment 124 Chris Rohde 2015-08-27 19:59:06 UTC
Glad to see recent movement on this.  Realize it's already made it to QA, but if there's anything I/my library can do to help or just encourage, I'm up for it.
Comment 125 Charles Farmer 2015-10-01 15:58:11 UTC Comment hidden (obsolete)
Comment 126 Liz Rea 2015-10-07 03:39:52 UTC Comment hidden (obsolete)
Comment 127 David Kuhn 2015-10-17 22:10:21 UTC
I get the following error message when I try to install patch:

Some problems occurred applying patches from bug 8753:
<h1>Something went wrong !</h1>Applying: Bug 8753 - Add forgot password link to OPAC
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-main.tt
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth.tt
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/includes/masthead.inc
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
Auto-merging installer/data/mysql/en/mandatory/sample_notices.sql
CONFLICT (content): Merge conflict in installer/data/mysql/en/mandatory/sample_notices.sql
Failed to merge in the changes.
Patch failed at 0001 Bug 8753 - Add forgot password link to OPAC
Comment 128 Liz Rea 2015-10-18 20:30:54 UTC Comment hidden (obsolete)
Comment 129 Liz Rea 2015-10-18 20:48:56 UTC
That latest one should resolve the merge issue.
Comment 130 Liz Rea 2015-11-03 00:46:06 UTC Comment hidden (obsolete)
Comment 131 Liz Rea 2015-11-03 00:46:35 UTC Comment hidden (obsolete)
Comment 132 Liz Rea 2015-11-03 00:46:57 UTC Comment hidden (obsolete)
Comment 133 Liz Rea 2015-11-03 00:47:14 UTC Comment hidden (obsolete)
Comment 134 Liz Rea 2015-11-03 00:47:30 UTC Comment hidden (obsolete)
Comment 135 Liz Rea 2015-11-03 00:47:49 UTC Comment hidden (obsolete)
Comment 136 Liz Rea 2015-11-03 00:48:06 UTC Comment hidden (obsolete)
Comment 137 Liz Rea 2015-11-03 00:49:51 UTC
I just squashed my followups to make this a bit tidier, where I could.
Comment 138 Marc Véron 2015-11-03 10:49:59 UTC
Comment on attachment 41192 [details]
Stray text appears above header

This is fixed.
Comment 139 Marc Véron 2015-11-03 11:04:43 UTC Comment hidden (obsolete)
Comment 140 Marc Véron 2015-11-03 11:06:42 UTC Comment hidden (obsolete)
Comment 141 Marc Véron 2015-11-03 11:07:44 UTC Comment hidden (obsolete)
Comment 142 Marc Véron 2015-11-03 11:08:43 UTC Comment hidden (obsolete)
Comment 143 Marc Véron 2015-11-03 11:09:45 UTC Comment hidden (obsolete)
Comment 144 Marc Véron 2015-11-03 11:21:11 UTC Comment hidden (obsolete)
Comment 145 Marc Véron 2015-11-03 11:22:41 UTC Comment hidden (obsolete)
Comment 146 Liz Rea 2015-11-27 01:12:02 UTC
This is a must for 3.24.
Comment 147 Josef Moravec 2015-11-27 12:19:30 UTC
I agree, but rebase needed...
Comment 148 Blou 2015-11-27 13:32:56 UTC
Will be done today.

We will never surrender...
Comment 149 Charles Farmer 2015-11-27 14:26:07 UTC Comment hidden (obsolete)
Comment 150 Charles Farmer 2015-11-27 14:28:52 UTC Comment hidden (obsolete)
Comment 151 Charles Farmer 2015-11-27 14:29:07 UTC Comment hidden (obsolete)
Comment 152 Charles Farmer 2015-11-27 14:29:15 UTC Comment hidden (obsolete)
Comment 153 Charles Farmer 2015-11-27 14:29:23 UTC Comment hidden (obsolete)
Comment 154 Charles Farmer 2015-11-27 14:29:31 UTC Comment hidden (obsolete)
Comment 155 Charles Farmer 2015-11-27 14:29:41 UTC Comment hidden (obsolete)
Comment 156 Charles Farmer 2015-11-27 14:33:04 UTC
I had to resubmit every patches because the first one had the merge conflicts.

If anybody knows how to obsolete the first patch of a queue without messing the queue order, let me know, it would be appreciated.
Comment 157 Josef Moravec 2015-11-27 22:07:49 UTC
I think that files kohastructure.sql and sysprefs.sql, should be updated too, not only to add atomicupdate sql file...
Comment 158 Charles Farmer 2015-11-30 14:26:47 UTC Comment hidden (obsolete)
Comment 159 Charles Farmer 2015-11-30 14:26:58 UTC Comment hidden (obsolete)
Comment 160 Charles Farmer 2015-11-30 14:27:07 UTC Comment hidden (obsolete)
Comment 161 Charles Farmer 2015-11-30 14:27:16 UTC Comment hidden (obsolete)
Comment 162 Charles Farmer 2015-11-30 14:27:24 UTC Comment hidden (obsolete)
Comment 163 Charles Farmer 2015-11-30 14:27:31 UTC Comment hidden (obsolete)
Comment 164 Charles Farmer 2015-11-30 14:27:39 UTC Comment hidden (obsolete)
Comment 165 Charles Farmer 2015-11-30 14:27:47 UTC Comment hidden (obsolete)
Comment 166 Charles Farmer 2015-11-30 14:30:28 UTC
(In reply to Josef Moravec from comment #157)
> I think that files kohastructure.sql and sysprefs.sql, should be updated
> too, not only to add atomicupdate sql file...

The changes brought by the atomicupdate file are now visible in the kohastructure and sysprefs files.
Comment 167 Liz Rea 2016-01-07 02:18:08 UTC Comment hidden (obsolete)
Comment 168 Kyle M Hall 2016-01-08 18:43:03 UTC
I don't think C4::Passwordrecovery is a good namespace. Should this not be something like Koha::Patron::Password::Recovery or Koha::Borrower::Password::Recovery? We should not be adding modules to the C4 namespace at this point.
Comment 169 Jonathan Druart 2016-01-11 15:47:02 UTC
Koha::Patron please :)
Comment 170 Liz Rea 2016-01-11 22:12:40 UTC
May we have this heavily tested version in, and move it to the new namespace in a follow up? This is an egregiously missing feature, and one that we have been waiting a long, long time for.

It's for the users, let's get it in.

Liz
Comment 171 David Cook 2016-01-11 22:42:45 UTC
As the local pedant in the Koha community, I'm with Liz on this one.

While we shouldn't be adding anything to the C4 namespace, I think perhaps an exception should be made for this particular patch set, as the feature is rather important in modernizing Koha.

Out in the world, there's pretty much no service where you can't reset your own password. Yet, you can't do it in Koha. Surely this frustrates users and puts a burden on librarians. It's an aspect of the modern web experience which Koha fails at.

This patch set has been a long time coming. While it should be fairly easy to do a search and replace through the patches, mistakes do happen, and we'd probably need to re-test all the patches again to make sure that nothing was missed. It seems like it's already been tested to the Nth degree, so perhaps a pass should be made this one time.

I suppose it would qualify as technical debt, and I hate technical debt. But this does seem to be an extraordinary case. If it were anything but a password reset, I'd say change the namespace. But this is such a missing piece of functionality in Koha, that I think the faster we get it in the better. 

Of course, I defer to the QA team and the RM. But just adding that perspective to the comments.
Comment 172 Tim Hannah 2016-01-11 23:26:34 UTC
As a librarian I can confirm that the lack of this feature frustrates me, my colleagues and our users. We've been waiting for this for a while and would dearly love to have it sooner rather than later.

I can't pretend to understand the ramifications of an imperfect namespace choice, but I know we currently lose users due to them being locked out and going elsewhere rather than approaching us for resets or reminders.

If it can be released as is and fixed later, as Liz suggests, I for one would be very grateful.

(And thanks for everyone's work on it. Looking forward to it, whenever we get it).

Cheers
Comment 173 Marc Véron 2016-01-12 08:52:28 UTC
+1 for getting it in!
Comment 174 Katrin Fischer 2016-01-12 08:59:06 UTC
After thinking this over for a bit I would be in favor of pushing it as it is. This is a long needed development that probably predates most of the work in the Koha namespace. Changing the namespace now would require more testing and it might give us a lot of conflicts in other places. So it might be easier to fix the namespace separately after this is pushed.
Comment 175 Magnus Enger 2016-01-12 09:05:18 UTC
+1 for getting this in ASAP. :-)
Comment 176 Blou 2016-01-12 13:03:59 UTC
In the name of the whole personnel at inLibro, which I've cycled through to support this patch in the past 2.5 years (so many good people fell on the battlefield, see comment #102), I say "yeah! it's about time!".

Of course, we're now at 180 comments and counting, so there's a side of me that would like to see that keep increasing and beat a record of some sort.  ;-)
Comment 177 Kyle M Hall 2016-01-14 19:32:46 UTC
(In reply to Katrin Fischer from comment #174)
> After thinking this over for a bit I would be in favor of pushing it as it
> is. This is a long needed development that probably predates most of the
> work in the Koha namespace. Changing the namespace now would require more
> testing and it might give us a lot of conflicts in other places. So it might
> be easier to fix the namespace separately after this is pushed.

If it doesn't get fixed now, it won't get fixed. This patch was submitted long after the creation of the Koha namespace. Changing the namespace is not that onerous a request. We can't allow code in that isn't fit just because we like the feature itself.
Comment 178 Blou 2016-01-14 19:42:52 UTC
Noooooo!!!  Not the dreaded "In Discussion" !!

I would have prefered Failed QA.

Not sure what is left to discuss.  Everyone already chimed in.  But I respect the QA's right to fail it.

At this point, since it would require "full retest", I will ask Charles to fix it and SQUASH everything.  Considering the amount of support this feature has, I suppose a re-signoff wouldn't be long.  *sigh*
Comment 179 Chris Cormack 2016-01-14 21:01:15 UTC
*sigh*

Oh well, can't be helped I guess. I will test the new patch as soon as it is up.

Hopefully this new super strict QA standard will be across the board and I won't see people pushing patches they wrote skipping qa steps anymore.

Switching back to assigned, while we wait for the new patch.
Comment 180 Marcel de Rooy 2016-01-15 07:15:36 UTC
(In reply to Chris Cormack from comment #179)
> Hopefully this new super strict QA standard will be across the board and I
> won't see people pushing patches they wrote skipping qa steps anymore.

I do not favor a super strict QA standard. Common_sense++
Comment 181 Chris Cormack 2016-01-15 08:56:55 UTC
(In reply to Marcel de Rooy from comment #180)
> (In reply to Chris Cormack from comment #179)
> > Hopefully this new super strict QA standard will be across the board and I
> > won't see people pushing patches they wrote skipping qa steps anymore.
> 
> I do not favor a super strict QA standard. Common_sense++

Neither do I, but if we are going to be super strict about this one, we need to at least be consistent. That was my point.
Comment 182 Jonathan Druart 2016-01-15 10:43:12 UTC
Kyle, can we imagine to make an exception for this one, open a new bug report to move the new module to the Koha namespace and ask for the author to commit to submit a patch?
Comment 183 Jonathan Druart 2016-01-15 10:46:17 UTC
(In reply to Blou from comment #176)
> Of course, we're now at 180 comments and counting, so there's a side of me
> that would like to see that keep increasing and beat a record of some sort. 
> ;-)

Don't worry about that, we won't reach it (there are 247 comments on bug 7167 and it as been abandoned, there is another one with more than 500 comments iirc).
Comment 184 Kyle M Hall 2016-01-15 12:45:39 UTC
(In reply to Jonathan Druart from comment #182)
> Kyle, can we imagine to make an exception for this one, open a new bug
> report to move the new module to the Koha namespace and ask for the author
> to commit to submit a patch?

Yes! I set this bug to in discussion while we were actually discussing the issue. I purposefully did not set it to failed qa.

My fear is that if this is pushed as is, there is no impetus for the namespace to be fixed. All I'd like to see is a commitment from someone that the patches to move this to a better namespace *will be submitted*, preferably before the next major release if possible. So far I haven't seen anyone make that commitment. I should have written that more clearly than I had.

Philippe, if you can vouch that Charles or someone will submit the patch to move this module to a new namespace, I have no problem with it proceeding as is. Now would be the time to file the followup bug report and assign it to the proper person. If this works for you please go ahead and reset the status to Signed Off.
Comment 185 Blou 2016-01-15 13:47:26 UTC
This is not a problem for us.  To be sure, just create the new bug and assign it to me immediately.  I assure you you'll get a patch within a week after this has been pushed to master.
Comment 186 Kyle M Hall 2016-01-15 14:28:01 UTC
(In reply to Blou from comment #185)
> This is not a problem for us.  To be sure, just create the new bug and
> assign it to me immediately.  I assure you you'll get a patch within a week
> after this has been pushed to master.

Thanks! I'll go ahead and take care of that!
Comment 187 Kyle M Hall 2016-01-15 14:37:40 UTC
(In reply to Chris Cormack from comment #179)
> Hopefully this new super strict QA standard will be across the board and I
> won't see people pushing patches they wrote skipping qa steps anymore.

I'd like to address this. I'm not in favor of super strict QA standards. What I'm in favor of are transparent, standardized, and consistently applied QA rules.

My experience as a developer has informed my work as a QA team member. I have had to rewrite C4 modules as Koha modules, I've asked others to rewrite C4 modules as Koha modules. Code quality guidelines should be completely divorced from the necessity or popularity of a bugfix or feature.

I think our coding guidelines need a bit of a revamp to make sure all of the things we've taken for granted as being necessary to pass QA are actually in the coding guidelines. If a QA team member fails QA on a patch, the QA'er should be able to link directly to the reason and explanation for it failing QA. If the QA'er believes the patch should fail QA but the reason is not part of the coding guidelines, then a meeting should be called, a vote should be made, and a new rule added to the guidelines.
Comment 188 Katrin Fischer 2016-01-15 14:48:31 UTC
While I have put coding guideline additions on the agenda a lot of times in the past, I don't believe that we can ever have a complete set of rules that will fit every situation and possible question.

Another common practice we QA has used where they haven't been sure about something is asking for more opinions - I think this has helped in a lot of cases to get more views and a better picture of the problem and possible solutions.

That said, I am happy with the 2 step approach here.
Comment 189 Kyle M Hall 2016-01-15 15:12:17 UTC
(In reply to Katrin Fischer from comment #188)
> While I have put coding guideline additions on the agenda a lot of times in
> the past, I don't believe that we can ever have a complete set of rules that
> will fit every situation and possible question.
> 
> Another common practice we QA has used where they haven't been sure about
> something is asking for more opinions - I think this has helped in a lot of
> cases to get more views and a better picture of the problem and possible
> solutions.

That simply highlights the problem. With this methodology, a developer will not know if code will pass qa or not until it has been written and submitted.

I agree, we will never have a complete set of rules, but we can build that set as we go. The more we have written down for developers to be aware in advance, the better. We need documented guidelines. The approach of "this doesn't feel right to me" just doesn't work any more. It can only lead to future frustration for developers. We can't have one set of rules for bug A and another set of rules for bug B.
Comment 190 Katrin Fischer 2016-01-15 15:46:40 UTC
I think we are actually not in disagreement - I just want to warn about wanting to solve everything with rules. That will be trying for the impossible. And a 20 pages catalog of guidelines... is kind of scaring away devs too.
Comment 191 Kyle M Hall 2016-01-15 19:52:59 UTC
(In reply to Katrin Fischer from comment #190)
> I think we are actually not in disagreement - I just want to warn about
> wanting to solve everything with rules. That will be trying for the
> impossible. And a 20 pages catalog of guidelines... is kind of scaring away
> devs too.

Lol, yeah, I agree with that too. I just think at the moment we have too few guidelines on paper and too many guidelines only in the heads of the QA team ; )
Comment 192 Kyle M Hall 2016-01-15 20:03:36 UTC
I also want to add that the guidelines would be an explanation of they something failed qa. Instead of "Here is a link to the community decided reason" it is currently more of a "because I said so" situation. I personally dislike failing qa on something without having those reasons. I'm far more apt to submit follow ups to avoid doing so.
Comment 193 Marcel de Rooy 2016-01-22 09:13:20 UTC
QA: Working on this one..
Comment 194 Marcel de Rooy 2016-01-22 09:47:38 UTC
Created attachment 47164 [details] [review]
Bug 8753 - Add forgot password link to OPAC

I've addressed a lot of Liz Rea's points.

1.  I have moved the code from updatedatabase.pl and kohastructure.sql to a file in the atomicupdates directory.
1a. The feature is now off by default when the atomicupdate is run.

2.  The password reset link is now visible on the home page, in the modal box and on opac-user.pl .

3.  The password recovery pages now use bootstrap markup.

4.  I am unsure here. I see "New Password:" and "Confirm new password:".

5.  This should still work :).

6.  I could not reproduce.

7.  I have added the userid field.
    You can now reset the password by submitting either your useid or email address.
    Both fields can be filled, but the email address must be one of the borrower's (email, emailpro or b_email).
    When entering only the email address and two borrowers use that same address, the system tells the user to try with another address or to specify his userid.

8.  The text is in the atomicupdate file. Have at it, anyone.

Concerning the email. It is inconvenient for the use to have to wait X minutes for the message queue the be processed.
Maybe we could add a sub in Letters.pm that:
    Takes the same argments as EnqueueLetter
    Sends the letter.
    Saves the letter in the message queue with a 'sent' status.

 TEST PLAN:

Setup)
    1) apply the patch
    2) go to system preferences OPAC>>Privacy and set 'OpacResetPassword' to ON.
    2b) make sure that OpacPasswordChange is also ON.
A)
    1) refresh front page, click on 'Forgot your password' and enter a VALID address
    1b) Also try an INVALID address (valid yet not in your koha db).  An error message will show up.
    2) An email should be received at that address with a link.
    3) Follow the link in the mail to fill the new password.
    Until a satisfactory new password is entered, the old password is not reset.
    4) Go to main page try the new password.
B)
    1) Repeat the password reset, this time use the userid (username) field.
    2) Try to reset the password using a userid and an email not linked to the account. An error appears.
    3) Make sure the borrower has many available email addresses.
    4) For each email, reset the password using both the userid and the email. The link should be sent to the specified address
C)
    1) Make sure two borrowers use the same email.
    2) Repeat the reset procedure in test case A). An error message appears

http://bugs.koha-community.org/show_bug.cgi?id=13068

 Author:    Maxime Beaulieu <maxime.beaulieu@inlibro.com>

Followed test plan. Works as described.
Signed-off-by: Marc Veron <veron@veron.ch>

New sign-off after testing all patches together
Signed-off-by: Marc Veron <veron@veron.ch>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 195 Marcel de Rooy 2016-01-22 09:47:47 UTC
Created attachment 47165 [details] [review]
Bug 8753 - Followup - change value text on syspref

Followup changes text from "The user can reset | can not reset their password on OPAC" to "Library users are allowed | not allowed to recover their password via e-mail in the OPAC"

This change more clearly differentiates the purpose of this new preference from OpacPasswordChange.

Bug 8753 - followup - update text for link to match common UI paradigms, fixes OpacPublic disabled view

Also corrects OpacNav being included on the reset page on private catalogues.

Updated the link for forgotten passwords to more closely match common UI paradigms, i.e. Facebook and Twitter

To test:
apply all patches, link should now be the less verbose "Forgot your password?"
disable OpacPublic, anything in opacnav should not appear (you may need to add something to opacnav to test properly)

Bug 8753 - [followup] fix the title on opac-password-recovery.tt

The title stanza was missing a <title></title> around it, causing the extra text to appear.

To test, apply all patches and make sure it looks ok and there is no extra text at the top or bottom of the page.

Bug 8753 - [followup} Correcting spelling mistakes

Make sure it all still works

Bug 8753 - [followup] fix error when no information is provided

To test:

All normal checks plus make sure that a nice error is displayed when no data is provided.

fixing the deprecated thing

Signed-off-by: Marc Veron <veron@veron.ch>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 196 Marcel de Rooy 2016-01-22 09:47:56 UTC
Created attachment 47166 [details] [review]
Bug 8753 - Use Koha::Borrowers instead of C4::Members

Use the new library to search for borrowers.
Changed how the $borrower variable is used since it is now a Koha::Borrower object.

Removed the $protocol parameter from the generated link. It should be included in the OPACBaseURL syspref.

	modified:   C4/Passwordrecovery.pm
	modified:   opac/opac-password-recovery.pl

Signed-off-by: Marc Veron <veron@veron.ch>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 197 Marcel de Rooy 2016-01-22 09:48:03 UTC
Created attachment 47167 [details] [review]
Bug 8753 - [followup] POD cleanup

Check to make sure the messages from the qa checker are gone.

Note I'm not sure about this one because my test tools are apparently broken, and I can't get the error to manifest.

Amended patch by Jonathan Druart: Add a blank line before =head2
It now passes.

Signed-off-by: Marc Veron <veron@veron.ch>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 198 Marcel de Rooy 2016-01-22 09:48:11 UTC
Created attachment 47168 [details] [review]
Bug 8753 - Smartmatch substitute, Math::Random::Secure, Perltidy, Passwordrecovery.t

This is a collection of changes taken from different comments (but mostly comment 21 and comment 122).

Passes qa and prove, on my machine at least.

There's also a new test file, Passwordrecovery.t, which covers every method of C4::Passwordrecovery.

To test:

All normal checks plus :

    1/ Receive the email
    2/ Click on the link
    3/ Change the pwd
    4/ Click again on the link
    5/ You should immediately get an error message

Problems with Math/Random/Secure.pm, is solved in following patch, signing off
Signed-off-by: Marc Veron <veron@veron.ch>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 199 Marcel de Rooy 2016-01-22 09:48:19 UTC
Created attachment 47169 [details] [review]
Bug 8753 - Various little things - removing new dependency, changes to errors, textual updates

Koha already has a sub that creates salts, so lets use that instead of math::Random::secure, so as not to add a new dependency.

Made the references to "Forgotten password" consistent, including adding it to the title of the page.

Also removed the individual error for "this email doesn't belong to this account" as that could expose the existence of a login, which I think we'd rather not do.

Made some of the text more grammatically correct, and more library specific.

To test:

Apply on top of all of the other patches.

All the usual checks, plus make sure there are no typos in any text references.

Signed-off-by: Marc Veron <veron@veron.ch>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 200 Marcel de Rooy 2016-01-22 09:48:29 UTC
Created attachment 47170 [details] [review]
Bug 8753 - propagating the changes to kohastructure.sql and sysprefs.sql

Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Looks good with a new install.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 201 Marcel de Rooy 2016-01-22 09:48:38 UTC
Created attachment 47171 [details] [review]
Bug 8753: [QA Follow-up] Primary key and collation

This patch includes:
[1] Adds primary key borrowernumber to new table.
[2] Fixes collation.
[3] Removes manual PK in DBIx schema file.
[4] Fixes typo CompletePasswordRevovery.
[5] Removes use strict from opac-password-recovery; Modern::Perl is used.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 202 Marcel de Rooy 2016-01-22 09:52:12 UTC
QA Comment:
With reference to the discussion above (and time passed), I am moving the status to Passed QA. 
Looks good to me. Added a small follow-up.
An important detail to fix before the new release is: New notice should be added in all language files! Please fix that asap in a follow-up report after this one has been pushed. This would normally be a reason to Fail QA on itself...
Note to RM: Watch the conflict with bug 15548 (Borrower vs Patron); and the promised fix under 15585.

Minor points:
Spotted "FIXME: That ugly shift-grep contraption".
The term "Forgotten password recovery"  sounds funny to me (no native speaker :) Is Password recovery not sufficient?
Are dots in the unique key (especially at the end) user friendly? 

Passed QA
Comment 203 Brendan Gallagher 2016-01-27 06:46:23 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks!
Comment 204 Katrin Fischer 2016-01-27 07:31:55 UTC
Blou - please don't forget the 2 follow-ups for the namespace and the notices (last comment from Marcel!)
Comment 205 Marcel de Rooy 2016-01-27 12:17:14 UTC
(In reply to Katrin Fischer from comment #204)
> Blou - please don't forget the 2 follow-ups for the namespace and the
> notices (last comment from Marcel!)

Yes. Please add the bug number in the See also or Blocks too.
Thnx
Comment 206 Marcel de Rooy 2016-01-27 16:05:44 UTC
Brendan
It seems that you forgot to move the atomicupdate revision to the code in updatedatabase.
Currently, part of your job as RM :)

Marcel
Comment 207 Brendan Gallagher 2016-01-27 17:05:40 UTC
(In reply to Marcel de Rooy from comment #206)
> Brendan
> It seems that you forgot to move the atomicupdate revision to the code in
> updatedatabase.
> Currently, part of your job as RM :)
> 
> Marcel

Yes thanks.
Comment 208 Kyle M Hall 2016-01-27 18:30:00 UTC Comment hidden (obsolete)
Comment 209 Katrin Fischer 2016-01-27 22:56:59 UTC
Created attachment 47366 [details] [review]
[PASSED QA] Bug 8753 [QA Followup] - Add notice to translations

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 210 Brendan Gallagher 2016-01-27 23:46:00 UTC
Follow up Pushed (Jan 27 - 2016)
Comment 211 Charles Farmer 2016-01-29 14:16:10 UTC
The follow-up patch to change the namespace of this development is in bug 15585
Comment 212 Mubassir Ahsan 2016-04-29 07:04:11 UTC
Comment on attachment 47164 [details] [review]
Bug 8753 - Add forgot password link to OPAC

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

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
@@ +357,3 @@
>              - pref: OPACPatronDetails
>                choices:
>                    yes: Allow

When I am applying this (v.3.22.06), OPAC Global preference page (cgi-bin/koha/admin/preferences.pl?tab=opac) is showing the following error 

Syck parser (line 356, column 0): syntax error at /usr/lib/perl5/YAML/Syck.pm line 72.

I have also tried with changing the syntax as 
    - pref: OpacResetPassword
              default: 1
              choices:
                  yes: "can reset"
                  no: "can not reset"
			-The user can reset their password on OPAC.	
		-
But no luck. 
Please review.
Comment 213 Katrin Fischer 2016-04-29 07:27:45 UTC
The patches here have been written for the next version 16.05 (3.24) and are not supposed to work with any earlier code. It's a completely new feature that will only be available in new versions.