Bug 12802 - Send notices using several email addresses
Summary: Send notices using several email addresses
Status: ASSIGNED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Matthias Meusburger
QA Contact: Testopia
URL:
Keywords:
: 19577 (view as bug list)
Depends on: 9043
Blocks: 18397 29393
  Show dependency treegraph
 
Reported: 2014-08-21 14:59 UTC by Jonathan Druart
Modified: 2022-08-02 00:16 UTC (History)
19 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress (3.37 KB, patch)
2014-08-21 16:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12802: Sent notices using several email addresses (33.42 KB, patch)
2014-08-21 16:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12802: FIX use the first valid email address if noemail is needed (1.19 KB, patch)
2014-08-25 10:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress (3.17 KB, patch)
2014-09-05 07:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12802: Sent notices using several email addresses (33.57 KB, patch)
2014-09-05 07:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12802: FIX use the first valid email address if noemail is needed (1.19 KB, patch)
2014-09-05 07:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12802: Fix call in C4::Letters (930 bytes, patch)
2014-09-23 11:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress (3.20 KB, patch)
2014-11-24 15:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12802: Sent notices using several email addresses (33.25 KB, patch)
2014-11-24 15:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12802: FIX use the first valid email address if noemail is needed (1.17 KB, patch)
2014-11-24 15:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12802: Fix if AutoEmailOpacUser is enabled (2.30 KB, patch)
2014-11-24 15:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress (3.20 KB, patch)
2015-02-13 08:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12802: Sent notices using several email addresses (33.33 KB, patch)
2015-02-13 08:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12802: FIX use the first valid email address if noemail is needed (1.17 KB, patch)
2015-02-13 08:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12802: Fix if AutoEmailOpacUser is enabled (2.30 KB, patch)
2015-02-13 08:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress (3.20 KB, patch)
2015-02-16 10:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12802: Sent notices using several email addresses (34.02 KB, patch)
2015-02-16 10:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12802: FIX use the first valid email address if noemail is needed (1.17 KB, patch)
2015-02-16 10:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12802: Fix if AutoEmailOpacUser is enabled (2.30 KB, patch)
2015-02-16 10:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12802: Fix QA script issues (1009 bytes, patch)
2015-02-16 10:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress (3.26 KB, patch)
2015-02-20 04:40 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 12802: Sent notices using several email addresses (34.09 KB, patch)
2015-02-20 04:41 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 12802: FIX use the first valid email address if noemail is needed (1.24 KB, patch)
2015-02-20 04:41 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 12802: Fix if AutoEmailOpacUser is enabled (2.37 KB, patch)
2015-02-20 04:41 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 12802: Fix QA script issues (1.05 KB, patch)
2015-02-20 04:41 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 12802: Fix tests in Letters.t (2.66 KB, patch)
2015-06-02 08:20 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 12802: EnqueueLetter must retrieve at least an email (for print messages). (903 bytes, patch)
2015-06-02 12:52 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 12802: Fix tests in Reserves.t (2.50 KB, patch)
2015-06-16 09:19 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress (3.20 KB, patch)
2015-12-08 09:41 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12802: Sent notices using several email addresses (33.97 KB, patch)
2015-12-08 09:41 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12802: FIX use the first valid email address if noemail is needed (1.24 KB, patch)
2015-12-08 09:41 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12802: Fix if AutoEmailOpacUser is enabled (2.37 KB, patch)
2015-12-08 09:42 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12802: Fix QA script issues (1.05 KB, patch)
2015-12-08 09:42 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12802: Fix tests in Letters.t (2.68 KB, patch)
2015-12-08 09:42 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12802: Fix tests in Reserves.t (2.50 KB, patch)
2015-12-08 09:42 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12802: Sent notices using several email addresses (30.27 KB, patch)
2016-11-08 08:22 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress (3.22 KB, patch)
2017-04-05 10:41 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: Sent notices using several email addresses (30.10 KB, patch)
2017-04-05 10:41 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: FIX use the first valid email address if noemail is needed (1.24 KB, patch)
2017-04-05 10:41 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: Fix if AutoEmailOpacUser is enabled (2.37 KB, patch)
2017-04-05 10:41 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: Fix QA script issues (1.06 KB, patch)
2017-04-05 10:41 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: Fix tests in Letters.t (2.68 KB, patch)
2017-04-05 10:41 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: Fix tests in Reserves.t (2.50 KB, patch)
2017-04-05 10:41 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress (3.22 KB, patch)
2017-09-07 13:05 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: Sent notices using several email addresses (30.79 KB, patch)
2017-09-07 13:05 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: FIX use the first valid email address if noemail is needed (1.24 KB, patch)
2017-09-07 13:05 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: Fix if AutoEmailOpacUser is enabled (2.37 KB, patch)
2017-09-07 13:05 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: Fix QA script issues (1.06 KB, patch)
2017-09-07 13:05 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: Fix tests in Letters.t (2.68 KB, patch)
2017-09-07 13:05 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: Fix tests in Reserves.t (2.50 KB, patch)
2017-09-07 13:05 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress (3.22 KB, patch)
2017-10-12 13:45 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: Sent notices using several email addresses (32.82 KB, patch)
2017-10-12 13:45 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: Fix tests in Letters.t (2.68 KB, patch)
2017-10-12 13:45 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: Fix tests in Reserves.t (2.50 KB, patch)
2017-10-12 13:45 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: Update all circulation alerts, not only the first one (3.26 KB, patch)
2017-10-12 13:45 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: Make tests pass (4.48 KB, patch)
2017-10-12 13:45 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress (3.22 KB, patch)
2017-10-12 14:01 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: Sent notices using several email addresses (33.22 KB, patch)
2017-10-12 14:01 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: Fix tests in Letters.t (2.68 KB, patch)
2017-10-12 14:01 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: Fix tests in Reserves.t (2.50 KB, patch)
2017-10-12 14:01 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: Update all circulation alerts, not only the first one (3.26 KB, patch)
2017-10-12 14:01 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: Make tests pass (4.48 KB, patch)
2017-10-12 14:01 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12802: DB changes - Set type multiple for AutoEmailPrimaryAddress (3.26 KB, patch)
2017-10-23 09:20 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 12802: Sent notices using several email addresses (33.22 KB, patch)
2017-10-23 09:20 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 12802: Fix tests in Letters.t (2.73 KB, patch)
2017-10-23 09:20 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 12802: Fix tests in Reserves.t (2.55 KB, patch)
2017-10-23 09:20 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 12802: Update all circulation alerts, not only the first one (3.30 KB, patch)
2017-10-23 09:20 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 12802: Make tests pass (4.52 KB, patch)
2017-10-23 09:20 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 12802: Sent notices using several email addresses (45.24 KB, patch)
2021-03-30 15:04 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 sandboxes@biblibre.com 2017-10-23 09:19:12 UTC
Patch tested with a sandbox, by Axelle Clarisse <axelle.clarisse@univ-amu.fr>
Comment 93 sandboxes@biblibre.com 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 sandboxes@biblibre.com 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 sandboxes@biblibre.com 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 sandboxes@biblibre.com 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 sandboxes@biblibre.com 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 sandboxes@biblibre.com 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.