Bug 28514 - C4::Letters::getletter must be rewritten
Summary: C4::Letters::getletter must be rewritten
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 28487
Blocks: 15449
  Show dependency treegraph
 
Reported: 2021-06-04 10:26 UTC by Jonathan Druart
Modified: 2021-07-02 13:42 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
21.11.00


Attachments
Bug 28514: Remove getletter (19.73 KB, patch)
2021-06-04 10:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28514: Remove getletter (19.78 KB, patch)
2021-06-09 08:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28514: Remove getletter (19.96 KB, patch)
2021-06-17 08:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28514: Remove getletter (20.52 KB, patch)
2021-06-21 11:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28514: Remove getletter (20.52 KB, patch)
2021-06-21 11:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28514: Set lang to default is not passed (1.16 KB, patch)
2021-06-22 09:57 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2021-06-04 10:26:54 UTC
The way we handle notice templates is confusing (see bug 27660, bug 26787, bug 28487). We should remove it and use Koha::Notice::Templates instead.
Comment 1 Jonathan Druart 2021-06-04 10:32:02 UTC
Created attachment 121628 [details] [review]
Bug 28514: Remove getletter

The way we handle notice templates is confusing (see bug 27660, bug 26787, bug 28487).

This patch remove C4::Letters::getletter and use either Koha::Notice::Templates->find
or the newly created methods ->find_effective_template that will do
all necessary to return the correct template.

Test plan:
- Create and modify notice templates
- Make sure you have TranslateNotices turned on and that some notices
templates have a translated version
- Use holds_reminder.pl and overdue_notices.pl cronjobs and confirm that
the generated notices are the expected ones
- Test also pos/printreceipt.pl
- And finally test some other notices (CHECKIN, RENEWAL for instance)
Comment 2 Martin Renvoize 2021-06-09 08:29:06 UTC
Created attachment 121727 [details] [review]
Bug 28514: Remove getletter

The way we handle notice templates is confusing (see bug 27660, bug 26787, bug 28487).

This patch remove C4::Letters::getletter and use either Koha::Notice::Templates->find
or the newly created methods ->find_effective_template that will do
all necessary to return the correct template.

Test plan:
- Create and modify notice templates
- Make sure you have TranslateNotices turned on and that some notices
templates have a translated version
- Use holds_reminder.pl and overdue_notices.pl cronjobs and confirm that
the generated notices are the expected ones
- Test also pos/printreceipt.pl
- And finally test some other notices (CHECKIN, RENEWAL for instance)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 3 Martin Renvoize 2021-06-09 08:31:13 UTC
A solid improvement.. it's reminded me I managed to somehow miss updating the pos receipt printing for GetPreparedLetter.. damn!

Also.. the overdues_notices code is rather inefficient I feel.. for a valid letter we lookup the template 3 times before actually using it!

Still.. it all works and we can do further cleaning in new bugs..

Signing off
Comment 4 Marcel de Rooy 2021-06-17 06:48:40 UTC
QA: Looking here
Comment 5 Marcel de Rooy 2021-06-17 06:51:13 UTC
 FAIL   Koha/Notice/Templates.pm
   OK     critic
   OK     forbidden patterns
   OK     git manipulation
   OK     pod
   FAIL   pod coverage
                POD is missing for find_effective_template
                POD is missing for object_class
Comment 6 Marcel de Rooy 2021-06-17 07:04:48 UTC
+    #$params = {%$params};
Comment 7 Jonathan Druart 2021-06-17 08:01:15 UTC
(In reply to Marcel de Rooy from comment #6)
> +    #$params = {%$params};

Hum yes, I remember that. As we modify $params we are modifying the hashref passed (and so unexpected behaviour could happened). I think it should be restored.
Comment 8 Marcel de Rooy 2021-06-17 08:13:40 UTC
Just seeing a mail problem here (not from this patch):

sub inbound_email_address {
    my ($self) = @_;

    return
         $self->branchreplyto
      || $self->branchemail
      || C4::Context->preference('ReplytoDefault')
      || C4::Context->preference('KohaAdminEmailAddress')
      || undef;

ReplyToDefault should imo not be used as the from address.
Note that the preferences advertize:

KohaAdminEmailAddress 	
Email address for the administrator of Koha: (This is the default From: address for emails unless there is one for the particular library, and is referred to when an internal error occurs.)
ReplytoDefault 	
Email address to be set as the replyto in emails: 

Same for branchreplyto. Note that my server may send mails as library.rijksmuseum.nl and wants replies to rijksmuseum.nl but is not allowed to send as rijksmuseum.nl. So this code is bad and triggers spam lists etc.

So actually we should do:
    return
      $self->branchemail
      || C4::Context->preference('KohaAdminEmailAddress');

An || undef makes not much sense?

Will open a new report too.
Comment 9 Martin Renvoize 2021-06-17 08:27:31 UTC
(In reply to Marcel de Rooy from comment #8)
> Just seeing a mail problem here (not from this patch):
> 
> sub inbound_email_address {
>     my ($self) = @_;
> 
>     return
>          $self->branchreplyto
>       || $self->branchemail
>       || C4::Context->preference('ReplytoDefault')
>       || C4::Context->preference('KohaAdminEmailAddress')
>       || undef;
> 
> ReplyToDefault should imo not be used as the from address.
> Note that the preferences advertize:
> 
> KohaAdminEmailAddress 	
> Email address for the administrator of Koha: (This is the default From:
> address for emails unless there is one for the particular library, and is
> referred to when an internal error occurs.)
> ReplytoDefault 	
> Email address to be set as the replyto in emails: 
> 
> Same for branchreplyto. Note that my server may send mails as
> library.rijksmuseum.nl and wants replies to rijksmuseum.nl but is not
> allowed to send as rijksmuseum.nl. So this code is bad and triggers spam
> lists etc.
> 
> So actually we should do:
>     return
>       $self->branchemail
>       || C4::Context->preference('KohaAdminEmailAddress');
> 
> An || undef makes not much sense?
> 
> Will open a new report too.

Please point out where inbound_email_address is being used as the from address.. that is not it's purpose... the `|| undef` does indeed feel a little odd.. but the rest of the fallthrough chain is correct in my opinion.
Comment 10 Marcel de Rooy 2021-06-17 08:30:10 UTC
Koha::Patron
general routine
not nice

sub queue_notice {
    my ( $self, $params ) = @_;
    my $letter_params = $params->{letter_params};
    my $test_mode = $params->{test_mode};

    return unless $letter_params;
    return unless exists $params->{message_name} xor $params->{message_transports}; # We only want one of these

    my $library = Koha::Libraries->find( $letter_params->{branchcode} );
    my $admin_email_address = $library->inbound_email_address;
Comment 11 Marcel de Rooy 2021-06-17 08:31:23 UTC
> Please point out where inbound_email_address is being used as the from
> address.. that is not it's purpose... the `|| undef` does indeed feel a
> little odd.. but the rest of the fallthrough chain is correct in my opinion.

The purpose might be unclear. What is an inbound email address actually?
Comment 12 Jonathan Druart 2021-06-17 08:31:33 UTC
(In reply to Martin Renvoize from comment #9)
> the `|| undef` does indeed feel a little odd..

Make sense to me, we will get "NULL" in DB instead of an empty string (or we move the test during the INSERT).
Comment 13 Marcel de Rooy 2021-06-17 08:32:38 UTC
=head3 inbound_email_address

  my $to_email = Koha::Library->inbound_email_address;

Returns an effective email address which should be accessible to librarians at the branch.
Comment 14 Marcel de Rooy 2021-06-17 08:33:11 UTC
> Make sense to me, we will get "NULL" in DB instead of an empty string (or we
> move the test during the INSERT).

Agreed
Comment 15 Marcel de Rooy 2021-06-17 08:36:14 UTC
Please move the discussion about inbound email to the other report.
Bug 28581
Comment 16 Martin Renvoize 2021-06-17 08:37:56 UTC
(In reply to Marcel de Rooy from comment #10)
> Koha::Patron
> general routine
> not nice
> 
> sub queue_notice {
>     my ( $self, $params ) = @_;
>     my $letter_params = $params->{letter_params};
>     my $test_mode = $params->{test_mode};
> 
>     return unless $letter_params;
>     return unless exists $params->{message_name} xor
> $params->{message_transports}; # We only want one of these
> 
>     my $library = Koha::Libraries->find( $letter_params->{branchcode} );
>     my $admin_email_address = $library->inbound_email_address;

Good catch, that's in incorrect usage I agree.. inbound_email_address was certainly designed to be used as the reply-to and not the from.
Comment 17 Marcel de Rooy 2021-06-17 08:41:33 UTC
Created attachment 122055 [details] [review]
Bug 28514: Remove getletter

The way we handle notice templates is confusing (see bug 27660, bug 26787, bug 28487).

This patch remove C4::Letters::getletter and use either Koha::Notice::Templates->find
or the newly created methods ->find_effective_template that will do
all necessary to return the correct template.

Test plan:
- Create and modify notice templates
- Make sure you have TranslateNotices turned on and that some notices
templates have a translated version
- Use holds_reminder.pl and overdue_notices.pl cronjobs and confirm that
the generated notices are the expected ones
- Test also pos/printreceipt.pl
- And finally test some other notices (CHECKIN, RENEWAL for instance)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Amended by removing comment for $params={%$params}
Comment 18 Jonathan Druart 2021-06-21 11:48:10 UTC
Created attachment 122224 [details] [review]
Bug 28514: Remove getletter

The way we handle notice templates is confusing (see bug 27660, bug 26787, bug 28487).

This patch remove C4::Letters::getletter and use either Koha::Notice::Templates->find
or the newly created methods ->find_effective_template that will do
all necessary to return the correct template.

Test plan:
- Create and modify notice templates
- Make sure you have TranslateNotices turned on and that some notices
templates have a translated version
- Use holds_reminder.pl and overdue_notices.pl cronjobs and confirm that
the generated notices are the expected ones
- Test also pos/printreceipt.pl
- And finally test some other notices (CHECKIN, RENEWAL for instance)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Amended by removing comment for $params={%$params}

JD amended patch:
* Add missing POD
* Fix spelling (dont  ==> don't)
Comment 19 Jonathan Druart 2021-06-21 11:50:19 UTC
Created attachment 122225 [details] [review]
Bug 28514: Remove getletter

The way we handle notice templates is confusing (see bug 27660, bug 26787, bug 28487).

This patch remove C4::Letters::getletter and use either Koha::Notice::Templates->find
or the newly created methods ->find_effective_template that will do
all necessary to return the correct template.

Test plan:
- Create and modify notice templates
- Make sure you have TranslateNotices turned on and that some notices
templates have a translated version
- Use holds_reminder.pl and overdue_notices.pl cronjobs and confirm that
the generated notices are the expected ones
- Test also pos/printreceipt.pl
- And finally test some other notices (CHECKIN, RENEWAL for instance)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Amended by removing comment for $params={%$params}

JD amended patch:
* Add missing POD
* Fix spelling (dont  ==> don't)
Comment 20 Jonathan Druart 2021-06-21 12:04:22 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 21 Jonathan Druart 2021-06-21 12:48:47 UTC
There is a random failure

14:32:04 koha_1       |     #   Failed test 'no lang passed, default is returned'
14:32:04 koha_1       |     #   at t/db_dependent/Koha/Notices.t line 105.
14:32:04 koha_1       |     #          got: 'es-ES'
14:32:04 koha_1       |     #     expected: 'default'
14:32:04 koha_1       |     # Looks like you failed 1 test of 7.
14:32:04 koha_1       | 
14:32:04 koha_1       | #   Failed test 'find_effective_template'
14:32:04 koha_1       | #   at t/db_dependent/Koha/Notices.t line 135.
Comment 22 Jonathan Druart 2021-06-22 09:57:49 UTC
Created attachment 122277 [details] [review]
Bug 28514: Set lang to default is not passed

It fixes the following test:
    #   Failed test 'no lang passed, default is returned'
    #   at t/db_dependent/Koha/Notices.t line 105.
    #          got: 'es-ES'
    #     expected: 'default'
    # Looks like you failed 1 test of 7.
Comment 23 Jonathan Druart 2021-06-22 10:14:59 UTC
Follow-up pushed to master.
Comment 24 Kyle M Hall 2021-07-02 13:42:05 UTC
Enhancement not backported to 21.05.x