The password recovery process triggers an e-mail to the patron with a link to reset his or her password. This e-mail is put into the message queue like any other notification (e.g. filled HOLD) and hence only sent the next time process_message_queue.pl is triggered with the time setting in cron.d (the default here being every 15 minutes!). This behavior is not what a user would expect from such a function (especially not with the wording from opac-password-recovery.tt: 'An email **has been sent**'). Accordingly -- even though this is not a malfunction -- I flagged this a bug and not an enhancement, as from the point of view of the patron it must seem to be a bug/problem if the e-mail does not arrive in a normal time span.
Yes, we would really like to see this as well. Bug 17648 remarks on how the ACCTDETAILS notice bypasses message_queue, and is sent directly to the linux mail queue (consequently, the message isn't logged in notices.pl). It would be good if the password recovery e-mail behaved in the same way. An instant e-mail is the response the patron is looking for, and staff do not need to see how many times a password reset has been requested by the patron; it is unnecessary clutter in the notices.pl
All our notifications should be sent through the same process (message_queue). What we should do is using a daemon to watch this table.
(In reply to Jonathan Druart from comment #2) > All our notifications should be sent through the same process > (message_queue). > > What we should do is using a daemon to watch this table. I agree, but at the moment a shorter interval breaks CHECKOUT and CHECKIN - those need to be handled separately or be rewritten in a better way.
(In reply to Katrin Fischer from comment #3) > (In reply to Jonathan Druart from comment #2) > > All our notifications should be sent through the same process > > (message_queue). > > > > What we should do is using a daemon to watch this table. > > I agree, but at the moment a shorter interval breaks CHECKOUT and CHECKIN - > those need to be handled separately or be rewritten in a better way. Maybe send such mails immediately and add the message with status "sent" to the table?
If someone plans to work on it, please contact us on the list first :)
(In reply to Marc Véron from comment #4) > (In reply to Katrin Fischer from comment #3) > > (In reply to Jonathan Druart from comment #2) > > > All our notifications should be sent through the same process > > > (message_queue). > > > > > > What we should do is using a daemon to watch this table. > > > > I agree, but at the moment a shorter interval breaks CHECKOUT and CHECKIN - > > those need to be handled separately or be rewritten in a better way. > > Maybe send such mails immediately and add the message with status "sent" to > the table? CHECKIN and CHECKOUT need to be sent when the circulation session is over ideally, so you have one email with all information. For the other emails - that would work, but feels like a bit of a workaround.
I wonder if this (sending of password recovery messages as well as CHECKIN/-OUT notices) could not be achieved by putting them in the message queue (i.e. the default behaviour) and then have misc/cronjobs/process_message_queue.pl triggered 'manually' by the relevant script. For CHECKIN/CHECKOUT this would obviously happen rather often (maybe too often?), but for password recovery -- rarely more than once an hour -- I can't see how it would pose any problems.
Created attachment 72499 [details] [review] Patch for bug 18570 Adds a new subroutine to C4/Letters.pm which sends the password reset email to the borrower through the message queue immediately. To test, set the syspref "OpacResetPassword" to "allowed", create a patron with an email address in their record, then click the "Forgotten password" link on the login screen. The new subroutine will match on borrowernumber and letter_code='PASSWORD_RESET' in the message queue and process the single entry, marking it as sent. If your mail server is not enabled, check the mail server logs to see the attempted send (e.g. /var/log/exim4/mainlog)
Comment on attachment 72499 [details] [review] Patch for bug 18570 Review of attachment 72499 [details] [review]: ----------------------------------------------------------------- ::: C4/Letters.pm @@ +1080,5 @@ > +Messages from the `message_queue` table are matched on borrowernumber and letter_code='PASSWORD_RESET' > + > +=cut > + > +sub SendPasswordEmailFromQueue { I believe this is the wrong way to do it. I think building a hash ref in SendQueuedMessages and using the built hashref on the _get_unsent_messages call would be better than introducing this, and this fails to handle how to send messages. After all, calling it with a borrower number and a letter code in the params hash ref should be easy enough to add, and then you could simply tweak the t/db_dependent/Letters.t test to test for the password reset case. @@ +1299,4 @@ > my @query_params = ('pending'); > if ( ref $params ) { > if ( $params->{'message_transport_type'} ) { > + $statement .= ' AND mq.message_transport_type = ? '; Not necessary, but okay. There is only one message_transport_type field. @@ +1303,4 @@ > push @query_params, $params->{'message_transport_type'}; > } > if ( $params->{'borrowernumber'} ) { > + $statement .= ' AND mq.borrowernumber = ? '; Not necessary, but okay. The LEFT JOIN makes sure the number is the same, and there is only one borrowernumber in the SELECT list of the SQL statement.
Failed QA for one major reason: Missing test coverage. Do not modify C4 or Koha libraries without including tests. Also, because I think it is the wrong way to do it, but test coverage either way.
(In reply to M. Tompsett from comment #10) > Failed QA for one major reason: Missing test coverage. > > Do not modify C4 or Koha libraries without including tests. > Also, because I think it is the wrong way to do it, but test coverage either > way. Hi Mark, if you think it's the wrong way to do it, please explain. It makes no sense to write tests when the idea of the patch needs to be changed later on. I was thinking the routine might be better not so single purpose. I think there might be other use cases like the patron registration verification emails (I even think there is a bug about those already) that we will want to send immediately. I Maybe just needs the letter code as parameter and a more general name? Could there be a scenario where we have a race condition? Entry not yet written to message_queue and tryint to send it or similar?
(In reply to Katrin Fischer from comment #11) > Hi Mark, if you think it's the wrong way to do it, please explain. "I think building a hash ref in SendQueuedMessages and using the built hashref on the _get_unsent_messages call would be better than introducing this, and this fails to handle how to send messages." > It makes no sense to write tests when the idea of the patch > needs to be changed later on. In this case, the tests could have been easily tweaked and pushed into t/db_dependent/Letter.t, but I understand why you say that. > I was thinking the routine might be better not so single purpose. -- which is why I suggested re-working SendQueuedMessages just a little. > I think there might be other use cases like the patron > registration verification emails (I even think there is > a bug about those already) that we will want to send immediately. True, which is what this tweak suggesting would aid in setting up for. > Maybe just needs the letter code as parameter and a > more general name? No, because I was thinking... > Could there be a scenario where we have a race condition? Entry not yet > written to message_queue and trying to send it or similar? Of this. This is why I said borrowernumber too. You aren't likely to have the same borrower trying to send the same letter, except mid-upgrade when the borrower has gone all "reset, it's not here, reset, it's not here, reset, it's not here" and generated multiple resets. So the whole $pw->[0] doesn't work. Another reason to use SendQueuedMessages. "After all, calling it with a borrower number and a letter code in the params hash ref should be easy enough to add, and then you could simply tweak the t/db_dependent/Letters.t test to test for the password reset case." You'll notice I quoted what I said in comment #9, because I thought I was clear enough. Sorry that I wasn't.
Sorry Mark, totally missed comment#9. :(
Created attachment 72524 [details] [review] Bug 18570: Tests to prove if email send was attempted kshell prove -v t/db_dependent/Passwordrecovery.t
Created attachment 72525 [details] [review] Bug 18570: Prove that the letter code parameter tweaks work Run the following commands: kshell prove -v t/db_dependent/Letters.t
Created attachment 72526 [details] [review] Bug 18570: Send Password Reset Emails immediately Run through the password reset process, and your server should send the message immediate, not waiting for the cronjob.
If you think the first patch is worthy of sign off, then go ahead and obsolete my three counter patches. Otherwise, apply my first patch alone, and the test should fail. Apply all my patches, and all the tests should pass. And, attempt a password reset process, the email should be sent immediately (it won't be 'pending' in the DB for more than a few seconds). I believe this has sufficient test coverage, and is a better solution by using the existing function.
It's all very subjective so I'm happy either way. It would be good to come to a resolution on this one as we've got a couple of customers waiting on this who would be delighted to see it come through!
Created attachment 72531 [details] [review] Bug 18570: Tests to prove if email send was attempted kshell prove -v t/db_dependent/Passwordrecovery.t Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 72532 [details] [review] Bug 18570: Prove that the letter code parameter tweaks work Run the following commands: kshell prove -v t/db_dependent/Letters.t Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 72533 [details] [review] Bug 18570: Send Password Reset Emails immediately Run through the password reset process, and your server should send the message immediate, not waiting for the cronjob. Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
I signed of counter patch, as it is a bit more generall solution and also tested. But thanks you both all for good job done ;)
Patch works as expected and tests pass, but: - POD of SendQueuedMessages need to be updated - Also, minimal POD for _get_unsent_messages would be a nice addition
Created attachment 72597 [details] [review] Bug 18570: QA Followup - Improved POD This tweaks the perldoc for SendQueuedMessages and adds some for _get_unsent_messages. TEST PLAN --------- perldoc C4::Letters -- look at _add_attachments (ugly), _get_unsent_messages (non-existent), SendQueuedMessages (no reference to borrowernumber or letter_code). apply patch perldoc C4::Letters -- confirm that SendQueuedMessages and _get_unsent_messages have reasonable POD information. -- notice how _add_attachments' POD is now readable run koha qa test tools
Changing to signed off, as POD doesn't affect code, and the Failed QA reason in comment #23 is solely POD.
Created attachment 72599 [details] [review] Bug 18570: Tests to prove if email send was attempted kshell prove -v t/db_dependent/Passwordrecovery.t Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 72600 [details] [review] Bug 18570: Prove that the letter code parameter tweaks work Run the following commands: kshell prove -v t/db_dependent/Letters.t Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 72601 [details] [review] Bug 18570: Send Password Reset Emails immediately Run through the password reset process, and your server should send the message immediate, not waiting for the cronjob. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 72602 [details] [review] Bug 18570: QA Followup - Improved POD This tweaks the perldoc for SendQueuedMessages and adds some for _get_unsent_messages. TEST PLAN --------- perldoc C4::Letters -- look at _add_attachments (ugly), _get_unsent_messages (non-existent), SendQueuedMessages (no reference to borrowernumber or letter_code). apply patch perldoc C4::Letters -- confirm that SendQueuedMessages and _get_unsent_messages have reasonable POD information. -- notice how _add_attachments' POD is now readable run koha qa test tools Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Thanks for the follow-up Passed QA
Pushed to master for 18.05, thanks to everybody involved!
Please provide a follow-up patch to remove these warnings: t/db_dependent/Passwordrecovery.t .. 1/20 Bad or missing From address: 'aWNWJnrqfp' at /home/vagrant/kohaclone/Koha/Patron/Password/Recovery.pm line 164. Bad or missing From address: 'aWNWJnrqfp' at /home/vagrant/kohaclone/Koha/Patron/Password/Recovery.pm line 164.
Created attachment 73221 [details] [review] Bug 18570: Follow up to fix test noise This sets a valid email and changes the tests to expect it was sent. prove t/db_dependent/Passwordrecovery.t -- noise before patch -- no noise after patch
Just the last patch as the rest are on master.
(In reply to M. Tompsett from comment #33) > Created attachment 73221 [details] [review] [review] > Bug 18570: Follow up to fix test noise Pushed to master. Please take a look at bug 20474.
Awesome work all, backported to stable for 17.11.05
I must find time to test this properly for backport on 17.05.x