The regex in gather_print_notices.pl is inserting <br /> tags at the end of every line when HTML template is used to create the letter
Created attachment 155196 [details] [review] Bug 34704: Fix regex This patch amends a regex check to stop <br /> tags from being added to every line of html template Test plan: 1) This is easy to see using a regex checking tool. In your browser go to regex101.com 2) In the "Test string", copy and paste the text from the comment in the bug 3) In the regular expression field, enter 's/\n/<br \/>/g' 4) Observe that the output has a <br /> tag inserted after every html element and it is now one line of text 5) Replace the regular expression with '^\s*\n' 6) The output should now be formatted correctly with no <br /> tags
Text to test with: <ul style="list-style-type: none; padding: 0px; margin-left: 40px" > <li><<borrowers.title>> <<borrowers.firstname>> <<borrowers.surname>></li> <li><<borrowers.address>></li> <li><<borrowers.city>></li> <li><<borrowers.state>></li> <li><<borrowers.zipcode>></li> </ul> <p><<today>></p> <table style="width:20%"> <tr> <td style="width:20%"><b>Library:</b></td> <td><<branches.branchname>></td> </tr> <tr> <td style="width:20%"><b>Telephone:</b></td> <td><<branches.branchphone>></td> </tr> <tr> <td style="width:20%"><b>Email:</b></td> <td><<branches.branchemail>></td> </tr> </table> <p>Dear <<borrowers.firstname>> <<borrowers.surname>></p> <p>Membership number: <<borrowers.cardnumber>></p> <p>HOLD AVAILABLE FOR PICKUP</p> <p>Your requested item is now available for collection from the following library:</p> <p><<branches.branchname>></p> <p><<branches.branchaddress1>></p> <p><<branches.branchaddress2>></p> <p><<branches.branchcity>></p> <p><<branches.branchzip>></p> <p>AUTHOR: <<biblio.author>></p> <p>TITLE: <<biblio.title>></p> <p>BARCODE: <<items.barcode>></p> <p>Please collect within 10 days from the date of this letter and don't forget to bring your library card with you.</p> <p>Be aware that opening hours vary at different library branches so if in doubt double check the times before you visit.</p> <p>Should you no longer require this item please contact us so we can make it available for another customer.</p> <p>Thank you</p>
Created attachment 155503 [details] [review] Bug 34704: Fix regex This patch amends a regex check to stop <br /> tags from being added to every line of html template Test plan: 1) This is easy to see using a regex checking tool. In your browser go to regex101.com 2) In the "Test string", copy and paste the text from the comment in the bug 3) In the regular expression field, enter 's/\n/<br \/>/g' 4) Observe that the output has a <br /> tag inserted after every html element and it is now one line of text 5) Replace the regular expression with '^\s*\n' 6) The output should now be formatted correctly with no <br /> tags Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Simple, but very nice quality of life improvement. I have been wrapping all of my HTML notices in [% filter COLLAPSE %] to avoid this.
Thanks Lucas.. I wasn't aware if the Collapse filter, but I think this is a great fix to have and it makes me happy to know it wasn't just us running into it.
I don't think this is correct. You will still need to add br tags for non HTML notice templates, or it will be on a single line :)
And please write a meaningful commit message.
And please write a meaningful commit *title*.
(In reply to Jonathan Druart from comment #6) > I don't think this is correct. You will still need to add br tags for non > HTML notice templates, or it will be on a single line :) Isn't this only when the html flag is passed? if ( $html ) {
(In reply to Lucas Gass from comment #9) > (In reply to Jonathan Druart from comment #6) > > I don't think this is correct. You will still need to add br tags for non > > HTML notice templates, or it will be on a single line :) > > Isn't this only when the html flag is passed? > > if ( $html ) { I have not tested the patch (what I should), but reading the diff it seems that the following use case will be problematic: Define a notice template that is not using HTML tags (so is_html=0) Run the print_notices script with the --html flag => All will be one line I guess that's why we had this replacement. If you think I am wrong, please switch back to SO and I will have a look at it again next week.
You're correct with your example but I've added bug 34728 to the "See also" section - in that bug we wrap any notice template where is_html=0 in a <pre> tag to preserve the formatting. That should fix the scenario you mention (I think) - the <br /> tag will only be inserted where there are empty lines rather than on every line, the \n will be preserved by the <pre> tag when the content is rendered. Let me know your thoughts, I'll update the commit title in the meantime
Created attachment 156229 [details] [review] Bug 34704: Stop <br /> tags being entered on every line in html letter templates This patch amends a regex check to stop <br /> tags from being added to every line of html template Test plan: 1) This is easy to see using a regex checking tool. In your browser go to regex101.com 2) In the "Test string", copy and paste the text from the comment in the bug 3) In the regular expression field, enter 's/\n/<br \/>/g' 4) Observe that the output has a <br /> tag inserted after every html element and it is now one line of text 5) Replace the regular expression with '^\s*\n' 6) The output should now be formatted correctly with no <br /> tags
Created attachment 156230 [details] [review] Bug 34704: Stop <br /> tags being entered on every line in html letter templates This patch amends a regex check to stop <br /> tags from being added to every line of html template Test plan: 1) This is easy to see using a regex checking tool. In your browser go to regex101.com 2) In the "Test string", copy and paste the text from the comment in the bug 3) In the regular expression field, enter 's/\n/<br \/>/g' 4) Observe that the output has a <br /> tag inserted after every html element and it is now one line of text 5) Replace the regular expression with '^\s*\n' 6) The output should now be formatted correctly with no <br /> tags Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
+ s/^\s*\n/<br \/>/g; Adding the ^ without multiline modifier makes it work on the first line only.
Created attachment 157303 [details] [review] Bug 34704: (QA follow-up): Add multi-line modifier This patch adds a multi line modifier to the regex to catch all lines
Completely missed this - I've added the modifier, good spot
I am still a bit confused by this patch/script. It looks like we always turn an empty line into a <br>. It seems an improvement from previous behavior, but I wonder if it's correct. What I would expect when the html flag is passed: * If the notice template is already HTML, do not make any changes. * If the notice is plain text, wrap in <pre>, do not add any additional line breaks. Why is the regex required at all? It seems to add additional breaks where they might not be needed/intended. Especially with TT sometimes it's nice to add some line breaks separating blocks etc. I thik we can now use the letter_id in message_queue to figure out if the template is HTML if that helps things.
We spent quite a bit of time working through this, and it's in production at all our sites now. I think you need to be clearer about what changes you're expecting, Katrin.. It's not at all clear how we move this on from here.
I thought I was clear: Can you explain why we are adding <br> at all? It feels like this is an unneded change. We either have an HTML template, then we are good. Or we have a plain text template and we are using pre - we are also good. I think it helps your sites, because it reduces unwanted additional breaks, but why add any at all?
I have a question. When we tick the box to make a notice an HTML message ( is_html=1 ) do we expect in the entire template to marked up? In Matt's example that is the case. I ask because we have a lot of templates where this is not the case. For instance someone might add a logo at the top of the notice, <img src="somesorc" width="500" alt="somealt" /> But after that they rest of the notice is NOT marked up. So here is my example template: **** <div id="test">TEST1</div> Our Address Our City Our State Thank you, Library **** With no patch applied I continue to have line breaks honored for the content NOT wrapped in HTML elements. With the patch my non-marked up content looks like this: **** Our Address Our City Our State Thank you, Library **** With no regex, like Cait suggests I get this for my non-marked up content: **** Our Address Our City Our State Thank you, Library ****
So I guess the question is whether or not we should allow for this mix of html/non-html content.
(In reply to Lucas Gass from comment #20) > I have a question. When we tick the box to make a notice an HTML message ( > is_html=1 ) do we expect in the entire template to marked up? In Matt's > example that is the case. Hm, I think we are getting to the bottom of this now. Yes, I think we do for email notices and we should also do that for print notices? If we use HTML and especially with TT mark-up we want the line breaks to be explicit, not implicit. You will want to format a CASE statement or similar on multiple lines to be readable. And it should collapse them as expected from HTML. Doing implicit line breaks might help people who didn't stick with HTML, but it removes control for everyone else and force us to do weird things. Our templates are getting more advanced and complicated all the time with TT, I could also see an IF-statement, that leaves blank lines if the condition is not met. I believe that this behaviour will bite us. > I ask because we have a lot of templates where this is not the case. For > instance someone might add a logo at the top of the notice, <img > src="somesorc" width="500" alt="somealt" /> > > > But after that they rest of the notice is NOT marked up. So here is my > example template: > > **** > <div id="test">TEST1</div> > > > Our Address > Our City > Our State > > > Thank you, > > > Library > **** > > > With no patch applied I continue to have line breaks honored for the content > NOT wrapped in HTML elements. > > With the patch my non-marked up content looks like this: > > **** > Our Address Our City Our State > Thank you, > Library > **** > > > With no regex, like Cait suggests I get this for my non-marked up content: > > **** > Our Address Our City Our State Thank you, Library > **** I can see where this whole thing is a problem now when this has worked before :( For consistency: How do these templates behave when looked at in the notices tab or when sent per email?
> For consistency: How do these templates behave when looked at in the notices > tab or when sent per email? Im my tests/examples I used overdue notices ( ODUE ), so what I see in notice tab/email remains consistent with the patch, without the patch, and when I remove the regex line. **** TEST1 Our Address Our City Our State Thank you, Library **** But this is because that content is generated by overdue_notices.pl and is not going through gather_print_notices.pl Question: Could the regex line we are changing in gather_print_notices.pl be something that we make configurable?
OK, I see you're points now. This was meant to be "least invasive", but I do agree it would be nice to drop it entirely and require proper HTML throughout for these notices. The challenge will be to write a DB update that looks at all the existing notices and fixes them for all current html notices to include the breaks. Also, we'd need to update all the existing default notices for install. This is all a much larger job that we were expecting to undertake here.. worthwhile I suppose, but I'm not sure where I'm going to find funding for that extra time.
I think if we're going whole hog here, we likely would want to switch the editor to allow the WYSIWYG option for html input to keep the simplicity option at user disposal.
In my perfect world you'd have WYSIWYG and Codemirror options.
I wanted to check the 'size' of this problem a bit more and did some testing. And it's not as bad as one would think: I tested the HTML notices especially and the results are interesting: 1) Slips: use explicit line breaks When a slip template is printed in the browser, only br and other block elements will cause a line break. I fixed some of our sample notices on bug 35187. 2) E-mail notices: use explicit line breaks When an email is sent in HTML format, only br and other block elements will cause a line break. I fixed the WELCOME notice also on bug 35187. 3) Print notices with gather_print_notices and --html: implicit line breaks ------------ Note that we have a known display bug on the notices tab: bug 30287. It shows implicit line breaks, not reflecting how the actual email will look later. ----------- (In reply to Martin Renvoize from comment #24) > OK, I see you're points now. > > This was meant to be "least invasive", but I do agree it would be nice to > drop it entirely and require proper HTML throughout for these notices. > > The challenge will be to write a DB update that looks at all the existing > notices and fixes them for all current html notices to include the breaks. We have never used the --html option on this job so I can't tell about how widely it would be used. I am not sure it's worth messing with the existing notices. * If you have a working email template that is used as a fallback for print, it will definitely add unnecessary/additional line breaks. * So only explicit print layouts that use HTML could be affected negatively. I'd like to avoid a system preference for this inconsistent behavior... but could a command line switch for 'implicit_line_breaks' help some? > Also, we'd need to update all the existing default notices for install. It's not as big an issue as one would think. I already did it: I fixed all HTML notices in the 3 bugs dependent on this one: 35185, 35186, 35187 With these 3 bugs I believe we can consider the sample notices fixed and improved. It helps that most our notices are still plain text (is_html: 0) > This is all a much larger job that we were expecting to undertake here.. > worthwhile I suppose, but I'm not sure where I'm going to find funding for > that extra time. We can have this go in as is, but I believe removing would be better instead of changing the output somewhat arbitrarily. Maybe we should just "bite the bullet" here?
I spent quite a bit of time on this one and on fixing our sample notices. It would be nice to keep the conversation going.
Thx for the help on the bugs - what is our conclusion here? Do we remove the regex?
I've asked Matt to take a look and comment as he was a little closer to the original request. I have a feeling we may need to further update the notices that were causing our customers problems to make them work without the regex.
I also vaguely wonder about br Vs p in the template update context.. but that's just a random thought after the fact
(In reply to Martin Renvoize from comment #31) > I also vaguely wonder about br Vs p in the template update context.. but > that's just a random thought after the fact I don't think there is a good way to automatically update the notices. We cannot tell if something was intentionally left outside an HTML tag or not. There are also: li, ul, ol, p, br, table, th, tr, td... People using this script now, might have left off extras, others might have added them...
My point wasn't to add a database update here.. more that we perhaps need to really clearly state that the processing for print notices has changed and that any html notices used for print will need adapting by the end user.
I just wrote bug 35279 to make this simpler to test ;).
Created attachment 158602 [details] [review] Bug 34704: Stop <br /> tags being entered on every line in html letter templates This patch amends a regex check to stop <br /> tags from being added to every line of html template Test plan: 1) This is easy to see using a regex checking tool. In your browser go to regex101.com 2) In the "Test string", copy and paste the text from the comment in the bug 3) In the regular expression field, enter 's/\n/<br \/>/g' 4) Observe that the output has a <br /> tag inserted after every html element and it is now one line of text 5) Replace the regular expression with '^\s*\n' 6) The output should now be formatted correctly with no <br /> tags Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Created attachment 158603 [details] [review] Bug 34704: (QA follow-up): Add multi-line modifier This patch adds a multi line modifier to the regex to catch all lines
Created attachment 158604 [details] [review] Bug 34704: Replace regex with wrap for non-html based notices If a notice is marked as HTML in the template editor we don't want to add any additional HTML markup. If a notice is not marked as HTML in the template editor, then it is assumed the notice is plaintext and thus includes formatting and so for an html output the content should be wrapped in <pre>.
Follow-up added that drops the regex entirely.. as requested.. and instead checks for content type of the notice generated itself and treats it appropriately by either leaving it as is (for html formatted notices) or wraps the content in a <pre> block for pre-formatted plaintext.
Back to you Katrin.
WARN misc/cronjobs/gather_print_notices.pl WARN tidiness The file is less tidy than before (bad/messy lines before: 92, now: 93) Fixed in amended last patch.
Since we changed course here - should we squash and adapt the test plan in first commit message?
We already add the <pre> somewhere else, because now we have a double up: <pre><pre>Centerville^M ... Copy: </pre></pre> I tested this way: * Make sure your patron doesn't have an email address * Make sure to check the hold filled notice in messaging preferences * Place a old on any item * Checkin the item * Confirm hold * Verify notices tab shows the Hold (print) notice * ./misc/cronjobs/gather_print_notices.pl ./ --html * Verified the generated file
It comes from here: my $template = C4::Templates::gettemplate( 'batch/print-notices.tt', 'intranet', CGI->new ); And then the template takes care of it: <div class="dialog message"> [% IF ( message.is_html ) %] [% message.content | $raw %] [% ELSE %] <pre>[% message.content | $raw %]</pre> [% END %] </div>
Created attachment 158637 [details] [review] Bug 34704: [Alternate] Remove regex for adding additional line breaks to print templates This makes sure that we don't change the line breaking for either HTML nor plain text print notices. For plain text, the <pre> is used to keep the line breaks as defined in the notices template. For HTML we require on the line breaks added by block elements in the HTML notice, not adding any additional ones like before. This gives the full control of the formatting to the notice editor. To test: * Make sure your patron doesn't have an email address * Make sure to check the hold filled notice in messaging preferences * Place a old on any item * Checkin the item * Confirm hold * Verify notices tab shows the Hold (print) notice * ./misc/cronjobs/gather_print_notices.pl ./ --html * Verify the generated file has the HTML output wrapped in pre. * Reformat the HOLD print notice to be HTML by setting the checkbox. Mmake sure to add some empty lines for (they should not print later) and some <br>, <p>, <div> - They should be reflected in the notice later. * Repeat test. * All should be well :) Current patch-complexity: Trivial patch
I thought instead of the 3 that are going to be a little confusing, I'll throw in an alternate - let me know what you think.
(In reply to Katrin Fischer from comment #43) > It comes from here: > > my $template = > C4::Templates::gettemplate( 'batch/print-notices.tt', 'intranet', > CGI->new ); > > > And then the template takes care of it: > > <div class="dialog message"> > [% IF ( message.is_html ) %] > [% message.content | $raw %] > [% ELSE %] > <pre>[% message.content | $raw %]</pre> > [% END %] > </div> Which we added in bug 34728, the counterpart of this but months ago.
Created attachment 158643 [details] [review] Bug 34704: Remove regex for adding additional line breaks to print templates This makes sure that we don't change the line breaking for either HTML nor plain text print notices. For plain text, the <pre> is used to keep the line breaks as defined in the notices template. For HTML we require on the line breaks added by block elements in the HTML notice, not adding any additional ones like before. This gives the full control of the formatting to the notice editor. To test: * Make sure your patron doesn't have an email address * Make sure to check the hold filled notice in messaging preferences * Place a old on any item * Checkin the item * Confirm hold * Verify notices tab shows the Hold (print) notice * ./misc/cronjobs/gather_print_notices.pl ./ --html * Verify the generated file has the HTML output wrapped in pre. * Reformat the HOLD print notice to be HTML by setting the checkbox. Mmake sure to add some empty lines for (they should not print later) and some <br>, <p>, <div> - They should be reflected in the notice later. * Repeat test. * All should be well :) Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Pushed to master for 23.11. Nice work everyone, thanks!
Pushed to 23.05.x for 23.05.06
Nice work everyone! Pushed to 22.11.x for next release