|
Description
Katrin Fischer
2021-03-04 13:34:39 UTC
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
|