See also bug 28514
Good catch, I agree the use in queue_notice is incorrect. I'll submit a patch to fix that and try to improve the POD of inbound_email_address to further clarify it. Perhaps a 'from_email_address' method might help clarify things too?
Koha::Patron seems to be the only place where we use the inbound_email routine as a from address.
Created attachment 122056 [details] [review] Bug 28581: Add 'from_email_address' method and use in Koha::Patron This patch adds a new 'from_email_address' method to Koha::Library to return the appropriate email address to use as the 'from' field for email notices from the library. We then update Koha::Patron->queue_notice to use this new method instead of the incorrect inbound_email_address. I also update the POD for inbound_email_address to clarify it's use case.
Created attachment 122057 [details] [review] Bug 28581: Add 'from_email_address' method and use in Koha::Patron This patch adds a new 'from_email_address' method to Koha::Library to return the appropriate email address to use as the 'from' field for email notices from the library. We then update Koha::Patron->queue_notice to use this new method instead of the incorrect inbound_email_address. I also update the POD for inbound_email_address to clarify it's use case. Test plan
Sorry, still working here too..
Created attachment 122061 [details] [review] Bug 28581: Add 'from_email_address' method and use in Koha::Patron This patch adds a new 'from_email_address' method to Koha::Library to return the appropriate email address to use as the 'from' field for email notices from the library. We then update Koha::Patron->queue_notice to use this new method instead of the incorrect inbound_email_address. I also update the POD for inbound_email_address to clarify it's use case. Test plan Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 122062 [details] [review] Bug 28581: Do not send mail without a from address Test plan: Make branch mail empty and KohaAdminEmailAddress. Trigger a message that goes thru the message queue. Check the log for a warning. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 122063 [details] [review] Bug 28581: Use from_email_address in the codebase Adding only a few trivial cases now. Test plan: Run password recovery or membership expiry cron. Check sender address. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 122067 [details] [review] Bug 28581: Add 'from_email_address' method and use in Koha::Patron This patch adds a new 'from_email_address' method to Koha::Library to return the appropriate email address to use as the 'from' field for email notices from the library. We then update Koha::Patron->queue_notice to use this new method instead of the incorrect inbound_email_address. I also update the POD for inbound_email_address to clarify it's use case. Test plan Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 122068 [details] [review] Bug 28581: Use from_email_address in the codebase Adding only a few (trivial) cases now. Changes in C4::Letters are not trivial after all.. We now add the KohaAdminEmail fallback implicitly when the from address was still empty. The extra check makes us not rely on a do or die action in Email::Stuffer. Test plan: Run password recovery or membership expiry cron. Check sender address. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 122071 [details] [review] Bug 28581: (QA follow-up) Prevent code failure This patch prevents a fatal error when both $params->{from} and $params->{borrowernumber} are undefined. We fallback to KohaAdminEmailAddress before finally falling through to setting a failure status for the message if that last fallback is not found.
Created attachment 122072 [details] [review] Bug 28581: (QA follow-up) Prevent code failure This patch prevents a fatal error when both $params->{from} and $params->{borrowernumber} are undefined. We fallback to KohaAdminEmailAddress before finally falling through to setting a failure status for the message if that last fallback is not found. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 122073 [details] [review] Bug 28581: Add 'from_email_address' method and use in Koha::Patron This patch adds a new 'from_email_address' method to Koha::Library to return the appropriate email address to use as the 'from' field for email notices from the library. We then update Koha::Patron->queue_notice to use this new method instead of the incorrect inbound_email_address. I also update the POD for inbound_email_address to clarify it's use case. Test plan Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 122074 [details] [review] Bug 28581: Use from_email_address in the codebase Adding only a few (trivial) cases now. Changes in C4::Letters are not trivial after all.. We now add the KohaAdminEmail fallback implicitly when the from address was still empty. The extra check makes us not rely on a do or die action in Email::Stuffer. Test plan: Run password recovery or membership expiry cron. Check sender address. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122075 [details] [review] Bug 28581: (QA follow-up) Prevent code failure This patch prevents a fatal error when both $params->{from} and $params->{borrowernumber} are undefined. We fallback to KohaAdminEmailAddress before finally falling through to setting a failure status for the message if that last fallback is not found. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 122076 [details] [review] Bug 28581: Use 'from_email_address' where appropriate This patch replaces a few more trivial cases where we were using library->branchemail with a fallback to KohaAdminEmail to just use the new library->from_email_address method directly instead. There were also a couple of cases where we were passing borrowernumber and the patrons library from address too.. this is unneccesary as the code in _send_email_massage will already default to patron library from address if we do not pass an override.
Created attachment 122078 [details] [review] Bug 28581: Enhance POD for EnqueueLetter
I think between Marcel and I we've signed off each others patches and worked through it together to catch all the edge cases.. Setting to signed off and asking Tomas and Nick to take a look for QA as they've both recently been involved in notice related code close to this.
[15:51] <marcelr> ashimema: misc/cronjobs/notice_unprocessed_suggestions.pl [15:52] <marcelr> do you intently leave from now empty ?? [15:52] == magnuse [~magnuse@89-162-88-217.fiber.signal.no] has joined #koha [15:52] == mode/#koha [+o magnuse] by ChanServ [15:52] <marcelr> + my $admin_email_address = $library->from_email_address [15:52] <marcelr> no comma at the end [15:52] <marcelr> in overdue [15:53] <marcelr> same question for suggestion as for unprocessed sugg [15:53] <marcelr> from is empty now
Created attachment 122080 [details] [review] Bug 28581: Use 'from_email_address' where appropriate This patch replaces a few more trivial cases where we were using library->branchemail with a fallback to KohaAdminEmail to just use the new library->from_email_address method directly instead. There were also a couple of cases where we were passing borrowernumber and the patrons library from address too.. this is unneccesary as the code in _send_email_massage will already default to patron library from address if we do not pass an override. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Added a semicolon.
Created attachment 122081 [details] [review] Bug 28581: Enhance POD for EnqueueLetter Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #19) > [15:51] <marcelr> ashimema: misc/cronjobs/notice_unprocessed_suggestions.pl > [15:52] <marcelr> do you intently leave from now empty ?? > [15:52] == magnuse [~magnuse@89-162-88-217.fiber.signal.no] has joined #koha > [15:52] == mode/#koha [+o magnuse] by ChanServ > [15:52] <marcelr> + my $admin_email_address = $library->from_email_address > [15:52] <marcelr> no comma at the end > [15:52] <marcelr> in overdue > [15:53] <marcelr> same question for suggestion as for unprocessed sugg > [15:53] <marcelr> from is empty now Semicolon fixed. Two empties are intended; will be filled when sending..
Created attachment 122136 [details] [review] Bug 28581: Add 'from_email_address' method and use in Koha::Patron This patch adds a new 'from_email_address' method to Koha::Library to return the appropriate email address to use as the 'from' field for email notices from the library. We then update Koha::Patron->queue_notice to use this new method instead of the incorrect inbound_email_address. I also update the POD for inbound_email_address to clarify it's use case. Test plan Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 122137 [details] [review] Bug 28581: Use from_email_address in the codebase Adding only a few (trivial) cases now. Changes in C4::Letters are not trivial after all.. We now add the KohaAdminEmail fallback implicitly when the from address was still empty. The extra check makes us not rely on a do or die action in Email::Stuffer. Test plan: Run password recovery or membership expiry cron. Check sender address. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 122138 [details] [review] Bug 28581: (QA follow-up) Prevent code failure This patch prevents a fatal error when both $params->{from} and $params->{borrowernumber} are undefined. We fallback to KohaAdminEmailAddress before finally falling through to setting a failure status for the message if that last fallback is not found. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 122139 [details] [review] Bug 28581: Use 'from_email_address' where appropriate This patch replaces a few more trivial cases where we were using library->branchemail with a fallback to KohaAdminEmail to just use the new library->from_email_address method directly instead. There were also a couple of cases where we were passing borrowernumber and the patrons library from address too.. this is unneccesary as the code in _send_email_massage will already default to patron library from address if we do not pass an override. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Added a semicolon. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 122140 [details] [review] Bug 28581: Enhance POD for EnqueueLetter Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment on attachment 122139 [details] [review] Bug 28581: Use 'from_email_address' where appropriate Review of attachment 122139 [details] [review]: ----------------------------------------------------------------- ::: C4/Reserves.pm @@ +1849,4 @@ > > my $library = Koha::Libraries->find( $hold->branchcode )->unblessed; > > + my $admin_email_address = $library->from_email_address; Hum, $library is a hashref here. # Failed test 'Set waiting flag' # at /kohadevbox/koha/t/db_dependent/Circulation.t line 3475. Can't call method "from_email_address" on unblessed reference at /kohadevbox/koha/C4/Reserves.pm line 1852.
Created attachment 122212 [details] [review] Bug 28581: (QA follow-up) Fix method on unblessed reference Speaks for itself. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Test still fails on problem with Renewal but waiting flag passes.
# Subtest: AddRenewal and AddIssuingCharge tests 1..13 not ok 1 - Exception is thrown when renewal update to issues fails # Failed test 'Exception is thrown when renewal update to issues fails' # at t/db_dependent/Circulation.t line 3912. # expecting: Koha::Exceptions::Checkout::FailedRenewal # found: normal exit ok 2 - renew log not added because of the syspref RenewalLog ok 3 - AddRenewal should set the renewal date with the time part not ok 4 # Failed test at t/db_dependent/Circulation.t line 3935. # got: '3' # expected: '2' ok 5 - The issue of item with issuing charge generates an accountline of the correct type ok 6 - AddIssuingCharge correctly sets branchcode ok 7 - AddIssue does not set a hardcoded description for the accountline ok 8 - The renewal of item with issuing charge generates an accountline of the correct type ok 9 - AddRenewal correctly sets branchcode ok 10 - AddRenewal does not set a hardcoded description for the accountline ok 11 - renew log successfully added ok 12 - renew statistic successfully added with passed branch not ok 13 - Passing skipfinecalc causes fine calculation on renewal to be skipped # Failed test 'Passing skipfinecalc causes fine calculation on renewal to be skipped' # at t/db_dependent/Circulation.t line 3972. # got: '6' # expected: '5' # Looks like you failed 3 tests of 13. not ok 42 - AddRenewal and AddIssuingCharge tests # Failed test 'AddRenewal and AddIssuingCharge tests' # at t/db_dependent/Circulation.t line 3974.
Pushed to master for 21.11, thanks to everybody involved!
(In reply to Marcel de Rooy from comment #30) > Test still fails on problem with Renewal but waiting flag passes. Not pushing then. I don't recreate however. How did you get it failing?
(In reply to Jonathan Druart from comment #33) > (In reply to Marcel de Rooy from comment #30) > > Test still fails on problem with Renewal but waiting flag passes. > > Not pushing then. I don't recreate however. How did you get it failing? It fails with me without this patch set too. Not sure. Bad data?
(In reply to Marcel de Rooy from comment #34) > (In reply to Jonathan Druart from comment #33) > > (In reply to Marcel de Rooy from comment #30) > > > Test still fails on problem with Renewal but waiting flag passes. > > > > Not pushing then. I don't recreate however. How did you get it failing? > > It fails with me without this patch set too. Not sure. Bad data? Yes I think so, maybe a pref did not have the default value.
Pushed to 21.05.x for 21.05.02
Can we tell how long this was broken and if the change is required in 20.11?
(In reply to Katrin Fischer from comment #38) > Can we tell how long this was broken and if the change is required in 20.11? queue_notice was introduced with bug 15986 which only appeared in 21.05.. so this shouldn't need porting to 20.11
Depends on Bug 15986 not in 20.11.x