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.
We've received sponsorship to get this patch applying cleanly again.
Created attachment 159780 [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>
Hi Aleisha Could you please update the test plan (new name of the syspref AutoEmailPrimaryAddress)?
Created attachment 159857 [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 EmailFieldPrimary 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 EmailFieldPrimary 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 EmailFieldPrimary 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 EmailFieldPrimary" 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 EmailFieldPrimary (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 EmailFieldPrimary. 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 EmailFieldPrimary; not sure if this is a bug or not. Signed-off-by: Mathieu Saby <mathsabypro@gmail.com>
I applied the patch. With it, the notice mechanism is broken : when Koha try to generate a notice after checkin, this message appears : Error trace Can't call method "borrowernumber" on unblessed reference at /kohadevbox/koha/C4/Letters.pm line 961 in C4::Letters::EnqueueLetter at /kohadevbox/koha/C4/Letters.pm line 961 958: $from_address = $params->{from_address}; 959: } elsif ( $params->{message_transport_type} eq 'email' ) { 960: if ( $params->{borrowernumber} ) { 961: my $patron = Koha::Patrons->find( $params->borrowernumber ); 962: if ( $patron ) { 963: $from_address = $patron->library->from_email_address; 964: } Show function arguments in C4::Message::enqueue at /kohadevbox/koha/C4/Message.pm line 166 163: $letter->{content} = $metadata->{header} . $body . $metadata->{footer}; 164: 165: $letter->{metadata} = Encode::decode_utf8( Dump($metadata) ); 166: C4::Letters::EnqueueLetter( 167: { 168: letter => $letter, 169: borrowernumber => $patron->id, Show function arguments in C4::Circulation::SendCirculationAlert at /kohadevbox/koha/C4/Circulation.pm line 3787 3784: unless ( @messages ) { 3785: C4::Context->dbh->do(q|UNLOCK TABLES|) unless $do_not_lock; 3786: my $patron = Koha::Patrons->find($borrower->{borrowernumber}); 3787: C4::Message->enqueue($letter, $patron, $mtt); 3788: } else { 3789: foreach my $message (@messages) { 3790: $message->append($letter); Show function arguments in C4::Circulation::AddIssue at /kohadevbox/koha/C4/Circulation.pm line 1833 1830: item_type => $item_object->effective_itemtype, 1831: notification => 'CHECKOUT', 1832: ); 1833: if ( $circulation_alert->is_enabled_for( \%conditions ) ) { 1834: SendCirculationAlert( 1835: { 1836: type => 'CHECKOUT', Show function arguments in (eval) at /kohadevbox/koha/circ/circulation.pl line 451 448: if ( my $booked = $question->{BOOKED_EARLY} // $alerts->{BOOKED} ) { 449: $datedue = $booked->end_date; 450: } 451: my $issue = AddIssue( 452: $patron, $barcode, $datedue, 453: $cancelreserve, 454: undef, undef, in CGI::Compile::ROOT::kohadevbox_koha_circ_circulation_2epl::__ANON__ at /kohadevbox/koha/circ/circulation.pl line 2 1: #!/usr/bin/perl 2: 3: # script to execute issuing of books 4: 5: # Copyright 2000-2002 Katipo Communications Show function arguments in CGI::Compile::ROOT::kohadevbox_koha_circ_circulation_2epl::circulation_2epl at /usr/share/perl5/CGI/Compile.pm line 151 148: # this is necessary for MSWin32 149: my $orig_warn = $SIG{__WARN__} || sub { warn(@_) }; 150: local $SIG{__WARN__} = sub { $orig_warn->(@_) unless $_[0] =~ /^No such signal/ }; 151: $code->($self, $data, $path, $dir, \@args) 152: }; 153: }; 154: in CGI::Emulate::PSGI::__ANON__ at /usr/share/perl5/CGI/Emulate/PSGI.pm line 30 27: local *STDOUT = $stdout; 28: local *STDERR = $env->{'psgi.errors'}; 29: 30: $code->(); 31: } 32: } 33: Show function arguments in Plack::App::WrapCGI::call at /usr/share/perl5/Plack/App/WrapCGI.pm line 95 92: 93: sub call { 94: my($self, $env) = @_; 95: $self->_app->($env); 96: } 97: 98: 1; Show function arguments in Plack::Component::__ANON__ at /usr/share/perl5/Plack/Component.pm line 50 47: sub to_app { 48: my $self = shift; 49: $self->prepare_app; 50: return sub { $self->call(@_) }; 51: } 52: 53: Show function arguments in Plack::App::CGIBin::serve_path at /usr/share/perl5/Plack/App/CGIBin.pm line 50 47: my $app = $self->{_compiled}->{$file} ||= Plack::App::WrapCGI->new( 48: script => $file, execute => $self->would_exec($file), 49: )->to_app; 50: $app->($env); 51: } 52: 53: 1; Show function arguments in Plack::App::File::call at /usr/share/perl5/Plack/App/File.pm line 34 31: $env->{'plack.file.PATH_INFO'} = ''; 32: } 33: 34: return $self->serve_path($env, $file); 35: } 36: 37: sub locate_file { Show function arguments in Plack::Component::__ANON__ at /usr/share/perl5/Plack/Component.pm line 50 47: sub to_app { 48: my $self = shift; 49: $self->prepare_app; 50: return sub { $self->call(@_) }; 51: } 52: 53: Show function arguments in Plack::Middleware::LogWarn::call at /usr/share/perl5/Plack/Middleware/LogWarn.pm line 22 19: message => join '', @_ 20: }); 21: }; 22: my $res = $self->app->($env); 23: 24: return $res; 25: } Show function arguments in Plack::Component::__ANON__ at /usr/share/perl5/Plack/Component.pm line 50 47: sub to_app { 48: my $self = shift; 49: $self->prepare_app; 50: return sub { $self->call(@_) }; 51: } 52: 53: Show function arguments in Plack::Middleware::Log4perl::call at /usr/share/perl5/Plack/Middleware/Log4perl.pm line 30 27: $self->logger->$level($args->{message}); 28: }; 29: 30: $self->app->($env); 31: } 32: 33: 1; Show function arguments in Plack::Component::__ANON__ at /usr/share/perl5/Plack/Component.pm line 50 47: sub to_app { 48: my $self = shift; 49: $self->prepare_app; 50: return sub { $self->call(@_) }; 51: } 52: 53: Show function arguments in Plack::Middleware::HTTPExceptions::try {...} at /usr/share/perl5/Plack/Middleware/HTTPExceptions.pm line 20 17: my($self, $env) = @_; 18: 19: my $res = try { 20: $self->app->($env); 21: } catch { 22: $self->transform_error($_, $env); 23: }; in (eval) at /usr/share/perl5/Try/Tiny.pm line 100 97: if ( $wantarray ) { 98: @ret = $try->(); 99: } elsif ( defined $wantarray ) { 100: $ret[0] = $try->(); 101: } else { 102: $try->(); 103: }; in Try::Tiny::try at /usr/share/perl5/Try/Tiny.pm line 93 90: 91: # failed will be true if the eval dies, because 1 will not be returned 92: # from the eval body 93: my $failed = not eval { 94: $@ = $prev_error; 95: 96: # evaluate the try block in the correct context Show function arguments in Plack::Middleware::HTTPExceptions::call at /usr/share/perl5/Plack/Middleware/HTTPExceptions.pm line 23 20: $self->app->($env); 21: } catch { 22: $self->transform_error($_, $env); 23: }; 24: 25: return $res if ref $res eq 'ARRAY'; 26: Show function arguments in Plack::Component::__ANON__ at /usr/share/perl5/Plack/Component.pm line 50 47: sub to_app { 48: my $self = shift; 49: $self->prepare_app; 50: return sub { $self->call(@_) }; 51: } 52: 53: Show function arguments in Plack::Middleware::ErrorDocument::call at /usr/share/perl5/Plack/Middleware/ErrorDocument.pm line 15 12: my $self = shift; 13: my $env = shift; 14: 15: my $r = $self->app->($env); 16: 17: $self->response_cb($r, sub { 18: my $r = shift; Show function arguments in Plack::Component::__ANON__ at /usr/share/perl5/Plack/Component.pm line 50 47: sub to_app { 48: my $self = shift; 49: $self->prepare_app; 50: return sub { $self->call(@_) }; 51: } 52: 53: Show function arguments in Plack::App::URLMap::call at /usr/share/perl5/Plack/App/URLMap.pm line 71 68: return $self->response_cb($app->($env), sub { 69: $env->{PATH_INFO} = $orig_path_info; 70: $env->{SCRIPT_NAME} = $orig_script_name; 71: }); 72: } 73: 74: DEBUG && warn "All matching failed.\n"; Show function arguments in Plack::Component::__ANON__ at /usr/share/perl5/Plack/Component.pm line 50 47: sub to_app { 48: my $self = shift; 49: $self->prepare_app; 50: return sub { $self->call(@_) }; 51: } 52: 53: Show function arguments in Koha::Middleware::RealIP::call at /kohadevbox/koha/Koha/Middleware/RealIP.pm line 64 61: } 62: } 63: 64: return $self->app->($env); 65: } 66: 67: =head2 get_real_ip Show function arguments in Plack::Component::__ANON__ at /usr/share/perl5/Plack/Component.pm line 50 47: sub to_app { 48: my $self = shift; 49: $self->prepare_app; 50: return sub { $self->call(@_) }; 51: } 52: 53: Show function arguments in Koha::Middleware::SetEnv::call at /kohadevbox/koha/Koha/Middleware/SetEnv.pm line 106 103: $env->{$key} = $setenvs{$key}; 104: } 105: 106: return $self->app->($env); 107: } 108: 109: =head1 AUTHOR Show function arguments in Plack::Component::__ANON__ at /usr/share/perl5/Plack/Component.pm line 50 47: sub to_app { 48: my $self = shift; 49: $self->prepare_app; 50: return sub { $self->call(@_) }; 51: } 52: 53: Show function arguments in Plack::Middleware::Static::call at /usr/share/perl5/Plack/Middleware/Static.pm line 18 15: return $res; 16: } 17: 18: return $self->app->($env); 19: } 20: 21: sub _handle_static { Show function arguments in Plack::Component::__ANON__ at /usr/share/perl5/Plack/Component.pm line 50 47: sub to_app { 48: my $self = shift; 49: $self->prepare_app; 50: return sub { $self->call(@_) }; 51: } 52: 53: Show function arguments in Plack::Middleware::ReverseProxy::call at /usr/share/perl5/Plack/Middleware/ReverseProxy.pm line 68 65: } 66: } 67: 68: $self->app->($env); 69: } 70: 71: 1; Show function arguments in Plack::Component::__ANON__ at /usr/share/perl5/Plack/Component.pm line 50 47: sub to_app { 48: my $self = shift; 49: $self->prepare_app; 50: return sub { $self->call(@_) }; 51: } 52: 53: Show function arguments in Plack::Middleware::Lint::call at /usr/share/perl5/Plack/Middleware/Lint.pm line 24 21: my $env = shift; 22: 23: $self->validate_env($env); 24: my $res = $self->app->($env); 25: return $self->validate_res($res); 26: } 27: Show function arguments in Plack::Middleware::StackTrace::try {...} at /usr/share/perl5/Plack/Component.pm line 50 47: sub to_app { 48: my $self = shift; 49: $self->prepare_app; 50: return sub { $self->call(@_) }; 51: } 52: 53: in (eval) at /usr/share/perl5/Try/Tiny.pm line 100 97: if ( $wantarray ) { 98: @ret = $try->(); 99: } elsif ( defined $wantarray ) { 100: $ret[0] = $try->(); 101: } else { 102: $try->(); 103: }; in Plack::Middleware::StackTrace::call at /usr/share/perl5/Try/Tiny.pm line 93 90: 91: # failed will be true if the eval dies, because 1 will not be returned 92: # from the eval body 93: my $failed = not eval { 94: $@ = $prev_error; 95: 96: # evaluate the try block in the correct context Show function arguments in Plack::Component::__ANON__ at /usr/share/perl5/Plack/Component.pm line 50 47: sub to_app { 48: my $self = shift; 49: $self->prepare_app; 50: return sub { $self->call(@_) }; 51: } 52: 53: Show function arguments in Plack::Middleware::AccessLog::call at /usr/share/perl5/Plack/Middleware/AccessLog.pm line 27 24: my $self = shift; 25: my($env) = @_; 26: 27: my $res = $self->app->($env); 28: 29: if ( ref($res) && ref($res) eq 'ARRAY' ) { 30: my $content_length = Plack::Util::content_length($res->[2]); Show function arguments in Plack::Component::__ANON__ at /usr/share/perl5/Plack/Component.pm line 50 47: sub to_app { 48: my $self = shift; 49: $self->prepare_app; 50: return sub { $self->call(@_) }; 51: } 52: 53: Show function arguments in (eval) at /usr/share/perl5/Plack/Util.pm line 145 142: sub run_app($$) { 143: my($app, $env) = @_; 144: 145: return eval { $app->($env) } || do { 146: my $body = "Internal Server Error"; 147: $env->{'psgi.errors'}->print($@); 148: [ 500, [ 'Content-Type' => 'text/plain', 'Content-Length' => length($body) ], [ $body ] ]; in Plack::Util::run_app at /usr/share/perl5/Plack/Util.pm line 145 142: sub run_app($$) { 143: my($app, $env) = @_; 144: 145: return eval { $app->($env) } || do { 146: my $body = "Internal Server Error"; 147: $env->{'psgi.errors'}->print($@); 148: [ 500, [ 'Content-Type' => 'text/plain', 'Content-Length' => length($body) ], [ $body ] ]; Show function arguments in Starman::Server::dispatch_request at /usr/share/perl5/Starman/Server.pm line 195 192: my ($self, $env) = @_; 193: 194: # Run PSGI apps 195: my $res = Plack::Util::run_app($self->{app}, $env); 196: 197: if (ref $res eq 'CODE') { 198: $res->(sub { $self->_finalize_response($env, $_[0]) }); Show function arguments in Starman::Server::process_request at /usr/share/perl5/Starman/Server.pm line 298 295: 296: $self->_prepare_env($env); 297: 298: $self->dispatch_request($env); 299: 300: DEBUG && warn "[$$] Request done\n"; 301: Show function arguments in Net::Server::run_client_connection at /usr/share/perl5/Net/Server.pm line 72 69: $self->post_accept_hook($c); # user customizable hook 70: 71: my $ok = $self->allow_deny($c) && $self->allow_deny_hook($c); # do allow/deny check on client info 72: if ($ok) { 73: $self->process_request($c); # This is where the core functionality of a Net::Server should be. 74: } else { 75: $self->request_denied_hook($c); # user customizable hook Show function arguments in (eval) at /usr/share/perl5/Net/Server/PreFork.pm line 225 222: $prop->{'connected'} = 1; 223: print $write "$$ processing\n"; 224: 225: my $ok = eval { $self->run_client_connection; 1 }; 226: if (! $ok) { 227: print $write "$$ exiting\n"; 228: die $@; in Net::Server::PreFork::run_child at /usr/share/perl5/Net/Server/PreFork.pm line 225 222: $prop->{'connected'} = 1; 223: print $write "$$ processing\n"; 224: 225: my $ok = eval { $self->run_client_connection; 1 }; 226: if (! $ok) { 227: print $write "$$ exiting\n"; 228: die $@; Show function arguments in Net::Server::PreFork::run_n_children at /usr/share/perl5/Net/Server/PreFork.pm line 183 180: if ($prop->{'child_communication'}) { 181: $prop->{'parent_sock'} = $childsock; 182: } 183: $self->run_child; 184: } 185: } 186: } Show function arguments in Net::Server::PreFork::loop at /usr/share/perl5/Net/Server/PreFork.pm line 107 104: 105: my $start = $prop->{'min_servers'}; 106: $self->log(3, "Beginning prefork ($start processes)"); 107: $self->run_n_children($start); 108: 109: $self->run_parent; 110: } Show function arguments in Net::Server::run at /usr/share/perl5/Net/Server.pm line 58 55: $self->post_bind; # allow for chrooting, becoming a different user and group 56: 57: $self->pre_loop_hook; # user customizable hook 58: $self->loop; # repeat accept/process cycle 59: 60: $self->server_close; # close the server and release the port 61: } Show function arguments in Starman::Server::run at /usr/share/perl5/Starman/Server.pm line 106 103: max_requests => $options->{max_requests} || 1000, 104: user => $options->{user} || $>, 105: group => $options->{group} || $), 106: listen => $options->{backlog} || 1024, 107: check_for_waiting => 1, 108: no_client_stdout => 1, 109: %extra Show function arguments in Plack::Handler::Starman::run at /usr/share/perl5/Plack/Handler/Starman.pm line 25 22: } 23: $self->{net_server_args} = \%nsa if %nsa; 24: 25: Starman::Server->new->run($app, {%$self}); 26: } 27: 28: 1; Show function arguments in Plack::Loader::Delayed::run at /usr/share/perl5/Plack/Loader/Delayed.pm line 20 17: }; 18: 19: $server->{psgi_app_builder} = $self->{builder}; 20: $server->run($app); 21: } 22: 23: 1; Show function arguments in Plack::Runner::run at /usr/share/perl5/Plack/Runner.pm line 279 276: $loader->preload_app($app); 277: 278: my $server = $self->load_server($loader); 279: $loader->run($server); 280: } 281: 282: 1; Show function arguments at /usr/bin/starman line 38 35: } 36: 37: $runner->set_options(argv => \@argv); 38: $runner->run; 39: 40: __END__ 41:
I'm not 100% on the implementation here.. do we really want to queue entire duplicate messages for each to address? With the cc work in bug 12532 we simply create a comma-delimited list of addresses in the cc header and Email::Sender does the rest for us. I think I'd rather see that approach here than the duplicate's in the message_queue.. with bug 18397 also in the queue you'd easily be able to see the corresponding To address list next to the message.
Created attachment 160051 [details] [review] Bug 12802: (alt) Update system preference Allow the EmailFieldPrimary preference to accept multiple selections
Created attachment 160052 [details] [review] Bug 12802: (alt) Update notice_email_address method to return a list This patch updates the notice_email_address method to return a comma separated list of addresses as expected by Email::Sender if you wish to send mail to multiple To addresses.
Simplified alternative attached.. local testing yields solid results. I based it on top of bug 18397 simply so I could see the To output in the notices tab.. Might be worth looking at the test plan in bug https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18397#c12 to see how to setup email for testing thoroughly.
Created attachment 160090 [details] [review] Bug 12802: Update system preference Allow the EmailFieldPrimary preference to accept multiple selections
Created attachment 160091 [details] [review] Bug 12802: Update notice_email_address method to return a list This patch updates the notice_email_address method to return a comma separated list of addresses as expected by Email::Sender if you wish to send mail to multiple To addresses. To test: 1. Install database update and restart services 2. Go to Koha Administration -> system preferences. Search for EmailFieldPrimary. Confirm the options show as checkboxes and you can select multiple. 3. Search for a patron. Put different email addresses in each of the possible email fields (i.e. primary email, secondary email, alternate email). 4. Make sure 'first valid' is checked in EmailFieldPrimary. 5. Go to your patron, click the More dropdown and send a welcome email. 6. In your terminal, go into the database to view the message queue. sudo koha-mysql instance select * from message_queue\G 7. Confirm your welcome email is visible, with ONLY the first valid email in the to_address field. 8. Select other combinations of emails in EmailFieldPrimary. Uncheck 'first valid' 9. Send another welcome email to the same patron, then run the message_queue SQL query again. 10. Confirm your welcome email is visible and shows all selected addresses concatenated by , in the to_address field. Sponsored-by: St Luke's Grammar School & Pymble Ladies' College
Created attachment 160092 [details] [review] Bug 12802: Add note to EmailFieldPrimary system preference To clarify that "first valid" should be unchecked for multiple emails to receive notices
(In reply to Martin Renvoize from comment #125) > Simplified alternative attached.. local testing yields solid results. I > based it on top of bug 18397 simply so I could see the To output in the > notices tab.. > > Might be worth looking at the test plan in bug > https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18397#c12 to see > how to setup email for testing thoroughly. Thanks, much prefer this approach. Have adjusted the syspref names, added a follow-up patch and attached a new test plan
Thanks for testing, and adding the proper test plan in, Aleisha. I'm glad you like the alternative approach.
Created attachment 160103 [details] [review] Bug 12802: Update system preference Allow the EmailFieldPrimary preference to accept multiple selections
Created attachment 160104 [details] [review] Bug 12802: Update notice_email_address method to return a list This patch updates the notice_email_address method to return a comma separated list of addresses as expected by Email::Sender if you wish to send mail to multiple To addresses. To test: 1. Install database update and restart services 2. Go to Koha Administration -> system preferences. Search for EmailFieldPrimary. Confirm the options show as checkboxes and you can select multiple. 3. Search for a patron. Put different email addresses in each of the possible email fields (i.e. primary email, secondary email, alternate email). 4. Make sure 'first valid' is checked in EmailFieldPrimary. 5. Go to your patron, click the More dropdown and send a welcome email. 6. In your terminal, go into the database to view the message queue. sudo koha-mysql instance select * from message_queue\G 7. Confirm your welcome email is visible, with ONLY the first valid email in the to_address field. 8. Select other combinations of emails in EmailFieldPrimary. 9. Send another welcome email to the same patron, then run the message_queue SQL query again. 10. Confirm your welcome email is visible and shows all selected addresses concatenated by , in the to_address field. 11. Enable KTD to send email [1] (without email configured you will not be able to confirm the delivery to multiple addresses). 12. Run misc/cronjobs/process_message_queue.pl to trigger sending the queued notices from above. 13. Check that the addresses in the to_address all actually recieve the notice. [1] To test sending emails using a Google account: - Set up an App password for your Google Account - Configure a new 'SMTP Server' under 'Administration > SMTP servers' using the following settings where `User name` = your Google email address and `Password` = your APP password, not your Google account password): Host: smtp.gmail.com Port: 587 Timeout: 120 SSL: STARTTLS User name: GOOGLEACCOUNTUSER Password: GOOGLEAPPPASSWORD - Set this server as default Sponsored-by: St Luke's Grammar School & Pymble Ladies' College
Created attachment 160105 [details] [review] Bug 12802: Add note to EmailFieldPrimary system preference To clarify that "first valid" should be unchecked for multiple emails to receive notices
Created attachment 160106 [details] [review] Bug 12802: Clarify system preferences I felt the switch to multi in the EmailFieldPrimary preference was a bit confusing given that type exposes a 'select all' option which doesn't make sense with the pre-existing 'first valid' option being an override in the code. This patch opts to switch it back to 'Choice', meaning that only one option can be picked and adds a 'selected addresses' option which prompts the use of a new 'EmailFieldSelection' preference which allows for the multi-select as before. To test: 1) Run though the test plan for 'Update notice_email_address method to return a list' but with the following ammendments: * 2) As aposed to being able to select multiple options under EmailFieldPrimary, you should now only be able to select one option at a time, but a new 'selected addresses' option should be present. * 8) Select the 'selected addresses' option for 'EmailFieldPrimary' and also select multiple fields for the new 'EmailFieldSelection' preference.
Created attachment 165494 [details] [review] Bug 12802: Update system preference Allow the EmailFieldPrimary preference to accept multiple selections Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Axelle Clarisse <axelle.clarisse@univ-amu.fr> Signed-off-by: Mathieu Saby <mathsabypro@gmail.com> Signed-off-by: Aleisha Amohia <aleisha@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165495 [details] [review] Bug 12802: Update notice_email_address method to return a list This patch updates the notice_email_address method to return a comma separated list of addresses as expected by Email::Sender if you wish to send mail to multiple To addresses. To test: 1. Install database update and restart services 2. Go to Koha Administration -> system preferences. Search for EmailFieldPrimary. Confirm the options show as checkboxes and you can select multiple. 3. Search for a patron. Put different email addresses in each of the possible email fields (i.e. primary email, secondary email, alternate email). 4. Make sure 'first valid' is checked in EmailFieldPrimary. 5. Go to your patron, click the More dropdown and send a welcome email. 6. In your terminal, go into the database to view the message queue. sudo koha-mysql instance select * from message_queue\G 7. Confirm your welcome email is visible, with ONLY the first valid email in the to_address field. 8. Select other combinations of emails in EmailFieldPrimary. 9. Send another welcome email to the same patron, then run the message_queue SQL query again. 10. Confirm your welcome email is visible and shows all selected addresses concatenated by , in the to_address field. 11. Enable KTD to send email [1] (without email configured you will not be able to confirm the delivery to multiple addresses). 12. Run misc/cronjobs/process_message_queue.pl to trigger sending the queued notices from above. 13. Check that the addresses in the to_address all actually recieve the notice. [1] To test sending emails using a Google account: - Set up an App password for your Google Account - Configure a new 'SMTP Server' under 'Administration > SMTP servers' using the following settings where `User name` = your Google email address and `Password` = your APP password, not your Google account password): Host: smtp.gmail.com Port: 587 Timeout: 120 SSL: STARTTLS User name: GOOGLEACCOUNTUSER Password: GOOGLEAPPPASSWORD - Set this server as default Sponsored-by: St Luke's Grammar School & Pymble Ladies' College Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Axelle Clarisse <axelle.clarisse@univ-amu.fr> Signed-off-by: Mathieu Saby <mathsabypro@gmail.com> Signed-off-by: Aleisha Amohia <aleisha@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165496 [details] [review] Bug 12802: Add note to EmailFieldPrimary system preference To clarify that "first valid" should be unchecked for multiple emails to receive notices Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Axelle Clarisse <axelle.clarisse@univ-amu.fr> Signed-off-by: Mathieu Saby <mathsabypro@gmail.com> Signed-off-by: Aleisha Amohia <aleisha@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165497 [details] [review] Bug 12802: Clarify system preferences I felt the switch to multi in the EmailFieldPrimary preference was a bit confusing given that type exposes a 'select all' option which doesn't make sense with the pre-existing 'first valid' option being an override in the code. This patch opts to switch it back to 'Choice', meaning that only one option can be picked and adds a 'selected addresses' option which prompts the use of a new 'EmailFieldSelection' preference which allows for the multi-select as before. To test: 1) Run though the test plan for 'Update notice_email_address method to return a list' but with the following ammendments: * 2) As aposed to being able to select multiple options under EmailFieldPrimary, you should now only be able to select one option at a time, but a new 'selected addresses' option should be present. * 8) Select the 'selected addresses' option for 'EmailFieldPrimary' and also select multiple fields for the new 'EmailFieldSelection' preference. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Axelle Clarisse <axelle.clarisse@univ-amu.fr> Signed-off-by: Mathieu Saby <mathsabypro@gmail.com> Signed-off-by: Aleisha Amohia <aleisha@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This code has had a lot of eyes over the years and the final patches are significantly simplified. QA scripts are happy, it's been tested a lot.. Passing QA
I quite like how this turned out in the end!
Pushed for 24.05! Well done everyone, thank you!
Not backported to 23.11.x