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.
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...)