Bug 9530 - Allow separate 'Reply To' and 'From' email address for notices
Summary: Allow separate 'Reply To' and 'From' email address for notices
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Chris Cormack
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 11140
  Show dependency treegraph
 
Reported: 2013-02-01 13:13 UTC by Katrin Fischer
Modified: 2015-12-03 22:11 UTC (History)
4 users (show)

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


Attachments
Bug 9530: Adding sysprefs and making changes to the branch table (5.84 KB, patch)
2014-08-11 22:56 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9530: Adding a new Koha::Email module (3.29 KB, patch)
2014-08-11 22:57 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9350: Making changes so that you can add the new fields to branches (4.83 KB, patch)
2014-08-11 22:57 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9530: Making changes to C4::Letters (6.46 KB, patch)
2014-08-11 22:58 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9530 making changes to basket/sendbasket.pl (1.53 KB, patch)
2014-08-11 22:58 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9530 Making changes to misc/cronjobs/runreport.pl (2.58 KB, patch)
2014-08-11 22:59 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9530 making changes to opac/opac-sendbasket.pl (2.17 KB, patch)
2014-08-11 22:59 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9530 making changes to opac/opac-sendshelf.pl (1.84 KB, patch)
2014-08-11 22:59 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9530 making changes to virtualshelves/sendshelf.pl (4.71 KB, patch)
2014-08-11 23:00 UTC, Chris Cormack
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9530: Adding sysprefs and making changes to the branch table (5.93 KB, patch)
2014-08-12 15:44 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9530: Adding a new Koha::Email module (3.40 KB, patch)
2014-08-12 15:45 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9350: Making changes so that you can add the new fields to branches (4.92 KB, patch)
2014-08-12 15:45 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9530: Making changes to C4::Letters (6.57 KB, patch)
2014-08-12 15:45 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9530 making changes to basket/sendbasket.pl (1.64 KB, patch)
2014-08-13 13:19 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9530 making changes to opac/opac-sendbasket.pl (2.28 KB, patch)
2014-08-13 13:19 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9530 making changes to opac/opac-sendshelf.pl (1.95 KB, patch)
2014-08-13 13:19 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9530 making changes to virtualshelves/sendshelf.pl (4.81 KB, patch)
2014-08-13 13:19 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 9530 Making changes to misc/cronjobs/runreport.pl (2.57 KB, patch)
2014-08-26 22:34 UTC, Chris Cormack
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9530 Making changes to misc/cronjobs/runreport.pl (2.68 KB, patch)
2014-08-27 16:28 UTC, Owen Leonard
Details | Diff | Splinter Review
[PASSED QA] Bug 9530: Adding sysprefs and making changes to the branch table (5.96 KB, patch)
2014-09-30 06:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
[PASSED QA] Bug 9530: Adding a new Koha::Email module (3.42 KB, patch)
2014-09-30 06:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
[PASSED QA] Bug 9350: Making changes so that you can add the new fields to branches (4.96 KB, patch)
2014-09-30 06:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
[PASSED QA] Bug 9530: Making changes to C4::Letters (6.59 KB, patch)
2014-09-30 06:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
[PASSED QA] Bug 9530 making changes to basket/sendbasket.pl (1.66 KB, patch)
2014-09-30 06:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
[PASSED QA] Bug 9530 making changes to opac/opac-sendbasket.pl (2.30 KB, patch)
2014-09-30 06:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
[PASSED QA] Bug 9530 making changes to opac/opac-sendshelf.pl (1.97 KB, patch)
2014-09-30 06:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
[PASSED QA] Bug 9530 making changes to virtualshelves/sendshelf.pl (4.83 KB, patch)
2014-09-30 06:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
[PASSED QA] Bug 9530 Making changes to misc/cronjobs/runreport.pl (2.70 KB, patch)
2014-09-30 06:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
[QA Followup] Bug 9530: Replace tabs with spaces (1.70 KB, patch)
2014-09-30 06:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 9530: Follow-up - Fix t/db_dependent/Branch.t (1.77 KB, patch)
2014-10-27 18:15 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2013-02-01 13:13:19 UTC
It should be possible to use different Reply To and From email addresses for sending out notices.
Comment 1 Chris Cormack 2014-08-11 22:56:30 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2014-08-11 22:57:04 UTC Comment hidden (obsolete)
Comment 3 Chris Cormack 2014-08-11 22:57:48 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2014-08-11 22:58:11 UTC Comment hidden (obsolete)
Comment 5 Chris Cormack 2014-08-11 22:58:51 UTC Comment hidden (obsolete)
Comment 6 Chris Cormack 2014-08-11 22:59:23 UTC Comment hidden (obsolete)
Comment 7 Chris Cormack 2014-08-11 22:59:38 UTC Comment hidden (obsolete)
Comment 8 Chris Cormack 2014-08-11 22:59:54 UTC Comment hidden (obsolete)
Comment 9 Chris Cormack 2014-08-11 23:00:37 UTC Comment hidden (obsolete)
Comment 10 Owen Leonard 2014-08-12 15:44:53 UTC Comment hidden (obsolete)
Comment 11 Owen Leonard 2014-08-12 15:45:00 UTC Comment hidden (obsolete)
Comment 12 Owen Leonard 2014-08-12 15:45:09 UTC Comment hidden (obsolete)
Comment 13 Owen Leonard 2014-08-12 15:45:19 UTC Comment hidden (obsolete)
Comment 14 Owen Leonard 2014-08-12 15:58:29 UTC
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?
Comment 15 Chris Cormack 2014-08-12 22:02:29 UTC
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
Comment 16 Owen Leonard 2014-08-13 13:19:25 UTC Comment hidden (obsolete)
Comment 17 Owen Leonard 2014-08-13 13:19:34 UTC Comment hidden (obsolete)
Comment 18 Owen Leonard 2014-08-13 13:19:42 UTC Comment hidden (obsolete)
Comment 19 Owen Leonard 2014-08-13 13:19:52 UTC Comment hidden (obsolete)
Comment 20 Owen Leonard 2014-08-13 13:24:20 UTC
(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.
Comment 21 Katrin Fischer 2014-08-26 14:04:47 UTC
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?
Comment 22 Chris Cormack 2014-08-26 22:12:30 UTC
> 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
Comment 23 Chris Cormack 2014-08-26 22:14:28 UTC
(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
Comment 24 Chris Cormack 2014-08-26 22:34:06 UTC Comment hidden (obsolete)
Comment 25 Katrin Fischer 2014-08-27 09:18:36 UTC
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.
Comment 26 Owen Leonard 2014-08-27 16:28:16 UTC Comment hidden (obsolete)
Comment 27 Owen Leonard 2014-08-27 16:29:43 UTC
(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.
Comment 28 Martin Renvoize 2014-09-30 06:31:14 UTC
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>
Comment 29 Martin Renvoize 2014-09-30 06:31:23 UTC
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>
Comment 30 Martin Renvoize 2014-09-30 06:31:31 UTC
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>
Comment 31 Martin Renvoize 2014-09-30 06:31:42 UTC
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>
Comment 32 Martin Renvoize 2014-09-30 06:31:51 UTC
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>
Comment 33 Martin Renvoize 2014-09-30 06:32:01 UTC
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>
Comment 34 Martin Renvoize 2014-09-30 06:32:12 UTC
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>
Comment 35 Martin Renvoize 2014-09-30 06:32:21 UTC
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>
Comment 36 Martin Renvoize 2014-09-30 06:32:29 UTC
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>
Comment 37 Martin Renvoize 2014-09-30 06:32:37 UTC
Created attachment 31935 [details] [review]
[QA Followup] Bug 9530: Replace tabs with spaces

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 38 Martin Renvoize 2014-09-30 06:34:03 UTC
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.
Comment 39 Martin Renvoize 2014-10-01 10:08:49 UTC
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
Comment 40 Tomás Cohen Arazi 2014-10-27 14:19:55 UTC
Patches pushed to master.

Thanks Chris!
Comment 41 Katrin Fischer 2014-10-27 18:15:45 UTC
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
Comment 42 Tomás Cohen Arazi 2014-10-28 13:25:54 UTC
Followup pushed to master.

Thanks Katrina!
Comment 43 Mark Tompsett 2015-04-03 16:54:00 UTC
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.
Comment 44 Katrin Fischer 2015-05-15 21:46:48 UTC
> ::: 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');