Bug 21947 - Filtering order generates html in notes
Summary: Filtering order generates html in notes
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Mark Tompsett
QA Contact: Testopia
URL:
Keywords:
Depends on: 13618
Blocks:
  Show dependency treegraph
 
Reported: 2018-12-04 18:57 UTC by Mark Tompsett
Modified: 2020-06-04 20:32 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 21947: Swap the filter order (1.58 KB, patch)
2018-12-04 19:11 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 21947: Address other misordered FILTER lines (7.84 KB, patch)
2018-12-05 03:50 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 21947: Swap the filter order (1.62 KB, patch)
2018-12-05 13:58 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 21947: Address other misordered FILTER lines (7.86 KB, patch)
2018-12-05 13:58 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 21947: Swap the filter order (1.70 KB, patch)
2018-12-06 16:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21947: Address other misordered FILTER lines (7.96 KB, patch)
2018-12-06 16:38 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 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 (kidclamp) 2018-12-11 20:59:16 UTC
Awesome work all!

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