Bug 28581

Summary: Patron's queue_notice uses inbound_email_address incorrectly
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: NoticesAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Nick Clemens <nick>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, katrin.fischer, kyle, martin.renvoize, nick
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.02
Bug Depends on: 15986    
Bug Blocks: 29223    
Attachments: Bug 28581: Add 'from_email_address' method and use in Koha::Patron
Bug 28581: Add 'from_email_address' method and use in Koha::Patron
Bug 28581: Add 'from_email_address' method and use in Koha::Patron
Bug 28581: Do not send mail without a from address
Bug 28581: Use from_email_address in the codebase
Bug 28581: Add 'from_email_address' method and use in Koha::Patron
Bug 28581: Use from_email_address in the codebase
Bug 28581: (QA follow-up) Prevent code failure
Bug 28581: (QA follow-up) Prevent code failure
Bug 28581: Add 'from_email_address' method and use in Koha::Patron
Bug 28581: Use from_email_address in the codebase
Bug 28581: (QA follow-up) Prevent code failure
Bug 28581: Use 'from_email_address' where appropriate
Bug 28581: Enhance POD for EnqueueLetter
Bug 28581: Use 'from_email_address' where appropriate
Bug 28581: Enhance POD for EnqueueLetter
Bug 28581: Add 'from_email_address' method and use in Koha::Patron
Bug 28581: Use from_email_address in the codebase
Bug 28581: (QA follow-up) Prevent code failure
Bug 28581: Use 'from_email_address' where appropriate
Bug 28581: Enhance POD for EnqueueLetter
Bug 28581: (QA follow-up) Fix method on unblessed reference

Description Marcel de Rooy 2021-06-17 08:35:22 UTC
See also bug 28514
Comment 1 Martin Renvoize 2021-06-17 08:42:13 UTC
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?
Comment 2 Marcel de Rooy 2021-06-17 08:48:44 UTC
Koha::Patron seems to be the only place where we use the inbound_email routine as a from address.
Comment 3 Martin Renvoize 2021-06-17 08:57:51 UTC
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.
Comment 4 Martin Renvoize 2021-06-17 09:13:51 UTC
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
Comment 5 Marcel de Rooy 2021-06-17 09:23:48 UTC
Sorry, still working here too..
Comment 6 Marcel de Rooy 2021-06-17 12:11:58 UTC
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>
Comment 7 Marcel de Rooy 2021-06-17 12:12:02 UTC
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>
Comment 8 Marcel de Rooy 2021-06-17 12:12:06 UTC
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>
Comment 9 Marcel de Rooy 2021-06-17 13:00:06 UTC
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>
Comment 10 Marcel de Rooy 2021-06-17 13:00:10 UTC
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>
Comment 11 Martin Renvoize 2021-06-17 13:14:25 UTC
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.
Comment 12 Marcel de Rooy 2021-06-17 13:26:54 UTC
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>
Comment 13 Martin Renvoize 2021-06-17 13:37:46 UTC
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>
Comment 14 Martin Renvoize 2021-06-17 13:37:50 UTC
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>
Comment 15 Martin Renvoize 2021-06-17 13:37:54 UTC
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>
Comment 16 Martin Renvoize 2021-06-17 13:37:58 UTC
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.
Comment 17 Martin Renvoize 2021-06-17 13:47:37 UTC
Created attachment 122078 [details] [review]
Bug 28581: Enhance POD for EnqueueLetter
Comment 18 Martin Renvoize 2021-06-17 13:48:46 UTC
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.
Comment 19 Marcel de Rooy 2021-06-17 13:55:31 UTC
[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
Comment 20 Marcel de Rooy 2021-06-17 13:58:55 UTC
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.
Comment 21 Marcel de Rooy 2021-06-17 13:58:59 UTC
Created attachment 122081 [details] [review]
Bug 28581: Enhance POD for EnqueueLetter

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2021-06-17 13:59:44 UTC
(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..
Comment 23 Nick Clemens 2021-06-18 13:43:22 UTC
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>
Comment 24 Nick Clemens 2021-06-18 13:43:25 UTC
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>
Comment 25 Nick Clemens 2021-06-18 13:43:28 UTC
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>
Comment 26 Nick Clemens 2021-06-18 13:43:33 UTC
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>
Comment 27 Nick Clemens 2021-06-18 13:43:36 UTC
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 28 Jonathan Druart 2021-06-21 08:40:53 UTC
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.
Comment 29 Marcel de Rooy 2021-06-21 08:51:08 UTC
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>
Comment 30 Marcel de Rooy 2021-06-21 08:52:24 UTC
Test still fails on problem with Renewal but waiting flag passes.
Comment 31 Marcel de Rooy 2021-06-21 08:53:48 UTC
# 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.
Comment 32 Jonathan Druart 2021-06-22 10:14:58 UTC Comment hidden (obsolete)
Comment 33 Jonathan Druart 2021-06-22 12:37:21 UTC
(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?
Comment 34 Marcel de Rooy 2021-06-22 12:43:44 UTC
(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?
Comment 35 Jonathan Druart 2021-06-22 12:53:14 UTC
(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.
Comment 36 Jonathan Druart 2021-06-23 13:06:55 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 37 Kyle M Hall 2021-07-09 13:06:24 UTC
Pushed to 21.05.x for 21.05.02
Comment 38 Katrin Fischer 2021-07-14 12:38:28 UTC
Can we tell how long this was broken and if the change is required in 20.11?
Comment 39 Martin Renvoize 2021-07-14 15:27:12 UTC
(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
Comment 40 Fridolin Somers 2021-08-02 20:25:03 UTC
Depends on Bug 15986 not in 20.11.x