Summary: | Replace C4::Letters::getletter with Koha::Notice::Templates->find_effective_template | ||
---|---|---|---|
Product: | Koha | Reporter: | Jonathan Druart <jonathan.druart> |
Component: | Architecture, internals, and plumbing | Assignee: | Jonathan Druart <jonathan.druart> |
Status: | CLOSED FIXED | QA Contact: | Marcel de Rooy <m.de.rooy> |
Severity: | enhancement | ||
Priority: | P5 - low | CC: | dcook, kyle, m.de.rooy, martin.renvoize, nick |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: | https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28581 | ||
Change sponsored?: | --- | Patch complexity: | Small patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: |
This patch simplifies and clarifies the process of getting a notice template for a notice.
|
Version(s) released in: |
21.11.00
|
Circulation function: | |||
Bug Depends on: | 28487 | ||
Bug Blocks: | 15449, 30667 | ||
Attachments: |
Bug 28514: Remove getletter
Bug 28514: Remove getletter Bug 28514: Remove getletter Bug 28514: Remove getletter Bug 28514: Remove getletter Bug 28514: Set lang to default is not passed |
Description
Jonathan Druart
2021-06-04 10:26:54 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) 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> 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 QA: Looking here 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 + #$params = {%$params}; (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. 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. (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. 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; > 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?
(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). =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. > Make sense to me, we will get "NULL" in DB instead of an empty string (or we
> move the test during the INSERT).
Agreed
Please move the discussion about inbound email to the other report. Bug 28581 (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. 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} 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) 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) Pushed to master for 21.11, thanks to everybody involved! 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. 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. Follow-up pushed to master. Enhancement not backported to 21.05.x |