It should be possible to use different Reply To and From email addresses for sending out notices.
Created attachment 30717 [details] [review] Bug 9530: Adding sysprefs and making changes to the branch table To allow for separate Reply To and From email addresses for notices This patch will make no functional changes, it just sets up the db changes needed. To Test: Apply patch, upgrade DB, make sure everything still works fine (Or wait to test with following patches)
Created attachment 30718 [details] [review] Bug 9530: Adding a new Koha::Email module To test 1/ Apply patch 2/ run t/Koha_Email.t No changes to behaviour have been implemented yet
Created attachment 30719 [details] [review] Bug 9350: Making changes so that you can add the new fields to branches To test 1/ Apply patch 2/ Add or Edit a branch, filling out the new fields 3/ Check that the changes saved
Created attachment 30720 [details] [review] Bug 9530: Making changes to C4::Letters So notices using it (circulation, holds etc) will now use the new behaviour To test: 1/ Edit the new systempreferences (ReplytoDefault and ReturnpathDefault) 2/ Optionally edit the branch the mail will be sent from, adding email addresses 3/ Test some mails, a circulation alert, an acquisitions claim, or a newly created borrower alert 4/ Check that the mails have the correct From, Replyto and ReturnPath set The rules are If the values are set in the branch use that, else use the syspref
Created attachment 30721 [details] [review] Bug 9530 making changes to basket/sendbasket.pl To Test 1/ Edit the new systempreferences (ReplytoDefault and ReturnpathDefault) 2/ Optionally edit the branch the mail will be sent from, adding email addresses 3/ Test some mails, test sending a cart email from the staff client 4/ Check that the mails have the correct From, Replyto and ReturnPath set The rules are If the values are set in the branch use that, else use the syspref
Created attachment 30722 [details] [review] Bug 9530 Making changes to misc/cronjobs/runreport.pl Note: mail from this doesnt work in current master, so you may not be able to test this fully To Test 1/ Edit the new systempreferences (ReplytoDefault and ReturnpathDefault) 2/ Optionally edit the branch the mail will be sent from, adding email addresses 3/ Test sending a mail from scheduled reports, note you will need to have the fix for 12031 applied 4/ Check that the mails have the correct From, Replyto and ReturnPath set The rules are If the values are set in the branch use that, else use the syspref
Created attachment 30723 [details] [review] Bug 9530 making changes to opac/opac-sendbasket.pl To Test 1/ Edit the new systempreferences (ReplytoDefault and ReturnpathDefault) 2/ Optionally edit the branch the mail will be sent from, adding email addresses 3/ Send a cart from the public site (OPAC) 4/ Check that the mails have the correct From, Replyto and ReturnPath set The rules are If the values are set in the branch use that, else use the syspref
Created attachment 30724 [details] [review] Bug 9530 making changes to opac/opac-sendshelf.pl To Test 1/ Edit the new systempreferences (ReplytoDefault and ReturnpathDefault) 2/ Optionally edit the branch the mail will be sent from, adding email addresses 3/ Send a list from the public (OPAC) interface 4/ Check that the mails have the correct From, Replyto and ReturnPath set The rules are If the values are set in the branch use that, else use the syspref
Created attachment 30725 [details] [review] Bug 9530 making changes to virtualshelves/sendshelf.pl To Test 1/ Edit the new systempreferences (ReplytoDefault and ReturnpathDefault) 2/ Optionally edit the branch the mail will be sent from, adding email addresses 3/ Send a list from the staff client 4/ Check that the mails have the correct From, Replyto and ReturnPath set The rules are If the values are set in the branch use that, else use the syspref
Created attachment 30740 [details] [review] [SIGNED-OFF] Bug 9530: Adding sysprefs and making changes to the branch table To allow for separate Reply To and From email addresses for notices This patch will make no functional changes, it just sets up the db changes needed. To Test: Apply patch, upgrade DB, make sure everything still works fine (Or wait to test with following patches) Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 30741 [details] [review] [SIGNED-OFF] Bug 9530: Adding a new Koha::Email module To test 1/ Apply patch 2/ run t/Koha_Email.t No changes to behaviour have been implemented yet Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 30742 [details] [review] [SIGNED-OFF] Bug 9350: Making changes so that you can add the new fields to branches To test 1/ Apply patch 2/ Add or Edit a branch, filling out the new fields 3/ Check that the changes saved http://bugs.koha-community.org/show_bug.cgi?id=9530 Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 30743 [details] [review] [SIGNED-OFF] Bug 9530: Making changes to C4::Letters So notices using it (circulation, holds etc) will now use the new behaviour To test: 1/ Edit the new systempreferences (ReplytoDefault and ReturnpathDefault) 2/ Optionally edit the branch the mail will be sent from, adding email addresses 3/ Test some mails, a circulation alert, an acquisitions claim, or a newly created borrower alert 4/ Check that the mails have the correct From, Replyto and ReturnPath set The rules are If the values are set in the branch use that, else use the syspref Signed-off-by: Owen Leonard <oleonard@myacpl.org>
The patches I have signed off on I was able to test successfully. The patches for sending Carts and Lists did not appear to be working as expected. > 4/ Check that the mails have the correct From, Replyto and ReturnPath set > The rules are > If the values are set in the branch use that, else use the syspref All messages were being sent with the default replyto/returnpath address whether or not I was logged in as a patron from a library which had a library-specific value in the branches table. In other words, in cases where I expected to see the values in branches.branchreplyto and branches.branchreturnpath used they were not. Or is that not the expected behavior in these scripts?
Ah yes sorry, I didn't change the existing behaviour for those. They were always sent from the syspref before, so I didnt change them to use the branch. I just added the replyto and returnpath, so for ones that used branch info before, now they use from, replyto and returnpath from the branch if it was set. Hope this makes sense
Created attachment 30760 [details] [review] [SIGNED-OFF] Bug 9530 making changes to basket/sendbasket.pl To Test 1/ Edit the new systempreferences (ReplytoDefault and ReturnpathDefault) 2/ Optionally edit the branch the mail will be sent from, adding email addresses 3/ Test some mails, test sending a cart email from the staff client 4/ Check that the mails have the correct From, Replyto and ReturnPath set The rules are If the values are set in the branch use that, else use the syspref Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 30761 [details] [review] [SIGNED-OFF] Bug 9530 making changes to opac/opac-sendbasket.pl To Test 1/ Edit the new systempreferences (ReplytoDefault and ReturnpathDefault) 2/ Optionally edit the branch the mail will be sent from, adding email addresses 3/ Send a cart from the public site (OPAC) 4/ Check that the mails have the correct From, Replyto and ReturnPath set The rules are If the values are set in the branch use that, else use the syspref Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 30762 [details] [review] [SIGNED-OFF] Bug 9530 making changes to opac/opac-sendshelf.pl To Test 1/ Edit the new systempreferences (ReplytoDefault and ReturnpathDefault) 2/ Optionally edit the branch the mail will be sent from, adding email addresses 3/ Send a list from the public (OPAC) interface 4/ Check that the mails have the correct From, Replyto and ReturnPath set The rules are If the values are set in the branch use that, else use the syspref Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 30763 [details] [review] [SIGNED-OFF] Bug 9530 making changes to virtualshelves/sendshelf.pl To Test 1/ Edit the new systempreferences (ReplytoDefault and ReturnpathDefault) 2/ Optionally edit the branch the mail will be sent from, adding email addresses 3/ Send a list from the staff client 4/ Check that the mails have the correct From, Replyto and ReturnPath set The rules are If the values are set in the branch use that, else use the syspref Signed-off-by: Owen Leonard <oleonard@myacpl.org>
(In reply to Chris Cormack from comment #15) > Ah yes sorry, I didn't change the existing behaviour for those. Thanks for the clarification. That confirms that the behavior I saw in testing was correct. The last problem for me is that I did not see the correct behavior from misc/cronjobs/runreport.pl. I tested it along with the fix for Bug 12031 on a package install. The scheduled report ran and the email was sent correctly, but I didn't see the ReplyTo and ReturnPath headers as defined in my system preferences. I'm going to leave this as "needs signoff" in the hopes that someone else can confirm or deny.
I can reproduce the problem Owen reported with the runreport.pl emails not using the preferences for return-path and reply-to. Using the task scheduler: 1) Applied all patches from this bug with the runreport one being the last. 2) Applied all patches from 12031. 3) Removed www-data user from /etc/at.deny 4) Scheduled a report with only KohaAdminEmailAddress = admin@localhost 5) Email was received correctly. 6) Scheduled another report 6) Added email addresses for ReplytoDefault = adminreply@localhost ReturnpathDefault = adminreturn@localhost 7) The email is sent, the content is correct. BUT the new prefs are not taken into account. Run from command line: 1) ./runreport.pl --email reportID 1 2) adminreply and adminreturn appear in the headers correctly. BUT: Email content is Koha::Email=HASH(0x4d749b0) or similar. Something else: The encoding for the emails is given as: charset="iso-8850-1" - but should probably be utf-8 like for the other emails?
> Something else: The encoding for the emails is given as: > charset="iso-8850-1" - but should probably be utf-8 like for the other > emails? I'll fix the others, but this one might be better on it's one bug. Chris
(In reply to Chris Cormack from comment #22) > > Something else: The encoding for the emails is given as: > > charset="iso-8850-1" - but should probably be utf-8 like for the other > > emails? > > I'll fix the others, but this one might be better on it's one bug. > > Chris Actually Koha::Email sets it to utf8 unless you explicitly tell it not to. So once we fix the issues that the script doesnt seem to be using Koha::Email then we will have fixed that too
Created attachment 31169 [details] [review] Bug 9530 Making changes to misc/cronjobs/runreport.pl Note: mail from this doesnt work in current master, so you may not be able to test this fully To Test 1/ Edit the new systempreferences (ReplytoDefault and ReturnpathDefault) 2/ Optionally edit the branch the mail will be sent from, adding email addresses 3/ Test sending a mail from scheduled reports, note you will need to have the fix for 12031 applied 4/ Check that the mails have the correct From, Replyto and ReturnPath set The rules are If the values are set in the branch use that, else use the syspref
Owen could you test again, please? The latest patch works nicely for me, after I figured out a problem with the git setup: I had to change the patch in scheduler.pl to point to my git install version of runreport.pl as it was looking in /usr/share... like for a package install. As that file also exists on my system, that caused some confusion :) Not sure about the paths, but that seems out of scope here.
Created attachment 31196 [details] [review] [SIGNED-OFF] Bug 9530 Making changes to misc/cronjobs/runreport.pl Note: mail from this doesnt work in current master, so you may not be able to test this fully To Test 1/ Edit the new systempreferences (ReplytoDefault and ReturnpathDefault) 2/ Optionally edit the branch the mail will be sent from, adding email addresses 3/ Test sending a mail from scheduled reports, note you will need to have the fix for 12031 applied 4/ Check that the mails have the correct From, Replyto and ReturnPath set The rules are If the values are set in the branch use that, else use the syspref Signed-off-by: Owen Leonard <oleonard@myacpl.org>
(In reply to Katrin Fischer from comment #25) > I had to change the patch in scheduler.pl to point to my git > install version of runreport.pl as it was looking in /usr/share... like for > a package install. Me too, using a gitified package installation. I assume this is unrelated to the normal functionality of the script.
Created attachment 31926 [details] [review] [PASSED QA] Bug 9530: Adding sysprefs and making changes to the branch table To allow for separate Reply To and From email addresses for notices This patch will make no functional changes, it just sets up the db changes needed. To Test: Apply patch, upgrade DB, make sure everything still works fine (Or wait to test with following patches) Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 31927 [details] [review] [PASSED QA] Bug 9530: Adding a new Koha::Email module To test 1/ Apply patch 2/ run t/Koha_Email.t No changes to behaviour have been implemented yet Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 31928 [details] [review] [PASSED QA] Bug 9350: Making changes so that you can add the new fields to branches To test 1/ Apply patch 2/ Add or Edit a branch, filling out the new fields 3/ Check that the changes saved http://bugs.koha-community.org/show_bug.cgi?id=9530 Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 31929 [details] [review] [PASSED QA] Bug 9530: Making changes to C4::Letters So notices using it (circulation, holds etc) will now use the new behaviour To test: 1/ Edit the new systempreferences (ReplytoDefault and ReturnpathDefault) 2/ Optionally edit the branch the mail will be sent from, adding email addresses 3/ Test some mails, a circulation alert, an acquisitions claim, or a newly created borrower alert 4/ Check that the mails have the correct From, Replyto and ReturnPath set The rules are If the values are set in the branch use that, else use the syspref Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 31930 [details] [review] [PASSED QA] Bug 9530 making changes to basket/sendbasket.pl To Test 1/ Edit the new systempreferences (ReplytoDefault and ReturnpathDefault) 2/ Optionally edit the branch the mail will be sent from, adding email addresses 3/ Test some mails, test sending a cart email from the staff client 4/ Check that the mails have the correct From, Replyto and ReturnPath set The rules are If the values are set in the branch use that, else use the syspref Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 31931 [details] [review] [PASSED QA] Bug 9530 making changes to opac/opac-sendbasket.pl To Test 1/ Edit the new systempreferences (ReplytoDefault and ReturnpathDefault) 2/ Optionally edit the branch the mail will be sent from, adding email addresses 3/ Send a cart from the public site (OPAC) 4/ Check that the mails have the correct From, Replyto and ReturnPath set The rules are If the values are set in the branch use that, else use the syspref Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 31932 [details] [review] [PASSED QA] Bug 9530 making changes to opac/opac-sendshelf.pl To Test 1/ Edit the new systempreferences (ReplytoDefault and ReturnpathDefault) 2/ Optionally edit the branch the mail will be sent from, adding email addresses 3/ Send a list from the public (OPAC) interface 4/ Check that the mails have the correct From, Replyto and ReturnPath set The rules are If the values are set in the branch use that, else use the syspref Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 31933 [details] [review] [PASSED QA] Bug 9530 making changes to virtualshelves/sendshelf.pl To Test 1/ Edit the new systempreferences (ReplytoDefault and ReturnpathDefault) 2/ Optionally edit the branch the mail will be sent from, adding email addresses 3/ Send a list from the staff client 4/ Check that the mails have the correct From, Replyto and ReturnPath set The rules are If the values are set in the branch use that, else use the syspref Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 31934 [details] [review] [PASSED QA] Bug 9530 Making changes to misc/cronjobs/runreport.pl Note: mail from this doesnt work in current master, so you may not be able to test this fully To Test 1/ Edit the new systempreferences (ReplytoDefault and ReturnpathDefault) 2/ Optionally edit the branch the mail will be sent from, adding email addresses 3/ Test sending a mail from scheduled reports, note you will need to have the fix for 12031 applied 4/ Check that the mails have the correct From, Replyto and ReturnPath set The rules are If the values are set in the branch use that, else use the syspref Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 31935 [details] [review] [QA Followup] Bug 9530: Replace tabs with spaces Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
With one minor whitespace issue which I've fixed in a tiny followup patch, I'm now happy to pass QA on this. It works as expected for me and I can't find any regressions caused. The code all passes the QA scripts and the tests all still pass. This will be a great addition.
This wiki snippet I did may be useful to anyone documenting this feature. I'll be happy to explain further upon request http://wiki.koha-community.org/wiki/MRenvoize/Email
Patches pushed to master. Thanks Chris!
Created attachment 32799 [details] [review] Bug 9530: Follow-up - Fix t/db_dependent/Branch.t Tests need to learn about the new fields in the branches table. To test: prove t/db_dependent/Branch.t
Followup pushed to master. Thanks Katrina!
Comment on attachment 31929 [details] [review] [PASSED QA] Bug 9530: Making changes to C4::Letters Review of attachment 31929 [details] [review]: ----------------------------------------------------------------- ::: C4/Letters.pm @@ +286,5 @@ > + my $message = Koha::Email->new(); > + my %mail = $message->create_message_headers( > + { > + to => $email, > + from => $branchdetails->{'branchemail'}, Was there a reason the || C4::Context->preference("KohaAdminEmailAddress") portion was removed? I'm trying to rescue bug 8000.
> ::: C4/Letters.pm > @@ +286,5 @@ > > + my $message = Koha::Email->new(); > > + my %mail = $message->create_message_headers( > > + { > > + to => $email, > > + from => $branchdetails->{'branchemail'}, > > Was there a reason the > || C4::Context->preference("KohaAdminEmailAddress") > portion was removed? > I'm trying to rescue bug 8000. Please take a look at Koha::Email: 45 $params->{from} ||= C4::Context->preference('KohaAdminEmailAddress');