-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: removal of content after problem type tags #479
fix: removal of content after problem type tags #479
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Feedback:
I love it!
Great fix.
I just added a little stylistic comment and after we resolve that discussion I'll approve.
Other Notes on parser improvement situation (no action required):
This is outside of this scope, but something popped into my head for avoiding this kind of problem we keep having. What do you think about regex validation to make sure the XML is in a format we support in the simple editor before opening the simple editor? I mean what we do here is kind of like a blacklist where in certain cases we exit the simple and go into the advanced editor. But we could maybe do something more like a whitelist where we have a very narrow definition of XML structure that we support for the simple editor, and in all other cases we always go to the advanced editor.
Then we could have tests for everything that follows the schema so that we know when we go to the simple editor that it's fully covered by tests.
For example:
<problem>
<multiplechoiceresponse>
<label>What Apple device competed with the portable CD player?</label>
<p>This is valid text</p>
</multiplechoiceresponse>
</problem>
or something like that will be valid for the regex - we'll define problem as allowed tag and then inside problem multiplechoiceresponse as allowed tag and inside that label and p as allowed tags. But everything not exactly fitting into this schema would be flagged and open the advanced editor instead.
So this would then be invalid:
<problem>
<multiplechoiceresponse>
<label>What Apple device competed with the portable CD player?</label>
</multiplechoiceresponse>
<p>This tag does not follow the schema</p>
</problem>
Like basically we make a schema and then test every single possibility for this schema. Even the slightest deviation from the schema (something that's not explicitly covered in our readthedocs for example) will go to the advanced editor.
If we imagine the schema for example as a yaml file for the schema:
problem:
multiplechoiceresponse:
- label
- p
I think this goes somewhat in the direction of something Ray suggested in the past. Do you think something like this makes sense?
There are several interconnected problems with the parser, of course, and this would only tackle a part of it.
@@ -634,14 +670,16 @@ export class OLXParser { | |||
throw new Error('Misc Attributes asscoiated with problem, opening in advanced editor'); | |||
} | |||
|
|||
const problemType = this.getProblemType(); | |||
|
|||
this.checkTextAfterProblemTypeTag(problemType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When reading the code, it's not immediately obvious to me that this function can throw an error that errors out of the running code. What do you think about making it explicit, like in the line above with throw new Error('Misc Attributes asscoiated with problem, opening in advanced editor');
?
The idea would be to put the throw Error
line in this function directly to make it visible instead of "hiding" it in a function that gets called.
For example, since the error message here is the same whether you have invalid keys or invalid tags, the checkTextAfterProblemTypeTag
can just be something like hasXMLAfterProblemTypeTag
that returns true or false, and then here you could do
if (this.hasXMLAfterProblemTypeTag(problemType)) {
throw new Error(`OLX found after the ${problemType} tags, opening in advanced editor`);
}
. The benefit of doing it in a similar way to what I suggested is that any reader of the code can immediately see that an error is thrown, which makes the code clearer and the logic less confusing in my opinion.
@jesperhodge I think your proposed solution makes sense. I think that is what we were trying to do initially, but as new visual editor exceptions appears, we re-oriented the code to look for the exceptions. |
JIRA Ticket: TNL-10674
This PR fixes the intermittent problem where there is OLX after the problem type tags, (
multiplechoiceresponse
,numericalresponse
, etc.) for simple problems. Currently the parser will some times put the OLX inside the question section; other times the parser will remove the OLX. When the OLX is removed, the user switches to the advanced editor expecting to see it, but it is not present. This PR updates the parser to check if there is OLX content after the problem type tags, and if found, redirect to the advanced editor. This change removes the possibility that previous OLX will be blindly removed and that content will be rearranged blindly.Testing
</problem>
) add<p>content after problem type tags</p>
<p>content after problem type tags</p>