[Bug 28514] New: C4::Letters::getletter must be rewritten
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 Bug ID: 28514 Summary: C4::Letters::getletter must be rewritten Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: ASSIGNED Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: jonathan.druart+koha@gmail.com Reporter: jonathan.druart+koha@gmail.com QA Contact: testopia@bugs.koha-community.org Depends on: 28487 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. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28487 [Bug 28487] Overdue_notices does not fall back to default language -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 --- Comment #1 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 121628 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121628&action=edit 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) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl, | |martin.renvoize@ptfs-europe | |.com, | |nick@bywatersolutions.com Blocks| |15449 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15449 [Bug 15449] Move stuff to the Koha namespace -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |RM_priority -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #121628|0 |1 is obsolete| | --- Comment #2 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 121727 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121727&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #3 from 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |BLOCKED --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- QA: Looking here -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 --- Comment #5 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 --- Comment #6 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- + #$params = {%$params}; -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 --- Comment #7 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 --- Comment #8 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 --- Comment #9 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 --- Comment #10 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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; -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> ---
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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 --- Comment #12 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- =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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> ---
Make sense to me, we will get "NULL" in DB instead of an empty string (or we move the test during the INSERT).
Agreed -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=28581 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 --- Comment #15 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Please move the discussion about inbound email to the other report. Bug 28581 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 --- Comment #16 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|BLOCKED |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #121727|0 |1 is obsolete| | --- Comment #17 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 122055 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122055&action=edit 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} -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #122055|0 |1 is obsolete| | --- Comment #18 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 122224 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122224&action=edit 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) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #122224|0 |1 is obsolete| | --- Comment #19 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 122225 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122225&action=edit 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) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |21.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 --- Comment #20 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Pushed to master for 21.11, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 --- Comment #21 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|RM_priority | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 --- Comment #22 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 122277 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122277&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 --- Comment #23 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Follow-up pushed to master. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com Resolution|--- |FIXED Status|Pushed to master |RESOLVED --- Comment #24 from Kyle M Hall <kyle@bywatersolutions.com> --- Enhancement not backported to 21.05.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|C4::Letters::getletter must |Replace |be rewritten |C4::Letters::getletter with | |Koha::Notice::Templates->fi | |nd_effective_template -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |This patch simplifies and release notes| |clarifies the process of | |getting a notice template | |for a notice. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |30667 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30667 [Bug 30667] Holds reminder cronjob (holds_reminder.pl) never uses default letter template -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28514 Bug 28514 depends on bug 28487, which changed state. Bug 28487 Summary: Overdue_notices does not fall back to default language https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28487 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to oldoldoldstable |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org