Bug 12802

Summary: Send notices using several email addresses
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: NoticesAssignee: Aleisha Amohia <aleisha>
Status: Needs Signoff --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: aleisha, alex.arnaud, baselibrary.consortium, black23, brendan, david, dcook, didier.gautheron, fridolin.somers, italo.barbosa.brasileiro, julian.maurice, katrin.fischer, koha.aixmarseille, koha, laurence.rault, marjorie.barry-vila, martin.renvoize, mathsabypro, matthias.meusburger, nick.weingardt, ruatuki, sandboxes, veron
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12532
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8000
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18398
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22343
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15121
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16786
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31376
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18397
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 9043    
Bug Blocks: 29393    
Attachments: Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress
Bug 12802: Sent notices using several email addresses
Bug 12802: FIX use the first valid email address if noemail is needed
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress
Bug 12802: Sent notices using several email addresses
Bug 12802: FIX use the first valid email address if noemail is needed
Bug 12802: Fix call in C4::Letters
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress
Bug 12802: Sent notices using several email addresses
Bug 12802: FIX use the first valid email address if noemail is needed
Bug 12802: Fix if AutoEmailOpacUser is enabled
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress
Bug 12802: Sent notices using several email addresses
Bug 12802: FIX use the first valid email address if noemail is needed
Bug 12802: Fix if AutoEmailOpacUser is enabled
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress
Bug 12802: Sent notices using several email addresses
Bug 12802: FIX use the first valid email address if noemail is needed
Bug 12802: Fix if AutoEmailOpacUser is enabled
Bug 12802: Fix QA script issues
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress
Bug 12802: Sent notices using several email addresses
Bug 12802: FIX use the first valid email address if noemail is needed
Bug 12802: Fix if AutoEmailOpacUser is enabled
Bug 12802: Fix QA script issues
Bug 12802: Fix tests in Letters.t
Bug 12802: EnqueueLetter must retrieve at least an email (for print messages).
Bug 12802: Fix tests in Reserves.t
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress
Bug 12802: Sent notices using several email addresses
Bug 12802: FIX use the first valid email address if noemail is needed
Bug 12802: Fix if AutoEmailOpacUser is enabled
Bug 12802: Fix QA script issues
Bug 12802: Fix tests in Letters.t
Bug 12802: Fix tests in Reserves.t
Bug 12802: Sent notices using several email addresses
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress
Bug 12802: Sent notices using several email addresses
Bug 12802: FIX use the first valid email address if noemail is needed
Bug 12802: Fix if AutoEmailOpacUser is enabled
Bug 12802: Fix QA script issues
Bug 12802: Fix tests in Letters.t
Bug 12802: Fix tests in Reserves.t
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress
Bug 12802: Sent notices using several email addresses
Bug 12802: FIX use the first valid email address if noemail is needed
Bug 12802: Fix if AutoEmailOpacUser is enabled
Bug 12802: Fix QA script issues
Bug 12802: Fix tests in Letters.t
Bug 12802: Fix tests in Reserves.t
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress
Bug 12802: Sent notices using several email addresses
Bug 12802: Fix tests in Letters.t
Bug 12802: Fix tests in Reserves.t
Bug 12802: Update all circulation alerts, not only the first one
Bug 12802: Make tests pass
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress
Bug 12802: Sent notices using several email addresses
Bug 12802: Fix tests in Letters.t
Bug 12802: Fix tests in Reserves.t
Bug 12802: Update all circulation alerts, not only the first one
Bug 12802: Make tests pass
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress
Bug 12802: Sent notices using several email addresses
Bug 12802: Fix tests in Letters.t
Bug 12802: Fix tests in Reserves.t
Bug 12802: Update all circulation alerts, not only the first one
Bug 12802: Make tests pass
Bug 12802: Sent notices using several email addresses
Bug 12802: Sent notices using several email addresses
Bug 12802: Sent notices using several email addresses
Bug 12802: Sent notices using several email addresses
Bug 12802: Sent notices using several email addresses
Bug 12802: Sent notices using several email addresses
Bug 12802: (alt) Update system preference
Bug 12802: (alt) Update notice_email_address method to return a list
Bug 12802: Update system preference
Bug 12802: Update notice_email_address method to return a list
Bug 12802: Add note to EmailFieldPrimary system preference
Bug 12802: Update system preference
Bug 12802: Update notice_email_address method to return a list
Bug 12802: Add note to EmailFieldPrimary system preference
Bug 12802: Clarify system preferences

Description Jonathan Druart 2014-08-21 14:59:17 UTC
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.
Comment 1 Jonathan Druart 2014-08-21 16:01:43 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-08-21 16:01:48 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2014-08-21 16:03:16 UTC
This patch depends on bug 9043 for the multiple-select jQuery plugin (multivalued syspref).
Comment 4 Jonathan Druart 2014-08-25 10:06:50 UTC Comment hidden (obsolete)
Comment 5 Koha Team AMU 2014-09-04 09:36:13 UTC
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 .
Comment 6 Jonathan Druart 2014-09-05 07:39:37 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2014-09-05 07:39:42 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2014-09-05 07:39:45 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2014-09-05 07:40:48 UTC
(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.
Comment 10 Jonathan Druart 2014-09-23 11:22:44 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2014-11-21 09:51:32 UTC
This patch set needs more work.
Comment 12 Jonathan Druart 2014-11-24 15:17:29 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2014-11-24 15:17:37 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2014-11-24 15:17:49 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2014-11-24 15:17:53 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2014-11-24 15:18:15 UTC
Last patch set fixes conflicts with bug 9530.
Comment 17 Brendan Gallagher 2015-02-13 03:55:31 UTC
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.
Comment 18 Jonathan Druart 2015-02-13 08:48:11 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2015-02-13 08:48:18 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2015-02-13 08:48:22 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2015-02-13 08:48:26 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2015-02-13 08:49:38 UTC
(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.
Comment 23 Brendan Gallagher 2015-02-13 22:08:05 UTC
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
Comment 24 Jonathan Druart 2015-02-16 10:09:18 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2015-02-16 10:09:34 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2015-02-16 10:09:41 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2015-02-16 10:09:50 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2015-02-16 10:10:07 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2015-02-16 10:11:03 UTC
(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 :-/
Comment 30 Brendan Gallagher 2015-02-19 06:29:09 UTC
(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
Comment 31 Brendan Gallagher 2015-02-20 04:40:58 UTC Comment hidden (obsolete)
Comment 32 Brendan Gallagher 2015-02-20 04:41:21 UTC Comment hidden (obsolete)
Comment 33 Brendan Gallagher 2015-02-20 04:41:30 UTC Comment hidden (obsolete)
Comment 34 Brendan Gallagher 2015-02-20 04:41:37 UTC Comment hidden (obsolete)
Comment 35 Brendan Gallagher 2015-02-20 04:41:46 UTC Comment hidden (obsolete)
Comment 36 Katrin Fischer 2015-04-22 22:53:38 UTC
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.
Comment 37 Matthias Meusburger 2015-06-02 08:20:56 UTC Comment hidden (obsolete)
Comment 38 Matthias Meusburger 2015-06-02 12:52:09 UTC Comment hidden (obsolete)
Comment 39 Matthias Meusburger 2015-06-02 12:53:36 UTC
The two patches I've attached fix the tests in Letters.t and Reserves.t
I'm putting this back in Signed Off.
Comment 40 Jonathan Druart 2015-06-03 10:05:30 UTC
(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.
Comment 41 Matthias Meusburger 2015-06-03 12:15:51 UTC
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({
  ...
}
Comment 42 Jonathan Druart 2015-06-03 12:36:19 UTC
(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.
Comment 43 Matthias Meusburger 2015-06-16 09:19:03 UTC Comment hidden (obsolete)
Comment 44 Matthias Meusburger 2015-06-16 09:20:24 UTC
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.
Comment 45 Katrin Fischer 2015-10-24 22:42:45 UTC
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
Comment 46 Alex Arnaud 2015-12-08 09:41:32 UTC
Created attachment 45476 [details] [review]
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 47 Alex Arnaud 2015-12-08 09:41:43 UTC
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>
Comment 48 Alex Arnaud 2015-12-08 09:41:53 UTC
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>
Comment 49 Alex Arnaud 2015-12-08 09:42:01 UTC
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>
Comment 50 Alex Arnaud 2015-12-08 09:42:10 UTC
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>
Comment 51 Alex Arnaud 2015-12-08 09:42:17 UTC
Created attachment 45481 [details] [review]
Bug 12802: Fix tests in Letters.t
Comment 52 Alex Arnaud 2015-12-08 09:42:24 UTC
Created attachment 45482 [details] [review]
Bug 12802: Fix tests in Reserves.t
Comment 53 Jonathan Druart 2016-11-07 15:03:41 UTC
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?
Comment 54 Matthias Meusburger 2016-11-08 08:22:38 UTC
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>
Comment 55 Julian Maurice 2017-04-05 10:41:25 UTC
Created attachment 61878 [details] [review]
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 56 Julian Maurice 2017-04-05 10:41:30 UTC
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>
Comment 57 Julian Maurice 2017-04-05 10:41:35 UTC
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>
Comment 58 Julian Maurice 2017-04-05 10:41:40 UTC
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>
Comment 59 Julian Maurice 2017-04-05 10:41:44 UTC
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>
Comment 60 Julian Maurice 2017-04-05 10:41:49 UTC
Created attachment 61883 [details] [review]
Bug 12802: Fix tests in Letters.t
Comment 61 Julian Maurice 2017-04-05 10:41:54 UTC
Created attachment 61884 [details] [review]
Bug 12802: Fix tests in Reserves.t
Comment 62 Julian Maurice 2017-04-05 10:43:16 UTC
Patches reattached to fix their order
Comment 63 Marc Véron 2017-04-07 11:22:36 UTC
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?
Comment 64 Jonathan Druart 2017-04-07 13:39:33 UTC
_update_message_to_address has been removed, please double check!
Comment 65 Marc Véron 2017-04-07 16:30:03 UTC
(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
Comment 66 Jonathan Druart 2017-04-07 16:51:08 UTC
Yes Marc, I was talking to the assignee. Switching the status accordingly.
Comment 67 Julian Maurice 2017-09-07 13:05:25 UTC
Created attachment 66934 [details] [review]
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 68 Julian Maurice 2017-09-07 13:05:30 UTC
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
Comment 69 Julian Maurice 2017-09-07 13:05:34 UTC
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>
Comment 70 Julian Maurice 2017-09-07 13:05:38 UTC
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>
Comment 71 Julian Maurice 2017-09-07 13:05:42 UTC
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>
Comment 72 Julian Maurice 2017-09-07 13:05:47 UTC
Created attachment 66939 [details] [review]
Bug 12802: Fix tests in Letters.t
Comment 73 Julian Maurice 2017-09-07 13:05:51 UTC
Created attachment 66940 [details] [review]
Bug 12802: Fix tests in Reserves.t
Comment 74 Julian Maurice 2017-09-07 13:06:55 UTC
Patches rebased on master, reintroducing the subroutine _update_message_to_address which is still needed.
Comment 75 Laurence Rault 2017-10-03 08:21:32 UTC
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.
Comment 76 Julian Maurice 2017-10-12 13:45:04 UTC
Created attachment 68002 [details] [review]
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 77 Julian Maurice 2017-10-12 13:45:10 UTC
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>
Comment 78 Julian Maurice 2017-10-12 13:45:16 UTC
Created attachment 68004 [details] [review]
Bug 12802: Fix tests in Letters.t
Comment 79 Julian Maurice 2017-10-12 13:45:21 UTC
Created attachment 68005 [details] [review]
Bug 12802: Fix tests in Reserves.t
Comment 80 Julian Maurice 2017-10-12 13:45:27 UTC
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
Comment 81 Julian Maurice 2017-10-12 13:45:32 UTC
Created attachment 68007 [details] [review]
Bug 12802: Make tests pass
Comment 82 Julian Maurice 2017-10-12 13:48:04 UTC
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 83 Julian Maurice 2017-10-12 13:48:27 UTC
Comment on attachment 66937 [details] [review]
Bug 12802: Fix if AutoEmailOpacUser is enabled

Patch squashed with the main one
Comment 84 Julian Maurice 2017-10-12 13:48:37 UTC
Comment on attachment 66938 [details] [review]
Bug 12802: Fix QA script issues

Patch squashed with the main one
Comment 85 Julian Maurice 2017-10-12 14:01:13 UTC
Created attachment 68008 [details] [review]
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 86 Julian Maurice 2017-10-12 14:01:20 UTC
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>
Comment 87 Julian Maurice 2017-10-12 14:01:25 UTC
Created attachment 68010 [details] [review]
Bug 12802: Fix tests in Letters.t
Comment 88 Julian Maurice 2017-10-12 14:01:32 UTC
Created attachment 68011 [details] [review]
Bug 12802: Fix tests in Reserves.t
Comment 89 Julian Maurice 2017-10-12 14:01:38 UTC
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
Comment 90 Julian Maurice 2017-10-12 14:01:44 UTC
Created attachment 68013 [details] [review]
Bug 12802: Make tests pass
Comment 91 Julian Maurice 2017-10-12 14:04:23 UTC
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
Comment 92 Biblibre Sandboxes 2017-10-23 09:19:12 UTC
Patch tested with a sandbox, by Axelle Clarisse <axelle.clarisse@univ-amu.fr>
Comment 93 Biblibre Sandboxes 2017-10-23 09:20:31 UTC
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>
Comment 94 Biblibre Sandboxes 2017-10-23 09:20:35 UTC
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>
Comment 95 Biblibre Sandboxes 2017-10-23 09:20:40 UTC
Created attachment 68377 [details] [review]
Bug 12802: Fix tests in Letters.t

Signed-off-by: Axelle Clarisse <axelle.clarisse@univ-amu.fr>
Comment 96 Biblibre Sandboxes 2017-10-23 09:20:44 UTC
Created attachment 68378 [details] [review]
Bug 12802: Fix tests in Reserves.t

Signed-off-by: Axelle Clarisse <axelle.clarisse@univ-amu.fr>
Comment 97 Biblibre Sandboxes 2017-10-23 09:20:50 UTC
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>
Comment 98 Biblibre Sandboxes 2017-10-23 09:20:54 UTC
Created attachment 68380 [details] [review]
Bug 12802: Make tests pass

Signed-off-by: Axelle Clarisse <axelle.clarisse@univ-amu.fr>
Comment 99 Katrin Fischer 2018-02-07 07:31:33 UTC
Patches no longer apply, please rebase!
Comment 100 Nick Weingardt 2018-02-20 17:17:51 UTC
*** Bug 19577 has been marked as a duplicate of this bug. ***
Comment 101 Julian Maurice 2021-03-30 15:04:36 UTC
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.
Comment 102 Julian Maurice 2021-03-30 15:05:28 UTC
Patches rebased and squashed. Back to needs signoff
Comment 103 Fridolin Somers 2021-09-23 20:08:38 UTC
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
Comment 104 Fridolin Somers 2022-07-23 07:31:05 UTC
This conflicts in resent feature C4::Letters::ResendMessage where to_address is touched.
Comment 105 Julian Maurice 2023-04-06 12:18:39 UTC
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.
Comment 106 Jonathan Druart 2023-05-04 13:51:37 UTC
AutoEmailPrimaryAddress has been renamed :-/
Comment 107 Julian Maurice 2023-07-17 12:42:19 UTC
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.
Comment 108 Julian Maurice 2023-07-17 12:43:06 UTC
Patch rebased on master. Ready to be tested again
Comment 109 Mathieu Saby 2023-07-17 18:31:35 UTC
Hi
I am trying to test on a sandbox. Is it possible to purge pending messages without completely recreating the sandbox ?
Comment 110 Biblibre Sandboxes 2023-07-30 14:37:39 UTC
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>
Comment 111 Mathieu Saby 2023-07-30 14:39:24 UTC
Works as described, but note that the syspref is now named EmailFieldPrimary
Comment 112 Katrin Fischer 2023-08-31 07:18:07 UTC
(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?
Comment 113 Martin Renvoize 2023-09-21 07:00:50 UTC
Sorry, this doesn't apply again and I'm not quite close enough to it to trust myself on the rebase.
Comment 114 David Cook 2023-09-24 23:36:54 UTC
I'm curious how this would work with SSO that relies on the email field...
Comment 115 Mathieu Saby 2023-09-25 10:46:59 UTC
(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
Comment 116 Martin Renvoize 2023-10-26 17:56:01 UTC
One for next cycle at this point I reckon.
Comment 117 Aleisha Amohia 2023-12-13 02:00:02 UTC
We've received sponsorship to get this patch applying cleanly again.
Comment 118 Aleisha Amohia 2023-12-13 02:11:51 UTC
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>
Comment 119 Mathieu Saby 2023-12-13 13:08:53 UTC
Hi Aleisha
Could you please update the test plan (new name of the syspref AutoEmailPrimaryAddress)?
Comment 120 Aleisha Amohia 2023-12-15 01:13:31 UTC
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>
Comment 121 Mathieu Saby 2023-12-18 11:12:17 UTC
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:
Comment 122 Martin Renvoize 2023-12-19 16:47:55 UTC
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.
Comment 123 Martin Renvoize 2023-12-19 17:19:43 UTC
Created attachment 160051 [details] [review]
Bug 12802: (alt) Update system preference

Allow the EmailFieldPrimary preference to accept multiple selections
Comment 124 Martin Renvoize 2023-12-19 17:19:48 UTC
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.
Comment 125 Martin Renvoize 2023-12-19 17:34:57 UTC
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.
Comment 126 Aleisha Amohia 2023-12-20 01:38:37 UTC
Created attachment 160090 [details] [review]
Bug 12802: Update system preference

Allow the EmailFieldPrimary preference to accept multiple selections
Comment 127 Aleisha Amohia 2023-12-20 01:38:42 UTC
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
Comment 128 Aleisha Amohia 2023-12-20 01:38:47 UTC
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
Comment 129 Aleisha Amohia 2023-12-20 01:41:03 UTC
(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
Comment 130 Martin Renvoize 2023-12-20 08:01:24 UTC
Thanks for testing, and adding the proper test plan in, Aleisha. I'm glad you like the alternative approach.
Comment 131 Martin Renvoize 2023-12-20 12:09:04 UTC
Created attachment 160103 [details] [review]
Bug 12802: Update system preference

Allow the EmailFieldPrimary preference to accept multiple selections
Comment 132 Martin Renvoize 2023-12-20 12:09:09 UTC
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
Comment 133 Martin Renvoize 2023-12-20 12:09:14 UTC
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
Comment 134 Martin Renvoize 2023-12-20 12:09:19 UTC
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.