|
Description
Christopher Brannon
2022-03-13 00:58:01 UTC
In what way is the preview different than the actual email? (In reply to Owen Leonard from comment #1) > In what way is the preview different than the actual email? The preview shows extra line breaks that the e-mail does not. Maybe the issue is that we need to treat HTML and text notices differently in the preview. For HTML notices we don't want to display the line breaks but only rely on the <br>, but for text notices we do. PCCLD is trying to switch all it's email notices our to template toolkit and I keep running into this issue. +1 to fixing or changing this. Created attachment 158517 [details] [review] Bug 30287: Conditionally render notice preview This patch adds conditional formatting based on whether the notice was generated from an HTML template or a plaintext one. I replace the html_line_break for plaintext formatted notices too as I believe this closer resembles how we actually output such generated notices. Created attachment 158519 [details] [review] Bug 30287: Conditionally render notice preview This patch adds conditional formatting based on whether the notice was generated from an HTML template or a plaintext one. We simply drop the 'html_line_break' filter for the case where the notice should already be html formatted. I was tempted to replace the html_line_break for plaintext formatted notices too as I believe this closer resembles how we actually output such generated notices.. but I decided that should be for another bug. Created attachment 158520 [details] [review] Bug 30287: (follow-up) Add content wrap matching send time This patch adds a new method to the Koha::Notice::Message class to wrap content in the same way we currently do at email send time. We then update the notice preview to use this method so we're embedding NoticeCSS into that iframe. This means the generated preview should now exactly match how your notice will display in email clients including any CSS you may have placed within NoticeCSS. This looks good and I am tempted to sign-off but the dependency is currently 'Failed QA'. Maybe we should sort that one out and then get this one done on top of Bug 34704? The dependency is moving again now and was a lot of the reason behind me working in this one. I'm considering adding back my initial swap from the html_line_break filter to wrapping in <pre> as I think that more accurately shows how such notices will be sent.. though email clients tend to auto wrap plaintext so it's still not perfect and can look a little odd in display here.. maybe stick with it as is. I'll add unit tests for the new method in Koha::Notice::Message Created attachment 158648 [details] [review] Bug 30287: Conditionally render notice preview This patch adds conditional formatting based on whether the notice was generated from an HTML template or a plaintext one. We simply drop the 'html_line_break' filter for the case where the notice should already be html formatted. I was tempted to replace the html_line_break for plaintext formatted notices too as I believe this closer resembles how we actually output such generated notices.. but I decided that should be for another bug. Created attachment 158649 [details] [review] Bug 30287: (follow-up) Add content wrap matching send time This patch adds a new method to the Koha::Notice::Message class to wrap content in the same way we currently do at email send time. We then update the notice preview to use this method so we're embedding NoticeCSS into that iframe. This means the generated preview should now exactly match how your notice will display in email clients including any CSS you may have placed within NoticeCSS. Created attachment 158650 [details] [review] Bug 30287: (follow-up) Unit test for html_content This patch adds a unit test for the new `html_content` method introduced to Koha::Notice::Template in the previous patch. Test 1) Run the new unit test ;P Created attachment 158651 [details] [review] Bug 30287: Conditionally render notice preview This patch adds conditional formatting based on whether the notice was generated from an HTML template or a plaintext one. We simply drop the 'html_line_break' filter for the case where the notice should already be html formatted. I was tempted to replace the html_line_break for plaintext formatted notices too as I believe this closer resembles how we actually output such generated notices.. but I decided that should be for another bug. Created attachment 158652 [details] [review] Bug 30287: (follow-up) Add content wrap matching send time This patch adds a new method to the Koha::Notice::Message class to wrap content in the same way we currently do at email send time. We then update the notice preview to use this method so we're embedding NoticeCSS into that iframe. This means the generated preview should now exactly match how your notice will display in email clients including any CSS you may have placed within NoticeCSS. Created attachment 158653 [details] [review] Bug 30287: (follow-up) Unit test for html_content This patch adds a unit test for the new `html_content` method introduced to Koha::Notice::Template in the previous patch. Test 1) Run the new unit test ;P Created attachment 158654 [details] [review] Bug 30287: Conditionally render notice preview This patch adds conditional formatting based on whether the notice was generated from an HTML template or a plaintext one. We simply drop the 'html_line_break' filter for the case where the notice should already be html formatted. I was tempted to replace the html_line_break for plaintext formatted notices too as I believe this closer resembles how we actually output such generated notices.. but I decided that should be for another bug. Created attachment 158655 [details] [review] Bug 30287: (follow-up) Add content wrap matching send time This patch adds a new method to the Koha::Notice::Message class to wrap content in the same way we currently do at email send time. We then update the notice preview to use this method so we're embedding NoticeCSS into that iframe. This means the generated preview should now exactly match how your notice will display in email clients including any CSS you may have placed within NoticeCSS. Created attachment 158656 [details] [review] Bug 30287: (follow-up) Unit test for html_content This patch adds a unit test for the new `html_content` method introduced to Koha::Notice::Template in the previous patch. Test 1) Run the new unit test ;P Final patch will fail the qa script on a trailing whitespace.. this is safe to ignore in this case as it's within the HERE document and the unit test would fail without the intentional whitespace here. Can we please have a test plan? I've tested, using the test plan below. I hope this covers things. Feel free to add other notices that better reflect the changes.
I think everything works as expected, however the tests fail - see [3].
Test plan:
1. Setup KTD so it can send emails.[1]
2. Add an email to a patron's account (I used Henry Acevedo) and the koha user.
3. Add an email address to KohaAdminEmailAddress.
4. Edit patron so that email messages are sent for holds filled, item check-in, and item checkout.
5. Generate and send notices:[2]
5.1 Send the welcome email (Patron account > More > Send welcome email
5.2 Place and fill a hold so that is waiting for pickup
5.3 Checkout the held item to the patron
5.4 Check in the item checked out
5.5 Enable two-factor authentication (TwoFactorAuthentication system preference) for the koha user under Patrons > [Patron account] > More > Manage two-factor authentication
5.6 Enable UseEmailReceipts system preference, add a manual invoice, and then pay it
5.7 Change the password for the patron (using Edit the patron, rather than Change password)
5.8 Run misc/cronjobs/process_message_queue.pl
6. Compare the emails received and the notice previews under notices for the patron - see [2] for what to expect.
7. Apply the patch, flush_memcached, and restart_all.
8. Repeat steps 5 and 6 - the preview for the notices should either still match or be better (extra lines remove in previews), as noted in [2].
9. Check that the new tests pass: prove t/db_dependent/Koha/Notice/Message.t
[1] To test sending emails using a Google account:
1. Set up an App password for your Google Account
2. Edit /etc/koha/sites/kohadev/koha-conf.xml file and add this configuration near the end (where <user_name> = your Google email address; <password> = your APP password, not your Google account password):
<smtp_server>
<host>smtp.gmail.com</host>
<port>587</port>
<timeout>5</timeout>
<ssl_mode>STARTTLS</ssl_mode>
<user_name>GOOGLEACCOUNTUSER</user_name>
<password>GOOGLEAPPPASSWORD</password>
<debug>1</debug>
</smtp_server>
[2] These notices were tested (includes any notes about before and after):
- WELCOME (HTML - uses <br>s): before - both email and preview match; after - both email and preview match
- HOLD (plaintext): before - both email and preview match; after - both email and preview match
- CHECKOUT (plaintext): before - both email and preview match (no lines between paragraphs); after - both email and preview match
- CHECKIN (plaintext): before - both email and preview match (no lines between paragraphs); after - both email and preview match
- 2FA_ENABLE (HTML - uses <p> tags): before - the email mesage is fine, the preview has extra blank lines between paragraphs; after - the email mesage is fine (no change), the preview is now displayed correctly with no extra blank lines between paragraphs
- ACCOUNT_PAYMENT (plaintext): before - both email and preview match; after - both email and preview match;
- PASSWORD_CHANGE (plaintext): before - email and preview match; after - email and preview match (note: sent directly, not through the message queue, but shows in notices)
[3] The tests fail for me after the patches are applied:
kohadev-koha@kohadevbox:koha(bz30287)$ prove t/db_dependent/Koha/Notice/Message.t
t/db_dependent/Koha/Notice/Message.t ..
# Failed test 'html_content returned the correct html wrapped letter'
# at t/db_dependent/Koha/Notice/Message.t line 91.
# got: '<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
# "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
# <html lang="en" xml:lang="en" xmlns="http://www.w3.org/1999/xhtml">
# <head>
# <title>fyOGZoKQa</title>
# <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
# <link rel="stylesheet" type="text/css" href="http://127.0.0.1:8081/intranet-tmpl/prog/css/notices.css">
# </head>
# <body>
# This is a test template using borrower 55
# </body>
# </html>
# '
# expected: '<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
# "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
# <html lang="en" xml:lang="en" xmlns="http://www.w3.org/1999/xhtml">
# <head>
# <title>fyOGZoKQa</title>
# <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
#
# </head>
# <body>
# This is a test template using borrower 55
# </body>
# </html>
# '
# Looks like you failed 1 test of 2.
t/db_dependent/Koha/Notice/Message.t .. 1/1
# Failed test 'html_content() tests'
# at t/db_dependent/Koha/Notice/Message.t line 117.
# Looks like you failed 1 test of 1.
t/db_dependent/Koha/Notice/Message.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/1 subtests
Test Summary Report
-------------------
t/db_dependent/Koha/Notice/Message.t (Wstat: 256 Tests: 1 Failed: 1)
Failed test: 1
Non-zero exit status: 1
Files=1, Tests=1, 1 wallclock secs ( 0.02 usr 0.00 sys + 0.94 cusr 0.15 csys = 1.11 CPU)
Result: FAIL
Other testing notes:
1. I tried to test sending the cart and a list (I realised after that these are sent these are sent direct, and not included in the patron's notices list). I had trouble sending these, although I had added email addresses everywhere I could think of. I got this message after submitting the form: "There was an error sending the list.". There is this error in the log: [WARN] Error sending mail: sender's email address is invalid at /usr/share/perl5/CGI/Compile.pm line 151. Not sure whether this is just something wrong on my KTD configuration, or a bug.
2. I tried testing using NoticeCSS:
2.1 Edit the welcome message:
. add a new line: <p class="highlight-notice">This is red.</p>
. add another new line: <p style="font-family:Arial,Helvetica,Serif; color:red;">This is also red</p>
2.2 Add this URL to NoticeCSS: http://127.0.0.1:8081/intranet-tmpl/prog/css/notices.css
2.3 Add a new CSS file called notices.css to koha-tmpl/intranet-tmpl/prog/css/
.highlight-notice {
color: red;
}
2.4 Before - NoticeCSS doesn't show in email or preview; after - doesn't show in email, shows in preview
2.5 Note: Inline CSS shows both before and after
2.6 Note: This Wiki page indicates that using NoticeCSS is not recommended (that is how I read it) https://wiki.koha-community.org/wiki/Customising_Notices_and_Slips#Quick_Tips_for_Custom_HTML_Notices.2FSlips
That's fantastic testing David, thankyou! I must admit, I was coding a little blind. As you've highlighted here, we were already displaying "most" notices correctly, it was only certain constructs that looked odd, but narrowing down which one's wasn't clear to me (you've proved that <p> without <br> was one case and I'm sure there are some others where this patch will help too). As for NoticeCSS, thanks for confirming my suspicions there too.. I wanted to make sure we covered it in the preview, but I am aware that using linked css sheets in emails is 'variable at best'.. I'm keen to keep working through the notices code and add options for embedding css at the top of the email as aposed to adding a linked sheet. Finally.. I'll take a look at the tests.. I wonder if there's a rebase issue crept in, they were working when I tested before uploading. I see now.. the test relies on NoticeCSS being empty prior to run.. I'll make it a little more reliable now. Created attachment 158834 [details] [review] Bug 30287: (follow-up) Make unit test more reliable in ktd This patch mocks NoticeCSS earlier to ensure we're not inadvertantly failing due to a local setup in ktd having added to the preference. Do you think it worth a little alert in the Modal when NoticeCSS is detected that the content may not display correctly in all email clients? (In reply to Martin Renvoize from comment #25) > Do you think it worth a little alert in the Modal when NoticeCSS is detected > that the content may not display correctly in all email clients? If we do want this, I think it should be a new bug... I just looked at it and because we're using iFrames here's it's not as trivial as first thought. Hi Martin.
It wouldn't hurt adding something like a little alert in the Modal when NoticeCSS is detected that the content may not display correctly in all email clients. But as you say, this should be a separate bug.
Alternatively or in addition, we could add a note to the NoticeCSS documentation about this. I'm assuming that it works fine for printed notices, and it doesn't always work with emails is because of the vagaries of HTML email and different email clients.
Tests are still failing for me:
kohadev-koha@kohadevbox:koha(bz30287)$ prove t/db_dependent/Koha/Notice/Message.t
t/db_dependent/Koha/Notice/Message.t ..
# Failed test 'html_content returned the correct html wrapped letter'
# at t/db_dependent/Koha/Notice/Message.t line 92.
# got: '<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
# "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
# <html lang="en" xml:lang="en" xmlns="http://www.w3.org/1999/xhtml">
# <head>
# <title>uLrVEOWv9</title>
# <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
#
# </head>
# <body>
# This is a test template using borrower 55
# </body>
# </html>
# '
# expected: '<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
# "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
# <html lang="en" xml:lang="en" xmlns="http://www.w3.org/1999/xhtml">
# <head>
# <title>uLrVEOWv9</title>
# <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
#
# </head>
# <body>
# This is a test template using borrower 55
# </body>
# </html>
# '
# Looks like you failed 1 test of 2.
t/db_dependent/Koha/Notice/Message.t .. 1/1
# Failed test 'html_content() tests'
# at t/db_dependent/Koha/Notice/Message.t line 118.
# Looks like you failed 1 test of 1.
t/db_dependent/Koha/Notice/Message.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/1 subtests
Test Summary Report
-------------------
t/db_dependent/Koha/Notice/Message.t (Wstat: 256 Tests: 1 Failed: 1)
Failed test: 1
Non-zero exit status: 1
Files=1, Tests=1, 1 wallclock secs ( 0.01 usr 0.01 sys + 0.89 cusr 0.18 csys = 1.09 CPU)
Result: FAIL
Hi David, I'm really struggling to replicate your test failure with that final follow-up. Did you take any other steps not listed in your test plan? I've followed it all through and the test just won't fail for me :( (In reply to Martin Renvoize from comment #28) > Hi David, > > I'm really struggling to replicate your test failure with that final > follow-up. Did you take any other steps not listed in your test plan? I've > followed it all through and the test just won't fail for me :( It's still failing for me - same message. I'm all up-to-date - git pulls for koha and KTD, ktd pull, apply patches, restart_all. I haven't done anything in the UI, or set up email sending. Created attachment 158869 [details] [review] Bug 30287: (follow-up) Make unit test more reliable in ktd This patch mocks NoticeCSS earlier to ensure we're not inadvertantly failing due to a local setup in ktd having added to the preference. Created attachment 158870 [details] [review] Bug 30287: Conditionally render notice preview This patch adds conditional formatting based on whether the notice was generated from an HTML template or a plaintext one. We simply drop the 'html_line_break' filter for the case where the notice should already be html formatted. I was tempted to replace the html_line_break for plaintext formatted notices too as I believe this closer resembles how we actually output such generated notices.. but I decided that should be for another bug. Created attachment 158871 [details] [review] Bug 30287: (follow-up) Add content wrap matching send time This patch adds a new method to the Koha::Notice::Message class to wrap content in the same way we currently do at email send time. We then update the notice preview to use this method so we're embedding NoticeCSS into that iframe. This means the generated preview should now exactly match how your notice will display in email clients including any CSS you may have placed within NoticeCSS. Created attachment 158872 [details] [review] Bug 30287: (follow-up) Unit test for html_content This patch adds a unit test for the new `html_content` method introduced to Koha::Notice::Template in the previous patch. Test 1) Run the new unit test ;P Created attachment 158873 [details] [review] Bug 30287: (follow-up) Make unit test more reliable in ktd This patch mocks NoticeCSS earlier to ensure we're not inadvertantly failing due to a local setup in ktd having added to the preference. QA script is happier now and the test should pass even with git whitespace munging. Created attachment 158892 [details] [review] Bug 30287: Conditionally render notice preview This patch adds conditional formatting based on whether the notice was generated from an HTML template or a plaintext one. We simply drop the 'html_line_break' filter for the case where the notice should already be html formatted. I was tempted to replace the html_line_break for plaintext formatted notices too as I believe this closer resembles how we actually output such generated notices.. but I decided that should be for another bug. Signed-off-by: David Nind <david@davidnind.com> Created attachment 158893 [details] [review] Bug 30287: (follow-up) Add content wrap matching send time This patch adds a new method to the Koha::Notice::Message class to wrap content in the same way we currently do at email send time. We then update the notice preview to use this method so we're embedding NoticeCSS into that iframe. This means the generated preview should now exactly match how your notice will display in email clients including any CSS you may have placed within NoticeCSS. Signed-off-by: David Nind <david@davidnind.com> Created attachment 158894 [details] [review] Bug 30287: (follow-up) Unit test for html_content This patch adds a unit test for the new `html_content` method introduced to Koha::Notice::Template in the previous patch. Test 1) Run the new unit test ;P Signed-off-by: David Nind <david@davidnind.com> Created attachment 158895 [details] [review] Bug 30287: (follow-up) Make unit test more reliable in ktd This patch mocks NoticeCSS earlier to ensure we're not inadvertantly failing due to a local setup in ktd having added to the preference. Signed-off-by: David Nind <david@davidnind.com> Thanks for your perseverance Martin! Thank you for all the testing David.. it's always a pleasure to work on something your testing :) Looking here. This feels a bit like an enhancement ? (In reply to Marcel de Rooy from comment #42) > Looking here. > This feels a bit like an enhancement ? The original bug wasn't, but it might have grown a bit. The problem was that the display in Koha was not matching the email in terms of line breaks. This caused confusion when testing etc. +sub html_content {
+<html lang="en" xml:lang="en" xmlns="http://www.w3.org/1999/xhtml">
Teasing a bit: but who says that this text is English ?
Created attachment 159431 [details] [review] Bug 30287: Conditionally render notice preview This patch adds conditional formatting based on whether the notice was generated from an HTML template or a plaintext one. We simply drop the 'html_line_break' filter for the case where the notice should already be html formatted. I was tempted to replace the html_line_break for plaintext formatted notices too as I believe this closer resembles how we actually output such generated notices.. but I decided that should be for another bug. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 159432 [details] [review] Bug 30287: (follow-up) Add content wrap matching send time This patch adds a new method to the Koha::Notice::Message class to wrap content in the same way we currently do at email send time. We then update the notice preview to use this method so we're embedding NoticeCSS into that iframe. This means the generated preview should now exactly match how your notice will display in email clients including any CSS you may have placed within NoticeCSS. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 159433 [details] [review] Bug 30287: (follow-up) Unit test for html_content This patch adds a unit test for the new `html_content` method introduced to Koha::Notice::Template in the previous patch. Test 1) Run the new unit test ;P Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 159434 [details] [review] Bug 30287: (follow-up) Make unit test more reliable in ktd This patch mocks NoticeCSS earlier to ensure we're not inadvertantly failing due to a local setup in ktd having added to the preference. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to Marcel de Rooy from comment #44) > +sub html_content { > +<html lang="en" xml:lang="en" xmlns="http://www.w3.org/1999/xhtml"> > > Teasing a bit: but who says that this text is English ? That's actually a really good point.. I didn't consider that at all.. but we really aught to be able to do better, I would have thought. (In reply to Martin Renvoize from comment #49) > (In reply to Marcel de Rooy from comment #44) > > +sub html_content { > > +<html lang="en" xml:lang="en" xmlns="http://www.w3.org/1999/xhtml"> > > > > Teasing a bit: but who says that this text is English ? > > That's actually a really good point.. I didn't consider that at all.. but we > really aught to be able to do better, I would have thought. Could you please file a separate bug for that? I think it makes sense to deal with this separately. Already did, it's in the tree ;) (In reply to Martin Renvoize from comment #51) > Already did, it's in the tree ;) Thanks! Also for the quick feedback. Testing this one right now, hopefully finishing a little later. Pushed for 24.05! Well done everyone, thank you! Pushed to 23.11.x for 23.11.02 Backported to 23.05.x for upcoming 23.05.08 |