Bug 5544

Summary: All Notices from Branch Email Address
Product: Koha Reporter: Nicole C. Engard <nengard>
Component: NoticesAssignee: Kyle M Hall <kyle.m.hall>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: chris, gmcharlt, jonathan.druart, katrin.fischer, kyle.m.hall, kyle, liz, m.de.rooy, savitra.sirohi
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8063
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 2533, 5929    
Bug Blocks:    
Attachments: Bug 5544 - All Notices from Branch Email Address
Bug 5544 - All Notices from Branch Email Address
Bug 5544 - All Notices from Branch Email Address
Bug 5544 - All Notices from Branch Email Address
Bug 5544 - All Notices from Branch Email Address
Bug 5544 - All Notices from Branch Email Address - QA Followup
Bug 5544 - All Notices from Branch Email Address
Bug 5544 - All Notices from Branch Email Address - QA Followup

Description Nicole C. Engard 2010-12-27 18:11:42 UTC
I think all notices should come from the branch email address. Right now overdues from from the branch, but the others come from the admin email address - this is a problem in multi-branch systems because they have to come up with one email address that all branches have access to.
Comment 1 Kyle M Hall 2012-05-09 16:49:51 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2012-06-12 13:14:25 UTC
QA Comment:

I think you can rewrite

+    my $branch = GetBranchDetail( $member->{'branchcode'} );
+        From => $message->{'from_address'} || $branch->{'branchemail'} || C4::Context->preference('KohaAdminEmailAddress'),

With:
    From => $message->{'from_address'} || GetBranchDetail( $member->{'branchcode'} )->{'branchemail'} || C4::Context->preference('KohaAdminEmailAddress'),

In order to bypassed an useless call (GetBranchDetail) in some cases ($message->{from_address} not defined).
Comment 3 Owen Leonard 2012-08-07 14:08:56 UTC
I tested this and it worked as expected when processing item checkout notices. I'm not sure how to test case 1) Address specified in message. Where does that arise?

Does comment 2 imply that this has failed QA?
Comment 4 Jonathan Druart 2012-08-07 14:25:38 UTC
(In reply to comment #3)

> Does comment 2 imply that this has failed QA?

No. It is a code optimization. It does not impact testings.
Comment 5 Kyle M Hall 2012-09-12 12:09:29 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2012-09-12 12:10:35 UTC
This new patch implements the suggestions made by Jonathan Druart in Comment 2.
Comment 7 Kyle M Hall 2012-09-12 12:16:18 UTC
(In reply to comment #3)
> I tested this and it worked as expected when processing item checkout
> notices. I'm not sure how to test case 1) Address specified in message.
> Where does that arise?

I checked the code, and as far as I can tell, there is no code in Koha the currently uses case 1.

_send_message_by_email is only called by SendQueuedMessages, which is only used in misc/cronjobs/process_message_queue.pl

I'm guessing that it is vestigial. We could remove but, but it does provide some flexibility that may be useful in the future.
Comment 8 Jonathan Druart 2012-12-13 12:47:28 UTC
Typo:
"From => From => "
Comment 9 Kyle M Hall 2013-01-28 14:08:36 UTC Comment hidden (obsolete)
Comment 10 Liz Rea 2013-03-20 01:46:46 UTC
No test plan, failed for now.
Comment 11 Kyle M Hall 2013-05-08 17:15:51 UTC Comment hidden (obsolete)
Comment 12 Chris Cormack 2013-07-06 09:16:22 UTC
This works as advertised, however, it may introduce a warning in the case where $member is not defined.
Comment 13 Chris Cormack 2013-07-06 09:17:41 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2013-07-07 10:26:33 UTC
(In reply to Chris Cormack from comment #12)
> This works as advertised, however, it may introduce a warning in the case
> where $member is not defined.

Bit worried about this comment - we tend to forget that we have a message that goes to the library and therefore has no borrowernumber in message_queue - overdues digest for patrons without email address.
Comment 15 Jonathan Druart 2013-07-18 09:16:54 UTC
(In reply to Katrin Fischer from comment #14)
> (In reply to Chris Cormack from comment #12)
> > This works as advertised, however, it may introduce a warning in the case
> > where $member is not defined.
> 
> Bit worried about this comment - we tend to forget that we have a message
> that goes to the library and therefore has no borrowernumber in
> message_queue - overdues digest for patrons without email address.

I agree with that, the undefined borrowernumber case should be take into account.

Marked as Failed QA.
Comment 16 Kyle M Hall 2013-09-05 16:46:26 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2013-09-23 10:18:38 UTC
Created attachment 21352 [details] [review]
Bug 5544 - All Notices from Branch Email Address

Right now overdues come from the branch, but the
others come from the admin email address - this
is a problem in multi-branch systems because they
have to come up with one email address that all
branches have access to.

C4::Letters::_send_message_by_email currently sets
the from address in the following order:
1) Address specified in message
2) Koha admin email address

The order will now be:
1) Address specified in message
2) Borrowers home library email address
3) Koha admin email address

Test Plan:
1) Set your branch email addresses, and the KohaAdminEmailAddress
   Make sure each of them are unqiue
2) Choose a borrower, enable the enhanced messaging and enable the
   checkout and checkin email notices. Use your email address for
   the borrower's email so you can recieve the emails.
3) Check out an item, check the from address of the email,
   it should be the email addres set in KohaAdminEmailAddress
4) Apply the patch
5) Return the item, check the from address of the email,
   it should match the email address set for the borrowers
   home library.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2013-09-23 10:18:43 UTC
Created attachment 21353 [details] [review]
Bug 5544 - All Notices from Branch Email Address - QA Followup

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2013-09-23 10:20:01 UTC
Looks good to me. Works as advertised. No complaints from qa tools.

Passed QA
Comment 20 Galen Charlton 2013-10-04 16:09:58 UTC
Pushed to master.  Thanks, Kyle!