Description
Liz Rea
2012-04-23 21:50:09 UTC
*** 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. Created attachment 21976 [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. I'm not comfortable enough to specify the best way to generate emails, I personnally had to hack it in my environment. Feedbacks welcomed. É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. Created attachment 22315 [details] [review] Bug 8000 - Override emails of every message sent from Koha Following Jonathan Druart's comments, I fixed every single sendmail occurence: C4/Letters.pm (fixed SendAlerts() in addition to the previous standard method) basket/sendbasket.pl misc/cronjobs/runreport.pl opac/opac-sendbasket.pl opac/opac-sendshelf.pl virtualshelves/sendshelf.pl Rebased on latest master 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. Created attachment 24784 [details] [review] Bug 8000 - Override emails of every message sent from Koha Note: rebased to 3.15 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. I'm not comfortable enough to specify the best way to generate emails, I personnally had to hack it in my environment. Feedbacks welcomed. Comment on attachment 22315 [details] [review] Bug 8000 - Override emails of every message sent from Koha Changes are in here that somehow got lost. Created attachment 25917 [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. Created attachment 25919 [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. Created attachment 25920 [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. I'm working on an improvement to this. Created attachment 25923 [details] [review] Bug 8000 - Correct inability to know where message was sent The added column in the previous patch is used to determine where the message was actually sent when using email. This include a modification to the C4/Letters.pm libraries to appropriate track and update the column and revising the test file to check that the system preference actually does override or not the destination. Created attachment 25924 [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. Created attachment 25925 [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. Created attachment 25926 [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 was generated by running ./misc/devel/update_dbix_class_files.pl using appropriate -db_name=... -db_user=... -db_passwd=... parameters. This file is necessary for 3.16 and higher. Created attachment 25927 [details] [review] Bug 8000 - Correct inability to know where message was sent The added column in the previous patch is used to determine where the message was actually sent when using email. This include a modification to the C4/Letters.pm libraries to appropriate track and update the column and revising the test file to check that the system preference actually does override or not the destination. Created attachment 25928 [details] [review] Bug 8000 - Correct inability to know where message was sent The added column in the previous patch is used to determine where the message was actually sent when using email. This include a modification to the C4/Letters.pm libraries to appropriate track and update the column and revising the test file to check that the system preference actually does override or not the destination. (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. ;) Created attachment 25939 [details] [review] Bug 8000 [QA Followup] - Change mis-used qw to q 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. Created attachment 25950 [details] [review] Bug 8000 - Override emails of every message sent from Koha This patch adds Data::Validate::Email to validate the "allEmailsTo" address before using it as a unique destination. Added to PerlDependencies.pm for this and future potential usages. Created attachment 25951 [details] [review] Bug 8000 - Validate allEmailsTo email address before using it This patch adds Data::Validate::Email to validate the "allEmailsTo" address before using it as a unique destination. Added to PerlDependencies.pm for this and future potential usages. 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. Created attachment 26446 [details] [review] Bug 8000: Replace Data::Validate::Email with Email::Valid 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 Created attachment 37463 [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. Created attachment 37464 [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. Created attachment 37465 [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 was generated by running ./misc/devel/update_dbix_class_files.pl using appropriate -db_name=... -db_user=... -db_passwd=... parameters. This file is necessary for 3.16 and higher. Created attachment 37466 [details] [review] Bug 8000 - Correct inability to know where message was sent The added column in the previous patch is used to determine where the message was actually sent when using email. This include a modification to the C4/Letters.pm libraries to appropriate track and update the column and revising the test file to check that the system preference actually does override or not the destination. Created attachment 37467 [details] [review] Bug 8000 [QA Followup] - Change mis-used qw to q Created attachment 37468 [details] [review] Bug 8000 - Validate allEmailsTo email address before using it This patch adds Data::Validate::Email to validate the "allEmailsTo" address before using it as a unique destination. Added to PerlDependencies.pm for this and future potential usages. Created attachment 37469 [details] [review] Bug 8000: Replace Data::Validate::Email with Email::Valid Created attachment 37472 [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. Created attachment 37473 [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. Created attachment 37474 [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 was generated by running ./misc/devel/update_dbix_class_files.pl using appropriate -db_name=... -db_user=... -db_passwd=... parameters. This file is necessary for 3.16 and higher. Created attachment 37475 [details] [review] Bug 8000 - Correct inability to know where message was sent The added column in the previous patch is used to determine where the message was actually sent when using email. This include a modification to the C4/Letters.pm libraries to appropriate track and update the column and revising the test file to check that the system preference actually does override or not the destination. Created attachment 37476 [details] [review] Bug 8000 [QA Followup] - Change mis-used qw to q Created attachment 37477 [details] [review] Bug 8000 - Validate allEmailsTo email address before using it This patch adds Data::Validate::Email to validate the "allEmailsTo" address before using it as a unique destination. Added to PerlDependencies.pm for this and future potential usages. Created attachment 37478 [details] [review] Bug 8000: Replace Data::Validate::Email with Email::Valid Created attachment 37481 [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. Created attachment 37482 [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. Created attachment 37483 [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 was generated by running ./misc/devel/update_dbix_class_files.pl using appropriate -db_name=... -db_user=... -db_passwd=... parameters. This file is necessary for 3.16 and higher. Created attachment 37484 [details] [review] Bug 8000 - Correct inability to know where message was sent The added column in the previous patch is used to determine where the message was actually sent when using email. This include a modification to the C4/Letters.pm libraries to appropriate track and update the column and revising the test file to check that the system preference actually does override or not the destination. Created attachment 37485 [details] [review] Bug 8000 [QA Followup] - Change mis-used qw to q Created attachment 37486 [details] [review] Bug 8000 - Validate allEmailsTo email address before using it This patch adds Data::Validate::Email to validate the "allEmailsTo" address before using it as a unique destination. Added to PerlDependencies.pm for this and future potential usages. Created attachment 37487 [details] [review] Bug 8000: Replace Data::Validate::Email with Email::Valid 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. Created attachment 39985 [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 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. Created attachment 39986 [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. Created attachment 39987 [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 was generated by running ./misc/devel/update_dbix_class_files.pl using appropriate -db_name=... -db_user=... -db_passwd=... parameters. This file is necessary for 3.16 and higher. Created attachment 39988 [details] [review] Bug 8000 - Correct inability to know where message was sent The added column in the previous patch is used to determine where the message was actually sent when using email. This include a modification to the C4/Letters.pm libraries to appropriate track and update the column and revising the test file to check that the system preference actually does override or not the destination. Created attachment 39989 [details] [review] Bug 8000 - Validate allEmailsTo email address before using it This patch adds Data::Validate::Email to validate the "allEmailsTo" address before using it as a unique destination. Added to PerlDependencies.pm for this and future potential usages. Created attachment 39990 [details] [review] Bug 8000: Replace Data::Validate::Email with Email::Valid Created attachment 39991 [details] [review] Bug 8000 - Updated the remaining email validation to Email::Valid . Replaced the "=~ /@/" validation with Email::Valid->address(); Created attachment 39992 [details] [review] Bug 8000 - Atomic Update 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 Created attachment 40215 [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. Created attachment 40216 [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. Created attachment 40217 [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 was generated by running ./misc/devel/update_dbix_class_files.pl using appropriate -db_name=... -db_user=... -db_passwd=... parameters. This file is necessary for 3.16 and higher. Created attachment 40218 [details] [review] Bug 8000 - Correct inability to know where message was sent The added column in the previous patch is used to determine where the message was actually sent when using email. This include a modification to the C4/Letters.pm libraries to appropriate track and update the column and revising the test file to check that the system preference actually does override or not the destination. Created attachment 40219 [details] [review] Bug 8000 - Validate allEmailsTo email address before using it This patch adds Data::Validate::Email to validate the "allEmailsTo" address before using it as a unique destination. Added to PerlDependencies.pm for this and future potential usages. Created attachment 40220 [details] [review] Bug 8000: Replace Data::Validate::Email with Email::Valid Created attachment 40221 [details] [review] Bug 8000 - Updated the remaining email validation to Email::Valid . Replaced the "=~ @" validation with Email::Valid->address(); Created attachment 40222 [details] [review] Bug 8000 - Atomic Update Oops... found a mistake in my rebase... Let's try again. :) Created attachment 40223 [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. Created attachment 40224 [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. Created attachment 40225 [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 was generated by running ./misc/devel/update_dbix_class_files.pl using appropriate -db_name=... -db_user=... -db_passwd=... parameters. This file is necessary for 3.16 and higher. Created attachment 40226 [details] [review] Bug 8000 - Correct inability to know where message was sent The added column in the previous patch is used to determine where the message was actually sent when using email. This include a modification to the C4/Letters.pm libraries to appropriate track and update the column and revising the test file to check that the system preference actually does override or not the destination. Created attachment 40227 [details] [review] Bug 8000 - Validate allEmailsTo email address before using it This patch adds Data::Validate::Email to validate the "allEmailsTo" address before using it as a unique destination. Added to PerlDependencies.pm for this and future potential usages. Created attachment 40228 [details] [review] Bug 8000: Replace Data::Validate::Email with Email::Valid Created attachment 40229 [details] [review] Bug 8000 - Updated the remaining email validation to Email::Valid . Replaced the "=~ @" validation with Email::Valid->address(); Created attachment 40230 [details] [review] Bug 8000 - Atomic Update 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 Created attachment 46354 [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. Created attachment 46355 [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. Created attachment 46356 [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. Created attachment 46357 [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 was generated by running ./misc/devel/update_dbix_class_files.pl using appropriate -db_name=... -db_user=... -db_passwd=... parameters. This file is necessary for 3.16 and higher. Created attachment 46358 [details] [review] Bug 8000 - Correct inability to know where message was sent The added column in the previous patch is used to determine where the message was actually sent when using email. This include a modification to the C4/Letters.pm libraries to appropriate track and update the column and revising the test file to check that the system preference actually does override or not the destination. Created attachment 46359 [details] [review] Bug 8000 - Validate allEmailsTo email address before using it This patch adds Data::Validate::Email to validate the "allEmailsTo" address before using it as a unique destination. Added to PerlDependencies.pm for this and future potential usages. Created attachment 46360 [details] [review] Bug 8000: Replace Data::Validate::Email with Email::Valid Created attachment 46361 [details] [review] Bug 8000 - Updated the remaining email validation to Email::Valid . Replaced the "=~ @" validation with Email::Valid->address(); Created attachment 46362 [details] [review] Bug 8000 - Atomic Update 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 Created attachment 49991 [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. Created attachment 49992 [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. Created attachment 49993 [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. 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. Created attachment 55940 [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. 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. Wanted to test, got merge conflicts. 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? Created attachment 67155 [details] [review] Bug 8000 - Redirect all emails to SendAllEmailsTo Koha/Email.pm Test plan: 1) Apply path 2) Clear all SendAllEmailsTo system preference 3) Send mail to a patron of your choosing, email will go to patron's email address as usual. 4) Set SendAllEmailsTo to a test email address 5) Send mail to the patron, email will be redirected to the email set in the systempreference. 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!) Forgot to mention : Step 1.5) Run updatedatabase.pl Step 7) Run prove t/db_dependent/Letters.t Created attachment 70086 [details] [review] Bug 8000 - Redirect all emails to SendAllEmailsTo Koha/Email.pm Test plan: 1) Apply path 2) Clear all SendAllEmailsTo system preference 3) Send mail to a patron of your choosing, email will go to patron's email address as usual. 4) Set SendAllEmailsTo to a test email address 5) Send mail to the patron, email will be redirected to the email set in the systempreference. 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!) 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 Created attachment 70087 [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!) 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 74110 [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> 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. *** |