Bug 30287 - Notices using HTML render differently in notices.pl
Summary: Notices using HTML render differently in notices.pl
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords: release-notes-needed
Depends on:
Blocks: 33260 35267 35285
  Show dependency treegraph
 
Reported: 2022-03-13 00:58 UTC by Christopher Brannon
Modified: 2024-02-29 01:50 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes notice previews for patrons in the staff interface (Patrons > [Patron account] > Notices), where HTML is used in the email notices. For example, previously if <br>s were used then the preview would match the email sent, however, using <p>s would add extra lines in the preview.
Version(s) released in:
24.05.00,23.11.02,23.05.08


Attachments
Bug 30287: Conditionally render notice preview (3.37 KB, patch)
2023-11-06 12:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30287: Conditionally render notice preview (2.94 KB, patch)
2023-11-06 14:02 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30287: (follow-up) Add content wrap matching send time (3.14 KB, patch)
2023-11-06 14:02 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30287: Conditionally render notice preview (2.94 KB, patch)
2023-11-08 09:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30287: (follow-up) Add content wrap matching send time (3.17 KB, patch)
2023-11-08 09:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30287: (follow-up) Unit test for html_content (4.54 KB, patch)
2023-11-08 09:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30287: Conditionally render notice preview (2.94 KB, patch)
2023-11-08 10:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30287: (follow-up) Add content wrap matching send time (3.34 KB, patch)
2023-11-08 10:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30287: (follow-up) Unit test for html_content (4.54 KB, patch)
2023-11-08 10:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30287: Conditionally render notice preview (2.94 KB, patch)
2023-11-08 10:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30287: (follow-up) Add content wrap matching send time (3.47 KB, patch)
2023-11-08 10:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30287: (follow-up) Unit test for html_content (4.57 KB, patch)
2023-11-08 10:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30287: (follow-up) Make unit test more reliable in ktd (992 bytes, patch)
2023-11-11 11:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30287: (follow-up) Make unit test more reliable in ktd (1.90 KB, patch)
2023-11-13 10:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30287: Conditionally render notice preview (2.94 KB, patch)
2023-11-13 10:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30287: (follow-up) Add content wrap matching send time (3.47 KB, patch)
2023-11-13 10:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30287: (follow-up) Unit test for html_content (4.56 KB, patch)
2023-11-13 10:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30287: (follow-up) Make unit test more reliable in ktd (1.90 KB, patch)
2023-11-13 10:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30287: Conditionally render notice preview (2.98 KB, patch)
2023-11-13 17:30 UTC, David Nind
Details | Diff | Splinter Review
Bug 30287: (follow-up) Add content wrap matching send time (3.51 KB, patch)
2023-11-13 17:30 UTC, David Nind
Details | Diff | Splinter Review
Bug 30287: (follow-up) Unit test for html_content (4.61 KB, patch)
2023-11-13 17:30 UTC, David Nind
Details | Diff | Splinter Review
Bug 30287: (follow-up) Make unit test more reliable in ktd (1.95 KB, patch)
2023-11-13 17:30 UTC, David Nind
Details | Diff | Splinter Review
Bug 30287: Conditionally render notice preview (3.08 KB, patch)
2023-12-01 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30287: (follow-up) Add content wrap matching send time (3.61 KB, patch)
2023-12-01 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30287: (follow-up) Unit test for html_content (4.70 KB, patch)
2023-12-01 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30287: (follow-up) Make unit test more reliable in ktd (2.05 KB, patch)
2023-12-01 09:57 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 Christopher Brannon 2022-03-13 00:58:01 UTC
When HTML is switched on for notices (e-mail), it renders differently between the editor, the actual e-mail, and the preview in notices.pl.

First, if you don't use <br/> at the end of lines, everything is smushed together in the e-mail.  But in notices.pl, it shows the way it was put in the editor.

If you use <br/>, it renders correctly in the e-mail, but will show your regular line breaks and the <br/>, spacing everything out weird.

It is a real trick trying to properly format an e-mail with html and have it look good everywhere.

I think the line breaks are being coded weird.  It would sure be nice to have an html editor for the notices, if you turn html on.  At the very least, I would like to see more consistency between the three.
Comment 1 Owen Leonard 2022-03-14 17:12:58 UTC
In what way is the preview different than the actual email?
Comment 2 Christopher Brannon 2022-03-15 16:12:20 UTC
(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.
Comment 3 Katrin Fischer 2022-03-22 10:14:59 UTC
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.
Comment 4 Daniel Gaghan 2023-02-14 17:46:44 UTC
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.
Comment 5 Martin Renvoize 2023-11-06 12:52:21 UTC
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.
Comment 6 Martin Renvoize 2023-11-06 14:02:55 UTC
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.
Comment 7 Martin Renvoize 2023-11-06 14:02:58 UTC
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.
Comment 8 Lucas Gass 2023-11-06 23:07:26 UTC
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?
Comment 9 Martin Renvoize 2023-11-08 07:06:25 UTC
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
Comment 10 Martin Renvoize 2023-11-08 09:39:55 UTC
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.
Comment 11 Martin Renvoize 2023-11-08 09:39:58 UTC
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.
Comment 12 Martin Renvoize 2023-11-08 09:40:00 UTC
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
Comment 13 Martin Renvoize 2023-11-08 10:05:58 UTC
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.
Comment 14 Martin Renvoize 2023-11-08 10:06:01 UTC
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.
Comment 15 Martin Renvoize 2023-11-08 10:06:04 UTC
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
Comment 16 Martin Renvoize 2023-11-08 10:09:48 UTC
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.
Comment 17 Martin Renvoize 2023-11-08 10:09:50 UTC
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.
Comment 18 Martin Renvoize 2023-11-08 10:09:53 UTC
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
Comment 19 Martin Renvoize 2023-11-08 10:11:20 UTC
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.
Comment 20 Katrin Fischer 2023-11-09 20:38:46 UTC
Can we please have a test plan?
Comment 21 David Nind 2023-11-11 10:14:30 UTC
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
Comment 22 Martin Renvoize 2023-11-11 11:00:27 UTC
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.
Comment 23 Martin Renvoize 2023-11-11 11:04:59 UTC
I see now.. the test relies on NoticeCSS being empty prior to run..  I'll make it a little more reliable now.
Comment 24 Martin Renvoize 2023-11-11 11:07:44 UTC
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.
Comment 25 Martin Renvoize 2023-11-11 11:08:46 UTC
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?
Comment 26 Martin Renvoize 2023-11-11 11:15:53 UTC
(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.
Comment 27 David Nind 2023-11-11 19:36:06 UTC
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
Comment 28 Martin Renvoize 2023-11-13 08:17:25 UTC
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 :(
Comment 29 David Nind 2023-11-13 08:40:11 UTC
(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.
Comment 30 Martin Renvoize 2023-11-13 10:31:02 UTC
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.
Comment 31 Martin Renvoize 2023-11-13 10:33:47 UTC
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.
Comment 32 Martin Renvoize 2023-11-13 10:33:50 UTC
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.
Comment 33 Martin Renvoize 2023-11-13 10:33:53 UTC
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
Comment 34 Martin Renvoize 2023-11-13 10:33:56 UTC
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.
Comment 35 Martin Renvoize 2023-11-13 10:34:55 UTC
QA script is happier now and the test should pass even with git whitespace munging.
Comment 36 David Nind 2023-11-13 17:30:45 UTC
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>
Comment 37 David Nind 2023-11-13 17:30:48 UTC
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>
Comment 38 David Nind 2023-11-13 17:30:51 UTC
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>
Comment 39 David Nind 2023-11-13 17:30:54 UTC
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>
Comment 40 David Nind 2023-11-13 17:31:55 UTC
Thanks for your perseverance Martin!
Comment 41 Martin Renvoize 2023-11-14 13:30:34 UTC
Thank you for all the testing David.. it's always a pleasure to work on something your testing :)
Comment 42 Marcel de Rooy 2023-12-01 09:40:57 UTC
Looking here.
This feels a bit like an enhancement ?
Comment 43 Katrin Fischer 2023-12-01 09:51:08 UTC
(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.
Comment 44 Marcel de Rooy 2023-12-01 09:56:52 UTC
+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 ?
Comment 45 Marcel de Rooy 2023-12-01 09:57:36 UTC
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>
Comment 46 Marcel de Rooy 2023-12-01 09:57:39 UTC
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>
Comment 47 Marcel de Rooy 2023-12-01 09:57:41 UTC
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>
Comment 48 Marcel de Rooy 2023-12-01 09:57:44 UTC
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>
Comment 49 Martin Renvoize 2023-12-01 11:15:18 UTC
(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.
Comment 50 Katrin Fischer 2023-12-19 10:53:10 UTC
(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.
Comment 51 Martin Renvoize 2023-12-19 11:20:52 UTC
Already did, it's in the tree ;)
Comment 52 Katrin Fischer 2023-12-19 11:31:51 UTC
(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.
Comment 53 Katrin Fischer 2023-12-19 12:43:57 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 54 Fridolin Somers 2024-01-03 15:05:07 UTC
Pushed to 23.11.x for 23.11.02
Comment 55 Lucas Gass 2024-01-04 20:35:50 UTC
Backported to 23.05.x for upcoming 23.05.08