Bug 40079 - C4::Scrubber "note" profile should allow for list (ul/ol and li) HTML tags
Summary: C4::Scrubber "note" profile should allow for list (ul/ol and li) HTML tags
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal
Assignee: Martin Renvoize (ashimema)
QA Contact: Testopia
URL:
Keywords:
: 38499 40077 40078 (view as bug list)
Depends on: 36328
Blocks:
  Show dependency treegraph
 
Reported: 2025-06-05 09:13 UTC by Martin Renvoize (ashimema)
Modified: 2025-06-06 01:43 UTC (History)
3 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This adds unordered lists to the HTML tags that are allowed in notes fields (for example, course reserves staff and public notes).
Version(s) released in:
Circulation function:


Attachments
Bug 40079: Add ul/ol and li to allowed tags in notes profile (2.06 KB, patch)
2025-06-05 10:07 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40079: Add ul/ol and li to allowed tags in notes profile (2.10 KB, patch)
2025-06-05 22:04 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize (ashimema) 2025-06-05 09:13:48 UTC
We added support for some additional tags support in the 'note' type scrubbed fields, but we didn't expose lists.

I see no security risk with allowing those tags, please correct me if I'm incorrect there, so I think we should allow these too.
Comment 1 Martin Renvoize (ashimema) 2025-06-05 10:07:22 UTC
Created attachment 182985 [details] [review]
Bug 40079: Add ul/ol and li to allowed tags in notes profile

This patch add the unordered list, ordered list and list item html tags
to the list of allowed tags in C4::Scrubber.

Test plan:
1) Add a course reserve and include the following in the public note
   field:
   <ul>
     <li>First item</li>
     <li>Second item</li>
   </ul>
2) View the course details page and note "First item Second item" all
   appears on the same line (the ul and li tags have been stripped)
3) View the course reserves page and note the same issue in the public
   notes field of the table display
4) Apply this patch
5) Repeat 2 and 3 and note you now have a bullet pointed list displayed
   in both places
6) The display of the note on the details page may look mis-aligned..
   that will be dealt with on it's own bug
Comment 2 Martin Renvoize (ashimema) 2025-06-05 10:27:41 UTC
*** Bug 38499 has been marked as a duplicate of this bug. ***
Comment 3 Phil Ringnalda 2025-06-05 19:24:22 UTC
*** Bug 40077 has been marked as a duplicate of this bug. ***
Comment 4 Phil Ringnalda 2025-06-05 19:24:45 UTC
*** Bug 40078 has been marked as a duplicate of this bug. ***
Comment 5 David Nind 2025-06-05 22:04:19 UTC
Created attachment 183014 [details] [review]
Bug 40079: Add ul/ol and li to allowed tags in notes profile

This patch add the unordered list, ordered list and list item html tags
to the list of allowed tags in C4::Scrubber.

Test plan:
1) Add a course reserve and include the following in the public note
   field:
   <ul>
     <li>First item</li>
     <li>Second item</li>
   </ul>
2) View the course details page and note "First item Second item" all
   appears on the same line (the ul and li tags have been stripped)
3) View the course reserves page and note the same issue in the public
   notes field of the table display
4) Apply this patch
5) Repeat 2 and 3 and note you now have a bullet pointed list displayed
   in both places
6) The display of the note on the details page may look mis-aligned..
   that will be dealt with on it's own bug

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2025-06-05 22:14:04 UTC
I've signed off, as things work as per the test plan.

However, I noticed that the description for the bug says it also allows ordered lists (<ol>) and description list (<dl>, <dt>, and <dd>).

Only unordered lists work for me - looking at the patch, only ul and li were added.
Comment 7 David Cook 2025-06-06 01:03:11 UTC
Sounds like a reasonable change to me, but it does seem like we need some clarification.

David: I don't the original description mentioned description lists per se? But I do see the patch says it adds "unordered list, ordered list and list item html tags", but it only adds "ul" and "li" - not "ol".

Martin: Did you intend to add "ol" as well?
Comment 8 David Cook 2025-06-06 01:06:01 UTC
I also like how this patch removes the useless "note" parameter from the constructor/new method (bug 38499). I've wanted that done for a while.

I'm going to update the title of this bug to specify the note profile.
Comment 9 David Nind 2025-06-06 01:28:50 UTC
(In reply to David Cook from comment #7)
> David: I don't the original description mentioned description lists per se?
> But I do see the patch says it adds "unordered list, ordered list and list
> item html tags", but it only adds "ul" and "li" - not "ol".

I meant the description in the commit message 8-).
Comment 10 David Cook 2025-06-06 01:43:04 UTC
Actually, could we get the t/Scrubber.t updated too? It looks like there's an existing test for the 'note' profile, although it's not a brilliant test. 

I'm also happy to do that if you don't want to. (Obviously the C4::Scrubber has turned into a bit of a passion for me...)