Bug 21947

Summary: Filtering order generates html in notes
Product: Koha Reporter: Mark Tompsett <mtompset>
Component: OPACAssignee: Mark Tompsett <mtompset>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, lucas, martin.renvoize, nick
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23412
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 13618    
Bug Blocks:    
Attachments: Bug 21947: Swap the filter order
Bug 21947: Address other misordered FILTER lines
Bug 21947: Swap the filter order
Bug 21947: Address other misordered FILTER lines
Bug 21947: Swap the filter order
Bug 21947: Address other misordered FILTER lines

Description Mark Tompsett 2018-12-04 18:57:35 UTC
Bug 13618 added a | html to line 717 of opac-details.tt but this generates strange <br> output in the 'Title notes' tab:

this<br /> <hr><br /> <br /> is<br /> <hr><br /> <br /> a<br /> <hr><br /> <br /> test

from blank lines entered into a 500$a field
--- BEGIN ---
this
<hr>

is
<hr>

a
<hr>

test
--- END ---

While HTML (<hr>) should not be in a note, this is used as sample data to also check the blank line to <br>-ish tags, as shown above.
Comment 1 Mark Tompsett 2018-12-04 19:11:28 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2018-12-04 19:13:03 UTC
Now if you have a 500$a of:
--- BEGIN ---
this

is

a

test
--- END ---
you should see
--- BEGIN ---
this

is

a

test
--- END ---
at the start of the Title notes.
Comment 3 Jonathan Druart 2018-12-04 19:29:19 UTC
What about other occurrences of `git grep "FILTER html_line_break | html"`?
Comment 4 Mark Tompsett 2018-12-05 03:50:25 UTC Comment hidden (obsolete)
Comment 5 Mark Tompsett 2018-12-05 03:51:55 UTC
(In reply to Jonathan Druart from comment #3)
> What about other occurrences of `git grep "FILTER html_line_break | html"`?

I've attached a patch which should address them all. However, given that I never knew how to trigger them. A test plan is left as an exercise for the reader.
Comment 6 Mark Tompsett 2018-12-05 12:34:58 UTC
(In reply to M. Tompsett from comment #0)
> While HTML (<hr>) should not be in a note, ...

It should be noted that David Cook corrected this poor assumption by pointing out 887: https://www.loc.gov/marc/bibliographic/bd887.html

But this patch doesn't touch anything 887-wise, as far as I know.
However, it would be a good thing to check for in another bug.
Comment 7 Owen Leonard 2018-12-05 13:58:48 UTC
Created attachment 82861 [details] [review]
Bug 21947: Swap the filter order

TEST PLAN
----------
1) start your kohadevbox
2) cd kohaclone
3) git checkout -b bug_21947 origin/master
4) git bz apply 21947
5) reset_all
6) log in to staff client and add a 500$a with lots
   of blank lines between strings with some HTML.
7) look at the opac record Title notes tab.
   -- does it have <br>-mess? Should not.
8) repeat steps 5-7 on master, and you'll see
   a <br>-mess.
9) run qa test tools

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 8 Owen Leonard 2018-12-05 13:58:51 UTC
Created attachment 82862 [details] [review]
Bug 21947: Address other misordered FILTER lines

As per comment #3, this patch changes the order for all
the filters found with the recommended git grep.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 9 Jonathan Druart 2018-12-06 16:38:13 UTC
Created attachment 82921 [details] [review]
Bug 21947: Swap the filter order

TEST PLAN
----------
1) start your kohadevbox
2) cd kohaclone
3) git checkout -b bug_21947 origin/master
4) git bz apply 21947
5) reset_all
6) log in to staff client and add a 500$a with lots
   of blank lines between strings with some HTML.
7) look at the opac record Title notes tab.
   -- does it have <br>-mess? Should not.
8) repeat steps 5-7 on master, and you'll see
   a <br>-mess.
9) run qa test tools

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Jonathan Druart 2018-12-06 16:38:18 UTC
Created attachment 82922 [details] [review]
Bug 21947: Address other misordered FILTER lines

As per comment #3, this patch changes the order for all
the filters found with the recommended git grep.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Nick Clemens 2018-12-11 20:59:16 UTC
Awesome work all!

Pushed to master for 19.05
Comment 12 Martin Renvoize 2018-12-12 11:09:14 UTC
Pushed to 18.11.x for 18.11.01
Comment 13 Lucas Gass 2018-12-24 15:27:49 UTC
depends on 13618, not backporting to 18.05