At the moment, the only place where a library can sent several emails to 1 patron is the overdue_notices.pl script. The mechanism will be extended to all notices.
Created attachment 31058 [details] [review] Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress
Created attachment 31059 [details] [review] Bug 12802: Sent notices using several email addresses Currently it is not possible to select several email addresses to notify a patron. The only place where the mechanism exists is in the overdue_notices.pl script. This patch reuses the AutoEmailPrimaryAddress syspref and changes its type to "multiple". Like that it is now possible to select several email addresses. Note that there is no sense to select OFF and another value for this pref. So if the 'OFF' (first valid) value exist, it takes the priority. It will add the ability to choose the email addresses to use to notify patrons. The option is "email", "emailpro" and "B_email". If "OFF" is selected, the first valid address will be returned (same behavior as before this patch). This patch also adds a new routine C4::Branch::GetBranchEmailFromBorrowernumber (UT provided). Note for the QA step: I found a possible regression, but IMO it's not a big deal: Before this patch if a letter did not contain a "to_address", the C4::Letters::_send_message_by_email subroutine retrieve the email from the given borrowernumber. This is now done in the EnqueueMessage subroutine. What it means: If a borrower didn't have an email address when the notice was sent to the queue, the email address won't be check again when the notice will really sent to the patron. This change permits to sent a letter to the queue (EnqueueLetter) without any information (from_address, to_address), only the borrowernumber is mandatory to retrieve them. The _send_message_by_email subroutine should not contain any useful code, only sent the letter we ask it to sent. The _update_message_to_address becomes useless since the to_address is retrieved previously. What this patch does: The GetNoticeEmailAddress subroutine has been renamed to GetNoticeEmailAddresses (reflect the plural). It only gets the patron' emails defined in the AutoEmailPrimaryAddress pref. If no 'to_address' parameter is given to EnqueueMessage, the emails will be retrieved at this moment. In C4::Message: An old form was found. The AutoEmailPrimaryAddress was not check. The smsalertnumber was sent for the to_address param, which is wrong. C4::Reserves: AddReserve and _koha_notify_reserve: The from address is built in the QueueLetter. It is useless to do it here. overdue_notices.pl: The script could be cleaned a little bit if we remove the --email parameter. Indeed it is redundant with this new enhancement. I can propose another patch with a die statement and a message: "you should use the pref AutoEmailPrimaryAddress" if the --email is provided. opac/opac-shareshelf.pl and opac/opac-memberentry.pl: just remove the from and to address. They will be filled on sending to the queue (because of the borrowernumber). Test plan: 1/ Apply this patch without updating the pref AutoEmailPrimaryAddress (or fill it with a single value if it is not done yet). 2/ Try the different way to sent notices to a patron (check the following letter code: HOLD, CHECKIN, CHECKOUT, PREDUE, RENEW, DUE). 3/ Verify the email is correctly sent to the message_queue. 4/ Fill the pref with email and emailpro (for instance) 5/ Verify that 2 notices are sent to the message_queue: 1 with the email and 1 with the emailpro. 6/ You can also verify that only 1 notice is generated if the emailpro is empty.
This patch depends on bug 9043 for the multiple-select jQuery plugin (multivalued syspref).
Created attachment 31132 [details] [review] Bug 12802: FIX use the first valid email address if noemail is needed
This patch doesn't apply in a sandbox : The sandbox you've requested is not ready. Some problems occurred applying patches from bug 12802: <h1>Something went wrong !</h1>Applying: Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress fatal: sha1 information is lacking or useless (installer/data/mysql/sysprefs.sql). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress 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 12802 - Sent notices using several email addresses 31058 - Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress 31059 - Bug 12802: Sent notices using several email addresses 31132 - Bug 12802: FIX use the first valid email address if noemail is needed Apply? [(y)es, (n)o, (i)nteractive] Patch left in tmpBug-12802-DB-changes---Set-type-multiple-for-AutoE-2MpQeS.patch .
Created attachment 31382 [details] [review] Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress
Created attachment 31383 [details] [review] Bug 12802: Sent notices using several email addresses Currently it is not possible to select several email addresses to notify a patron. The only place where the mechanism exists is in the overdue_notices.pl script. This patch reuses the AutoEmailPrimaryAddress syspref and changes its type to "multiple". Like that it is now possible to select several email addresses. Note that there is no sense to select OFF and another value for this pref. So if the 'OFF' (first valid) value exist, it takes the priority. It will add the ability to choose the email addresses to use to notify patrons. The option is "email", "emailpro" and "B_email". If "OFF" is selected, the first valid address will be returned (same behavior as before this patch). This patch also adds a new routine C4::Branch::GetBranchEmailFromBorrowernumber (UT provided). Note for the QA step: I found a possible regression, but IMO it's not a big deal: Before this patch if a letter did not contain a "to_address", the C4::Letters::_send_message_by_email subroutine retrieve the email from the given borrowernumber. This is now done in the EnqueueMessage subroutine. What it means: If a borrower didn't have an email address when the notice was sent to the queue, the email address won't be check again when the notice will really sent to the patron. This change permits to sent a letter to the queue (EnqueueLetter) without any information (from_address, to_address), only the borrowernumber is mandatory to retrieve them. The _send_message_by_email subroutine should not contain any useful code, only sent the letter we ask it to sent. The _update_message_to_address becomes useless since the to_address is retrieved previously. What this patch does: The GetNoticeEmailAddress subroutine has been renamed to GetNoticeEmailAddresses (reflect the plural). It only gets the patron' emails defined in the AutoEmailPrimaryAddress pref. If no 'to_address' parameter is given to EnqueueMessage, the emails will be retrieved at this moment. In C4::Message: An old form was found. The AutoEmailPrimaryAddress was not check. The smsalertnumber was sent for the to_address param, which is wrong. C4::Reserves: AddReserve and _koha_notify_reserve: The from address is built in the QueueLetter. It is useless to do it here. overdue_notices.pl: The script could be cleaned a little bit if we remove the --email parameter. Indeed it is redundant with this new enhancement. I can propose another patch with a die statement and a message: "you should use the pref AutoEmailPrimaryAddress" if the --email is provided. opac/opac-shareshelf.pl and opac/opac-memberentry.pl: just remove the from and to address. They will be filled on sending to the queue (because of the borrowernumber). Test plan: 1/ Apply this patch without updating the pref AutoEmailPrimaryAddress (or fill it with a single value if it is not done yet). 2/ Try the different way to sent notices to a patron (check the following letter code: HOLD, CHECKIN, CHECKOUT, PREDUE, RENEW, DUE). 3/ Verify the email is correctly sent to the message_queue. 4/ Fill the pref with email and emailpro (for instance) 5/ Verify that 2 notices are sent to the message_queue: 1 with the email and 1 with the emailpro. 6/ You can also verify that only 1 notice is generated if the emailpro is empty.
Created attachment 31384 [details] [review] Bug 12802: FIX use the first valid email address if noemail is needed
(In reply to Koha Team AMU from comment #5) > This patch doesn't apply in a sandbox : These patches depend on another bug report. Moreover the sandboxes don't have cronjobs defined, so it's not possible to test notices.
Created attachment 31833 [details] [review] Bug 12802: Fix call in C4::Letters Reproduce on a production server (perl 5.10), not on my local instance (perl 5.18).
This patch set needs more work.
Created attachment 33864 [details] [review] Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress
Created attachment 33865 [details] [review] Bug 12802: Sent notices using several email addresses Currently it is not possible to select several email addresses to notify a patron. The only place where the mechanism exists is in the overdue_notices.pl script. This patch reuses the AutoEmailPrimaryAddress syspref and changes its type to "multiple". Like that it is now possible to select several email addresses. Note that there is no sense to select OFF and another value for this pref. So if the 'OFF' (first valid) value exist, it takes the priority. It will add the ability to choose the email addresses to use to notify patrons. The option is "email", "emailpro" and "B_email". If "OFF" is selected, the first valid address will be returned (same behavior as before this patch). This patch also adds a new routine C4::Branch::GetBranchEmailFromBorrowernumber (UT provided). Note for the QA step: I found a possible regression, but IMO it's not a big deal: Before this patch if a letter did not contain a "to_address", the C4::Letters::_send_message_by_email subroutine retrieve the email from the given borrowernumber. This is now done in the EnqueueMessage subroutine. What it means: If a borrower didn't have an email address when the notice was sent to the queue, the email address won't be check again when the notice will really sent to the patron. This change permits to sent a letter to the queue (EnqueueLetter) without any information (from_address, to_address), only the borrowernumber is mandatory to retrieve them. The _send_message_by_email subroutine should not contain any useful code, only sent the letter we ask it to sent. The _update_message_to_address becomes useless since the to_address is retrieved previously. What this patch does: The GetNoticeEmailAddress subroutine has been renamed to GetNoticeEmailAddresses (reflect the plural). It only gets the patron' emails defined in the AutoEmailPrimaryAddress pref. If no 'to_address' parameter is given to EnqueueMessage, the emails will be retrieved at this moment. In C4::Message: An old form was found. The AutoEmailPrimaryAddress was not check. The smsalertnumber was sent for the to_address param, which is wrong. C4::Reserves: AddReserve and _koha_notify_reserve: The from address is built in the QueueLetter. It is useless to do it here. overdue_notices.pl: The script could be cleaned a little bit if we remove the --email parameter. Indeed it is redundant with this new enhancement. I can propose another patch with a die statement and a message: "you should use the pref AutoEmailPrimaryAddress" if the --email is provided. opac/opac-shareshelf.pl and opac/opac-memberentry.pl: just remove the from and to address. They will be filled on sending to the queue (because of the borrowernumber). Test plan: 1/ Apply this patch without updating the pref AutoEmailPrimaryAddress (or fill it with a single value if it is not done yet). 2/ Try the different way to sent notices to a patron (check the following letter code: HOLD, CHECKIN, CHECKOUT, PREDUE, RENEW, DUE). 3/ Verify the email is correctly sent to the message_queue. 4/ Fill the pref with email and emailpro (for instance) 5/ Verify that 2 notices are sent to the message_queue: 1 with the email and 1 with the emailpro. 6/ You can also verify that only 1 notice is generated if the emailpro is empty.
Created attachment 33866 [details] [review] Bug 12802: FIX use the first valid email address if noemail is needed
Created attachment 33867 [details] [review] Bug 12802: Fix if AutoEmailOpacUser is enabled Test plan: Verify that the behavior of AutoEmailOpacUser still works as before. Note that now the email can receive several emails if AutoEmailPrimaryAddress has several values.
Last patch set fixes conflicts with bug 9530.
Jonathan - I got a merge CONFLICT on the updatedatabase.pl part. After I fix the updatedatabase.pl part - I git-bz apply --continue and get this.. Auto-merging t/db_dependent/Members.t CONFLICT (content): Merge conflict in t/db_dependent/Members.t Auto-merging t/db_dependent/Letters.t Auto-merging opac/opac-shareshelf.pl Auto-merging opac/opac-memberentry.pl Auto-merging misc/cronjobs/overdue_notices.pl Auto-merging C4/Reserves.pm CONFLICT (content): Merge conflict in C4/Reserves.pm Auto-merging C4/Message.pm CONFLICT (content): Merge conflict in C4/Message.pm Auto-merging C4/Members.pm Auto-merging C4/Letters.pm Can you rebase for me? Send me IRC ping (or @later) and I will test once you have rebased. *Also note I do see the depends on 9043. But 9043 is already pushed to master ( so I think I don't need to also include 9043 patch - that is correct right? ) Let me know if I am wrong here.
Created attachment 35890 [details] [review] Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress
Created attachment 35891 [details] [review] Bug 12802: Sent notices using several email addresses Currently it is not possible to select several email addresses to notify a patron. The only place where the mechanism exists is in the overdue_notices.pl script. This patch reuses the AutoEmailPrimaryAddress syspref and changes its type to "multiple". Like that it is now possible to select several email addresses. Note that there is no sense to select OFF and another value for this pref. So if the 'OFF' (first valid) value exist, it takes the priority. It will add the ability to choose the email addresses to use to notify patrons. The option is "email", "emailpro" and "B_email". If "OFF" is selected, the first valid address will be returned (same behavior as before this patch). This patch also adds a new routine C4::Branch::GetBranchEmailFromBorrowernumber (UT provided). Note for the QA step: I found a possible regression, but IMO it's not a big deal: Before this patch if a letter did not contain a "to_address", the C4::Letters::_send_message_by_email subroutine retrieve the email from the given borrowernumber. This is now done in the EnqueueMessage subroutine. What it means: If a borrower didn't have an email address when the notice was sent to the queue, the email address won't be check again when the notice will really sent to the patron. This change permits to sent a letter to the queue (EnqueueLetter) without any information (from_address, to_address), only the borrowernumber is mandatory to retrieve them. The _send_message_by_email subroutine should not contain any useful code, only sent the letter we ask it to sent. The _update_message_to_address becomes useless since the to_address is retrieved previously. What this patch does: The GetNoticeEmailAddress subroutine has been renamed to GetNoticeEmailAddresses (reflect the plural). It only gets the patron' emails defined in the AutoEmailPrimaryAddress pref. If no 'to_address' parameter is given to EnqueueMessage, the emails will be retrieved at this moment. In C4::Message: An old form was found. The AutoEmailPrimaryAddress was not check. The smsalertnumber was sent for the to_address param, which is wrong. C4::Reserves: AddReserve and _koha_notify_reserve: The from address is built in the QueueLetter. It is useless to do it here. overdue_notices.pl: The script could be cleaned a little bit if we remove the --email parameter. Indeed it is redundant with this new enhancement. I can propose another patch with a die statement and a message: "you should use the pref AutoEmailPrimaryAddress" if the --email is provided. opac/opac-shareshelf.pl and opac/opac-memberentry.pl: just remove the from and to address. They will be filled on sending to the queue (because of the borrowernumber). Test plan: 1/ Apply this patch without updating the pref AutoEmailPrimaryAddress (or fill it with a single value if it is not done yet). 2/ Try the different way to sent notices to a patron (check the following letter code: HOLD, CHECKIN, CHECKOUT, PREDUE, RENEW, DUE). 3/ Verify the email is correctly sent to the message_queue. 4/ Fill the pref with email and emailpro (for instance) 5/ Verify that 2 notices are sent to the message_queue: 1 with the email and 1 with the emailpro. 6/ You can also verify that only 1 notice is generated if the emailpro is empty.
Created attachment 35892 [details] [review] Bug 12802: FIX use the first valid email address if noemail is needed
Created attachment 35893 [details] [review] Bug 12802: Fix if AutoEmailOpacUser is enabled Test plan: Verify that the behavior of AutoEmailOpacUser still works as before. Note that now the email can receive several emails if AutoEmailPrimaryAddress has several values.
(In reply to Brendan Gallagher from comment #17) > Jonathan - > Can you rebase for me? Send me IRC ping (or @later) and I will test once > you have rebased. Done :) > *Also note I do see the depends on 9043. But 9043 is already pushed to > master ( so I think I don't need to also include 9043 patch - that is > correct right? ) Let me know if I am wrong here. Yes, if it is pushed to master, don't care of the dependency.
Hello Jonathan I run this through the qa script and find errors. Global symbol "$to_address" requires explicit package name BEGIN failed--compilation aborted Compilation failed in require That happens in multiple spots. I did remember to run updatadatebase and I have all dependencies. If this is something that I don't have right - just let me know. Thanks, Brendan
Created attachment 35926 [details] [review] Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress
Created attachment 35927 [details] [review] Bug 12802: Sent notices using several email addresses
Created attachment 35928 [details] [review] Bug 12802: FIX use the first valid email address if noemail is needed
Created attachment 35929 [details] [review] Bug 12802: Fix if AutoEmailOpacUser is enabled
Created attachment 35930 [details] [review] Bug 12802: Fix QA script issues FAIL C4/Reserves.pm FAIL pod Spurious =cut command in file C4/Reserves.pm
(In reply to Brendan Gallagher from comment #23) > Hello Jonathan > > I run this through the qa script and find errors. > > Global symbol "$to_address" requires explicit package name > BEGIN failed--compilation aborted > Compilation failed in require Sorry about that, I did not launch the qa script before submitting :-/
(In reply to Jonathan Druart from comment #29) > (In reply to Brendan Gallagher from comment #23) > > Hello Jonathan > > > > I run this through the qa script and find errors. > > > > Global symbol "$to_address" requires explicit package name > > BEGIN failed--compilation aborted > > Compilation failed in require > > Sorry about that, I did not launch the qa script before submitting :-/ Excellent - all pass now. Running some email tests tonight. Will not have time to finish. Since it is late enough, I was thinking I would set a cronjob to test that, since I will see result in the morning (10:30pm here now). -Brendan
Created attachment 36055 [details] [review] Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 36056 [details] [review] Bug 12802: Sent notices using several email addresses Currently it is not possible to select several email addresses to notify a patron. The only place where the mechanism exists is in the overdue_notices.pl script. This patch reuses the AutoEmailPrimaryAddress syspref and changes its type to "multiple". Like that it is now possible to select several email addresses. Note that there is no sense to select OFF and another value for this pref. So if the 'OFF' (first valid) value exist, it takes the priority. It will add the ability to choose the email addresses to use to notify patrons. The option is "email", "emailpro" and "B_email". If "OFF" is selected, the first valid address will be returned (same behavior as before this patch). This patch also adds a new routine C4::Branch::GetBranchEmailFromBorrowernumber (UT provided). Note for the QA step: I found a possible regression, but IMO it's not a big deal: Before this patch if a letter did not contain a "to_address", the C4::Letters::_send_message_by_email subroutine retrieve the email from the given borrowernumber. This is now done in the EnqueueMessage subroutine. What it means: If a borrower didn't have an email address when the notice was sent to the queue, the email address won't be check again when the notice will really sent to the patron. This change permits to sent a letter to the queue (EnqueueLetter) without any information (from_address, to_address), only the borrowernumber is mandatory to retrieve them. The _send_message_by_email subroutine should not contain any useful code, only sent the letter we ask it to sent. The _update_message_to_address becomes useless since the to_address is retrieved previously. What this patch does: The GetNoticeEmailAddress subroutine has been renamed to GetNoticeEmailAddresses (reflect the plural). It only gets the patron' emails defined in the AutoEmailPrimaryAddress pref. If no 'to_address' parameter is given to EnqueueMessage, the emails will be retrieved at this moment. In C4::Message: An old form was found. The AutoEmailPrimaryAddress was not check. The smsalertnumber was sent for the to_address param, which is wrong. C4::Reserves: AddReserve and _koha_notify_reserve: The from address is built in the QueueLetter. It is useless to do it here. overdue_notices.pl: The script could be cleaned a little bit if we remove the --email parameter. Indeed it is redundant with this new enhancement. I can propose another patch with a die statement and a message: "you should use the pref AutoEmailPrimaryAddress" if the --email is provided. opac/opac-shareshelf.pl and opac/opac-memberentry.pl: just remove the from and to address. They will be filled on sending to the queue (because of the borrowernumber). Test plan: 1/ Apply this patch without updating the pref AutoEmailPrimaryAddress (or fill it with a single value if it is not done yet). 2/ Try the different way to sent notices to a patron (check the following letter code: HOLD, CHECKIN, CHECKOUT, PREDUE, RENEW, DUE). 3/ Verify the email is correctly sent to the message_queue. 4/ Fill the pref with email and emailpro (for instance) 5/ Verify that 2 notices are sent to the message_queue: 1 with the email and 1 with the emailpro. 6/ You can also verify that only 1 notice is generated if the emailpro is empty. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 36057 [details] [review] Bug 12802: FIX use the first valid email address if noemail is needed Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 36058 [details] [review] Bug 12802: Fix if AutoEmailOpacUser is enabled Test plan: Verify that the behavior of AutoEmailOpacUser still works as before. Note that now the email can receive several emails if AutoEmailPrimaryAddress has several values. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 36059 [details] [review] Bug 12802: Fix QA script issues FAIL C4/Reserves.pm FAIL pod Spurious =cut command in file C4/Reserves.pm Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
This seems to make some tests fail: t/db_dependent/Reserves.t: not ok 29 - patron notified when item set to waiting # Failed test 'patron notified when item set to waiting' # at t/db_dependent/Reserves.t line 348. # got: '4' # expected: '5' not ok 30 - patron not notified a second time (bug 11445) # Failed test 'patron not notified a second time (bug 11445)' # at t/db_dependent/Reserves.t line 353. # got: '4' # expected: '5' C4::Context->userenv not defined! at /home/katrin/kohaclone/C4/Items.pm line 2275. t/db_dependent/Letter.t: ok 69 - AutoEmailPrimaryAddress=OFF, EnqueueLetter used the patron email address if no to_address is given in parameter DBD::mysql::db selectcol_arrayref failed: Unknown column 'OFF' in 'field list' at /home/katrin/kohaclone/C4/Members.pm line 1569. DBD::mysql::db selectcol_arrayref failed: Unknown column 'OFF' in 'field list' at /home/katrin/kohaclone/C4/Members.pm line 1569. # Looks like you planned 76 tests but ran 69.
Created attachment 39762 [details] [review] Bug 12802: Fix tests in Letters.t
Created attachment 39770 [details] [review] Bug 12802: EnqueueLetter must retrieve at least an email (for print messages).
The two patches I've attached fix the tests in Letters.t and Reserves.t I'm putting this back in Signed Off.
(In reply to Matthias Meusburger from comment #38) > Created attachment 39770 [details] [review] [review] > Bug 12802: EnqueueLetter must retrieve at least an email (for print > messages). Could you explain this patch please? The tests pass without it.
Letters.t tests are ok without the patch, but Reserves.t are not. (cf http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12802#c36 ). This is because when sending a print message, if the email address is not gathered, this loop in C4::Letters::EnqueueLetter is never entered: for my $to_address ( @to_addresses ) { $rs->create({ ... }
(In reply to Matthias Meusburger from comment #41) > Letters.t tests are ok without the patch, but Reserves.t are not. (cf > http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12802#c36 ). > > This is because when sending a print message, if the email address is not > gathered, this loop in C4::Letters::EnqueueLetter is never entered: > > for my $to_address ( @to_addresses ) { > $rs->create({ > ... > } The to_addresses should only be populated for email. Otherwise if the patron does not have an email defined, the print/sms/phone/whatever letters won't be sent. Actually there is something wrong, but it should be fixed cleanly.
Created attachment 40167 [details] [review] Bug 12802: Fix tests in Reserves.t
This new patch fixes the problem by making separate calls for email based messages and others. Altough not ideal, this is much cleaner than before.
Conflicts in both first and second patch, please rebase! Apply? [(y)es, (n)o, (i)nteractive] i Applying: Bug 12802: Sent notices using several email addresses Using index info to reconstruct a base tree... M C4/Branch.pm M C4/Letters.pm M C4/Members.pm M C4/Message.pm M C4/Reserves.pm M misc/cronjobs/overdue_notices.pl M opac/opac-memberentry.pl M opac/opac-shareshelf.pl M t/db_dependent/Letters.t M t/db_dependent/Members.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Members.t Auto-merging t/db_dependent/Letters.t CONFLICT (content): Merge conflict in t/db_dependent/Letters.t Auto-merging opac/opac-shareshelf.pl Auto-merging opac/opac-memberentry.pl Auto-merging misc/cronjobs/overdue_notices.pl Auto-merging C4/Reserves.pm CONFLICT (content): Merge conflict in C4/Reserves.pm Auto-merging C4/Message.pm Auto-merging C4/Members.pm Auto-merging C4/Letters.pm Auto-merging C4/Branch.pm Failed to merge in the changes. Patch failed at 0001 Bug 12802: Sent notices using several email addresses The copy of the patch that failed is found in: /home/katrin/kohaclone/.git/rebase-apply/patch 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". Patch left in /tmp/Bug-12802-Sent-notices-using-several-email-address-C2M_Ge.patch
Created attachment 45476 [details] [review] Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 45477 [details] [review] Bug 12802: Sent notices using several email addresses Currently it is not possible to select several email addresses to notify a patron. The only place where the mechanism exists is in the overdue_notices.pl script. This patch reuses the AutoEmailPrimaryAddress syspref and changes its type to "multiple". Like that it is now possible to select several email addresses. Note that there is no sense to select OFF and another value for this pref. So if the 'OFF' (first valid) value exist, it takes the priority. It will add the ability to choose the email addresses to use to notify patrons. The option is "email", "emailpro" and "B_email". If "OFF" is selected, the first valid address will be returned (same behavior as before this patch). This patch also adds a new routine C4::Branch::GetBranchEmailFromBorrowernumber (UT provided). Note for the QA step: I found a possible regression, but IMO it's not a big deal: Before this patch if a letter did not contain a "to_address", the C4::Letters::_send_message_by_email subroutine retrieve the email from the given borrowernumber. This is now done in the EnqueueMessage subroutine. What it means: If a borrower didn't have an email address when the notice was sent to the queue, the email address won't be check again when the notice will really sent to the patron. This change permits to sent a letter to the queue (EnqueueLetter) without any information (from_address, to_address), only the borrowernumber is mandatory to retrieve them. The _send_message_by_email subroutine should not contain any useful code, only sent the letter we ask it to sent. The _update_message_to_address becomes useless since the to_address is retrieved previously. What this patch does: The GetNoticeEmailAddress subroutine has been renamed to GetNoticeEmailAddresses (reflect the plural). It only gets the patron' emails defined in the AutoEmailPrimaryAddress pref. If no 'to_address' parameter is given to EnqueueMessage, the emails will be retrieved at this moment. In C4::Message: An old form was found. The AutoEmailPrimaryAddress was not check. The smsalertnumber was sent for the to_address param, which is wrong. C4::Reserves: AddReserve and _koha_notify_reserve: The from address is built in the QueueLetter. It is useless to do it here. overdue_notices.pl: The script could be cleaned a little bit if we remove the --email parameter. Indeed it is redundant with this new enhancement. I can propose another patch with a die statement and a message: "you should use the pref AutoEmailPrimaryAddress" if the --email is provided. opac/opac-shareshelf.pl and opac/opac-memberentry.pl: just remove the from and to address. They will be filled on sending to the queue (because of the borrowernumber). Test plan: 1/ Apply this patch without updating the pref AutoEmailPrimaryAddress (or fill it with a single value if it is not done yet). 2/ Try the different way to sent notices to a patron (check the following letter code: HOLD, CHECKIN, CHECKOUT, PREDUE, RENEW, DUE). 3/ Verify the email is correctly sent to the message_queue. 4/ Fill the pref with email and emailpro (for instance) 5/ Verify that 2 notices are sent to the message_queue: 1 with the email and 1 with the emailpro. 6/ You can also verify that only 1 notice is generated if the emailpro is empty. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 45478 [details] [review] Bug 12802: FIX use the first valid email address if noemail is needed Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 45479 [details] [review] Bug 12802: Fix if AutoEmailOpacUser is enabled Test plan: Verify that the behavior of AutoEmailOpacUser still works as before. Note that now the email can receive several emails if AutoEmailPrimaryAddress has several values. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 45480 [details] [review] Bug 12802: Fix QA script issues FAIL C4/Reserves.pm FAIL pod Spurious =cut command in file C4/Reserves.pm Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 45481 [details] [review] Bug 12802: Fix tests in Letters.t
Created attachment 45482 [details] [review] Bug 12802: Fix tests in Reserves.t
I'd like to move C4::Members::GetNoticeEmailAddress, It would be preferable to base this rewrite on top of this patchset. Could you rebase it asap please?
Created attachment 57296 [details] [review] Bug 12802: Sent notices using several email addresses Currently it is not possible to select several email addresses to notify a patron. The only place where the mechanism exists is in the overdue_notices.pl script. This patch reuses the AutoEmailPrimaryAddress syspref and changes its type to "multiple". Like that it is now possible to select several email addresses. Note that there is no sense to select OFF and another value for this pref. So if the 'OFF' (first valid) value exist, it takes the priority. It will add the ability to choose the email addresses to use to notify patrons. The option is "email", "emailpro" and "B_email". If "OFF" is selected, the first valid address will be returned (same behavior as before this patch). Note for the QA step: I found a possible regression, but IMO it's not a big deal: Before this patch if a letter did not contain a "to_address", the C4::Letters::_send_message_by_email subroutine retrieve the email from the given borrowernumber. This is now done in the EnqueueMessage subroutine. What it means: If a borrower didn't have an email address when the notice was sent to the queue, the email address won't be check again when the notice will really sent to the patron. This change permits to sent a letter to the queue (EnqueueLetter) without any information (from_address, to_address), only the borrowernumber is mandatory to retrieve them. The _send_message_by_email subroutine should not contain any useful code, only sent the letter we ask it to sent. The _update_message_to_address becomes useless since the to_address is retrieved previously. What this patch does: The GetNoticeEmailAddress subroutine has been renamed to GetNoticeEmailAddresses (reflect the plural). It only gets the patron' emails defined in the AutoEmailPrimaryAddress pref. If no 'to_address' parameter is given to EnqueueMessage, the emails will be retrieved at this moment. In C4::Message: An old form was found. The AutoEmailPrimaryAddress was not check. The smsalertnumber was sent for the to_address param, which is wrong. C4::Reserves: AddReserve and _koha_notify_reserve: The from address is built in the QueueLetter. It is useless to do it here. overdue_notices.pl: The script could be cleaned a little bit if we remove the --email parameter. Indeed it is redundant with this new enhancement. I can propose another patch with a die statement and a message: "you should use the pref AutoEmailPrimaryAddress" if the --email is provided. opac/opac-shareshelf.pl and opac/opac-memberentry.pl: just remove the from and to address. They will be filled on sending to the queue (because of the borrowernumber). Test plan: 1/ Apply this patch without updating the pref AutoEmailPrimaryAddress (or fill it with a single value if it is not done yet). 2/ Try the different way to sent notices to a patron (check the following letter code: HOLD, CHECKIN, CHECKOUT, PREDUE, RENEW, DUE). 3/ Verify the email is correctly sent to the message_queue. 4/ Fill the pref with email and emailpro (for instance) 5/ Verify that 2 notices are sent to the message_queue: 1 with the email and 1 with the emailpro. 6/ You can also verify that only 1 notice is generated if the emailpro is empty. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 61878 [details] [review] Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 61879 [details] [review] Bug 12802: Sent notices using several email addresses Currently it is not possible to select several email addresses to notify a patron. The only place where the mechanism exists is in the overdue_notices.pl script. This patch reuses the AutoEmailPrimaryAddress syspref and changes its type to "multiple". Like that it is now possible to select several email addresses. Note that there is no sense to select OFF and another value for this pref. So if the 'OFF' (first valid) value exist, it takes the priority. It will add the ability to choose the email addresses to use to notify patrons. The option is "email", "emailpro" and "B_email". If "OFF" is selected, the first valid address will be returned (same behavior as before this patch). Note for the QA step: I found a possible regression, but IMO it's not a big deal: Before this patch if a letter did not contain a "to_address", the C4::Letters::_send_message_by_email subroutine retrieve the email from the given borrowernumber. This is now done in the EnqueueMessage subroutine. What it means: If a borrower didn't have an email address when the notice was sent to the queue, the email address won't be check again when the notice will really sent to the patron. This change permits to sent a letter to the queue (EnqueueLetter) without any information (from_address, to_address), only the borrowernumber is mandatory to retrieve them. The _send_message_by_email subroutine should not contain any useful code, only sent the letter we ask it to sent. The _update_message_to_address becomes useless since the to_address is retrieved previously. What this patch does: The GetNoticeEmailAddress subroutine has been renamed to GetNoticeEmailAddresses (reflect the plural). It only gets the patron' emails defined in the AutoEmailPrimaryAddress pref. If no 'to_address' parameter is given to EnqueueMessage, the emails will be retrieved at this moment. In C4::Message: An old form was found. The AutoEmailPrimaryAddress was not check. The smsalertnumber was sent for the to_address param, which is wrong. C4::Reserves: AddReserve and _koha_notify_reserve: The from address is built in the QueueLetter. It is useless to do it here. overdue_notices.pl: The script could be cleaned a little bit if we remove the --email parameter. Indeed it is redundant with this new enhancement. I can propose another patch with a die statement and a message: "you should use the pref AutoEmailPrimaryAddress" if the --email is provided. opac/opac-shareshelf.pl and opac/opac-memberentry.pl: just remove the from and to address. They will be filled on sending to the queue (because of the borrowernumber). Test plan: 1/ Apply this patch without updating the pref AutoEmailPrimaryAddress (or fill it with a single value if it is not done yet). 2/ Try the different way to sent notices to a patron (check the following letter code: HOLD, CHECKIN, CHECKOUT, PREDUE, RENEW, DUE). 3/ Verify the email is correctly sent to the message_queue. 4/ Fill the pref with email and emailpro (for instance) 5/ Verify that 2 notices are sent to the message_queue: 1 with the email and 1 with the emailpro. 6/ You can also verify that only 1 notice is generated if the emailpro is empty. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 61880 [details] [review] Bug 12802: FIX use the first valid email address if noemail is needed Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 61881 [details] [review] Bug 12802: Fix if AutoEmailOpacUser is enabled Test plan: Verify that the behavior of AutoEmailOpacUser still works as before. Note that now the email can receive several emails if AutoEmailPrimaryAddress has several values. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 61882 [details] [review] Bug 12802: Fix QA script issues FAIL C4/Reserves.pm FAIL pod Spurious =cut command in file C4/Reserves.pm Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 61883 [details] [review] Bug 12802: Fix tests in Letters.t
Created attachment 61884 [details] [review] Bug 12802: Fix tests in Reserves.t
Patches reattached to fix their order
With patch applied, I run into an endless loop filling plack error log: Global symbol "$to_address" requires explicit package name at /home/marc/koha/C4/Letters.pm line 1356. prove t/Letters.t t/Letters.t .. 2/6 # Failed test 'use C4::Letters;' # at t/Letters.t line 52. # Tried to use 'C4::Letters'. # Error: Global symbol "$to_address" requires explicit package name at /home/marc/koha/C4/Letters.pm line 1356. # Compilation failed in require at t/Letters.t line 52. Seems to be similar to Comment #23 Am i missing something?
_update_message_to_address has been removed, please double check!
(In reply to Jonathan Druart from comment #64) > _update_message_to_address has been removed, please double check! With patches applied: $ git grep '_update_message_to_address' C4/Letters.pm: _update_message_to_address( $message_id, undef ) if $rv && C4/Letters.pm: _update_message_to_address($message->{'message_id'},$to_address) unless $message->{to_address}; #if initial message address was empty, coming here means that a to address was found and queue should be updated
Yes Marc, I was talking to the assignee. Switching the status accordingly.
Created attachment 66934 [details] [review] Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 66935 [details] [review] Bug 12802: Sent notices using several email addresses Currently it is not possible to select several email addresses to notify a patron. The only place where the mechanism exists is in the overdue_notices.pl script. This patch reuses the AutoEmailPrimaryAddress syspref and changes its type to "multiple". Like that it is now possible to select several email addresses. Note that there is no sense to select OFF and another value for this pref. So if the 'OFF' (first valid) value exist, it takes the priority. It will add the ability to choose the email addresses to use to notify patrons. The option is "email", "emailpro" and "B_email". If "OFF" is selected, the first valid address will be returned (same behavior as before this patch). Note for the QA step: I found a possible regression, but IMO it's not a big deal: Before this patch if a letter did not contain a "to_address", the C4::Letters::_send_message_by_email subroutine retrieve the email from the given borrowernumber. This is now done in the EnqueueMessage subroutine. What it means: If a borrower didn't have an email address when the notice was sent to the queue, the email address won't be check again when the notice will really sent to the patron. This change permits to sent a letter to the queue (EnqueueLetter) without any information (from_address, to_address), only the borrowernumber is mandatory to retrieve them. The _send_message_by_email subroutine should not contain any useful code, only sent the letter we ask it to sent. What this patch does: The GetNoticeEmailAddress subroutine has been renamed to GetNoticeEmailAddresses (reflect the plural). It only gets the patron' emails defined in the AutoEmailPrimaryAddress pref. If no 'to_address' parameter is given to EnqueueMessage, the emails will be retrieved at this moment. In C4::Message: An old form was found. The AutoEmailPrimaryAddress was not check. The smsalertnumber was sent for the to_address param, which is wrong. C4::Reserves: AddReserve and _koha_notify_reserve: The from address is built in the QueueLetter. It is useless to do it here. overdue_notices.pl: The script could be cleaned a little bit if we remove the --email parameter. Indeed it is redundant with this new enhancement. I can propose another patch with a die statement and a message: "you should use the pref AutoEmailPrimaryAddress" if the --email is provided. opac/opac-shareshelf.pl and opac/opac-memberentry.pl: just remove the from and to address. They will be filled on sending to the queue (because of the borrowernumber). Test plan: 1/ Apply this patch without updating the pref AutoEmailPrimaryAddress (or fill it with a single value if it is not done yet). 2/ Try the different way to sent notices to a patron (check the following letter code: HOLD, CHECKIN, CHECKOUT, PREDUE, RENEW, DUE). 3/ Verify the email is correctly sent to the message_queue. 4/ Fill the pref with email and emailpro (for instance) 5/ Verify that 2 notices are sent to the message_queue: 1 with the email and 1 with the emailpro. 6/ You can also verify that only 1 notice is generated if the emailpro is empty. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> tmp
Created attachment 66936 [details] [review] Bug 12802: FIX use the first valid email address if noemail is needed Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 66937 [details] [review] Bug 12802: Fix if AutoEmailOpacUser is enabled Test plan: Verify that the behavior of AutoEmailOpacUser still works as before. Note that now the email can receive several emails if AutoEmailPrimaryAddress has several values. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 66938 [details] [review] Bug 12802: Fix QA script issues FAIL C4/Reserves.pm FAIL pod Spurious =cut command in file C4/Reserves.pm Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 66939 [details] [review] Bug 12802: Fix tests in Letters.t
Created attachment 66940 [details] [review] Bug 12802: Fix tests in Reserves.t
Patches rebased on master, reintroducing the subroutine _update_message_to_address which is still needed.
Tested on sandbox Test plan: 1/ Apply this patch without updating the pref AutoEmailPrimaryAddress (or fill it with a single value if it is not done yet). 2/ Try the different way to sent notices to a patron (check the following letter code: HOLD, CHECKIN, CHECKOUT, PREDUE, RENEW, DUE). 3/ Verify the email is correctly sent to the message_queue. OK at least for HOLD and CHECKOUT 4/ Fill the pref with email and emailpro (for instance) 5/ Verify that 2 notices are sent to the message_queue: 1 with the email and 1 with the emailpro. OK for HOLD KO for CHECKOUT : no notice sent 6/ You can also verify that only 1 notice is generated if the emailpro is empty.
Created attachment 68002 [details] [review] Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 68003 [details] [review] Bug 12802: Sent notices using several email addresses Currently it is not possible to select several email addresses to notify a patron. The only place where the mechanism exists is in the overdue_notices.pl script. This patch reuses the AutoEmailPrimaryAddress syspref and changes its type to "multiple". Like that it is now possible to select several email addresses. Note that there is no sense to select OFF and another value for this pref. So if the 'OFF' (first valid) value exist, it takes the priority. It will add the ability to choose the email addresses to use to notify patrons. The option is "email", "emailpro" and "B_email". If "OFF" is selected, the first valid address will be returned (same behavior as before this patch). Note for the QA step: I found a possible regression, but IMO it's not a big deal: Before this patch if a letter did not contain a "to_address", the C4::Letters::_send_message_by_email subroutine retrieve the email from the given borrowernumber. This is now done in the EnqueueMessage subroutine. What it means: If a borrower didn't have an email address when the notice was sent to the queue, the email address won't be check again when the notice will really sent to the patron. This change permits to sent a letter to the queue (EnqueueLetter) without any information (from_address, to_address), only the borrowernumber is mandatory to retrieve them. The _send_message_by_email subroutine should not contain any useful code, only sent the letter we ask it to sent. What this patch does: The GetNoticeEmailAddress subroutine has been renamed to GetNoticeEmailAddresses (reflect the plural). It only gets the patron' emails defined in the AutoEmailPrimaryAddress pref. If no 'to_address' parameter is given to EnqueueMessage, the emails will be retrieved at this moment. In C4::Message: An old form was found. The AutoEmailPrimaryAddress was not check. The smsalertnumber was sent for the to_address param, which is wrong. C4::Reserves: AddReserve and _koha_notify_reserve: The from address is built in the QueueLetter. It is useless to do it here. overdue_notices.pl: The script could be cleaned a little bit if we remove the --email parameter. Indeed it is redundant with this new enhancement. I can propose another patch with a die statement and a message: "you should use the pref AutoEmailPrimaryAddress" if the --email is provided. opac/opac-shareshelf.pl and opac/opac-memberentry.pl: just remove the from and to address. They will be filled on sending to the queue (because of the borrowernumber). Test plan: 1/ Apply this patch without updating the pref AutoEmailPrimaryAddress (or fill it with a single value if it is not done yet). 2/ Try the different way to sent notices to a patron (check the following letter code: HOLD, CHECKIN, CHECKOUT, PREDUE, RENEW, DUE). 3/ Verify the email is correctly sent to the message_queue. 4/ Fill the pref with email and emailpro (for instance) 5/ Verify that 2 notices are sent to the message_queue: 1 with the email and 1 with the emailpro. 6/ You can also verify that only 1 notice is generated if the emailpro is empty. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 68004 [details] [review] Bug 12802: Fix tests in Letters.t
Created attachment 68005 [details] [review] Bug 12802: Fix tests in Reserves.t
Created attachment 68006 [details] [review] Bug 12802: Update all circulation alerts, not only the first one Since SendCirculationAlert can generate multiple entries in message_queue, it should update them all if pending messages already exist
Created attachment 68007 [details] [review] Bug 12802: Make tests pass
Comment on attachment 66936 [details] [review] Bug 12802: FIX use the first valid email address if noemail is needed Patch squashed with the main one
Comment on attachment 66937 [details] [review] Bug 12802: Fix if AutoEmailOpacUser is enabled Patch squashed with the main one
Comment on attachment 66938 [details] [review] Bug 12802: Fix QA script issues Patch squashed with the main one
Created attachment 68008 [details] [review] Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 68009 [details] [review] Bug 12802: Sent notices using several email addresses Currently it is not possible to select several email addresses to notify a patron. The only place where the mechanism exists is in the overdue_notices.pl script. This patch reuses the AutoEmailPrimaryAddress syspref and changes its type to "multiple". Like that it is now possible to select several email addresses. Note that there is no sense to select OFF and another value for this pref. So if the 'OFF' (first valid) value exist, it takes the priority. It will add the ability to choose the email addresses to use to notify patrons. The option is "email", "emailpro" and "B_email". If "OFF" is selected, the first valid address will be returned (same behavior as before this patch). Note for the QA step: I found a possible regression, but IMO it's not a big deal: Before this patch if a letter did not contain a "to_address", the C4::Letters::_send_message_by_email subroutine retrieve the email from the given borrowernumber. This is now done in the EnqueueMessage subroutine. What it means: If a borrower didn't have an email address when the notice was sent to the queue, the email address won't be check again when the notice will really sent to the patron. This change permits to sent a letter to the queue (EnqueueLetter) without any information (from_address, to_address), only the borrowernumber is mandatory to retrieve them. The _send_message_by_email subroutine should not contain any useful code, only sent the letter we ask it to sent. What this patch does: The GetNoticeEmailAddress subroutine has been renamed to GetNoticeEmailAddresses (reflect the plural). It only gets the patron' emails defined in the AutoEmailPrimaryAddress pref. If no 'to_address' parameter is given to EnqueueMessage, the emails will be retrieved at this moment. In C4::Message: An old form was found. The AutoEmailPrimaryAddress was not check. The smsalertnumber was sent for the to_address param, which is wrong. C4::Reserves: AddReserve and _koha_notify_reserve: The from address is built in the QueueLetter. It is useless to do it here. overdue_notices.pl: The script could be cleaned a little bit if we remove the --email parameter. Indeed it is redundant with this new enhancement. I can propose another patch with a die statement and a message: "you should use the pref AutoEmailPrimaryAddress" if the --email is provided. opac/opac-shareshelf.pl and opac/opac-memberentry.pl: just remove the from and to address. They will be filled on sending to the queue (because of the borrowernumber). Test plan: 1/ Apply this patch without updating the pref AutoEmailPrimaryAddress (or fill it with a single value if it is not done yet). 2/ Try the different way to sent notices to a patron (check the following letter code: HOLD, CHECKIN, CHECKOUT, PREDUE, RENEW, DUE). 3/ Verify the email is correctly sent to the message_queue. 4/ Fill the pref with email and emailpro (for instance) 5/ Verify that 2 notices are sent to the message_queue: 1 with the email and 1 with the emailpro. 6/ You can also verify that only 1 notice is generated if the emailpro is empty. Important note for testers: Pending messages must be removed each time you change the value of AutoEmailPrimaryAddress. When there are already existing pending messages for a borrower, Koha tries to update them rather than enqueuing new ones, but it will not update the recipient's email address, nor add/remove messages depending on the new value of AutoEmailPrimaryAddress; not sure if this is a bug or not. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 68010 [details] [review] Bug 12802: Fix tests in Letters.t
Created attachment 68011 [details] [review] Bug 12802: Fix tests in Reserves.t
Created attachment 68012 [details] [review] Bug 12802: Update all circulation alerts, not only the first one Since SendCirculationAlert can generate multiple entries in message_queue, it should update them all if pending messages already exist
Created attachment 68013 [details] [review] Bug 12802: Make tests pass
Patches rebased on master, some patches (the signed-off ones) squashed with the main patch for simplicity. Testers, be sure to read the added "Important note for testers" after the test plan in comment 86
Patch tested with a sandbox, by Axelle Clarisse <axelle.clarisse@univ-amu.fr>
Created attachment 68375 [details] [review] Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Axelle Clarisse <axelle.clarisse@univ-amu.fr>
Created attachment 68376 [details] [review] Bug 12802: Sent notices using several email addresses Currently it is not possible to select several email addresses to notify a patron. The only place where the mechanism exists is in the overdue_notices.pl script. This patch reuses the AutoEmailPrimaryAddress syspref and changes its type to "multiple". Like that it is now possible to select several email addresses. Note that there is no sense to select OFF and another value for this pref. So if the 'OFF' (first valid) value exist, it takes the priority. It will add the ability to choose the email addresses to use to notify patrons. The option is "email", "emailpro" and "B_email". If "OFF" is selected, the first valid address will be returned (same behavior as before this patch). Note for the QA step: I found a possible regression, but IMO it's not a big deal: Before this patch if a letter did not contain a "to_address", the C4::Letters::_send_message_by_email subroutine retrieve the email from the given borrowernumber. This is now done in the EnqueueMessage subroutine. What it means: If a borrower didn't have an email address when the notice was sent to the queue, the email address won't be check again when the notice will really sent to the patron. This change permits to sent a letter to the queue (EnqueueLetter) without any information (from_address, to_address), only the borrowernumber is mandatory to retrieve them. The _send_message_by_email subroutine should not contain any useful code, only sent the letter we ask it to sent. What this patch does: The GetNoticeEmailAddress subroutine has been renamed to GetNoticeEmailAddresses (reflect the plural). It only gets the patron' emails defined in the AutoEmailPrimaryAddress pref. If no 'to_address' parameter is given to EnqueueMessage, the emails will be retrieved at this moment. In C4::Message: An old form was found. The AutoEmailPrimaryAddress was not check. The smsalertnumber was sent for the to_address param, which is wrong. C4::Reserves: AddReserve and _koha_notify_reserve: The from address is built in the QueueLetter. It is useless to do it here. overdue_notices.pl: The script could be cleaned a little bit if we remove the --email parameter. Indeed it is redundant with this new enhancement. I can propose another patch with a die statement and a message: "you should use the pref AutoEmailPrimaryAddress" if the --email is provided. opac/opac-shareshelf.pl and opac/opac-memberentry.pl: just remove the from and to address. They will be filled on sending to the queue (because of the borrowernumber). Test plan: 1/ Apply this patch without updating the pref AutoEmailPrimaryAddress (or fill it with a single value if it is not done yet). 2/ Try the different way to sent notices to a patron (check the following letter code: HOLD, CHECKIN, CHECKOUT, PREDUE, RENEW, DUE). 3/ Verify the email is correctly sent to the message_queue. 4/ Fill the pref with email and emailpro (for instance) 5/ Verify that 2 notices are sent to the message_queue: 1 with the email and 1 with the emailpro. 6/ You can also verify that only 1 notice is generated if the emailpro is empty. Important note for testers: Pending messages must be removed each time you change the value of AutoEmailPrimaryAddress. When there are already existing pending messages for a borrower, Koha tries to update them rather than enqueuing new ones, but it will not update the recipient's email address, nor add/remove messages depending on the new value of AutoEmailPrimaryAddress; not sure if this is a bug or not. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Axelle Clarisse <axelle.clarisse@univ-amu.fr>
Created attachment 68377 [details] [review] Bug 12802: Fix tests in Letters.t Signed-off-by: Axelle Clarisse <axelle.clarisse@univ-amu.fr>
Created attachment 68378 [details] [review] Bug 12802: Fix tests in Reserves.t Signed-off-by: Axelle Clarisse <axelle.clarisse@univ-amu.fr>
Created attachment 68379 [details] [review] Bug 12802: Update all circulation alerts, not only the first one Since SendCirculationAlert can generate multiple entries in message_queue, it should update them all if pending messages already exist Signed-off-by: Axelle Clarisse <axelle.clarisse@univ-amu.fr>
Created attachment 68380 [details] [review] Bug 12802: Make tests pass Signed-off-by: Axelle Clarisse <axelle.clarisse@univ-amu.fr>
Patches no longer apply, please rebase!
*** Bug 19577 has been marked as a duplicate of this bug. ***
Created attachment 119016 [details] [review] Bug 12802: Sent notices using several email addresses Currently it is not possible to select several email addresses to notify a patron. The only place where the mechanism exists is in the overdue_notices.pl script. This patch reuses the AutoEmailPrimaryAddress syspref and changes its type to "multiple". Like that it is now possible to select several email addresses. Note that there is no sense to select OFF and another value for this pref. So if the 'OFF' (first valid) value exist, it takes the priority. It will add the ability to choose the email addresses to use to notify patrons. The option is "email", "emailpro" and "B_email". If "OFF" is selected, the first valid address will be returned (same behavior as before this patch). Note for the QA step: I found a possible regression, but IMO it's not a big deal: Before this patch if a letter did not contain a "to_address", the C4::Letters::_send_message_by_email subroutine retrieve the email from the given borrowernumber. This is now done in the EnqueueMessage subroutine. What it means: If a borrower didn't have an email address when the notice was sent to the queue, the email address won't be check again when the notice will really sent to the patron. This change permits to sent a letter to the queue (EnqueueLetter) without any information (from_address, to_address), only the borrowernumber is mandatory to retrieve them. The _send_message_by_email subroutine should not contain any useful code, only sent the letter we ask it to sent. What this patch does: The Koha::Patron::notice_email_address subroutine has been renamed to Koha::Patron::notice_email_addresses (plural). It only gets the patron emails defined in the AutoEmailPrimaryAddress pref. If no 'to_address' parameter is given to EnqueueMessage, the emails will be retrieved at this moment. In C4::Message: An old form was found. The AutoEmailPrimaryAddress was not check. The smsalertnumber was sent for the to_address param, which is wrong. C4::Reserves: AddReserve and _koha_notify_reserve: The from address is built in the QueueLetter. It is useless to do it here. overdue_notices.pl: The script could be cleaned a little bit if we remove the --email parameter. Indeed it is redundant with this new enhancement. I can propose another patch with a die statement and a message: "you should use the pref AutoEmailPrimaryAddress" if the --email is provided. opac/opac-shareshelf.pl and opac/opac-memberentry.pl: just remove the from and to address. They will be filled on sending to the queue (because of the borrowernumber). Test plan: 1. Apply this patch without updating the pref AutoEmailPrimaryAddress (or fill it with a single value if it is not done yet). 2. Try the different way to sent notices to a patron (check the following letter code: HOLD, CHECKIN, CHECKOUT, PREDUE, RENEW, DUE). 3. Verify the email is correctly sent to the message_queue. 4. Fill the pref with email and emailpro (for instance) 5. Verify that 2 notices are sent to the message_queue: 1 with the email and 1 with the emailpro. 6. You can also verify that only 1 notice is generated if the emailpro is empty. Important note for testers: Pending messages must be removed each time you change the value of AutoEmailPrimaryAddress. When there are already existing pending messages for a borrower, Koha tries to update them rather than enqueuing new ones, but it will not update the recipient's email address, nor add/remove messages depending on the new value of AutoEmailPrimaryAddress; not sure if this is a bug or not.
Patches rebased and squashed. Back to needs signoff
With this feature there is actually an exception thows by Koha/Email.pm : Koha::Exceptions::BadParameter->throw("Invalid 'to' parameter: ".$args->{to}) unless Email::Valid->address($args->{to}); # to is mandatory
This conflicts in resent feature C4::Letters::ResendMessage where to_address is touched.
Created attachment 149232 [details] [review] Bug 12802: Sent notices using several email addresses Currently it is not possible to select several email addresses to notify a patron. The only place where the mechanism exists is in the overdue_notices.pl script. This patch reuses the AutoEmailPrimaryAddress syspref and changes its type to "multiple". Like that it is now possible to select several email addresses. Note that there is no sense to select OFF and another value for this pref. So if the 'OFF' (first valid) value exist, it takes the priority. It will add the ability to choose the email addresses to use to notify patrons. The option is "email", "emailpro" and "B_email". If "OFF" is selected, the first valid address will be returned (same behavior as before this patch). Note for the QA step: I found a possible regression, but IMO it's not a big deal: Before this patch if a letter did not contain a "to_address", the C4::Letters::_send_message_by_email subroutine retrieve the email from the given borrowernumber. This is now done in the EnqueueMessage subroutine. What it means: If a borrower didn't have an email address when the notice was sent to the queue, the email address won't be check again when the notice will really sent to the patron. This change permits to sent a letter to the queue (EnqueueLetter) without any information (from_address, to_address), only the borrowernumber is mandatory to retrieve them. The _send_message_by_email subroutine should not contain any useful code, only sent the letter we ask it to sent. What this patch does: The Koha::Patron::notice_email_address subroutine has been renamed to Koha::Patron::notice_email_addresses (plural). It only gets the patron emails defined in the AutoEmailPrimaryAddress pref. If no 'to_address' parameter is given to EnqueueMessage, the emails will be retrieved at this moment. In C4::Message: An old form was found. The AutoEmailPrimaryAddress was not check. The smsalertnumber was sent for the to_address param, which is wrong. C4::Reserves: AddReserve and _koha_notify_reserve: The from address is built in the QueueLetter. It is useless to do it here. overdue_notices.pl: The script could be cleaned a little bit if we remove the --email parameter. Indeed it is redundant with this new enhancement. I can propose another patch with a die statement and a message: "you should use the pref AutoEmailPrimaryAddress" if the --email is provided. opac/opac-shareshelf.pl and opac/opac-memberentry.pl: just remove the from and to address. They will be filled on sending to the queue (because of the borrowernumber). Test plan: 1. Apply this patch without updating the pref AutoEmailPrimaryAddress (or fill it with a single value if it is not done yet). 2. Try the different way to sent notices to a patron (check the following letter code: HOLD, CHECKIN, CHECKOUT, PREDUE, RENEW, DUE). 3. Verify the email is correctly sent to the message_queue. 4. Fill the pref with email and emailpro (for instance) 5. Verify that 2 notices are sent to the message_queue: 1 with the email and 1 with the emailpro. 6. You can also verify that only 1 notice is generated if the emailpro is empty. Important note for testers: Pending messages must be removed each time you change the value of AutoEmailPrimaryAddress. When there are already existing pending messages for a borrower, Koha tries to update them rather than enqueuing new ones, but it will not update the recipient's email address, nor add/remove messages depending on the new value of AutoEmailPrimaryAddress; not sure if this is a bug or not.
AutoEmailPrimaryAddress has been renamed :-/
Created attachment 153540 [details] [review] Bug 12802: Sent notices using several email addresses Currently it is not possible to select several email addresses to notify a patron. The only place where the mechanism exists is in the overdue_notices.pl script. This patch reuses the AutoEmailPrimaryAddress syspref and changes its type to "multiple". Like that it is now possible to select several email addresses. Note that there is no sense to select OFF and another value for this pref. So if the 'OFF' (first valid) value exist, it takes the priority. It will add the ability to choose the email addresses to use to notify patrons. The option is "email", "emailpro" and "B_email". If "OFF" is selected, the first valid address will be returned (same behavior as before this patch). Note for the QA step: I found a possible regression, but IMO it's not a big deal: Before this patch if a letter did not contain a "to_address", the C4::Letters::_send_message_by_email subroutine retrieve the email from the given borrowernumber. This is now done in the EnqueueMessage subroutine. What it means: If a borrower didn't have an email address when the notice was sent to the queue, the email address won't be check again when the notice will really sent to the patron. This change permits to sent a letter to the queue (EnqueueLetter) without any information (from_address, to_address), only the borrowernumber is mandatory to retrieve them. The _send_message_by_email subroutine should not contain any useful code, only sent the letter we ask it to sent. What this patch does: The Koha::Patron::notice_email_address subroutine has been renamed to Koha::Patron::notice_email_addresses (plural). It only gets the patron emails defined in the AutoEmailPrimaryAddress pref. If no 'to_address' parameter is given to EnqueueMessage, the emails will be retrieved at this moment. In C4::Message: An old form was found. The AutoEmailPrimaryAddress was not check. The smsalertnumber was sent for the to_address param, which is wrong. C4::Reserves: AddReserve and _koha_notify_reserve: The from address is built in the QueueLetter. It is useless to do it here. overdue_notices.pl: The script could be cleaned a little bit if we remove the --email parameter. Indeed it is redundant with this new enhancement. I can propose another patch with a die statement and a message: "you should use the pref AutoEmailPrimaryAddress" if the --email is provided. opac/opac-shareshelf.pl and opac/opac-memberentry.pl: just remove the from and to address. They will be filled on sending to the queue (because of the borrowernumber). Test plan: 1. Apply this patch without updating the pref AutoEmailPrimaryAddress (or fill it with a single value if it is not done yet). 2. Try the different way to sent notices to a patron (check the following letter code: HOLD, CHECKIN, CHECKOUT, PREDUE, RENEW, DUE). 3. Verify the email is correctly sent to the message_queue. 4. Fill the pref with email and emailpro (for instance) 5. Verify that 2 notices are sent to the message_queue: 1 with the email and 1 with the emailpro. 6. You can also verify that only 1 notice is generated if the emailpro is empty. Important note for testers: Pending messages must be removed each time you change the value of AutoEmailPrimaryAddress. When there are already existing pending messages for a borrower, Koha tries to update them rather than enqueuing new ones, but it will not update the recipient's email address, nor add/remove messages depending on the new value of AutoEmailPrimaryAddress; not sure if this is a bug or not.
Patch rebased on master. Ready to be tested again
Hi I am trying to test on a sandbox. Is it possible to purge pending messages without completely recreating the sandbox ?
Created attachment 154076 [details] [review] Bug 12802: Sent notices using several email addresses Currently it is not possible to select several email addresses to notify a patron. The only place where the mechanism exists is in the overdue_notices.pl script. This patch reuses the AutoEmailPrimaryAddress syspref and changes its type to "multiple". Like that it is now possible to select several email addresses. Note that there is no sense to select OFF and another value for this pref. So if the 'OFF' (first valid) value exist, it takes the priority. It will add the ability to choose the email addresses to use to notify patrons. The option is "email", "emailpro" and "B_email". If "OFF" is selected, the first valid address will be returned (same behavior as before this patch). Note for the QA step: I found a possible regression, but IMO it's not a big deal: Before this patch if a letter did not contain a "to_address", the C4::Letters::_send_message_by_email subroutine retrieve the email from the given borrowernumber. This is now done in the EnqueueMessage subroutine. What it means: If a borrower didn't have an email address when the notice was sent to the queue, the email address won't be check again when the notice will really sent to the patron. This change permits to sent a letter to the queue (EnqueueLetter) without any information (from_address, to_address), only the borrowernumber is mandatory to retrieve them. The _send_message_by_email subroutine should not contain any useful code, only sent the letter we ask it to sent. What this patch does: The Koha::Patron::notice_email_address subroutine has been renamed to Koha::Patron::notice_email_addresses (plural). It only gets the patron emails defined in the AutoEmailPrimaryAddress pref. If no 'to_address' parameter is given to EnqueueMessage, the emails will be retrieved at this moment. In C4::Message: An old form was found. The AutoEmailPrimaryAddress was not check. The smsalertnumber was sent for the to_address param, which is wrong. C4::Reserves: AddReserve and _koha_notify_reserve: The from address is built in the QueueLetter. It is useless to do it here. overdue_notices.pl: The script could be cleaned a little bit if we remove the --email parameter. Indeed it is redundant with this new enhancement. I can propose another patch with a die statement and a message: "you should use the pref AutoEmailPrimaryAddress" if the --email is provided. opac/opac-shareshelf.pl and opac/opac-memberentry.pl: just remove the from and to address. They will be filled on sending to the queue (because of the borrowernumber). Test plan: 1. Apply this patch without updating the pref AutoEmailPrimaryAddress (or fill it with a single value if it is not done yet). 2. Try the different way to sent notices to a patron (check the following letter code: HOLD, CHECKIN, CHECKOUT, PREDUE, RENEW, DUE). 3. Verify the email is correctly sent to the message_queue. 4. Fill the pref with email and emailpro (for instance) 5. Verify that 2 notices are sent to the message_queue: 1 with the email and 1 with the emailpro. 6. You can also verify that only 1 notice is generated if the emailpro is empty. Important note for testers: Pending messages must be removed each time you change the value of AutoEmailPrimaryAddress. When there are already existing pending messages for a borrower, Koha tries to update them rather than enqueuing new ones, but it will not update the recipient's email address, nor add/remove messages depending on the new value of AutoEmailPrimaryAddress; not sure if this is a bug or not. Signed-off-by: Mathieu Saby <mathsabypro@gmail.com>
Works as described, but note that the syspref is now named EmailFieldPrimary
(In reply to mathieu saby from comment #111) > Works as described, but note that the syspref is now named EmailFieldPrimary It would be good if the test plan could be updated to reflect the name change. Are we sure this plays well with the new priority setting?
Sorry, this doesn't apply again and I'm not quite close enough to it to trust myself on the rebase.
I'm curious how this would work with SSO that relies on the email field...
(In reply to Katrin Fischer from comment #112) > (In reply to mathieu saby from comment #111) > > Works as described, but note that the syspref is now named EmailFieldPrimary > > It would be good if the test plan could be updated to reflect the name > change. > Are we sure this plays well with the new priority setting? Yes, it works
One for next cycle at this point I reckon.