[Bug 33350] New: Show items info several times in notice content
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33350 Bug ID: 33350 Summary: Show items info several times in notice content Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Notices Assignee: koha-bugs@lists.koha-community.org Reporter: adolfo.rodriguez@xercode.es QA Contact: testopia@bugs.koha-community.org In Notices such as Overdue notice (ODUE) you may want to show more than once the info of the overdue items. <item>"<<biblio.title>>" by <<biblio.author>>, <<items.itemcallnumber>>, Barcode: <<items.barcode>> Fine: <<items.fine>></item> Some clients may want to have the info in several languages in the same notice, instead of having a different notice for every language. Right now, if you put <item></item> more than once, it doesn't work properly, as the function GetPreparedLetter (C4/Letters.pm) does the following. Example <item>"<<biblio.title>>" by <<biblio.author>>, <<items.itemcallnumber>>, Barcode: <<items.barcode>> Fine: <<items.fine>></item> <item>"<<biblio.title>>" de <<biblio.author>>, <<items.itemcallnumber>>, Código de barras: <<items.barcode>> Sanción: <<items.fine>></item> It picks everything from the first <item> to the last </item> and replaces <<items.>> with their values. "<<biblio.title>>" by <<biblio.author>>, <<items.itemcallnumber>>, Barcode: <<items.barcode>> Fine: <<items.fine>></item> <item>"<<biblio.title>>" de <<biblio.author>>, <<items.itemcallnumber>>, Código de barras: <<items.barcode>> Sanción: <<items.fine>> The result is a mess that looks like this: The following item(s) is/are currently overdue: "Sucker punch " by Hamilton, Laurell K., , Barcode: CPL22090005 Fine: $0.00 Los siguientes ejemplares tienen retrasos: "Sucker punch " de Hamilton, Laurell K., , Código de barras: CPL22090005 Multa: $0.00 "Legion of Super-Heroes" by Levitz, Paul, , Barcode: CPL23010002 Fine: $0.00 Los siguientes ejemplares tienen retrasos: "Legion of Super-Heroes" de Levitz, Paul, , Código de barras: CPL23010002 Multa: $0.00 It should pick the content between the first <item> and the first </item>, and then the same with the second pair, the third pair and so on. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33350 Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |adolfo.rodriguez@xercode.es |ity.org | Status|NEW |Needs Signoff --- Comment #1 from Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> --- Created attachment 148796 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=148796&action=edit Allow to show item info several times in notices content This patch allow to show correctly the info of the items more than once in a notice. To test 1 Change you ODUE notice to something like this: Dear <<borrowers.firstname>> <<borrowers.surname>>, According to our current records, you have items that are overdue.Your library does not charge late fines, but please return or renew them at the branch below as soon as possible. <<branches.branchname>> <<branches.branchaddress1>> <<branches.branchaddress2>> <<branches.branchaddress3>> Phone: <<branches.branchphone>> Fax: <<branches.branchfax>> Email: <<branches.branchemail>> If you have registered a password with the library, and you have a renewal available, you may renew online. If an item becomes more than 30 days overdue, you will be unable to use your library card until the item is returned. The following item(s) is/are currently overdue: <item>"<<biblio.title>>" by <<biblio.author>>, <<items.itemcallnumber>>, Barcode: <<items.barcode>> Fine: <<items.fine>></item> Thank-you for your prompt attention to this matter. <<branches.branchname>> Staff Estimado <<borrowers.firstname>> <<borrowers.surname>>, De acuerdo con nuestros registros actuales, tiene artículos vencidos. Su biblioteca no cobra multas por demora, pero devuélvalos o renuévelos en la sucursal a continuación lo antes posible. <<branches.branchname>> <<branches.branchaddress1>> <<branches.branchaddress2>> <<branches.branchaddress3>> Teléfono: <<branches.branchphone>> Fax: <<branches.branchfax>> Email: <<branches.branchemail>> Los siguientes ejemplares tienen retrasos: <item>"<<biblio.title>>" de <<biblio.author>>, <<items.itemcallnumber>>, Código de barras: <<items.barcode>> Sanción: <<items.fine>></item> Gracias por tu atención. <<branches.branchname>> Equipo 2 Make sure you have patrons with overdue notices and that they will get the notice you have modified (email, print...) 3 Run the overdue notices cronjob misc/cronjobs/overdue_notices.pl -v 4 Check that the notice isn't generated as expected. 5 Apply patch, restart services 6 Run again the overdue notices cronjob misc/cronjobs/overdue_notices.pl -v 7 Check that the notice has been properly generated. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33350 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com Status|Needs Signoff |Failed QA --- Comment #2 from Lucas Gass <lucas@bywatersolutions.com> --- Can't we already do this with Template Toolkit? Not sure if adding more complexity to GetPreparedLetter when there is another way to already do this. But if we do add this I think it needs a unit test in this area. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33350 Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #148796|0 |1 is obsolete| | --- Comment #3 from Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> --- Created attachment 201637 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201637&action=edit Fix greedy regex in repeat_enclosing_tags — non-greedy while loop processes each block independently. Includes unit test. The root cause is a greedy .* in GetPreparedLetter's repeat_enclosing_tags block (C4/Letters.pm). When a template has multiple <item> blocks (e.g. bilingual ODUE with one block per language), the greedy match consumes from the first <item> to the last </item>, merging all blocks and swallowing the text between them. Fix: non-greedy .*? in a while loop — each block matched and expanded independently. Tested manually in a local instance against a customized ODUE email template with the pattern English + Spanish <item> blocks. Re Lucas's comment: yes, TT's [% FOREACH item IN items %]...[% END %] can do this, but it uses a different code path in GetPreparedLetter (the loops parameter, not repeat). The <item>...</item> syntax is the legacy/historical notice format and both coexist. Many production templates rely on this syntax. Migrating them all to TT would be a separate, potentially breaking effort. Re the unit test request: included. New subtest in t/db_dependent/Letters.t — "Bug 33350 - repeat_enclosing_tags expands each block independently" — creates a template with two <item> blocks, calls GetPreparedLetter with two pre-resolved item hashrefs, and asserts each block is expanded independently with all items. Greetings, Adolfo. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33350 Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33350 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #4 from Lisette Scheer <lisette@bywatersolutions.com> --- Re:
Yes, TT's [% FOREACH item IN items %]...[% END %] can do this, but it uses a different code path in GetPreparedLetter (the loops parameter, not repeat). The <item>...</item> syntax is the legacy/historical notice format and both coexist. Many production templates rely on this syntax. Migrating them all to TT would be a separate, potentially breaking effort.
NOTICE1: Default Notices should now be writing using template toolkit The notices system has supported the use of template toolkit for some time, all developments that add new notices or update existing notices should now use Template Toolkit. If you'd like to discuss if there's' a reason you think this would be an exception, please add it to the next Development meeting agenda -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33350 Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |In Discussion --- Comment #5 from Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> --- Thanks for the pointer re: notices policy. Before taking this to the dev meeting, I want to lay out why I think this specific case is different from "new development should use TT" — and ask for input on how you'd like to proceed. repeat / <item>...</item> is not a marginal legacy format — it's a documented, currently-supported parameter of GetPreparedLetter (C4/Letters.pm:559-565: "replaces <token>...</token> subtemplate for each @tables row"), and it's actively used by core Koha code today, not just by user-customized templates: * C4/Overdues.pm:738 — repeat => { item => \@item_tables } — this is the call that builds the stock ODUE (overdue) notice shipped with Koha. * C4/Members.pm:481 — same mechanism for member-related notices. * The PDF slip layouts in Koha/pdfformat/layout*.pm. So this isn't "please add more support for an old syntax" — it's a correctness bug in a mechanism the core codebase itself relies on. The actual bug is one line, in repeat_enclosing_tags inside GetPreparedLetter: if ( $letter->{content} =~ m!<$tag>(.*)</$tag>!s ) { That greedy (.*) matches from the first <$tag> to the very last </$tag> in the whole content, so when a template contains the same tag twice (e.g. an English <item> block and a Spanish <item> block in a bilingual notice — a legitimate use of the documented repeat hashref form), both blocks get merged into a single match and the text between them is lost. A template with a single <item> block, which is the overwhelming majority of installs, is unaffected either way. The fix (attachment 201637) only replaces that one match with a non-greedy m!<$tag>(.*?)</$tag>!sg loop that expands each block independently, and ships with a unit test. "Use TT instead" isn't a straightforward migration path for the libraries hitting this: notice templates are edited by library staff through the GUI, not by developers, and TT's [% FOREACH %] relies on the loops parameter, not repeat — so affected libraries would need to rewrite their customized notices from scratch, with no deprecation notice or migration guide for repeat/<item> anywhere in the manual. If the actual intent is to deprecate repeat/<item> entirely going forward, I think that's a bigger decision than a Failed QA on this bug — it would mean deprecating a documented, core-used parameter of GetPreparedLetter, including the Overdues.pm/Members.pm call sites, and probably deserves its own RFC/dev meeting discussion with a proper migration plan, rather than quietly blocking bug fixes to it piecemeal. Happy to bring this to the next dev meeting if that's the right venue — let me know if you still think this specific bug fix shouldn't move forward in the meantime. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org