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"
A library client brought this to my attention yesterday. I'll have a patch up in just a few minutes :).
Created attachment 34655 [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.
Created attachment 34656 [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.
Created attachment 34657 [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
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.
(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...
(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.
(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).
(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?
Created attachment 36174 [details] [review] [SIGNED OFF] Bug 12739 - SendAlerts function does not take care of "html" format or UTF-8 Pt 1 Thanks, David. I've made some more tests. I've applied tha patch Pt.1 against 3.19.00.012 head 13473. I saw "utf-8" as required. So I attach the "SIGNED OFF" version of the patch Pt.1. NB1. > The UTF-8 charset [....], which causes UTF-8 characters to display incorrectly. Using the mail client Thunderbird, there were no differences in displaying UTF-8 chars of the received mail. In particular, viewing the received mail (resulting by "Opening the message" on the incoming-mail list) all the UTF-8 chars [in subject and body] of the mail were OK, either on master or on applied.
Created attachment 36175 [details] [review] [SIGNED OFF] Bug 12739 - SendAlerts function does not take care of "html" format Pt. 2 (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 has been trivially rebased. Everything was OK just as required by the test plan. So the patch Pt.2 can be signed off. I pass all the 2 patches Pt.1 and Pt.2 to "Signed OFF" status.
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.
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... :/
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>
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>
(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
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 :)
BTW Running the Letters.t test also includes a *test* of claimacquisition.
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.
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>
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?
(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.
Patches pushed to master. Thanks David!
Pushed to 3.18.x will be in 3.18.6