Bug 35187 - Fix line breaks in some HTML notices, including WELCOME
Summary: Fix line breaks in some HTML notices, including WELCOME
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial (vote)
Assignee: Katrin Fischer
QA Contact: Martin Renvoize
URL:
Keywords: rel_23_05_candidate
: 35018 (view as bug list)
Depends on:
Blocks: 34704
  Show dependency treegraph
 
Reported: 2023-10-29 16:01 UTC by Katrin Fischer
Modified: 2023-11-14 14:50 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.06,22.11.12


Attachments
Bug 35187: Make sure HTML notices use explicit line breaks (6.97 KB, patch)
2023-10-29 16:24 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 35187: Make sure HTML notices use explicit line breaks (6.96 KB, patch)
2023-10-29 16:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 35187: Make sure HTML notices use explicit line breaks (7.01 KB, patch)
2023-10-30 00:56 UTC, David Nind
Details | Diff | Splinter Review
Bug 35187: Make sure HTML notices use explicit line breaks (7.08 KB, patch)
2023-11-06 11:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35187: (follow-up) Use <br> consistently over <br /> (8.58 KB, patch)
2023-11-06 11:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
[23.05] Bug 35187: Fix missing line breaks in WELCOME notice (2.51 KB, patch)
2023-11-10 18:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 35187: [23.05] (follow-up) Fix missing line breaks in WELCOME notice (1.10 KB, patch)
2023-11-11 10:04 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2023-10-29 16:01:04 UTC
When a notices is marked as HTML, we need to use block elements (p, div, li, etc.) or explicit line breaks.
This adds the line breaks where required.
Comment 1 Katrin Fischer 2023-10-29 16:01:30 UTC
*** Bug 35018 has been marked as a duplicate of this bug. ***
Comment 2 Katrin Fischer 2023-10-29 16:24:15 UTC
Created attachment 158049 [details] [review]
Bug 35187: Make sure HTML notices use explicit line breaks

When a notices is marked as HTML, we need to use block elements
(p, div, li, etc.) or explicit line breaks.
This adds the line breaks where required.

Affects:

* ISSUEQSLIP
* ISSUESLIP
* TRANSFERSLIP
* CHECKINSLIP
* HOLD_SLIP
* WELCOME

The biggest change is the WELCOME notices. With this patch
it appears no longer on only one line, but properly formatted.

To test:

WELCOME
* Without patch
* Make sure your Koha can send email (set up SMTP server, KohaAdminLibraryAddress)
* Make sure your patron has an email set
* In patron account, use "More > Welcome email"
* Verify all text is on one line
* Apply patch
* reset_all to install new sample notice templates
* Repeat test, verify welcome notice is now nice

ISSUESLIP, ISSUEQSLIP
* Checkout at least 2 items
* Print > Print slip
* Print > Print quick slip
* Verify both look nice!

CHECKINSLIP
* Return both items
* Print checkin slip (button in list of checkouts)
* Verfiy... you know, that it looks nice!

TRANSFERSLIP
* Checkin an item from another library
* "Print transfer slip"
* Verify again.

HOLD_SLIP
* Place a hold on a record
* Return an item that can fill the hold
* "Print slip and confirm"
* Verify.
* Note: if you compare to before, this also removes a <> appearing instead
  of the city.
Comment 3 Katrin Fischer 2023-10-29 16:25:03 UTC
Created attachment 158050 [details] [review]
Bug 35187: Make sure HTML notices use explicit line breaks

When a notices is marked as HTML, we need to use block elements
(p, div, li, etc.) or explicit line breaks.
This adds the line breaks where required.

Affects:

* ISSUEQSLIP
* ISSUESLIP
* TRANSFERSLIP
* CHECKINSLIP
* HOLD_SLIP
* WELCOME

The biggest change is the WELCOME notices. With this patch
it appears no longer on only one line, but properly formatted.

To test:

WELCOME
* Without patch
* Make sure your Koha can send email (set up SMTP server, KohaAdminLibraryAddress)
* Make sure your patron has an email set
* In patron account, use "More > Welcome email"
* Verify all text is on one line
* Apply patch
* reset_all to install new sample notice templates
* Repeat test, verify welcome notice is now nice

ISSUESLIP, ISSUEQSLIP
* Checkout at least 2 items
* Print > Print slip
* Print > Print quick slip
* Verify both look nice!

CHECKINSLIP
* Return both items
* Print checkin slip (button in list of checkouts)
* Verfiy... you know, that it looks nice!

TRANSFERSLIP
* Checkin an item from another library
* "Print transfer slip"
* Verify again.

HOLD_SLIP
* Place a hold on a record
* Return an item that can fill the hold
* "Print slip and confirm"
* Verify.
* Note: if you compare to before, this also removes a <> appearing instead
  of the city.
Comment 4 David Nind 2023-10-30 00:56:58 UTC
Created attachment 158055 [details] [review]
Bug 35187: Make sure HTML notices use explicit line breaks

When a notices is marked as HTML, we need to use block elements
(p, div, li, etc.) or explicit line breaks.
This adds the line breaks where required.

Affects:

* ISSUEQSLIP
* ISSUESLIP
* TRANSFERSLIP
* CHECKINSLIP
* HOLD_SLIP
* WELCOME

The biggest change is the WELCOME notices. With this patch
it appears no longer on only one line, but properly formatted.

To test:

WELCOME
* Without patch
* Make sure your Koha can send email (set up SMTP server, KohaAdminLibraryAddress)
* Make sure your patron has an email set
* In patron account, use "More > Welcome email"
* Verify all text is on one line
* Apply patch
* reset_all to install new sample notice templates
* Repeat test, verify welcome notice is now nice

ISSUESLIP, ISSUEQSLIP
* Checkout at least 2 items
* Print > Print slip
* Print > Print quick slip
* Verify both look nice!

CHECKINSLIP
* Return both items
* Print checkin slip (button in list of checkouts)
* Verfiy... you know, that it looks nice!

TRANSFERSLIP
* Checkin an item from another library
* "Print transfer slip"
* Verify again.

HOLD_SLIP
* Place a hold on a record
* Return an item that can fill the hold
* "Print slip and confirm"
* Verify.
* Note: if you compare to before, this also removes a <> appearing instead
  of the city.

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2023-10-30 01:04:26 UTC
All nice improvements!

The only thing I noticed with the WELCOME notice, is that it displayed OK for me before the patch under Patrons > [Account] > Notices, but the email received was all on one line. After the patch, there are extra lines displayed under Patrons > [Account] > Notices, but the email received is now nicely formatted. I tried using <p>s instead of <br/>s - no visible change for email message received, and only slightly better under Patrons > [Account] > Notices. Better that the email message that goes out is correct by default.

I changed the bug assignee 8-)

Testing notes (using KTD):

1. Add email to KohaAdminEmailAddress system preference. 

2. To test sending emails using a Google account:
   3.1 Set up an App password in your Google Account
   3.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>

3. WELCOME
   - Before the patch:
     . Displays OK for me if you view the message under Patron > [Account] > Notices
     . The email received (after running misc/cronjobs/process_message_queue.pl) is all on one line
   - After the patch:
     . There are extra lines if you view the message under Patron > Notices (not a showstopper for me - better that the email that goes out is correct)
     . The email received (after running misc/cronjobs/process_message_queue.pl) is now correctly formatted and looks great!
     . I tried using <p></p> tags instead of <br/> tags: the email message comes through the same, the preview under Patrons > [Account] > Notices looks slightly better, but still has extra lines
     . Not sure what the best way to do this is!
Comment 6 David Nind 2023-10-30 01:08:02 UTC
(In reply to David Nind from comment #5)
> The only thing I noticed with the WELCOME notice, is that it displayed OK
> for me before the patch under Patrons > [Account] > Notices, but the email
> received was all on one line. After the patch, there are extra lines
> displayed under Patrons > [Account] > Notices, but the email received is now
> nicely formatted. I tried using <p>s instead of <br/>s - no visible change
> for email message received, and only slightly better under Patrons >
> [Account] > Notices. Better that the email message that goes out is correct
> by default.

Please ignore these comments - just read the comments for the other bugs to address/look at this!
Comment 7 Katrin Fischer 2023-10-30 07:26:22 UTC
(In reply to David Nind from comment #6)
> (In reply to David Nind from comment #5)
> > The only thing I noticed with the WELCOME notice, is that it displayed OK
> > for me before the patch under Patrons > [Account] > Notices, but the email
> > received was all on one line. After the patch, there are extra lines
> > displayed under Patrons > [Account] > Notices, but the email received is now
> > nicely formatted. I tried using <p>s instead of <br/>s - no visible change
> > for email message received, and only slightly better under Patrons >
> > [Account] > Notices. Better that the email message that goes out is correct
> > by default.
> 
> Please ignore these comments - just read the comments for the other bugs to
> address/look at this!

Thx David! For the next one looking at this, what David describes is bug 30287. The notices tab doesn't distinguish between plain text and HTML notices correctly adding extra line breaks to the latter.
Comment 8 Martin Renvoize 2023-11-06 11:44:50 UTC
Created attachment 158509 [details] [review]
Bug 35187: Make sure HTML notices use explicit line breaks

When a notices is marked as HTML, we need to use block elements
(p, div, li, etc.) or explicit line breaks.
This adds the line breaks where required.

Affects:

* ISSUEQSLIP
* ISSUESLIP
* TRANSFERSLIP
* CHECKINSLIP
* HOLD_SLIP
* WELCOME

The biggest change is the WELCOME notices. With this patch
it appears no longer on only one line, but properly formatted.

To test:

WELCOME
* Without patch
* Make sure your Koha can send email (set up SMTP server, KohaAdminLibraryAddress)
* Make sure your patron has an email set
* In patron account, use "More > Welcome email"
* Verify all text is on one line
* Apply patch
* reset_all to install new sample notice templates
* Repeat test, verify welcome notice is now nice

ISSUESLIP, ISSUEQSLIP
* Checkout at least 2 items
* Print > Print slip
* Print > Print quick slip
* Verify both look nice!

CHECKINSLIP
* Return both items
* Print checkin slip (button in list of checkouts)
* Verfiy... you know, that it looks nice!

TRANSFERSLIP
* Checkin an item from another library
* "Print transfer slip"
* Verify again.

HOLD_SLIP
* Place a hold on a record
* Return an item that can fill the hold
* "Print slip and confirm"
* Verify.
* Note: if you compare to before, this also removes a <> appearing instead
  of the city.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2023-11-06 11:44:52 UTC
Created attachment 158510 [details] [review]
Bug 35187: (follow-up) Use <br> consistently over <br />

The first patch highlighted a minor inconsistency to me and this patch
just brings us closer to html5 consistency throughout the notices by
replacing any existing occurences of `<br />` and `<br/>` with the
recommended `<br>`.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2023-11-06 11:46:52 UTC
This is a great consistency improvement, thanks for spending time on it Katrin.

All works as expected and QA scripts are happy too.

I did a very minor follow-up to replace existing cases of <br /> with the current best practice of <br>.

Passing QA
Comment 11 Tomás Cohen Arazi 2023-11-06 14:36:52 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 12 Fridolin Somers 2023-11-10 07:41:19 UTC
Does not apply on 23.05.x
Since this is minor we may let it for 23.11 only
Comment 13 Katrin Fischer 2023-11-10 17:44:12 UTC
(In reply to Fridolin Somers from comment #12)
> Does not apply on 23.05.x
> Since this is minor we may let it for 23.11 only

The WELCOME notice is the main issue here, as it all comes out on one line. If you'd like I could provide a patch just fixing that for 23.05.
Comment 14 Katrin Fischer 2023-11-10 18:29:05 UTC
Created attachment 158813 [details] [review]
[23.05] Bug 35187: Fix missing line breaks in WELCOME notice

When a notices is marked as HTML, we need to use block elements
(p, div, li, etc.) or explicit line breaks.
This adds the line breaks where required.

To test:

WELCOME
* Without patch
* Make sure your Koha can send email (set up SMTP server, KohaAdminLibraryAddress)
* Make sure your patron has an email set
* In patron account, use "More > Welcome email"
* Verify all text is on one line
* Apply patch
* reset_all to install new sample notice templates
* Repeat test, verify welcome notice is now nice
Comment 15 Katrin Fischer 2023-11-10 18:29:43 UTC
Offering a low risk patch to fix the most urgent issue :)
Comment 16 Fridolin Somers 2023-11-10 20:46:50 UTC
Pushed to 23.05.x for 23.05.06

Thanks for the rebase ;)
Comment 17 Jonathan Druart 2023-11-11 10:04:27 UTC
Wrong rebase...

-            - "Hello [% borrower.title %] [% borrower.firstname %] [% borrower.surname %]."
-            - ""
-            - "Thank you for joining [% IF Koha.Preference('LibraryName') %][% Koha.Preference('LibraryName') %][% ELSE %]the library[% END %]"
-            - ""
-            - "You can search for all our materials in our <a href='[% Koha.Preference('OPACBaseURL') %]'>catalog</a>."
-            - ""
-            - "Your library card number is [% borrower.cardnumber %]"
-            - ""
-            - "If you have any problems or questions regarding your account, please contact the library."
+            - "Hello [% borrower.title %] [% borrower.firstname %] [% borrower.surname %].<br>"
+            - "<br>"
+            - "Thank you for joining [% IF Koha.Preference('LibraryName') %][% Koha.Preference('LibraryName') %][% ELSE %]the library[% END %]<br>"
+            - "<br>"
+            - "You can search for all our materials in our <a href='[% Koha.Preference('OPACBaseURL') %]'>catalog</a>.<br>"
+            - "<br>"
+            - "Your library card number is [% borrower.cardnumber %]<br>"
+            - "<br>"
+             - "If you have any problems or questions regarding your account, please contact the library."

Extra space at last line.
Comment 18 Jonathan Druart 2023-11-11 10:04:54 UTC
Created attachment 158833 [details] [review]
Bug 35187: [23.05] (follow-up) Fix missing line breaks in WELCOME notice

yaml was broken because of extra space
Comment 19 Jonathan Druart 2023-11-11 10:05:25 UTC
(In reply to Jonathan Druart from comment #18)
> Created attachment 158833 [details] [review] [review]
> Bug 35187: [23.05] (follow-up) Fix missing line breaks in WELCOME notice
> 
> yaml was broken because of extra space

Pushed to 23.05.x
Comment 20 Katrin Fischer 2023-11-11 10:47:10 UTC
Ugh. Sorry.
Comment 21 Fridolin Somers 2023-11-13 16:37:17 UTC
Great thanks
Comment 22 Pedro Amorim 2023-11-14 14:50:20 UTC
Nice work everyone!

Pushed to 22.11.x for next release