Description
Martin Renvoize (ashimema)
2025-06-05 09:13:48 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 *** Bug 38499 has been marked as a duplicate of this bug. *** *** Bug 40077 has been marked as a duplicate of this bug. *** *** Bug 40078 has been marked as a duplicate of this bug. *** 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> 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. 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? 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. (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-). 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...) Created attachment 183534 [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> Created attachment 183535 [details] [review] Bug 40079: (follow-up) Add dl, dt, dd tags to notes profile scrubber Adds support for HTML description list elements (dl, dt, dd) to the 'note' profile in the HTML scrubber configuration. This allows librarians to use description lists in note fields for better structured content presentation. Created attachment 183536 [details] [review] Bug 40079: (follow-up) Update unit test Created attachment 183537 [details] [review] Bug 40079: (follow-up) Improve test coverage and fix undef handling This patch modernizes the Scrubber.t unit test with comprehensive coverage following Koha best practices: - Uses subtest structure for logical grouping - Tests all scrubber types (default, comment, note) - Includes security testing for XSS prevention - Covers edge cases and error handling - Tests new list elements added in previous commits Also fixes C4::Scrubber to handle undef parameters cleanly by treating them as 'default' type instead of generating warnings. Test plan: 1. Run prove t/Scrubber.t 2. Verify all tests pass without warnings 3. Confirm comprehensive coverage of module functionality Ooops.. seems I submitted the patch without ol.. my mistake, could have swarn I'd added it. Anyway, thanks or the testing and spotting that. I've corrected that inline and I've also added a follow-up to add the definitions list options.. Finally I've fixed the existing tests in one follow-up and then completely reworked them in a second follow-up which drastically increases our coverage and spots an edge case we hadn't found before which I've also fixed inline. Thanks Martin! I can confirm that ordered list and description list tags now work as well! David Created attachment 183583 [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> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 183584 [details] [review] Bug 40079: (follow-up) Add dl, dt, dd tags to notes profile scrubber Adds support for HTML description list elements (dl, dt, dd) to the 'note' profile in the HTML scrubber configuration. This allows librarians to use description lists in note fields for better structured content presentation. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 183585 [details] [review] Bug 40079: (follow-up) Update unit test Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 183586 [details] [review] Bug 40079: (follow-up) Improve test coverage and fix undef handling This patch modernizes the Scrubber.t unit test with comprehensive coverage following Koha best practices: - Uses subtest structure for logical grouping - Tests all scrubber types (default, comment, note) - Includes security testing for XSS prevention - Covers edge cases and error handling - Tests new list elements added in previous commits Also fixes C4::Scrubber to handle undef parameters cleanly by treating them as 'default' type instead of generating warnings. Test plan: 1. Run prove t/Scrubber.t 2. Verify all tests pass without warnings 3. Confirm comprehensive coverage of module functionality Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Comment on attachment 183586 [details] [review] Bug 40079: (follow-up) Improve test coverage and fix undef handling Review of attachment 183586 [details] [review]: ----------------------------------------------------------------- ::: C4/Scrubber.pm @@ +35,4 @@ > sub new { > shift; # ignore our class we are wrapper > my $type = (@_) ? shift : 'default'; > + $type = 'default' if !defined $type; Nice one. I have been thinking this would be a good addition, so glad to see it. Nice work everyone! Pushed to main for 25.11 Nice work everyone! Pushed to 25.05.x Pushed to 24.11.x for 24.11.08 |