Bug 8062 - Cart email broken for non english templates
Summary: Cart email broken for non english templates
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: I18N/L10N (show other bugs)
Version: 3.8
Hardware: All All
: P5 - low minor (vote)
Assignee: Marcel de Rooy
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 9189
  Show dependency treegraph
 
Reported: 2012-05-09 11:05 UTC by Katrin Fischer
Modified: 2013-12-05 20:01 UTC (History)
5 users (show)

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


Attachments
Proposed patch (8.12 KB, patch)
2012-05-14 08:04 UTC, Frédéric Demians
Details | Diff | Splinter Review
Proposed patch 2nd version (8.54 KB, patch)
2012-05-14 19:54 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 8062 [SIGNED-OFF] Cart email broken for non english templates (8.61 KB, patch)
2012-06-01 11:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Add Perl module dependency (861 bytes, patch)
2012-06-04 08:09 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 8062 [SIGNED-OFF] Perl module dependency Template::Plugin::HtmlToText (932 bytes, patch)
2012-06-06 08:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Patch for HTML::FormatText (863 bytes, patch)
2012-07-18 14:07 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 Katrin Fischer 2012-05-09 11:05:22 UTC
Sending an email with the contents of your cart is broken in non english templates.

To reproduce:
- Search OPAC
- Add some titles to your cart
- Open cart and try to send you the contents
- log in, if you didn't before
- Send cart to any email address
- Check email text and attachements

The mail sent from the translated templats has subject "no subject" and is empty. The marc files are attached.

The mail sent from the english templates is all correct.

This is an important OPAC feature for some libraries. 

Bug has been confirmed in 3.6.3 with German templates and in 3.6.4 with Arabic templates. Still has to be confirmed against master and 3.8.
Comment 1 Owen Leonard 2012-05-09 13:59:56 UTC
Confirmed in master using fresh nb-NO templates.
Comment 2 Katrin Fischer 2012-05-13 21:13:13 UTC
Ok, I compared the English with the translated German templates. The translated file is missing all line breaks after line 7 <MESSAGE> and is broken because of that. I am not sure how to fix it as it seems to be a problem with the translation scripts not preserving the line breaks correctly.
Comment 3 Katrin Fischer 2012-05-13 21:16:10 UTC
Ok, the error is starting even before line 7. Still investigating.
Comment 4 Katrin Fischer 2012-05-14 06:04:21 UTC
After giving it some thought I think the main problem here is that our translation scripts are not made to parse a plain text file, but TT and HTML. I also tried adding the line breaks back in Poedit, but they get taken out again on saving the file. We can also not use \n or similar, because that will also cause problems (bug 4883)

Without the line breaks the formatting is broken and the regex searching for the markers (<SUBJECT><END_SUBJECT> etc) are not working.

I am a bit out of ideas right now on how to fix that.

This is a big problem for every translated Koha installation. It's not only affecting the OPAC cart, but also staff cart and sending lists from both staff and cart.
Comment 5 Frédéric Demians 2012-05-14 06:12:32 UTC
I confirm the bug, its severity, and your diagnostic. There should be
already get an issue before 3.8. Email sent by Koha comes from a
template which isn't strictly a template. There is a pseudo-syntax
allowing subject/message extraction. But this mini-templating system
isn't compliant with our current translation system which doesn't
preserve new lines: it doesn't matter in HTML; it's important in raw
text, which is used in basket email.

We must rewrite in HTML the email sent by OPAC Send your Basket
functionality.
Comment 6 Frédéric Demians 2012-05-14 08:04:09 UTC Comment hidden (obsolete)
Comment 7 Katrin Fischer 2012-05-14 12:07:15 UTC
Hi Frederic, 
thank you for providing a patch! I have tried to find out when the feature broke and tracked it back to the switch to TT. I am not sure how it could go unnoticed for so long.

I am a bit worried about switching to HTML, because normally Koha is sending text mails. Is there a way we could make it work as text mail still?

We could perhaps trick the translation tools by using <br/> and a paragraph in the notice template, but remove those (HTML::Scrubber?) before sending the mail out.
Comment 8 Frédéric Demians 2012-05-14 19:18:22 UTC
> I am a bit worried about switching to HTML, because normally Koha is
> sending text mails.

Take a look a the resulting mail. It's not that bad. 

> Is there a way we could make it work as text mail still?

We could fix the translator, and get it preserving new line characters.
But it's not that easy.

> We could perhaps trick the translation tools by using <br/> and a
> paragraph in the notice template, but remove those (HTML::Scrubber?)
> before sending the mail out.

I fear that chunk of HTML finish in the .po file, and so disturb
translators. We could also transform the HTML into Text just before
sending the email.
Comment 9 Frédéric Demians 2012-05-14 19:54:15 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2012-06-01 11:47:05 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2012-06-01 11:48:30 UTC
It works fine :)

Perharps a followup is required to specified the new dependency (in C4/Installer/PerlDependencies.pm ?)
Comment 12 Frédéric Demians 2012-06-01 15:44:25 UTC
> Perharps a followup is required to specified the new dependency (in
> C4/Installer/PerlDependencies.pm ?)

The template contains a 'USE HtmlToText' directive, which use a
Template::Toolkit module. It's not a Perl module, and it's included by
default with Template::Toolkit. Is it the dependency we were thinking
about?
Comment 13 Jonathan Druart 2012-06-04 07:57:17 UTC
To test your patch, I had to install the Template::Plugin::HtmlToText module via cpan.
Comment 14 Frédéric Demians 2012-06-04 08:09:48 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2012-06-06 08:04:07 UTC Comment hidden (obsolete)
Comment 16 Paul Poulain 2012-06-09 17:34:20 UTC
QA comment:
 * small patch, that uses a T::T plugin to fix an annoying problem
 * introduces a new dependency that could be usefull in other circumstances, so it's OK
 * remove some commented/dead code ++
 * opac/opac-sendbasket.pl still passes perlcritic

passed QA
Comment 17 Paul Poulain 2012-06-09 17:34:43 UTC
critical problem in non-english languages, worth porting it to 3.8
Comment 18 Chris Cormack 2012-06-10 10:05:06 UTC
pushed to 3.8.x will be in 3.8.2
Comment 19 Jared Camins-Esakov 2012-06-14 12:15:13 UTC
The patch for this major bug will require a few things for inclusion in 3.6.x:
1. A new patch that does not conflict on koha-tmpl/opac-tmpl/prog/en/modules/opac-sendbasket.tt
2. A follow-up patch that adds instructions for installing the new Perl module via CPAN and/or the debian.koha-community.org apt repository
3. A packaged version of libtemplate-plugin-htmltotext-perl for the oldstable distribution on debian.koha-community.org
4. An explanation of what the new dependency is required for so that I can put it in the release notes.
Comment 20 Frédéric Demians 2012-06-14 12:37:16 UTC
I have no time now for your points 1-3.

> 4. An explanation of what the new dependency is required for so that I
> can put it in the release notes.

With Template::Plugin::HtmlToText, it's possible to transform HTML into
Text! as suggested by the module name. It allows Koha developers to use
standard (for Koha) Template::Toolkit templating system to generate HTML,
and then, if necessary to transform this HTML into raw text. This is
required for example for email sent by Koha, which use to be in Text.
Benefits: --email templates can have a rich HTML-base layout, but still
being sent in Text ; --email templates are translatable like Koha web
pages
Comment 21 Robin Sheat 2012-06-26 13:19:12 UTC
I think this patch introduces a bug:

+use HTML::FormatText;

this isn't mentioned in the dependency list, and doesn't appear to be used in the code. Thus, opac-sendbasket.pl breaks unless you happen to have that module installed. Can someone confirm that this is the case?
Comment 22 Robin Sheat 2012-06-26 13:51:55 UTC
OK, it is a dependency of Template::Plugin::HtmlToText, however that doesn't declare it as a dependency in the META.yml, so the packages don't either, and so it never gets installed. Guess I'll have to redo the packages some time soon to account for that.
Comment 23 Marcel de Rooy 2012-06-28 07:34:47 UTC
(In reply to comment #21)
> I think this patch introduces a bug:
> 
> +use HTML::FormatText;
> 
> this isn't mentioned in the dependency list, and doesn't appear to be used
> in the code. Thus, opac-sendbasket.pl breaks unless you happen to have that
> module installed. Can someone confirm that this is the case?

Confirmed: Can't locate HTML/FormatText.pm in @INC

Paul: When you passed this through QA, you mentioned the new dependency. But nothing was done in a followup for dependency list, etc. ?
Comment 24 Marcel de Rooy 2012-07-18 14:03:21 UTC
I submitted a patch for the dependency under 7368.
But Robin is right that the code in opac-basket does not directly use FormatText. So the use statement can be deleted.
But note that the Template::Plugin::HtmlToText requires HTML::FormatText.
So it is a core Koha dependency.
Comment 25 Marcel de Rooy 2012-07-18 14:07:25 UTC
Created attachment 10969 [details] [review]
Patch for HTML::FormatText

Deletes use statement only in opac-sendbasket.pl
Template::Plugin::HtmlToText does require it on its own.
Comment 26 Jonathan Druart 2012-07-23 12:19:29 UTC
QA Comments:

Bug 8062 introduced this new dependency in opac-sendbasket. But it did not call a routine of this module. 
As no new call have been added since, it's useless.

Marking as Passed QA.
Comment 27 Paul Poulain 2012-07-24 15:31:52 UTC
Follow-up pushed
(yikes : I wanted to remove the self-signoff & put mine, but pushed to quickly...)
Comment 28 Chris Cormack 2012-07-25 09:18:42 UTC
Follow up pushed to 3.8.x, will be in 3.8.4