Bug 18570 - Password recovery e-mail only sent after message queue is processed
Summary: Password recovery e-mail only sent after message queue is processed
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Mark Tompsett
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 20474
  Show dependency treegraph
 
Reported: 2017-05-10 14:55 UTC by Jens Weber
Modified: 2019-10-14 19:55 UTC (History)
18 users (show)

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


Attachments
Patch for bug 18570 (3.35 KB, patch)
2018-03-07 11:02 UTC, Mark G
Details | Diff | Splinter Review
Bug 18570: Tests to prove if email send was attempted (1.32 KB, patch)
2018-03-08 00:18 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 18570: Prove that the letter code parameter tweaks work (1.58 KB, patch)
2018-03-08 00:18 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 18570: Send Password Reset Emails immediately (2.72 KB, patch)
2018-03-08 00:18 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 18570: Tests to prove if email send was attempted (1.38 KB, patch)
2018-03-08 11:37 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18570: Prove that the letter code parameter tweaks work (1.63 KB, patch)
2018-03-08 11:37 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18570: Send Password Reset Emails immediately (2.77 KB, patch)
2018-03-08 11:38 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18570: QA Followup - Improved POD (3.19 KB, patch)
2018-03-09 14:14 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 18570: Tests to prove if email send was attempted (1.44 KB, patch)
2018-03-09 14:23 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18570: Prove that the letter code parameter tweaks work (1.70 KB, patch)
2018-03-09 14:23 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18570: Send Password Reset Emails immediately (2.84 KB, patch)
2018-03-09 14:23 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18570: QA Followup - Improved POD (3.26 KB, patch)
2018-03-09 14:23 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18570: Follow up to fix test noise (1.62 KB, patch)
2018-03-23 19:27 UTC, Mark Tompsett
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jens Weber 2017-05-10 14:55:44 UTC
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.
Comment 1 Sally 2017-05-26 15:44:52 UTC
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
Comment 2 Jonathan Druart 2017-06-05 19:05:42 UTC
All our notifications should be sent through the same process (message_queue).

What we should do is using a daemon to watch this table.
Comment 3 Katrin Fischer 2017-06-05 20:19:50 UTC
(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.
Comment 4 Marc Véron 2017-06-05 20:35:29 UTC
(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?
Comment 5 Jonathan Druart 2017-06-05 20:37:21 UTC
If someone plans to work on it, please contact us on the list first :)
Comment 6 Katrin Fischer 2017-06-05 21:02:34 UTC
(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.
Comment 7 Jens Weber 2017-10-11 09:26:19 UTC
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.
Comment 8 Mark G 2018-03-07 11:02:20 UTC
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 9 Mark Tompsett 2018-03-07 16:40:26 UTC
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.
Comment 10 Mark Tompsett 2018-03-07 16:42:40 UTC
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.
Comment 11 Katrin Fischer 2018-03-07 16:59:37 UTC
(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?
Comment 12 Mark Tompsett 2018-03-07 17:41:47 UTC
(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.
Comment 13 Katrin Fischer 2018-03-07 22:02:39 UTC
Sorry Mark, totally missed comment#9. :(
Comment 14 Mark Tompsett 2018-03-08 00:18:28 UTC
Created attachment 72524 [details] [review]
Bug 18570: Tests to prove if email send was attempted

kshell
prove -v t/db_dependent/Passwordrecovery.t
Comment 15 Mark Tompsett 2018-03-08 00:18:32 UTC
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
Comment 16 Mark Tompsett 2018-03-08 00:18:35 UTC
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.
Comment 17 Mark Tompsett 2018-03-08 00:22:35 UTC
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.
Comment 18 Mark G 2018-03-08 10:51:11 UTC
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!
Comment 19 Josef Moravec 2018-03-08 11:37:52 UTC
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>
Comment 20 Josef Moravec 2018-03-08 11:37:56 UTC
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>
Comment 21 Josef Moravec 2018-03-08 11:38:01 UTC
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>
Comment 22 Josef Moravec 2018-03-08 11:41:19 UTC
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 ;)
Comment 23 Julian Maurice 2018-03-09 09:18:37 UTC
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
Comment 24 Mark Tompsett 2018-03-09 14:14:07 UTC
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
Comment 25 Mark Tompsett 2018-03-09 14:16:14 UTC
Changing to signed off, as POD doesn't affect code, and the Failed QA reason in comment #23 is solely POD.
Comment 26 Julian Maurice 2018-03-09 14:23:02 UTC
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>
Comment 27 Julian Maurice 2018-03-09 14:23:07 UTC
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>
Comment 28 Julian Maurice 2018-03-09 14:23:14 UTC
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>
Comment 29 Julian Maurice 2018-03-09 14:23:18 UTC
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>
Comment 30 Julian Maurice 2018-03-09 14:24:08 UTC
Thanks for the follow-up
Passed QA
Comment 31 Jonathan Druart 2018-03-19 18:30:20 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 32 Jonathan Druart 2018-03-23 15:14:27 UTC
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.
Comment 33 Mark Tompsett 2018-03-23 19:27:14 UTC
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
Comment 34 Mark Tompsett 2018-03-23 21:22:07 UTC
Just the last patch as the rest are on master.
Comment 35 Jonathan Druart 2018-03-26 14:55:32 UTC
(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.
Comment 36 Nick Clemens 2018-04-24 01:32:27 UTC
Awesome work all, backported to stable for 17.11.05
Comment 37 Fridolin Somers 2018-04-24 05:38:56 UTC
I must find time to test this properly for backport on 17.05.x