Bug 25379 - HTML in circulation notes doesn't show correctly on checkin
Summary: HTML in circulation notes doesn't show correctly on checkin
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Katrin Fischer
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-05-05 14:42 UTC by Kelly McElligott
Modified: 2023-06-15 19:00 UTC (History)
8 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:
23.05.00,22.11.06


Attachments
on returns.pl page (39.96 KB, image/png)
2020-05-05 14:42 UTC, Kelly McElligott
Details
Bug 25379: Add raw filter to circulation note on checkin page (2.10 KB, patch)
2023-03-17 21:40 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25379: Add raw filter to circulation note on checkin page (2.14 KB, patch)
2023-03-21 23:02 UTC, David Nind
Details | Diff | Splinter Review
Bug 25379: Add raw filter to circulation note on checkin page (2.20 KB, patch)
2023-04-28 16:08 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25379: (QA follow-up) Adjust other occurrences and add line break (5.54 KB, patch)
2023-04-28 16:08 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kelly McElligott 2020-05-05 14:42:49 UTC
Created attachment 104375 [details]
on returns.pl page

With the introduction of this bug: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23409. ( Show circulation note and OPAC note with line feeds ), the ability to add HTML was included.  However, when a patron note with HTML is seen during circulation (check-in)- the HTML tags are visible.
Comment 1 Lucas Gass 2020-05-05 16:35:09 UTC
This uses the $raw filter for the circulation note and HTML can be added since it is input from the staff side.

As for the opacnote, I think this filter needs to remain HTML as OPAC users should not be able to add their own HTML.
Comment 2 Katrin Fischer 2020-05-05 20:04:33 UTC
Hi Lucas, but the patrons can't change/set these notes - only the library staff can. They are only shown in OPAC.
Comment 3 Lucas Gass 2020-05-05 20:05:48 UTC
Cait,

In that case it would be OK to change those filters from HTML to $raw?
Comment 4 Katrin Fischer 2020-05-05 20:06:48 UTC
I think we have done in other places like the OPAC item notes - so it might be ok.
Comment 5 Katrin Fischer 2023-03-17 21:40:46 UTC
Created attachment 148387 [details] [review]
Bug 25379: Add raw filter to circulation note on checkin page

This is a consistency patch: we already allow adding HTML tags
to the patron circulation note and the contents display
correctly in the patron account and in the patron search results
in the staff interface. But the HTML tags are printed on the
checkins page.

To test:
* Add a circulation note including HTML tags to a patron
  Example: <h1>This patron is great!</h1>
* Check an item out to this patron
* Verify that the note displays formatted in patron search results
* Verify that the note displays formatted in the patron record
* Return the item
* Verify the note shows the printed tags
* Apply patch
* Check item out again and check it in again
* Verify the note now displays correctly in list of checkins
Comment 6 David Nind 2023-03-21 23:02:23 UTC
Created attachment 148496 [details] [review]
Bug 25379: Add raw filter to circulation note on checkin page

This is a consistency patch: we already allow adding HTML tags
to the patron circulation note and the contents display
correctly in the patron account and in the patron search results
in the staff interface. But the HTML tags are printed on the
checkins page.

To test:
* Add a circulation note including HTML tags to a patron
  Example: <h1>This patron is great!</h1>
* Check an item out to this patron
* Verify that the note displays formatted in patron search results
* Verify that the note displays formatted in the patron record
* Return the item
* Verify the note shows the printed tags
* Apply patch
* Check item out again and check it in again
* Verify the note now displays correctly in list of checkins

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 Jonathan Druart 2023-04-25 19:29:07 UTC
Shouldn't we use html_linebreak as well?
We do in moremember:
[% patron.borrowernotes | $raw | html_line_break %]

Also, why are we only dealing with the checkin page? There are other places where it's displayed escaped (`git grep 'borrowernotes | html'`)
Comment 8 Nick Clemens 2023-04-28 16:08:55 UTC
Created attachment 150395 [details] [review]
Bug 25379: Add raw filter to circulation note on checkin page

This is a consistency patch: we already allow adding HTML tags
to the patron circulation note and the contents display
correctly in the patron account and in the patron search results
in the staff interface. But the HTML tags are printed on the
checkins page.

To test:
* Add a circulation note including HTML tags to a patron
  Example: <h1>This patron is great!</h1>
* Check an item out to this patron
* Verify that the note displays formatted in patron search results
* Verify that the note displays formatted in the patron record
* Return the item
* Verify the note shows the printed tags
* Apply patch
* Check item out again and check it in again
* Verify the note now displays correctly in list of checkins

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 9 Nick Clemens 2023-04-28 16:08:57 UTC
Created attachment 150396 [details] [review]
Bug 25379: (QA follow-up) Adjust other occurrences and add line break

This updates the other occurrences and adds html_line_break to all
except the edit page - it seems html or raw doesn't affect the value of the textarea,
but moved to raw for consistency

To test:
1 - Add an HTML note to a patron, make sure to add a line break:
<h1>Testing</h1>
<h2>Newline</h2>
2 - Checkout to patron and return
3 - Confirm note shows correctly
4 - Add patron to a list
5 - Confirm note displays correctly
6 - Edit patron, confirm note field is correct
7 - Batch modify borrower
8 - Confirm note displays correctly in batch list

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Nick Clemens 2023-04-28 16:09:18 UTC
Minor followups, RM can veirfy, PQA
Comment 11 Katrin Fischer 2023-04-28 17:53:44 UTC
Thanks, Nick! 

I am not sure really about allowing HTML and then also meddling with line breaks. It creates a somewhat weird combination.

html_line_break
This filter replaces any newlines with <br> HTML tags, thus preserving the line breaks of the original text in the HTML output. 

So you can do now without <br>, but if you add any line breaks just for making the HTML more readable, they will also all create a line break. Should we not do either/or?
Comment 12 Nick Clemens 2023-04-28 18:10:34 UTC
(In reply to Katrin Fischer from comment #11)
>if you add any line breaks just for
> making the HTML more readable, they will also all create a line break.
> Should we not do either/or?

We had calls that already did this - if we want either or, we need a flag to determine whether the field is HTML or not, otherwise we need to test with regex?

I don't think the additional breaks for HTML cause a problem, but I'll follow your viewpoint. Maybe a new bug - so we can make a decision and be consistent on all the calls?
Comment 13 Katrin Fischer 2023-05-01 13:08:00 UTC
(In reply to Nick Clemens from comment #12)
> (In reply to Katrin Fischer from comment #11)
> >if you add any line breaks just for
> > making the HTML more readable, they will also all create a line break.
> > Should we not do either/or?
> 
> We had calls that already did this - if we want either or, we need a flag to
> determine whether the field is HTML or not, otherwise we need to test with
> regex?
> 
> I don't think the additional breaks for HTML cause a problem, but I'll
> follow your viewpoint. Maybe a new bug - so we can make a decision and be
> consistent on all the calls?

I think mostly people will probably use bold, italic etc and not do whole paragraphs here, so we are probably OK with the line breaks. You are right, we should discuss separately when needed, but I feel we can wait and see if it actually turns out to be an issue.
Comment 14 Tomás Cohen Arazi 2023-05-09 14:47:42 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 15 Matt Blenkinsop 2023-05-15 12:21:12 UTC
Nice work everyone!

Pushed to stable for 22.11.x