When the patron self registration form is used with email verification and a bad entry in the KohaAdminEmailAddress system preference an error will be shown, but the message is still created and added to the message_queue table. Now every time a patron tries to register the system tries to send the already generated message with the wrong from address, which fails. This means the error will remain, although the pref has been corrected meanwhile. Settings: - PatronSelfRegistrationVerifyByEmail = Require - KohaAdminEmailAdress like koha@localhost - PatronSelfRegistration activated
The fix is to retrieve message_id from EnqueueLetter then pass it to SendQueueMessages _get_unsent_messages needs adjustments to get message_id as parameter.
This might also affect the password reset feature as it uses the same mechanism for sending the email without delay.
Created attachment 117796 [details] [review] Bug 27860: Add message_id param to SendQueuedMessages We could use C4::Letter::GetMessage, but the query in _get_unsent_messages join on borrowers and return the branchcode. To prevent any regressions it's preferable to not modify SendQueuedMessages. Ideally we obviously need a Koha methods to have better and clean code..
Created attachment 117797 [details] [review] Bug 27860: Fix self-registration if messages with incorrect email are inqueued If the pref KohaAdminEmailAddress was not set correctly (invalid email) and has been used to generate message, the message_queue table can contain some messages that are not sent (pending) and will be processed each time a new self-reg is done. The PatronSelfRegistrationVerifyByEmail feature must send only the notice we just generated, not the whole pending queue. Test plan: 1. Do not apply the patches 2. Set KohaAdminEmailAddress to an invalid email (root@localhost for instance) 3. Turn on PatronSelfRegistrationVerifyByEmail 4. Self-reg a patron 5. Boom (that must be fixed on a separate bug report) 6. Set KohaAdminEmailAddress to a valid email (root@example.org) 7. Self-reg a patron 8. Still Boom! (because it's processing the first invalid email) 9. Apply the patches, restart_all 10. Self-reg a patron => The email is sent! Notice the change in DB, the first email is still there with status "pending" and second email is sent! QA: at step 10. TODO first email must be 'failed' (highlight in tests)
Created attachment 117798 [details] [review] Bug 27860: Fix password recovery Same as the previous patch but for the password recovery feature. Test plan: 1. Do not apply the patches 2. Set KohaAdminEmailAddress to an invalid email (root@localhost for instance) 3. Use the password recovery feature 5. Boom (that must be fixed on a separate bug report) 6. Set KohaAdminEmailAddress to a valid email (root@example.org) 7. Use again the password recovery for a different user 8. Still Boom! (because it's processing the first invalid email) 9. Apply the patches, restart_all 10. Try again the password recovery => The email is sent! Notice the change in DB, the first email is still there with status "pending" and second email is sent!
Should we also perhaps add a warning to the about page for queued messages with bad from addresses?
Created attachment 117931 [details] [review] Bug 27860: Add message_id param to SendQueuedMessages We could use C4::Letter::GetMessage, but the query in _get_unsent_messages join on borrowers and return the branchcode. To prevent any regressions it's preferable to not modify SendQueuedMessages. Ideally we obviously need a Koha methods to have better and clean code.. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 117932 [details] [review] Bug 27860: Fix self-registration if messages with incorrect email are inqueued If the pref KohaAdminEmailAddress was not set correctly (invalid email) and has been used to generate message, the message_queue table can contain some messages that are not sent (pending) and will be processed each time a new self-reg is done. The PatronSelfRegistrationVerifyByEmail feature must send only the notice we just generated, not the whole pending queue. Test plan: 1. Do not apply the patches 2. Set KohaAdminEmailAddress to an invalid email (root@localhost for instance) 3. Turn on PatronSelfRegistrationVerifyByEmail 4. Self-reg a patron 5. Boom (that must be fixed on a separate bug report) 6. Set KohaAdminEmailAddress to a valid email (root@example.org) 7. Self-reg a patron 8. Still Boom! (because it's processing the first invalid email) 9. Apply the patches, restart_all 10. Self-reg a patron => The email is sent! Notice the change in DB, the first email is still there with status "pending" and second email is sent! QA: at step 10. TODO first email must be 'failed' (highlight in tests) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 117933 [details] [review] Bug 27860: Fix password recovery Same as the previous patch but for the password recovery feature. Test plan: 1. Do not apply the patches 2. Set KohaAdminEmailAddress to an invalid email (root@localhost for instance) 3. Use the password recovery feature 5. Boom (that must be fixed on a separate bug report) 6. Set KohaAdminEmailAddress to a valid email (root@example.org) 7. Use again the password recovery for a different user 8. Still Boom! (because it's processing the first invalid email) 9. Apply the patches, restart_all 10. Try again the password recovery => The email is sent! Notice the change in DB, the first email is still there with status "pending" and second email is sent! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Works as expected.. adding a warning on the about page is a 'nice to have'. Signing off
(In reply to Martin Renvoize from comment #10) > Works as expected.. adding a warning on the about page is a 'nice to have'. We have that already, it's enough IMO: commit 8850b7cf68dada380b8d7e5a8f6c83b1da76ebff Bug 22343: Add warning to the about page if KohaAdminEmailAddress is invalid
(In reply to Jonathan Druart from comment #11) > (In reply to Martin Renvoize from comment #10) > > Works as expected.. adding a warning on the about page is a 'nice to have'. > > We have that already, it's enough IMO: > commit 8850b7cf68dada380b8d7e5a8f6c83b1da76ebff > Bug 22343: Add warning to the about page if KohaAdminEmailAddress is > invalid It doesn't warn on existing data within the queue.. but actually, I think perhaps that's an excessive for the user really.
Created attachment 118201 [details] [review] Bug 27860: Add message_id param to SendQueuedMessages We could use C4::Letter::GetMessage, but the query in _get_unsent_messages join on borrowers and return the branchcode. To prevent any regressions it's preferable to not modify SendQueuedMessages. Ideally we obviously need a Koha methods to have better and clean code.. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 118202 [details] [review] Bug 27860: Fix self-registration if messages with incorrect email are inqueued If the pref KohaAdminEmailAddress was not set correctly (invalid email) and has been used to generate message, the message_queue table can contain some messages that are not sent (pending) and will be processed each time a new self-reg is done. The PatronSelfRegistrationVerifyByEmail feature must send only the notice we just generated, not the whole pending queue. Test plan: 1. Do not apply the patches 2. Set KohaAdminEmailAddress to an invalid email (root@localhost for instance) 3. Turn on PatronSelfRegistrationVerifyByEmail 4. Self-reg a patron 5. Boom (that must be fixed on a separate bug report) 6. Set KohaAdminEmailAddress to a valid email (root@example.org) 7. Self-reg a patron 8. Still Boom! (because it's processing the first invalid email) 9. Apply the patches, restart_all 10. Self-reg a patron => The email is sent! Notice the change in DB, the first email is still there with status "pending" and second email is sent! QA: at step 10. TODO first email must be 'failed' (highlight in tests) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 118203 [details] [review] Bug 27860: Fix password recovery Same as the previous patch but for the password recovery feature. Test plan: 1. Do not apply the patches 2. Set KohaAdminEmailAddress to an invalid email (root@localhost for instance) 3. Use the password recovery feature 5. Boom (that must be fixed on a separate bug report) 6. Set KohaAdminEmailAddress to a valid email (root@example.org) 7. Use again the password recovery for a different user 8. Still Boom! (because it's processing the first invalid email) 9. Apply the patches, restart_all 10. Try again the password recovery => The email is sent! Notice the change in DB, the first email is still there with status "pending" and second email is sent! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Pushed to master for 21.05, thanks to everybody involved!
Pushed to 20.11.x for 20.11.04
This didn't apply cleanly to 20.05 and then failed a test when I cleared the conflicts. Please rebase if needed for 20.05. Here's the error: root@kohadevbox:koha(rmain2005)$ prove t/db_dependent/Letters.t t/db_dependent/Letters.t .. 6/74 Fake sendmail at t/db_dependent/Letters.t line 33. Fake sendmail at t/db_dependent/Letters.t line 33. Fake sendmail at t/db_dependent/Letters.t line 33. Fake sendmail at t/db_dependent/Letters.t line 33. Fake sendmail at t/db_dependent/Letters.t line 33. # Failed test 'Exception thrown if invalid email is passed' # at t/db_dependent/Letters.t line 901. # expecting: Koha::Exceptions::BadParameter # found: normal exit # Failed test 'Invalid KohaAdminEmailAddress => status pending' # at t/db_dependent/Letters.t line 905. # got: 'sent' # expected: 'pending' Fake sendmail at t/db_dependent/Letters.t line 33. # Failed test 'Message 1 status is unchanged' # at t/db_dependent/Letters.t line 912. # got: 'sent' # expected: 'pending' # Looks like you planned 5 tests but ran 4. # Looks like you failed 3 tests of 4 run. # Failed test 'Test message_id parameter for SendQueuedMessages' # at t/db_dependent/Letters.t line 914. # Looks like you failed 1 test of 74. t/db_dependent/Letters.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/74 subtests Test Summary Report ------------------- t/db_dependent/Letters.t (Wstat: 256 Tests: 74 Failed: 1) Failed test: 74 Non-zero exit status: 1