Bug 28581 - Patron's queue_notice uses inbound_email_address incorrectly
Summary: Patron's queue_notice uses inbound_email_address incorrectly
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Martin Renvoize
QA Contact: Nick Clemens
URL:
Keywords:
Depends on: 15986
Blocks: 29223
  Show dependency treegraph
 
Reported: 2021-06-17 08:35 UTC by Marcel de Rooy
Modified: 2022-06-06 20:27 UTC (History)
6 users (show)

See Also:
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


Attachments
Bug 28581: Add 'from_email_address' method and use in Koha::Patron (2.65 KB, patch)
2021-06-17 08:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28581: Add 'from_email_address' method and use in Koha::Patron (4.21 KB, patch)
2021-06-17 09:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28581: Add 'from_email_address' method and use in Koha::Patron (4.30 KB, patch)
2021-06-17 12:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28581: Do not send mail without a from address (1.75 KB, patch)
2021-06-17 12:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28581: Use from_email_address in the codebase (3.57 KB, patch)
2021-06-17 12:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28581: Add 'from_email_address' method and use in Koha::Patron (4.30 KB, patch)
2021-06-17 13:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28581: Use from_email_address in the codebase (4.89 KB, patch)
2021-06-17 13:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28581: (QA follow-up) Prevent code failure (1.36 KB, patch)
2021-06-17 13:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28581: (QA follow-up) Prevent code failure (1.46 KB, patch)
2021-06-17 13:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28581: Add 'from_email_address' method and use in Koha::Patron (4.27 KB, patch)
2021-06-17 13:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28581: Use from_email_address in the codebase (4.86 KB, patch)
2021-06-17 13:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28581: (QA follow-up) Prevent code failure (1.42 KB, patch)
2021-06-17 13:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28581: Use 'from_email_address' where appropriate (6.67 KB, patch)
2021-06-17 13:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28581: Enhance POD for EnqueueLetter (1.86 KB, patch)
2021-06-17 13:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28581: Use 'from_email_address' where appropriate (6.79 KB, patch)
2021-06-17 13:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28581: Enhance POD for EnqueueLetter (1.95 KB, patch)
2021-06-17 13:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28581: Add 'from_email_address' method and use in Koha::Patron (4.32 KB, patch)
2021-06-18 13:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28581: Use from_email_address in the codebase (4.91 KB, patch)
2021-06-18 13:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28581: (QA follow-up) Prevent code failure (1.47 KB, patch)
2021-06-18 13:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28581: Use 'from_email_address' where appropriate (6.81 KB, patch)
2021-06-18 13:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28581: Enhance POD for EnqueueLetter (1.97 KB, patch)
2021-06-18 13:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28581: (QA follow-up) Fix method on unblessed reference (965 bytes, patch)
2021-06-21 08:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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