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.
Created attachment 9491 [details] [review] Bug 5544 - All Notices from 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. 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
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).
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?
(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.
Created attachment 12164 [details] [review] Bug 5544 - All Notices from 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. 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
This new patch implements the suggestions made by Jonathan Druart in Comment 2.
(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.
Typo: "From => From => "
Created attachment 14888 [details] [review] Bug 5544 - All Notices from 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. 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
No test plan, failed for now.
Created attachment 18036 [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.
This works as advertised, however, it may introduce a warning in the case where $member is not defined.
Created attachment 19437 [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>
(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.
(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.
Created attachment 20807 [details] [review] Bug 5544 - All Notices from Branch Email Address - QA Followup
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>
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>
Looks good to me. Works as advertised. No complaints from qa tools. Passed QA
Pushed to master. Thanks, Kyle!