Fully strip block XML before TTSifying #16718
Conversation
| @@ -100,6 +100,18 @@ def self.tts_upload_to_s3(text, filename) | |||
| end | |||
| end | |||
|
|
|||
| def self.sanitize(text) | |||
There was a problem hiding this comment.
Perhaps sanitize_and_render? Otherwise, I would expect sanitize to accept and return a string, nothing more.
There was a problem hiding this comment.
I might be confused; accepting and returning a string is precisely what it does
There was a problem hiding this comment.
Nope, I didn't realize what TTSSafeRenderer.render did.
| test 'sanitize html' do | ||
| assert_equal @level_with_raw_html.tts_markdown_instructions_text, "This should have no excess formatting \n" | ||
| test 'sanitize html and xml' do | ||
| assert_equal @level_with_raw_html.tts_markdown_instructions_text, "This should have no excess formatting\n" |
There was a problem hiding this comment.
The expected and actual values should be flipped (for better error output on failure).
(also elsewhere in the file, feel welcome to do as a separate PR, or leave for me to do)
| test 'sanitize html and xml' do | ||
| assert_equal @level_with_raw_html.tts_markdown_instructions_text, "This should have no excess formatting\n" | ||
| assert_equal @level_with_block_html.tts_markdown_instructions_text, "This block should get stripped:\n" | ||
| assert_equal @level_with_xml.tts_markdown_instructions_text, "This block should get stripped:\n" |
There was a problem hiding this comment.
Should I be surprised that there is only one \n rather than two in the expected output?
There was a problem hiding this comment.
Depends on how familiar you are with Redcarpet 😄
Two newlines in the input are necessary to separate paragraphs and to treat the xml as its own "block", as is standard for markdown input.
Newlines within the content itself are irrelevant, as is standard for markdown output, but for some reason one is always added to the end.
| # | ||
| # to avoid this, we simply and aggressively strip out any xml before passing | ||
| # the rest to redcarpet, despite the risk of invoking the wrath of Zalgo. | ||
| TTSSafeRenderer.render(text.gsub(/<xml>.*<\/xml>/m, '')) |
There was a problem hiding this comment.
Is there any concern of there being multiple </xml> strings in text? If so, does the /m invoke the desired behavior?
There was a problem hiding this comment.
Oh, good catch. There is indeed and I should make this regex less greedy.
The /m doesn't influence that side effect, though; it's just there to make sure the regex with deal with newlines
Hamms
force-pushed
the
blockly-tts-no-xml
branch
from
a9d088a
to
6d016f5
Compare
|
Added a Loofah implementation rather than regex because why on earth was I trying to parse XML using regex. PTAL |
|
LGTM (Ignore the PTAL [now deleted]). |
Hamms
force-pushed
the
blockly-tts-no-xml
branch
from
d541846
to
8a00df2
Compare
No description provided.