Bug 8000

Summary: Test mode for notices
Product: Koha Reporter: Liz Rea <wizzyrea>
Component: NoticesAssignee: Charles Farmer <charles.farmer>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: aleisha, barton, ByWaterSandboxes, charles.farmer, dcook, eric.begin, gmcharlt, indradg, jdemuth, jens.weber, jonathan.druart, josef.moravec, julian.maurice, jzairo, katrin.fischer, koha, kyle, lisettepalouse+koha, m.de.rooy, marjorie.barry-vila, martin.renvoize, maryse.simard, mtompset, niamh.walker-headon, nick, philippe.blouin, sally.healey, severine.queune, tmisilo, veron, victor, wizzyrea
Version: MainKeywords: dependency
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12802
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This enhancement adds a system preference 'SendAllEmailsTo' which, when populated with a valid email address, will redirect all outgoing mail from Koha to this address. This feature is intended for use during testing/setup of Koha, to prevent spamming users.
Version(s) released in:
19.05.00
Bug Depends on:    
Bug Blocks: 28870    
Attachments: Bug 8000 - Override emails of every message sent from Koha
Bug 8000 - Override emails of every message sent from Koha
Bug 8000 - Override emails of every message sent from Koha
Bug 8000 - Override emails of every message sent from Koha
Bug 8000 - Missing test to trigger code change
Bug 8000 - Add a sentto_address column to message_queue
Bug 8000 - Correct inability to know where message was sent
Bug 8000 - Override emails of every message sent from Koha
Bug 8000 - Missing test to trigger code change
Bug 8000 - Add a sentto_address column to message_queue
Bug 8000 - Correct inability to know where message was sent
Bug 8000 - Correct inability to know where message was sent
Bug 8000 [QA Followup] - Change mis-used qw to q
Bug 8000 - Override emails of every message sent from Koha
Bug 8000 - Validate allEmailsTo email address before using it
Bug 8000: Replace Data::Validate::Email with Email::Valid
Bug 8000 - Override emails of every message sent from Koha
Bug 8000 - Missing test to trigger code change
Bug 8000 - Add a sentto_address column to message_queue
Bug 8000 - Correct inability to know where message was sent
Bug 8000 [QA Followup] - Change mis-used qw to q
Bug 8000 - Validate allEmailsTo email address before using it
Bug 8000: Replace Data::Validate::Email with Email::Valid
Bug 8000 - Override emails of every message sent from Koha
Bug 8000 - Missing test to trigger code change
Bug 8000 - Add a sentto_address column to message_queue
Bug 8000 - Correct inability to know where message was sent
Bug 8000 [QA Followup] - Change mis-used qw to q
Bug 8000 - Validate allEmailsTo email address before using it
Bug 8000: Replace Data::Validate::Email with Email::Valid
Bug 8000 - Override emails of every message sent from Koha
Bug 8000 - Missing test to trigger code change
Bug 8000 - Add a sentto_address column to message_queue
Bug 8000 - Correct inability to know where message was sent
Bug 8000 [QA Followup] - Change mis-used qw to q
Bug 8000 - Validate allEmailsTo email address before using it
Bug 8000: Replace Data::Validate::Email with Email::Valid
Bug 8000 - Override emails of every message sent from Koha
Bug 8000 - Missing test to trigger code change
Bug 8000 - Add a sentto_address column to message_queue
Bug 8000 - Correct inability to know where message was sent
Bug 8000 - Validate allEmailsTo email address before using it
Bug 8000: Replace Data::Validate::Email with Email::Valid
Bug 8000 - Updated the remaining email validation to Email::Valid .
Bug 8000 - Atomic Update
Bug 8000 - Override emails of every message sent from Koha
Bug 8000 - Missing test to trigger code change
Bug 8000 - Add a sentto_address column to message_queue
Bug 8000 - Correct inability to know where message was sent
Bug 8000 - Validate allEmailsTo email address before using it
Bug 8000: Replace Data::Validate::Email with Email::Valid
Bug 8000 - Updated the remaining email validation to Email::Valid .
Bug 8000 - Atomic Update
Bug 8000 - Override emails of every message sent from Koha
Bug 8000 - Missing test to trigger code change
Bug 8000 - Add a sentto_address column to message_queue
Bug 8000 - Correct inability to know where message was sent
Bug 8000 - Validate allEmailsTo email address before using it
Bug 8000: Replace Data::Validate::Email with Email::Valid
Bug 8000 - Updated the remaining email validation to Email::Valid .
Bug 8000 - Atomic Update
Bug 8000 - Override emails of every message sent from Koha
Bug 8000 - Override emails of every message sent from Koha
Bug 8000 - Missing test to trigger code change
Bug 8000 - Add a sentto_address column to message_queue
Bug 8000 - Correct inability to know where message was sent
Bug 8000 - Validate allEmailsTo email address before using it
Bug 8000: Replace Data::Validate::Email with Email::Valid
Bug 8000 - Updated the remaining email validation to Email::Valid .
Bug 8000 - Atomic Update
Bug 8000 - Override emails of every message sent from Koha
Bug 8000 - Missing test to trigger code change
Bug 8000 - Add a sentto_address column to message_queue
Bug 8000 - Override emails of every message sent from Koha
Bug 8000 - Missing test to trigger code change
Bug 8000 - Add a sentto_address column to message_queue
Bug 8000 - Override emails of every message sent from Koha
Bug 8000 - Override emails of every message sent from Koha
Bug 8000 - Override emails of every message sent from Koha
Bug 8000 - Redirect all emails to SendAllEmailsTo
Bug 8000 - Redirect all emails to SendAllEmailsTo
Bug 8000 - Redirect all emails to SendAllEmailsTo
Bug 8000 - Redirect all emails to SendAllEmailsTo
Bug 8000: Redirect all emails to SendAllEmailsTo
Bug 8000: Make all branches use Koha::Email in C4::Letters
Bug 8000: Redirect all emails to SendAllEmailsTo
Bug 8000: Redirect all emails to SendAllEmailsTo
Bug 8000: Redirect all emails to SendAllEmailsTo
Bug 8000: (QA follow-up) Centralise Bcc too
Bug 8000: Redirect all emails to SendAllEmailsTo
Bug 8000: (QA follow-up) Centralise Bcc too
Bug 8000: (QA follow-up) Fix accidently edited sysprefs.sql file
Bug 8000: Redirect all emails to SendAllEmailsTo
Bug 8000: (QA follow-up) Centralise Bcc too
Bug 8000: (QA follow-up) Fix accidently edited sysprefs.sql file
Bug 8000: (QA follow-up) Update the to_address in the message queue
Bug 8000: (QA follow-up) Add email validation to system preferences
Bug 8000: Redirect all emails to SendAllEmailsTo
Bug 8000: (QA follow-up) Centralise Bcc too
Bug 8000: (QA follow-up) Fix accidently edited sysprefs.sql file
Bug 8000: (QA follow-up) Update the to_address in the message queue
Bug 8000: (QA follow-up) Add email validation to system preferences
Bug 8000: Redirect all emails to SendAllEmailsTo
Bug 8000: (QA follow-up) Centralise Bcc too
Bug 8000: (QA follow-up) Fix accidently edited sysprefs.sql file
Bug 8000: (QA follow-up) Update the to_address in the message queue
Bug 8000: (QA follow-up) Add email validation to system preferences
Bug 8000: (QA follow-up) Two lastminute fixes

Description Liz Rea 2012-04-23 21:50:09 UTC
Koha should have a test mode for notices so a library can see what all of the notices will look like before go live, or to troubleshoot difficulties.

Test mode would do the following:

-redirect all mails to a specified (by default the defined library email address) address

- Ability to specify a patron as a "test" patron
Comment 1 Nicole C. Engard 2012-04-24 01:32:49 UTC
*** Bug 7999 has been marked as a duplicate of this bug. ***
Comment 2 Eric Bégin 2013-10-11 12:48:59 UTC
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.
Comment 3 Blou 2013-10-11 20:02:45 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2013-10-23 08:51:40 UTC
É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.
Comment 5 Blou 2013-10-23 14:03:59 UTC Comment hidden (obsolete)
Comment 6 Blou 2013-10-23 14:05:07 UTC
Both patches are required.
Comment 7 Marcel de Rooy 2013-10-24 08:40:37 UTC
+            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.
Comment 8 Blou 2013-10-24 12:03:14 UTC
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 :)
Comment 9 Kyle M Hall 2013-10-25 14:40:54 UTC
(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.
Comment 10 Jonathan Druart 2014-01-27 12:32:28 UTC
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.
Comment 11 Blou 2014-01-27 19:16:32 UTC
> 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 12 Blou 2014-01-27 21:32:44 UTC Comment hidden (obsolete)
Comment 13 Mark Tompsett 2014-03-06 19:08:54 UTC
Comment on attachment 22315 [details] [review]
Bug 8000 - Override emails of every message sent from Koha

Changes are in here that somehow got lost.
Comment 14 Blou 2014-03-06 20:28:05 UTC Comment hidden (obsolete)
Comment 15 Mark Tompsett 2014-03-06 22:01:14 UTC Comment hidden (obsolete)
Comment 16 Mark Tompsett 2014-03-06 22:22:44 UTC Comment hidden (obsolete)
Comment 17 Mark Tompsett 2014-03-06 22:23:30 UTC
I'm working on an improvement to this.
Comment 18 Mark Tompsett 2014-03-07 03:29:42 UTC Comment hidden (obsolete)
Comment 19 Mark Tompsett 2014-03-07 03:56:24 UTC Comment hidden (obsolete)
Comment 20 Mark Tompsett 2014-03-07 03:56:35 UTC Comment hidden (obsolete)
Comment 21 Mark Tompsett 2014-03-07 03:56:43 UTC Comment hidden (obsolete)
Comment 22 Mark Tompsett 2014-03-07 03:56:51 UTC Comment hidden (obsolete)
Comment 23 Mark Tompsett 2014-03-07 04:14:14 UTC Comment hidden (obsolete)
Comment 24 Mark Tompsett 2014-03-07 04:36:23 UTC
(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.
Comment 25 Mark Tompsett 2014-03-07 04:56:22 UTC
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. ;)
Comment 26 Kyle M Hall 2014-03-07 16:37:23 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2014-03-07 16:41:51 UTC
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.
Comment 28 Blou 2014-03-07 18:33:56 UTC Comment hidden (obsolete)
Comment 29 Blou 2014-03-07 18:36:18 UTC Comment hidden (obsolete)
Comment 30 Blou 2014-03-07 18:41:52 UTC
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.
Comment 31 Mark Tompsett 2014-03-07 20:29:04 UTC
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?
Comment 32 Jonathan Druart 2014-03-19 09:31:09 UTC
(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.
Comment 33 Jonathan Druart 2014-03-19 09:41:37 UTC Comment hidden (obsolete)
Comment 34 Jonathan Druart 2015-02-16 16:02:28 UTC
No signoffers interested?

This patch could be very useful for developers and pre-production installations.
Comment 35 Mark Tompsett 2015-02-16 16:33:41 UTC
(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.
Comment 36 koha 2015-03-09 10:58:22 UTC
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
Comment 37 Mark Tompsett 2015-04-03 17:33:51 UTC Comment hidden (obsolete)
Comment 38 Mark Tompsett 2015-04-03 17:33:57 UTC Comment hidden (obsolete)
Comment 39 Mark Tompsett 2015-04-03 17:34:05 UTC Comment hidden (obsolete)
Comment 40 Mark Tompsett 2015-04-03 17:34:11 UTC Comment hidden (obsolete)
Comment 41 Mark Tompsett 2015-04-03 17:34:16 UTC Comment hidden (obsolete)
Comment 42 Mark Tompsett 2015-04-03 17:34:23 UTC Comment hidden (obsolete)
Comment 43 Mark Tompsett 2015-04-03 17:34:28 UTC Comment hidden (obsolete)
Comment 44 Mark Tompsett 2015-04-03 17:44:07 UTC Comment hidden (obsolete)
Comment 45 Mark Tompsett 2015-04-03 17:44:12 UTC Comment hidden (obsolete)
Comment 46 Mark Tompsett 2015-04-03 17:44:16 UTC Comment hidden (obsolete)
Comment 47 Mark Tompsett 2015-04-03 17:44:21 UTC Comment hidden (obsolete)
Comment 48 Mark Tompsett 2015-04-03 17:44:25 UTC Comment hidden (obsolete)
Comment 49 Mark Tompsett 2015-04-03 17:44:31 UTC Comment hidden (obsolete)
Comment 50 Mark Tompsett 2015-04-03 17:44:39 UTC Comment hidden (obsolete)
Comment 51 Mark Tompsett 2015-04-03 17:48:06 UTC Comment hidden (obsolete)
Comment 52 Mark Tompsett 2015-04-03 17:48:12 UTC Comment hidden (obsolete)
Comment 53 Mark Tompsett 2015-04-03 17:48:17 UTC Comment hidden (obsolete)
Comment 54 Mark Tompsett 2015-04-03 17:48:22 UTC Comment hidden (obsolete)
Comment 55 Mark Tompsett 2015-04-03 17:48:28 UTC Comment hidden (obsolete)
Comment 56 Mark Tompsett 2015-04-03 17:48:32 UTC Comment hidden (obsolete)
Comment 57 Mark Tompsett 2015-04-03 17:48:37 UTC Comment hidden (obsolete)
Comment 58 Mark Tompsett 2015-04-03 18:08:25 UTC
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.
Comment 59 Maxime Beaulieu 2015-06-08 13:29:27 UTC Comment hidden (obsolete)
Comment 60 Maxime Beaulieu 2015-06-08 13:30:15 UTC Comment hidden (obsolete)
Comment 61 Maxime Beaulieu 2015-06-08 13:30:38 UTC Comment hidden (obsolete)
Comment 62 Maxime Beaulieu 2015-06-08 13:30:51 UTC Comment hidden (obsolete)
Comment 63 Maxime Beaulieu 2015-06-08 13:31:23 UTC Comment hidden (obsolete)
Comment 64 Maxime Beaulieu 2015-06-08 13:31:46 UTC Comment hidden (obsolete)
Comment 65 Maxime Beaulieu 2015-06-08 13:32:53 UTC Comment hidden (obsolete)
Comment 66 Maxime Beaulieu 2015-06-08 13:33:19 UTC Comment hidden (obsolete)
Comment 67 Maxime Beaulieu 2015-06-08 13:37:01 UTC
M. Tompsett's patches would not apply.

I rebased everything again.
Hopefully this will the last time we need to do this.
Comment 68 Indranil Das Gupta 2015-06-17 08:16:35 UTC
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
Comment 69 Mark Tompsett 2015-06-17 10:58:41 UTC Comment hidden (obsolete)
Comment 70 Mark Tompsett 2015-06-17 10:58:46 UTC Comment hidden (obsolete)
Comment 71 Mark Tompsett 2015-06-17 10:58:51 UTC Comment hidden (obsolete)
Comment 72 Mark Tompsett 2015-06-17 10:58:56 UTC Comment hidden (obsolete)
Comment 73 Mark Tompsett 2015-06-17 10:59:01 UTC Comment hidden (obsolete)
Comment 74 Mark Tompsett 2015-06-17 10:59:06 UTC Comment hidden (obsolete)
Comment 75 Mark Tompsett 2015-06-17 10:59:11 UTC Comment hidden (obsolete)
Comment 76 Mark Tompsett 2015-06-17 10:59:16 UTC Comment hidden (obsolete)
Comment 77 Mark Tompsett 2015-06-17 11:08:46 UTC
Oops... found a mistake in my rebase... Let's try again. :)
Comment 78 Mark Tompsett 2015-06-17 11:09:07 UTC Comment hidden (obsolete)
Comment 79 Mark Tompsett 2015-06-17 11:09:12 UTC Comment hidden (obsolete)
Comment 80 Mark Tompsett 2015-06-17 11:09:18 UTC Comment hidden (obsolete)
Comment 81 Mark Tompsett 2015-06-17 11:09:24 UTC Comment hidden (obsolete)
Comment 82 Mark Tompsett 2015-06-17 11:09:29 UTC Comment hidden (obsolete)
Comment 83 Mark Tompsett 2015-06-17 11:09:35 UTC Comment hidden (obsolete)
Comment 84 Mark Tompsett 2015-06-17 11:09:40 UTC Comment hidden (obsolete)
Comment 85 Mark Tompsett 2015-06-17 11:09:46 UTC Comment hidden (obsolete)
Comment 86 Mark Tompsett 2015-06-17 11:10:57 UTC
All those changes in Letters.t and the line removals in those two conflicting areas: ARG! :) Anyways... Should be good to go again.
Comment 87 Indranil Das Gupta 2015-06-17 11:27:06 UTC
(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.
Comment 88 Aleisha Amohia 2015-12-28 21:59:14 UTC
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
Comment 89 Rémi Mayrand-Provencher 2016-01-06 20:25:47 UTC Comment hidden (obsolete)
Comment 90 Rémi Mayrand-Provencher 2016-01-06 20:31:33 UTC Comment hidden (obsolete)
Comment 91 Rémi Mayrand-Provencher 2016-01-06 20:31:41 UTC Comment hidden (obsolete)
Comment 92 Rémi Mayrand-Provencher 2016-01-06 20:31:47 UTC Comment hidden (obsolete)
Comment 93 Rémi Mayrand-Provencher 2016-01-06 20:31:53 UTC Comment hidden (obsolete)
Comment 94 Rémi Mayrand-Provencher 2016-01-06 20:31:59 UTC Comment hidden (obsolete)
Comment 95 Rémi Mayrand-Provencher 2016-01-06 20:32:04 UTC Comment hidden (obsolete)
Comment 96 Rémi Mayrand-Provencher 2016-01-06 20:32:10 UTC Comment hidden (obsolete)
Comment 97 Rémi Mayrand-Provencher 2016-01-06 20:32:17 UTC Comment hidden (obsolete)
Comment 98 Rémi Mayrand-Provencher 2016-01-06 20:33:39 UTC
Every patch should now apply correctly
Comment 99 Nick Clemens 2016-04-06 00:37:59 UTC
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
Comment 100 Charles Farmer 2016-04-06 15:33:07 UTC Comment hidden (obsolete)
Comment 101 Charles Farmer 2016-04-06 15:33:32 UTC Comment hidden (obsolete)
Comment 102 Charles Farmer 2016-04-06 15:33:48 UTC Comment hidden (obsolete)
Comment 103 Charles Farmer 2016-04-06 15:40:54 UTC
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
Comment 104 Nick Clemens 2016-04-06 16:20:48 UTC
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>
Comment 105 Nick Clemens 2016-04-06 16:20:55 UTC
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>
Comment 106 Nick Clemens 2016-04-06 16:21:01 UTC
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>
Comment 107 Katrin Fischer 2016-04-07 06:09:21 UTC
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.
Comment 108 Marcel de Rooy 2016-04-08 06:57:33 UTC
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.
Comment 109 Katrin Fischer 2016-04-12 06:16:37 UTC
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.
Comment 110 Charles Farmer 2016-09-29 19:58:26 UTC Comment hidden (obsolete)
Comment 111 Charles Farmer 2016-09-29 20:14:00 UTC
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.
Comment 112 Blou 2016-09-29 20:25:41 UTC
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.
Comment 113 Marc Véron 2016-12-16 08:25:12 UTC Comment hidden (obsolete)
Comment 114 Charles Farmer 2016-12-27 14:45:55 UTC
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.
Comment 115 Séverine Queune 2017-03-23 10:10:55 UTC
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.
Comment 116 Jonathan Druart 2017-03-23 14:44:17 UTC
(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.
Comment 117 Philippe Audet-Fortin 2017-04-28 19:10:42 UTC
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.
Comment 118 Philippe Audet-Fortin 2017-04-28 19:38:20 UTC
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'.
Comment 119 Katrin Fischer 2017-05-01 21:11:22 UTC
(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?
Comment 120 Barton Chittenden 2017-05-01 22:04:41 UTC
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.
Comment 121 Katrin Fischer 2017-05-01 22:11:26 UTC
Hm, you could already do that with Apache I think?
Comment 122 Barton Chittenden 2017-05-02 01:40:01 UTC
(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...
Comment 123 Katrin Fischer 2017-05-02 07:50:14 UTC
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?
Comment 124 David Bourgault 2017-09-15 19:24:02 UTC Comment hidden (obsolete)
Comment 125 David Bourgault 2017-09-15 19:26:26 UTC Comment hidden (obsolete)
Comment 126 Charles Farmer 2017-12-21 21:03:20 UTC Comment hidden (obsolete)
Comment 127 Charles Farmer 2017-12-21 21:04:26 UTC
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
Comment 128 Charles Farmer 2017-12-21 21:07:39 UTC Comment hidden (obsolete)
Comment 129 ByWaterSandboxes 2018-01-14 18:40:11 UTC
Patch tested with a sandbox, by Ed Veal <eveal@mckinneytexas.org>
Comment 130 ByWaterSandboxes 2018-01-14 18:40:34 UTC
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>
Comment 131 Jonathan Druart 2018-01-23 14:13:00 UTC
(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?
Comment 132 Julian Maurice 2018-04-10 05:58:35 UTC
(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
Comment 133 Katrin Fischer 2018-04-10 21:42:15 UTC
(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)
Comment 134 Jonathan Druart 2018-04-11 15:20:57 UTC
(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.
Comment 135 Jonathan Druart 2018-04-11 15:21:19 UTC
I do not think the test coverage is enough.
Comment 136 Jonathan Druart 2018-04-11 15:23:36 UTC
(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' ) {
Comment 137 Jonathan Druart 2018-04-11 15:26:44 UTC
And what about NoticeBcc?

Failing QA.
Comment 138 Katrin Fischer 2018-04-12 09:17:49 UTC
(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?
Comment 139 Jonathan Druart 2018-04-12 14:08:06 UTC
(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.
Comment 140 Katrin Fischer 2018-04-12 14:52:40 UTC
ok
Comment 141 David Bourgault 2018-04-12 15:45:57 UTC Comment hidden (obsolete)
Comment 142 David Bourgault 2018-04-12 15:55:58 UTC
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)
Comment 143 David Bourgault 2018-04-25 16:01:35 UTC
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>
Comment 144 Séverine Queune 2018-10-06 09:36:10 UTC
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
Comment 145 Jonathan Druart 2018-10-09 14:21:15 UTC
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'
Comment 146 Blou 2018-10-10 21:24:02 UTC
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>
Comment 147 Martin Renvoize 2018-10-29 12:35:27 UTC
Wouldn't this be a little cleaner if the Bcc was also handled within Koha::Email the way Cc is with this patch?
Comment 148 Martin Renvoize 2018-10-29 13:04:21 UTC
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>
Comment 149 Martin Renvoize 2018-10-29 13:04:28 UTC
Created attachment 81488 [details] [review]
Bug 8000: (QA follow-up) Centralise Bcc too
Comment 150 Martin Renvoize 2018-10-29 13:07:46 UTC
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.
Comment 151 Josef Moravec 2018-10-29 21:39:32 UTC
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>
Comment 152 Josef Moravec 2018-10-29 21:39:41 UTC
Created attachment 81590 [details] [review]
Bug 8000: (QA follow-up) Centralise Bcc too

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 153 Josef Moravec 2018-10-29 21:39:53 UTC
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>
Comment 154 Nick Clemens 2018-11-08 11:38:50 UTC
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
Comment 155 Liz Rea 2019-01-12 03:34:45 UTC
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?
Comment 156 Maryse Simard 2019-02-05 23:51:54 UTC
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>
Comment 157 Maryse Simard 2019-02-05 23:52:06 UTC
Created attachment 84802 [details] [review]
Bug 8000: (QA follow-up) Centralise Bcc too

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 158 Maryse Simard 2019-02-05 23:52:16 UTC
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>
Comment 159 Maryse Simard 2019-02-05 23:52:26 UTC
Created attachment 84804 [details] [review]
Bug 8000: (QA follow-up) Update the to_address in the message queue
Comment 160 Maryse Simard 2019-02-05 23:53:15 UTC
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.
Comment 161 Maryse Simard 2019-02-06 00:02:05 UTC
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.
Comment 162 Victor Grousset/tuxayo 2019-02-18 16:43:49 UTC
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.
Comment 163 Katrin Fischer 2019-02-18 17:04:33 UTC
(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.
Comment 164 Liz Rea 2019-03-21 19:27:43 UTC
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>
Comment 165 Liz Rea 2019-03-21 19:27:51 UTC
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>
Comment 166 Liz Rea 2019-03-21 19:27:57 UTC
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>
Comment 167 Liz Rea 2019-03-21 19:28:04 UTC
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>
Comment 168 Liz Rea 2019-03-21 19:28:11 UTC
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>
Comment 169 Marcel de Rooy 2019-04-26 09:11:45 UTC
QA: Looking here
Comment 170 Marcel de Rooy 2019-04-26 10:20:41 UTC
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>
Comment 171 Marcel de Rooy 2019-04-26 10:20:48 UTC
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>
Comment 172 Marcel de Rooy 2019-04-26 10:20:56 UTC
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>
Comment 173 Marcel de Rooy 2019-04-26 10:21:04 UTC
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>
Comment 174 Marcel de Rooy 2019-04-26 10:21:11 UTC
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>
Comment 175 Marcel de Rooy 2019-04-26 10:21:19 UTC
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>
Comment 176 Nick Clemens 2019-04-29 16:22:35 UTC
Awesome work all!

Pushed to master for 19.05
Comment 177 Martin Renvoize 2019-05-08 14:07:45 UTC
Enhancement will not be backported to 18.11.x series.
Comment 178 Katrin Fischer 2020-01-08 23:37:45 UTC
*** Bug 15857 has been marked as a duplicate of this bug. ***