*** Bug 7999 has been marked as a duplicate of this bug. ***

We do have a simple feature allowing part of this.

We added a SendAllEmailsToLibrarian. We will change this for allowing to SendAllEmailTo in order to specify any email address.

Éric, it is a great idea!
But I think your patch should replace *all* locations where a mail is sent in order to avoid nasty surprise.
Indeed, sendmail is used at some places into Koha (use git grep sendmail).

Marked as Failed QA.

Both patches are required.

+            if (!($sendAllEmailsTo && $sendAllEmailsTo =~ /@/) ){ # some validation.  This could be improved.

Why not use Email::Valid->address() ? (This would be a new dependency.)
I suggested to use this cpan module under report 9032 (failed qa for now).
From there we could go further in checking email addresses in Koha in a consistent way.

I'd go one step further: All emailing should go through one and only one Koha method.
It's good practice to have system calls isolated so that when upgrade or a new platform require adjustment, we don't need to run around touching 10 files.
Or when we simply want to improve the process with validation...

I'd be happy to do that... on a different bug :)

(In reply to Blou from comment #8)
> I'd go one step further: All emailing should go through one and only one
> Koha method.
> It's good practice to have system calls isolated so that when upgrade or a
> new platform require adjustment, we don't need to run around touching 10
> files.
> Or when we simply want to improve the process with validation...
> 
> I'd be happy to do that... on a different bug :)

I completely agree, to the point that I think we whould *require* the centralization of this code, and this patch depend on said centralization.

I have a concern: we can't know if the user has been notified or if the email used was the SendAllEmailsTo content.
It could cause problem on debugging if the pref is available on a production server.
I don't have a suggestion (except adding a new column in the message_queue table (which is quite ugly).

I am not sure it is blocker.

> I have a concern: we can't know if the user has been notified or if the
> email used was the SendAllEmailsTo content.
On most situation where Koha sends an email to ... anyone, we have no way to know such action is taking place (script, cron...).  But if the email specified in the pref starts receiving a lot of emails, that should make it clear.  I rather have lot of emails in that box that I then need to reforward, rather than the reverse (call back emails sent wrongly to user).

> It could cause problem on debugging if the pref is available on a production
> server.
Actually, we need it on a production server.  (Our) usage is to set as initial email when things are being set up.  We do not want emails start flying to users while still getting set up.

Another common usage is for testing, but on a copy a production database.  Obviously, we do not want users to receive email based on now out of date/sync data.

This can act as a safety valve on various situation.

Comment on attachment 22315 [details] [review]
Bug 8000 - Override emails of every message sent from Koha

Changes are in here that somehow got lost.

I'm working on an improvement to this.

(In reply to Jonathan Druart from comment #10)
> I have a concern: we can't know if the user has been notified or if the
> email used was the SendAllEmailsTo content.
> It could cause problem on debugging if the pref is available on a production
> server.
> I don't have a suggestion (except adding a new column in the message_queue
> table (which is quite ugly).

This is why I generated the last two patches.
I added some not quite perfect testing for the first patch too.
However, the testing becomes nicer when all four patches are applied.

I did not add in the Email validation library as Marcel suggested in comment #7.

I did not centralize anything. I don't know if Kyle was intending to imply that not centralizing it would mean this patch wouldn't make it past QA in comment #9.

I forgot to mention: prove -v t/db_dependent/Letters.t
This should effectively test all the changes in C4::Letters.

Testing of:
- basket/sendbasket.pl
- misc/cronjobs/runreport.pl
- opac/opac-sendbasket.pl
- opac/opac-sendshelf.pl
- virtualshelves/sendshelf.pl
- C4::Letters::SendAlerts
will require arcane librarian/koha knowledge. ;)

I think it would either be best to either use Data::Validate::Email to validate the email address or just remove the simple email validation altogether. Since this validation isn't exactly valid it seems like a bit of a step backwards.

in adding the new patch to validate the emails, I kept the same "patch name" and it overrode the previous one with the same name.  So I
1) canceled the new "Override emails.,.." patch
2) submitted again the new one (under new name)
3) un-obsoleted the main one.

It would seem that Kyle recommended Data::Validate::Email in comment 27 and Marcel recommended Email::Valid in comment 7. Email::Valid is far more active than Data::Validate::Email. Just thought I'd point out the activity. Is there a preference?

(In reply to M. Tompsett from comment #31)
> It would seem that Kyle recommended Data::Validate::Email in comment 27 and
> Marcel recommended Email::Valid in comment 7. Email::Valid is far more
> active than Data::Validate::Email. Just thought I'd point out the activity.
> Is there a preference?

I agree with the activity. Moreover Data::Validate::Email depends on Data::Validate::Domain whereas Email::Valid does not depend on new dependencies.

No signoffers interested?

This patch could be very useful for developers and pre-production installations.

(In reply to Jonathan Druart from comment #34)
> No signoffers interested?
> 
> This patch could be very useful for developers and pre-production
> installations.

Could you post a full and complete test plan, so we don't have to read through the comments? This has been off my radar for while, sorry.

Patch failed at 0001 Bug 8000 - Override emails of every message sent from Koha

Details:

$ git show-ref -s HEAD
66c9e7a183e8e56c29b34e57d6c3c0b3cee09699

$ git bz apply 11140
No patches on bug 11140

$ git bz apply 8000

Bug 8000 - Test mode for notices

25924 - Bug 8000 - Override emails of every message sent from Koha
25925 - Bug 8000 - Missing test to trigger code change
25926 - Bug 8000 - Add a sentto_address column to message_queue
25928 - Bug 8000 - Correct inability to know where message was sent
25939 - Bug 8000 [QA Followup] - Change mis-used qw to q
25951 - Bug 8000 - Validate allEmailsTo email address before using it
26446 - Bug 8000: Replace Data::Validate::Email with Email::Valid

Applying: Bug 8000 - Override emails of every message sent from Koha
Auto-merging virtualshelves/sendshelf.pl
Auto-merging opac/opac-sendshelf.pl
Auto-merging opac/opac-sendbasket.pl
Auto-merging misc/cronjobs/runreport.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql
Auto-merging basket/sendbasket.pl
Auto-merging C4/Letters.pm
CONFLICT (content): Merge conflict in C4/Letters.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 8000 - Override emails of every message sent from Koha

The FailedQA is more likely to get someone to fix this. I believe I have corrected the existing patches to apply to master. The issue now is that the email validations done in the very first patch were not updated when the validation of emails was added. They still have the =~ /@/ type code in them.

M. Tompsett's patches would not apply.

I rebased everything again.
Hopefully this will the last time we need to do this.

Hi Maxime,

On Koha 3.21.00.008:

CONFLICT (content): Merge conflict in opac/opac-sendbasket.pl

CONFLICT (content): Merge conflict in basket/sendbasket.pl

Patch failed at 0001 Bug 8000 - Override emails of every message sent from Koha

Oops... found a mistake in my rebase... Let's try again. :)

All those changes in Letters.t and the line removals in those two conflicting areas: ARG! :) Anyways... Should be good to go again.

(In reply to M. Tompsett from comment #86)
> All those changes in Letters.t and the line removals in those two
> conflicting areas: ARG! :) Anyways... Should be good to go again.

yes. all commits in the set apply cleanly now. thank you mark tompsett.

Applying: Bug 8000 - Override emails of every message sent from Koha
fatal: sha1 information is lacking or useless (C4/Letters.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 8000 - Override emails of every message sent from Koha

Setting to Patch Doesn't Apply

Every patch should now apply correctly

Simple conflict in Letters.pm but then:
Applying: Bug 8000 - Correct inability to know where message was sent
fatal: sha1 information is lacking or useless (t/db_dependent/Letters.t).

If rebased I will try to test quickly

Rebased, solved merge issues and solved the SHA1 problem.

All the changes are now split into three commits :
   - The feature
   - The added test in Letter.t
   - The modified Koha/Schema

Passes qa -c 3, seems to apply correctly on my master.

Needs signoff

Created attachment 49998 [details] [review]
Bug 8000 - Override emails of every message sent from Koha

For testing purposes, we do not want emails sent to (legitimate) users.
And sometime we also like to actually see what would be generated for the users.
This preference will allow to override every message sent by koha with
a new (temporary) To address.  Leave it empty for normal usage.

    C4/Letters.pm
    installer/data/mysql/sysprefs.sql
    installer/data/mysql/updatedatabase.pl
    koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref

TESTING:
    0) Run updatedatabase.pl
    1) Enter a valid address for new preference SendAllEmailsTo, something that will be easily identified
       as NOT the normal destination for the Koha emails.
    2) Have koha generate an email to a user.  Or any other mean prefered that cause Koha to send email.
    3) Validate that the email is NOT sent to the user.
    4) Validate that the email IS sent to the overriding address.
    5) Clean the preference
    6) Redo the test, validate that the email is going to the right address.

    PS Not sure if those steps are precise enough.  Here's one way:
       a) set AutoEmailOpacUser to true
       b) create a new user, with an email address
       c) Normally, a confirmation email is sent to the user. Validate that it goes to the SendAllEmailsTo one.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 49999 [details] [review]
Bug 8000 - Missing test to trigger code change

This adds a test case which will trigger _send_message_by_email
and thus provide a way to run the changes made.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 50000 [details] [review]
Bug 8000 - Add a sentto_address column to message_queue

The problem with message_queue is the inability to tell if the
to_address was overriden at the time it was sent/attempted.
This column will reflect what was attempted.

MessageQueue.pm was generated by running the script
installer/data/mysql/updatedatabase.pl and then git add'ing
the resulting Koha/Schema/Result/MessageQueue.pm file. This is
necessary for 3.16 and higher.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Looking at this I am wondering if it wouldn't be better to implement the fallback to the central email address in Koha/Email.pm. It also takes care of other fallbacks like the reply-to addresses. 
It could avoid changes in some of the files and put things in a central spot.

Functionality
I have my doubts if some user will ever want to use this in a production system.
In a test system, I run one sql statement to achieve the same:  UPDATE borrowers SET email=, emailpro = .. etc
For testing a notice on a test patron, you do not need this address. 

Data::Validate::Email
It seems that you add a new dependency and then not use it? Can you remove it and not change PerlDependencies?

sentto_address
Do we really need that? If you choose to override the to_address, why not update the existing column?

sendbasket/sendshelf
I do not see the advantage of overriding an email address that you entered on that form. I would recommend not to change these scripts.
Similar (but maybe little bit less strong) for runreport.pl. Just update the to argument for the cronjob.

I will not change the status of this report. Another QAer may add his opinion.

I can see use of this for a system close to go live - as a security measure for last minute testing. But it's a bit difficult as you usually wouldn't want to test with real patrons.

I am also a bit concerned about the changes on sendbasket/sendshelf and doubt if we need to necessarily change those - the address you sent the notice to is entered manually, so this should not cause problems in testing.

I am going to fail this for now, mostly for thinking it could be simplified - please take a look at all the recent comments and comment/explain or provide a new patch.

Following comments from Marcel and Katrin, the patch was streamlined to keep only what was mandatory.

Data::Validate::Email: I don't know why it was there; removed.
sentto_address: Can't argue against keeping the table structure clean; removed.
sendbasket,sendshelf,runreport: Manually entered email addresses don't have to be overridden; removed.

Even with these changes out, I believe this patch is still valuable to anybody who has to troubleshoot a live system from time to time.

I'll answer the initial rebuke: this provides a secure way to test/validate the system without risk of sending emails to the users.  Changing values in the DB is not a real option, since there's no turning back.  Consider this a sleep mode, not a screw-this-db mode.

This helps us during migration, initial installation, as well as the specific testing of the various alert messages.

Created attachment 58434 [details] [review]
Bug 8000 - Override emails of every message sent from Koha

For testing purposes, we do not want emails sent to (legitimate) users.
And sometime we also like to actually see what would be generated for the users.
This preference will allow to override every message sent by koha with
a new (temporary) To address.  Leave it empty for normal usage.

    C4/Letters.pm
    installer/data/mysql/sysprefs.sql
    installer/data/mysql/updatedatabase.pl
    koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref

TESTING:
    0) Run updatedatabase.pl
    1) Enter a valid address for new preference SendAllEmailsTo, something that will be easily identified
       as NOT the normal destination for the Koha emails.
    2) Have koha generate an email to a user.  Or any other mean prefered that cause Koha to send email.
    3) Validate that the email is NOT sent to the user.
    4) Validate that the email IS sent to the overriding address.
    5) Clean the preference
    6) Redo the test, validate that the email is going to the right address.

    PS Not sure if those steps are precise enough.  Here's one way:
       a) set AutoEmailOpacUser to true
       b) create a new user, with an email address
       c) Normally, a confirmation email is sent to the user. Validate that it goes to the SendAllEmailsTo one.

I tested this patch twice using 2 different ways :
- I created a new patron with an adress A, send notices and then activated the syspref with an adress B --> I received mail on the A adress but not on the B adress. On the "message_queue" table, the "to_address" field is NULL instead of showing adress B
- I recharged the patch on the sandbox and activated the syspref immediately with an adresse B, then disativated it to use the A adress on the patron account --> I received mail on the B adress but when I empty the syspref, I received no mail on the A adress and on the "Notices" tab in the patron account, mails are "failed" and the field "to_address" on the "message_queue" table is NULL.

While I was testing this, I thought it would be quite "dangerous" to use in a production instance, for there is no way to send another time the notices that are supposed to be sent to patrons (hold available, overdues, etc.). Personnaly, I think it's very useful but will use it when pushed in master only on a test instance.

(In reply to Séverine Queune from comment #115)
> Personnaly, I think it's very useful but will use it when pushed in master
> only on a test instance.

Yes of course, that's how it must be used.

Created attachment 62863 [details] [review]
Bug 8000 - Override emails of every message sent from Koha

For testing purposes, we do not want emails sent to (legitimate) users.
And sometime we also like to actually see what would be generated for the users.
This preference will allow to override every message sent by koha with
a new (temporary) To address.  Leave it empty for normal usage.

    C4/Letters.pm
    installer/data/mysql/sysprefs.sql
    installer/data/mysql/updatedatabase.pl
    koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref

TESTING:
    0) Run updatedatabase.pl
    1) Enter a valid address for new preference SendAllEmailsTo, something that will be easily identified
       as NOT the normal destination for the Koha emails.
    2) Have koha generate an email to a user.  Or any other mean prefered that cause Koha to send email.
    3) Validate that the email is NOT sent to the user.
    4) Validate that the email IS sent to the overriding address.
    5) Clean the preference
    6) Redo the test, validate that the email is going to the right address.

    PS Not sure if those steps are precise enough.  Here's one way:
       a) set AutoEmailOpacUser to true
       b) create a new user, with an email address
       c) Normally, a confirmation email is sent to the user. Validate that it goes to the SendAllEmailsTo one.

I tested this patch with the latest version and everything seems to work. I did all the steps Séverine did and I received all the emails in 'message_queue' at the address set in 'SendAllEmailsTo'. I also checked for the value in 'to_address' and it was the same as 'SendAllEmailsTo'.

(In reply to Katrin Fischer from comment #107)
> Looking at this I am wondering if it wouldn't be better to implement the
> fallback to the central email address in Koha/Email.pm. It also takes care
> of other fallbacks like the reply-to addresses. 
> It could avoid changes in some of the files and put things in a central spot.

I am still confused about having so much duplicated code if we could just override the To => $params->{to} in Koha::Email? What am I missing?
Can you please explain why this other approach was taken?

The other place that this feature is likely to be used is in large libraries which keep a test server in place... these libraries tend to be *very* sensitive to sending out large numbers of emails to patrons.

As such, I think that it should be possible to override the syspref using a line in koha-conf.xml, so that libraries can load their production database into a test server, without having to manually re-set the syspref to make them safe.

Hm, you could already do that with Apache I think?

(In reply to Katrin Fischer from comment #121)
> Hm, you could already do that with Apache I think?

I thought of that, but Tomas pointed out that overdue_notices.pl doesn't run via apache...

It's a valid point then, but not sure we should make it part of this bug - maybe better to get foundations in and then add to it?

With the latest patch reworked by David, I feel like this bug can be placed once again in "Needs Signoff".

Rebased on the current master

Patch tested with a sandbox, by Ed Veal <eveal@mckinneytexas.org>

Created attachment 70484 [details] [review]
Bug 8000 - Redirect all emails to SendAllEmailsTo

Koha/Email.pm

Test plan:

1) Apply path
2) Run updatedatabase.pl
3) Clear all SendAllEmailsTo system preference
4) Send mail to a patron of your choosing, email will go to patron's
   email address as usual.
5) Set SendAllEmailsTo to a test email address
6) Send mail to the patron, email will be redirected to the email set
   in the systempreference.
7) Run prove -v t/db_dependent/Letters.t

It does not affect messages in the message_queue.

This patch obsoletes previous patches, because it achieves the same
functionality in a much more centralized way. (4 lines of code!)

Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Signed-off-by: BWS Sandboxes <ByWaterSandboxes@gmail.com>

(In reply to ByWaterSandboxes from comment #130)
> It does not affect messages in the message_queue.

So it's not "all emails" :)
I was going to fail QA this patch because I was testing overdue notices.

However it seems that is was the first intention of this patch (see first patch, in comment 3). Why did you change your mind?

(In reply to Jonathan Druart from comment #131)
> (In reply to ByWaterSandboxes from comment #130)
> > It does not affect messages in the message_queue.
> 
> So it's not "all emails" :)
> I was going to fail QA this patch because I was testing overdue notices.
> 
> However it seems that is was the first intention of this patch (see first
> patch, in comment 3). Why did you change your mind?

This comment needs an answer. Changing status to In Discussion

(In reply to Jonathan Druart from comment #131)
> (In reply to ByWaterSandboxes from comment #130)
> > It does not affect messages in the message_queue.
> 
> So it's not "all emails" :)
> I was going to fail QA this patch because I was testing overdue notices.
> 
> However it seems that is was the first intention of this patch (see first
> patch, in comment 3). Why did you change your mind?

Why do you think it would not affect overdue notices?

It changes Koha::Email create_message_headers which is used by:
SendAlerts (emails sent directly)
_send_message_by_emai (processes emails from the message_queue)

Actually I think it should cover all emails and if not, other emails not using Koha::Email would be a bug.

Maybe the commit message is wrong? (or I am)

(In reply to Katrin Fischer from comment #133)
> (In reply to Jonathan Druart from comment #131)
> > (In reply to ByWaterSandboxes from comment #130)
> > > It does not affect messages in the message_queue.
> > 
> > So it's not "all emails" :)
> > I was going to fail QA this patch because I was testing overdue notices.
> > 
> > However it seems that is was the first intention of this patch (see first
> > patch, in comment 3). Why did you change your mind?
> 
> Why do you think it would not affect overdue notices?
> 
> It changes Koha::Email create_message_headers which is used by:
> SendAlerts (emails sent directly)
> _send_message_by_emai (processes emails from the message_queue)
> 
> Actually I think it should cover all emails and if not, other emails not
> using Koha::Email would be a bug.
> 
> Maybe the commit message is wrong? (or I am)

Indeed, sounds like it would work. I cannot remember what I tested 3 months ago however.

I do not think the test coverage is enough.

(very) Quick at the code, I'd say there is one missing:

C4::Letters::SendAlerts, this block does not use Koha::Email

 467     elsif ( $type eq 'claimacquisition' or $type eq 'claimissues' or $type eq 'orderacquisition' ) {

And what about NoticeBcc?

Failing QA.

(In reply to Jonathan Druart from comment #137)
> And what about NoticeBcc?
> 
> Failing QA.

Sorry, can you explain the problem with NoticeBCC?

Should we file a new bug about the problem in SendAlerts?

(In reply to Katrin Fischer from comment #138)
> (In reply to Jonathan Druart from comment #137)
> > And what about NoticeBcc?
> > 
> > Failing QA.
> 
> Sorry, can you explain the problem with NoticeBCC?

It should be "mocked" as well.

> Should we file a new bug about the problem in SendAlerts?

Why? If we push this change we must make sure nothing will be sent unintentionally.

ok

Created attachment 74111 [details] [review]
Bug 8000: Make all branches use Koha::Email in C4::Letters

Corrects the unique branch in C4::Letters that didn't use Koha::Email to build the %mail hash. From a search in all pl/pm files, all %mail now built with Koha::Email.
Addresses CC and BCC fields (CC and BCC fields are ignored, except for NoticeBCC which is overriden by SendAllEmailsTo)

Created attachment 74848 [details] [review]
Bug 8000: Redirect all emails to SendAllEmailsTo

Rebased and squashed after changes to master.
Only difference from previous patches are small adjustements to conflicts in t/db_dependent/Letters.t

Test plan:

1) Apply path
2) Run updatedatabase.pl
3) Clear all SendAllEmailsTo system preference
4) Send mail to a patron of your choosing, email will go to patron's
   email address as usual.
5) Set SendAllEmailsTo to a test email address
6) Send mail to the patron, email will be redirected to the email set
   in the systempreference.
7) Run prove -v t/db_dependent/Letters.t

It does not affect messages in the message_queue.

This patch obsoletes previous patches, because it achieves the same
functionality in a much more centralized way. (4 lines of code!)

Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Signed-off-by: BWS Sandboxes <ByWaterSandboxes@gmail.com>

Patch doesn't apply on devbox & sandbox :

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 8000: Redirect all emails to SendAllEmailsTo
Using index info to reconstruct a base tree...
M       C4/Letters.pm
M       installer/data/mysql/sysprefs.sql
M       t/db_dependent/Letters.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Letters.t
CONFLICT (content): Merge conflict in t/db_dependent/Letters.t
Auto-merging installer/data/mysql/sysprefs.sql
Auto-merging C4/Letters.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 8000: Redirect all emails to SendAllEmailsTo

I fixed the conflict but tests are failing then:

t/db_dependent/Letters.t .. 1/67 
    #   Failed test 'mailto correct in sent claim'
    #   at t/db_dependent/Letters.t line 574.
    #          got: 'robert.tables@mail.com'
    #     expected: 'testemail@mydomain.com'
Fake sendmail at t/db_dependent/Letters.t line 32.
    # Looks like you failed 1 test of 8.
t/db_dependent/Letters.t .. 62/67 
#   Failed test 'SendAlerts - claimissue'

Created attachment 80370 [details] [review]
Bug 8000: Redirect all emails to SendAllEmailsTo

Rebased and squashed after changes to master.
Only difference from previous patches are small adjustements to conflicts in t/db_dependent/Letters.t

Test plan:

1) Apply path
2) Run updatedatabase.pl
3) Clear all SendAllEmailsTo system preference
4) Send mail to a patron of your choosing, email will go to patron's
   email address as usual.
5) Set SendAllEmailsTo to a test email address
6) Send mail to the patron, email will be redirected to the email set
   in the systempreference.
7) Run prove -v t/db_dependent/Letters.t

It does not affect messages in the message_queue.

This patch obsoletes previous patches, because it achieves the same
functionality in a much more centralized way. (4 lines of code!)

Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Signed-off-by: BWS Sandboxes <ByWaterSandboxes@gmail.com>

Wouldn't this be a little cleaner if the Bcc was also handled within Koha::Email the way Cc is with this patch?

Created attachment 81487 [details] [review]
Bug 8000: Redirect all emails to SendAllEmailsTo

Rebased and squashed after changes to master.
Only difference from previous patches are small adjustements to conflicts in t/db_dependent/Letters.t

Test plan:

1) Apply path
2) Run updatedatabase.pl
3) Clear all SendAllEmailsTo system preference
4) Send mail to a patron of your choosing, email will go to patron's
   email address as usual.
5) Set SendAllEmailsTo to a test email address
6) Send mail to the patron, email will be redirected to the email set
   in the systempreference.
7) Run prove -v t/db_dependent/Letters.t

It does not affect messages in the message_queue.

This patch obsoletes previous patches, because it achieves the same
functionality in a much more centralized way. (4 lines of code!)

Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Signed-off-by: BWS Sandboxes <ByWaterSandboxes@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 81488 [details] [review]
Bug 8000: (QA follow-up) Centralise Bcc too

I couldn't find any other occurrences where Koha::Email wasn't used either.

I've added a QA follow-up to try and catch any future uses of Bcc fields by moving the code to Koha::Email for that and updating the calling code appropriately.

It looks like this has been signed off a few times, so I've reset signed-off and asked another QAer to do a final pass to double check my followup more than anything else.

Created attachment 81589 [details] [review]
Bug 8000: Redirect all emails to SendAllEmailsTo

Rebased and squashed after changes to master.
Only difference from previous patches are small adjustements to conflicts in t/db_dependent/Letters.t

Test plan:

1) Apply path
2) Run updatedatabase.pl
3) Clear all SendAllEmailsTo system preference
4) Send mail to a patron of your choosing, email will go to patron's
   email address as usual.
5) Set SendAllEmailsTo to a test email address
6) Send mail to the patron, email will be redirected to the email set
   in the systempreference.
7) Run prove -v t/db_dependent/Letters.t

It does not affect messages in the message_queue.

This patch obsoletes previous patches, because it achieves the same
functionality in a much more centralized way. (4 lines of code!)

Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Signed-off-by: BWS Sandboxes <ByWaterSandboxes@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 81590 [details] [review]
Bug 8000: (QA follow-up) Centralise Bcc too

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 81591 [details] [review]
Bug 8000: (QA follow-up) Fix accidently edited sysprefs.sql file

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Couple questions:
1 - Is dependency on 11140 relevant?
2 - As far as I can tell this will never update the message_queue to reflect where the message was sent - can this be done? The mail logs would reflect the truth, but it feels like this is not optimal

Blocker:
1 - Message is only redirected if email in pref is valid, however, the email is not validated at entry - a typo could cause the redirection to fail

What were you thinking for the blocker? Interface validation that the email address is an actual address, or some kind of verification that the email address is one that actually receives mail? And what would you prefer to happen if the address given doesn't receive mail?

Created attachment 84801 [details] [review]
Bug 8000: Redirect all emails to SendAllEmailsTo

Rebased and squashed after changes to master.
Only difference from previous patches are small adjustements to conflicts in t/db_dependent/Letters.t

Test plan:

1) Apply path
2) Run updatedatabase.pl
3) Clear all SendAllEmailsTo system preference
4) Send mail to a patron of your choosing, email will go to patron's
   email address as usual.
5) Set SendAllEmailsTo to a test email address
6) Send mail to the patron, email will be redirected to the email set
   in the systempreference.
7) Run prove -v t/db_dependent/Letters.t

It does not affect messages in the message_queue.

This patch obsoletes previous patches, because it achieves the same
functionality in a much more centralized way. (4 lines of code!)

Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Signed-off-by: BWS Sandboxes <ByWaterSandboxes@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 84802 [details] [review]
Bug 8000: (QA follow-up) Centralise Bcc too

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 84803 [details] [review]
Bug 8000: (QA follow-up) Fix accidently edited sysprefs.sql file

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 84804 [details] [review]
Bug 8000: (QA follow-up) Update the to_address in the message queue

Created attachment 84805 [details] [review]
Bug 8000: (QA follow-up) Add email validation to system preferences

This patch uses the JQuery validator plugin to add validation to preferences of class email in the system preferences page. A field containing an invalid value (even if not modified) should prevent saving.

To test, confirm that when entering an invalid email address in the SendAllEmailsTo field, an error message appears and saving is prevented.
Correcting the value should hide the message and let you save as normal.

I have rebased the patches because I was getting conflicts in installer/data/mysql/sysprefs.sql.

Concerning comment #154

> 1 - Is dependency on 11140 relevant?

I don't suppose it is needed anymore, so I've removed it.

> 2 - As far as I can tell this will never update the message_queue to reflect
> where the message was sent - can this be done? The mail logs would reflect
> the truth, but it feels like this is not optimal

I added a patch to take care of updating the message queue with the to address used.

> Blocker:
> 1 - Message is only redirected if email in pref is valid, however, the email
> is not validated at entry - a typo could cause the redirection to fail

My last patch adds validation to the system preferences page making sure the entered email is valid. That way, the redirection should not fail because of an invalid email.

From the test plan, I don't get how «test mode for notices so a library can see what all of the notices will look like before go live» (from the description)

Can we trigger emails without the queue being consumed?

i.e. What would be the workflow to use this feature to go back and forth between tuning the notices and seeing the result.

(In reply to Victor Grousset/tuxayo from comment #162)
> From the test plan, I don't get how «test mode for notices so a library can
> see what all of the notices will look like before go live» (from the
> description)
> 
> Can we trigger emails without the queue being consumed?
> 
> i.e. What would be the workflow to use this feature to go back and forth
> between tuning the notices and seeing the result.

They are still send the same way (via the queue if they normally are), but to another email address that you specified.

Created attachment 86861 [details] [review]
Bug 8000: Redirect all emails to SendAllEmailsTo

Rebased and squashed after changes to master.
Only difference from previous patches are small adjustements to conflicts in t/db_dependent/Letters.t

Test plan:

1) Apply path
2) Run updatedatabase.pl
3) Clear all SendAllEmailsTo system preference
4) Send mail to a patron of your choosing, email will go to patron's
   email address as usual.
5) Set SendAllEmailsTo to a test email address
6) Send mail to the patron, email will be redirected to the email set
   in the systempreference.
7) Run prove -v t/db_dependent/Letters.t

It does not affect messages in the message_queue.

This patch obsoletes previous patches, because it achieves the same
functionality in a much more centralized way. (4 lines of code!)

Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Signed-off-by: BWS Sandboxes <ByWaterSandboxes@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Created attachment 86862 [details] [review]
Bug 8000: (QA follow-up) Centralise Bcc too

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Created attachment 86863 [details] [review]
Bug 8000: (QA follow-up) Fix accidently edited sysprefs.sql file

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Created attachment 86864 [details] [review]
Bug 8000: (QA follow-up) Update the to_address in the message queue

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Created attachment 86865 [details] [review]
Bug 8000: (QA follow-up) Add email validation to system preferences

This patch uses the JQuery validator plugin to add validation to preferences of class email in the system preferences page. A field containing an invalid value (even if not modified) should prevent saving.

To test, confirm that when entering an invalid email address in the SendAllEmailsTo field, an error message appears and saving is prevented.
Correcting the value should hide the message and let you save as normal.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

QA: Looking here

Created attachment 88874 [details] [review]
Bug 8000: Redirect all emails to SendAllEmailsTo

Rebased and squashed after changes to master.
Only difference from previous patches are small adjustements to conflicts in t/db_dependent/Letters.t

Test plan:

1) Apply path
2) Run updatedatabase.pl
3) Clear all SendAllEmailsTo system preference
4) Send mail to a patron of your choosing, email will go to patron's
   email address as usual.
5) Set SendAllEmailsTo to a test email address
6) Send mail to the patron, email will be redirected to the email set
   in the systempreference.
7) Run prove -v t/db_dependent/Letters.t

It does not affect messages in the message_queue.

This patch obsoletes previous patches, because it achieves the same
functionality in a much more centralized way. (4 lines of code!)

Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Signed-off-by: BWS Sandboxes <ByWaterSandboxes@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 88875 [details] [review]
Bug 8000: (QA follow-up) Centralise Bcc too

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 88876 [details] [review]
Bug 8000: (QA follow-up) Fix accidently edited sysprefs.sql file

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 88877 [details] [review]
Bug 8000: (QA follow-up) Update the to_address in the message queue

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 88878 [details] [review]
Bug 8000: (QA follow-up) Add email validation to system preferences

This patch uses the JQuery validator plugin to add validation to preferences of class email in the system preferences page. A field containing an invalid value (even if not modified) should prevent saving.

To test, confirm that when entering an invalid email address in the SendAllEmailsTo field, an error message appears and saving is prevented.
Correcting the value should hide the message and let you save as normal.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 88879 [details] [review]
Bug 8000: (QA follow-up) Two lastminute fixes

[1] Number of tests Letters.t
[2] Resolving uninitialized warn on Letters, L1327

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Awesome work all!

Pushed to master for 19.05

Enhancement will not be backported to 18.11.x series.

*** Bug 15857 has been marked as a duplicate of this bug. ***