Adresses TNL-3096

[mirjamsk]

JIRA ticket: https://openedx.atlassian.net/browse/TNL-3096

[openedx-webhooks]

Thanks for the pull request, @mirjamsk! I've created OSPR-773 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

• supporting documentation
• edx-code email threads
• timeline information ('this must be merged by XX date', and why that is)
• partner information ('this is a course on edx.org')
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the Github pull request interface. As a reminder, our process documentation is here.

We can't start reviewing your pull request until you've submitted a signed contributor agreement or indicated your institutional affiliation and added yourself to the AUTHORS file. Please see the CONTRIBUTING file for more information.

[sarina]

Hi @mirjamsk and welcome!

We'll need to know your institutional affiliation or have a contributor's agreement on file for you, as the bot mentions.

@Colin-Fredericks filed this bug; perhaps youd like to be the first reviewer, Colin?

[mirjamsk]

hi @sarina, i've just sent the signed contributor's agreement to legal@edx.org

[sarina]

Please add some comments here to explain what you're doing and why.

[sarina]

This still doesn't work for me; I'm using the example from the TNL-3096 description.

[Colin-Fredericks]

It seems like the change in the code doesn't address the issue - whatever was stripping out the HTML, it's still there somewhere else.

[sarina]

Mirjam: You should write a test case that fails on the current version of
the code, perhaps using the example code from the ticket.

On Tue, Aug 25, 2015 at 2:24 PM, Colin-Fredericks notifications@github.com
wrote:

It seems like the change in the code doesn't address the issue - whatever
was stripping out the HTML, it's still there somewhere else.

—
Reply to this email directly or view it on GitHub
https://github.com/edx/edx-platform/pull/9464#issuecomment-134693109.

[mirjamsk]

@sarina, @Colin-Fredericks i committed and pushed the changes which address the stated issues

[sarina]

jenkins run all

[sarina]

@Colin-Fredericks could you take a pass at reviewing this? I'm out of the office for most of today.

[Colin-Fredericks]

I'll see what I can do.

[Colin-Fredericks]

I think you want to name this "test_get_inner...."

[Colin-Fredericks]

Ok - have tried this out on my local devstack.

Good news: It mostly works! I've only checked MC questions so far, but it's encouraging.

Only slightly bad news: If your hint starts with a tag, it doesn't work. For instance, if your hint is entirely an image, that doesn't work, but it definitely should. Adding a single space doesn't make it work, but adding some text beforehand does. Adding text after the tag doesn't work.

Strangely, that's only true for the check hints (the inline, answer-specific ones) - not for the demand hints (the ones you get from clicking the "Hint" button). Demand hints can be just an image tag and it works just fine.

Actually bad news: Your changes are causing a test failure in test_hint_functionality.py, which you definitely want to resolve.

[mirjamsk]

@Colin-Fredericks thank you for the feedback.
The tests in test_hint_functionality.py were failing due to difference in encoding of symbols.
e.g. the new function encodes the greek symbol Ω as the html variant Ω
i've changed the tests to take this into account.
i'll take a look at the img situation now

[sarina]

if your hint is entirely an image, that doesn't work, but it definitely should

I disagree with this; this would not be accessible.

[sarina]

To understand the output of your builds, see https://openedx.atlassian.net/wiki/display/TE/Jenkins+Guide

Particularly you have a quality issue which is why your quality build is failing.

[Colin-Fredericks]

All images should have alt text, which a screen reader should be able to get at just fine. I will happily bow to the accessibility team on this one, though.

The issue also comes up for other tags, such as a span, so it's still worth addressing.

[sarina]

Sure, I just don't want to recommend that course teams do this because we
don't have a way of enforcing an alt tag.

On Wed, Aug 26, 2015 at 11:12 AM, Colin-Fredericks <notifications@github.com

wrote:

All images should have alt text, which a screen reader should be able to
get at just fine. I will happily bow to the accessibility team on this one,
though.

The issue also comes up for other tags, such as a span, so it's still
worth addressing.

—
Reply to this email directly or view it on GitHub
https://github.com/edx/edx-platform/pull/9464#issuecomment-135056634.

[mirjamsk]

@Colin-Fredericks changes have been made so it works as expected when adding a tag immediately after the open hint tags

[sarina]

Can you make this clearer as to what this test case is testing?

[sarina]

@mirjamsk I'm 👍 on this once you've responded to my last few comments and squashed your commits.

@Colin-Fredericks could you do a review pass as well (at least making sure this has fully addressed your needs, perhaps also looking at the code a little if you're able?)

[mirjamsk]

comments addressed and commits squashed

[sarina]

@mirjamsk great! Once Colin has given his approval I will merge this.

I appreciate your hard work and fast turn around!

[sarina]

@mhoeber FYI. I don't think this has doc impact because it fixes an existing bug that so far only Colin has reported.
(replaced @lamagnifica with @mhoeber -- Alison Hodges 27 Aug 15)

[mhoeber]

@sarina thanks. doc is ok (never said you couldn't use other tags). noted in release notes list to include when merged

[Colin-Fredericks]

I took another look through the code, then double-checked on my devstack to make sure it would work in certain unexpected cases because I know (from personal experience) that parsing HTML with regex is often counter-indicated. Luckily everything works fine.

Thanks for fixing this so quickly!

👍

[cahrens]

@mirjamsk Since this PR was merged, we have been seeing a flaky acceptance test failure. Would you be able to investigate this?

https://openedx.atlassian.net/browse/TNL-3183

[mirjamsk]

@cahrens this issue has been addressed here https://github.com/edx/edx-platform/pull/9571

[sarina]

Christina I would be grateful if you want to review that pr :)
On Sep 3, 2015 5:42 PM, "mirjamsk" notifications@github.com wrote:

@cahrens https://github.com/cahrens this issue has been addressed here #9571
(comment)
https://github.com/edx/edx-platform/pull/9571#issuecomment-137142080

—
Reply to this email directly or view it on GitHub
https://github.com/edx/edx-platform/pull/9464#issuecomment-137582248.

[cahrens]

@sarina I'm on vacation until the 15th and didn't review the original PR (so I don't have context for the fix).

@mirjamsk thanks so much for the quick fix!