Bug 12739 - SendAlerts function does not take care of "html" format or UTF-8
Summary: SendAlerts function does not take care of "html" format or UTF-8
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: David Cook
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-08-08 13:57 UTC by Sophie MEYNIEUX
Modified: 2015-12-03 22:02 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 12739 - SendAlerts function does not take care of "html" format or UTF-8 Pt 1 (2.53 KB, patch)
2014-12-23 03:17 UTC, David Cook
Details | Diff | Splinter Review
Bug 12739 - SendAlerts function does not take care of "html" format or UTF-8 Pt 1 (2.53 KB, patch)
2014-12-23 03:18 UTC, David Cook
Details | Diff | Splinter Review
Bug 12739 - SendAlerts function does not take care of "html" format or UTF-8 Pt. 2 (3.45 KB, patch)
2014-12-23 03:18 UTC, David Cook
Details | Diff | Splinter Review
[SIGNED OFF] Bug 12739 - SendAlerts function does not take care of "html" format or UTF-8 Pt 1 (2.66 KB, patch)
2015-02-26 14:01 UTC, Paola Rossi
Details | Diff | Splinter Review
[SIGNED OFF] Bug 12739 - SendAlerts function does not take care of "html" format Pt. 2 (3.57 KB, patch)
2015-02-26 14:12 UTC, Paola Rossi
Details | Diff | Splinter Review
Bug 12739 - SendAlerts function does not take care of "html" format or UTF-8 Pt 1 (2.68 KB, patch)
2015-03-19 16:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12739 - SendAlerts function does not take care of "html" format or UTF-8 Pt. 2 (3.59 KB, patch)
2015-03-19 16:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12739: (QA followup) use 'UTF-8' instead of 'utf8' (5.09 KB, patch)
2015-03-19 18:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Sophie MEYNIEUX 2014-08-08 13:57:53 UTC
If you want to add some html tags in your notification letters (except overdues or holds), for exemple a link on you FB page or some hilghtning, it is not taken into account.

Tags are written as plain text in the email even if html is activated. 

That's because C4:Letters::SendAlerts function does not check letter format but always sends email with Content-Type = "text/plain"
Comment 1 David Cook 2014-12-23 02:28:18 UTC
A library client brought this to my attention yesterday. I'll have a patch up in just a few minutes :).
Comment 2 David Cook 2014-12-23 03:17:36 UTC Comment hidden (obsolete)
Comment 3 David Cook 2014-12-23 03:18:15 UTC Comment hidden (obsolete)
Comment 4 David Cook 2014-12-23 03:18:20 UTC Comment hidden (obsolete)
Comment 5 David Cook 2014-12-23 03:19:16 UTC
Note to testers:

These patches can be tested one by one or all at once, since Pt. 2 carries forward the change from Pt. 1.
Comment 6 Brendan Gallagher 2015-02-25 05:38:17 UTC
(In reply to David Cook from comment #5)
> Note to testers:
> 
> These patches can be tested one by one or all at once, since Pt. 2 carries
> forward the change from Pt. 1.

Testing with  ֓ ֔ ֕ ֖ ֗ ֘ ֙ ֚ ֛ ֜ ֝    Hebrew...
Comment 7 Paola Rossi 2015-02-25 16:43:33 UTC
(In reply to David Cook from comment #4)
> Created attachment 34657 [details] [review] [review]
> Bug 12739 - SendAlerts function does not take care of "html" format or UTF-8
> Pt. 2
> 
> The emails sent via SendAlerts don't take into account HTML format.
> 
> _TEST PLAN_
> 
> Before applying:
> 
> 1) Change system preference "AutoEmailOpacUser" to "Send"
> 2) Change "ACCTDETAILS" notice to HTML and add HTML to it
> 3) Create a new user with your email address
> 4) Note how the email displays the HTML tags as plain text
> 
> Apply patch
> 
> 5) Create a new user with your email address
> 6) Note how the email displays the email as an HTML email
> 
> For thoroughness:
> 
> 7) Change "ACCTDETAILS" notice to non-HTML
> 8) Create a new user with your email address
> 9) Note how the email displays the HTML as plain text

I've applied the patch Pt.2 against master 3.19.00.012 head 13473.

The patch needed a trivial rebase.

Everything was OK just as required by the test plan.

After the rebase, the patch Pt.2 about "html/plain" option will be able to be signed off.
Comment 8 Paola Rossi 2015-02-25 17:01:23 UTC
(In reply to David Cook from comment #3)
> Created attachment 34656 [details] [review] [review]
> Bug 12739 - SendAlerts function does not take care of "html" format or UTF-8
> Pt 1
> 
> The UTF-8 charset in the content type is written as "utf8" instead of "utf-8"
> in SendAlerts(), which causes UTF-8 characters to display incorrectly.
> 
> _TEST PLAN_
> 
> Before Applying:
> 
> 1) Edit ACCTDETAILS
> 2) Add some UTF-8 characters
> I recommend using the following website
> http://www.ltg.ed.ac.uk/~richard/unicode-sample.html
> 
> In my tests, I added the samples from Hebrew, Arabic, Basic Latin,
> Latin-1 Supplement, Latin Extended-A, and Latin Extended-B.
> 
> 3) Set the system preference "AutoEmailOpacUser" to "Send"
> 4) Create a new user account with your email address
> 
> 5) Note that the email in your inbox doesn't display the Unicode
> characters correctly
> 
> Apply the patch
> 
> 6) Create a new user account with your email address
> 7) Note that the email in your inbox _does_ display the
> Unicode characters correctly.

I've applied the patch Pt.1 against master 3.19.00.012 head 13473.

I wrote some Greek chars in the Notice in:
the Message subject,
the Message body,
in the Surname of the newing borrower/patron.

There was nearby no difference between master and applied.
This was a part of the source of the received mail, with the 1 diff I saw:
-------
Subject: Your new Koha account details CPL- noHTML APPl ΑΒΓΔΕ
Charset: utf8
Date: Wed, 25 Feb 2015 17:53:39 +0100
Mime-Version: 1.0
Content-Type: text/plain; charset="utf-8" (1 diff: "utf8" on master)
To: a.rossi@cineca.it
Content-Transfer-Encoding: quoted-printable
From: JeffSummCPL@gmail.com
Message-Id: <E1YQfDP-00014r-AW@linux-test.prossi-host>
[...]
-----------
I think the patch Pt.1 about UTF-8 didn't fix the problem.

So I pass this Bug 12739 to "Failed QA" Status (see also Comment #7 about Pt.2).
Comment 9 David Cook 2015-02-25 22:38:32 UTC
(In reply to Paola Rossi from comment #8)
> I think the patch Pt.1 about UTF-8 didn't fix the problem.
> 
> So I pass this Bug 12739 to "Failed QA" Status (see also Comment #7 about
> Pt.2).

That's rather bizarre... it definitely worked for me when I wrote the patch.

I'll have to look at this again when I find some time some day.

What email client are you using?
Comment 10 Paola Rossi 2015-02-26 14:01:53 UTC Comment hidden (obsolete)
Comment 11 Paola Rossi 2015-02-26 14:12:07 UTC Comment hidden (obsolete)
Comment 12 Katrin Fischer 2015-03-03 11:27:36 UTC
There is something here I am not so happy with - the test plan tells you test one out of 4 notices that are touched by this patch set:
- notifications about new serial issues
- claims in serials
- claims in acq

I'd love if those could be tested as well. Could a tester take a look at these as well please?

Another thing that I am not sure how to solve: there seems a lot of code repeat in Letters.pm that might not be necessary, but is out of the scope of this bug.
Comment 13 David Cook 2015-03-04 00:42:10 UTC
I'm going to take another look at this bug when I have a chance. Tempted to break this into 2 separate bugs as well... one for the HTML and one for the UTF-8 stuff.

A lot of the code around Notices && Slips could use a good refactoring... :/
Comment 14 Marcel de Rooy 2015-03-19 16:11:31 UTC
Created attachment 37015 [details] [review]
Bug 12739 - SendAlerts function does not take care of "html" format or UTF-8 Pt 1

The UTF-8 charset in the content type is written as "utf8" instead of "utf-8"
in SendAlerts(), which causes UTF-8 characters to display incorrectly.

_TEST PLAN_

Before Applying:

1) Edit ACCTDETAILS
2) Add some UTF-8 characters
I recommend using the following website
http://www.ltg.ed.ac.uk/~richard/unicode-sample.html

In my tests, I added the samples from Hebrew, Arabic, Basic Latin,
Latin-1 Supplement, Latin Extended-A, and Latin Extended-B.

3) Set the system preference "AutoEmailOpacUser" to "Send"
4) Create a new user account with your email address

5) Note that the email in your inbox doesn't display the Unicode
characters correctly

Apply the patch

6) Create a new user account with your email address
7) Note that the email in your inbox _does_ display the
Unicode characters correctly.

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2015-03-19 16:11:37 UTC
Created attachment 37016 [details] [review]
Bug 12739 - SendAlerts function does not take care of "html" format or UTF-8 Pt. 2

The emails sent via SendAlerts don't take into account HTML format.

_TEST PLAN_

Before applying:

1) Change system preference "AutoEmailOpacUser" to "Send"
2) Change "ACCTDETAILS" notice to HTML and add HTML to it
3) Create a new user with your email address
4) Note how the email displays the HTML tags as plain text

Apply patch

5) Create a new user with your email address
6) Note how the email displays the email as an HTML email

For thoroughness:

7) Change "ACCTDETAILS" notice to non-HTML
8) Create a new user with your email address
9) Note how the email displays the HTML as plain text

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2015-03-19 16:16:51 UTC
(In reply to Katrin Fischer from comment #12)
> There is something here I am not so happy with - the test plan tells you
> test one out of 4 notices that are touched by this patch set:
> - notifications about new serial issues
> - claims in serials
> - claims in acq
> 
> I'd love if those could be tested as well. Could a tester take a look at
> these as well please?

I have spent some time on getting that above the water (Dutch expression only?)

With a small test script I managed to generate all four notices in plain text and html. These two patches are fine.
With the sign-off from Paola and the fairly trivial changes, I take the liberty to pass QA on them too.
But I definitely agree with Katrin and David that the code around these changes needs attention :)
I will put some additional comments below.

Passed QA
Comment 17 Marcel de Rooy 2015-03-19 16:26:13 UTC
What I found while testing is:

[1] There is no claimissues notice in the system. (The claimacquisition is created in the installer, the other one is not there.) Bad.
[2] The fact that $type is passed to GetPreparedLetter as module name looks strange. 
[3] The second parameter of SendAlerts is externalid. In some cases this should be an id, in other cases an array reference. Does not look good.
[4] For claimacquisition and claimissues, the From address is userenv->{emailaddress}. This is uncommon.
[5] In the same part of the code, two fetch rows are behind each other without any testing. If there are no records found, this will generate an error.

If I would look a little bit longer, I would find even more :)
Comment 18 Marcel de Rooy 2015-03-19 18:00:07 UTC
BTW Running the Letters.t test also includes a *test* of claimacquisition.
Comment 19 Tomás Cohen Arazi 2015-03-19 18:08:56 UTC
David, why aren't you using encode('UTF-8',$string) as stated in http://wiki.koha-community.org/wiki/Handling_UTF8_in_development

Please take a look at my followup and check it is ok.
Comment 20 Tomás Cohen Arazi 2015-03-19 18:21:56 UTC
Created attachment 37018 [details] [review]
Bug 12739: (QA followup) use 'UTF-8' instead of 'utf8'

This patch makes Encode::encode be passed the 'UTF-8' encoding
instead of the discouraged 'utf8'.

Please read:
http://wiki.koha-community.org/wiki/Handling_UTF8_in_development

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 21 Marcel de Rooy 2015-03-19 19:00:40 UTC
Looks good to me, Tomas. Thanks.

Make another point also to mention to the above list:

[6] In two cases Koha::Email is used with parameter contenttype, in the other case a hash is formed directly with parameter Content-Type. Consistent?
Comment 22 David Cook 2015-03-20 00:39:10 UTC
(In reply to Tomás Cohen Arazi from comment #19)
> David, why aren't you using encode('UTF-8',$string) as stated in
> http://wiki.koha-community.org/wiki/Handling_UTF8_in_development
> 
> Please take a look at my followup and check it is ok.

I didn't eve know that was a thing! Neat! It might be an idea to add the thing about using "utf-8" in emails... or "UTF-8" if that works as well for the content type.

At a glance, your patch looks good to me.
Comment 23 Tomás Cohen Arazi 2015-03-30 16:48:18 UTC
Patches pushed to master.

Thanks David!
Comment 24 Chris Cormack 2015-03-31 05:27:12 UTC
Pushed to 3.18.x will be in 3.18.6