Another attempt at fixing student_ans HTML escaping issues.

[drgrice1]

This is to address issue #1021 and the display issue discovered in #990, that was not quite fixed in #1004. This backs up one step from what was done in #1004. Instead of calling protectHTML on $ans->{student_ans} in the cmp_error method, that is instead called in the cmp_parse method in the case that the $ans->{student_value} is undefined.

With #1004, if student_value was defined, and then a problem author calls Value::Error in a custom answer checker, it results in the student_ans being escaped twice because it is escaped in cmp_parse and then again after the custom answer checker is called. With this approach the student_ans is escaped when cmp_parse is called in either case regardless if if student_value is defined, and then not again later (if student_value is defined and thus the checker is called)

This is also before the format_matrix method is called for array answers. This fixes an issue also caused by escaping those already HTML formatted answers. In that case the student_ans is completely redefined after this from the student_formula. With #1004, the student_ans is first HTML formatted in this case, and then later escaped which results in an incorrect display.

This of course still fixes the issue that #1004 attempted to fix, because the student_value is not defined, and so the protectHTML now called in cmp_parse covers this case.

I am sure that this is not the end of this issue, but is better than both before #1004 and after. I think that something along the lines of @dpvc's suggestion in #990 (comment) will be needed to fully resolve this issue. I suggest that rather than having student_ans_text, student_ans_html, student_ans_latex, and preview_ans_text as suggested there, we use student_ans, preview_latex_string, and preview_text_string. student_ans would always remain a text answer, and never be HTML formatted, preview_ans_latex would stay the same and be LaTeX, and preview_text_string would always be HTML. The naming is not the best since preview_text_string is actually HTML, but it is the closest to the current usage, and so would take the least work. Note that currently preview_text_string is not actually used for anything. Even though it is documented in lib/AnswerHash.pm as being what is supposed to be displayed for the preview. So this proposal just aligns that answer hash key with its intended use.

Test cases:

DOCUMENT();

loadMacros('PGstandard.pl', 'PGML.pl');

Context('Matrix')->flags->set(requireConstantVectors => 1);
Context()->variables->are(s => 'Real', t => 'Real');

$ans     = Formula('s<1, 0> + t<0, 1>');
$ans_cmp = $ans->cmp(
    checker => sub {
        my ($c, $s, $self) = @_;
        Value::Error('Try again') unless $c == $s;
        return 1;
    }
);

BEGIN_PGML
Enter [`[$ans]`]: [_]{$ans_cmp}
END_PGML

ENDDOCUMENT();

With this problem enter <s,0>+t<0,1>. Prior to #1004 the first vector would disappear from the displayed answer because student_ans was not escaped. After #1004 and still with this pull request this is displayed correctly.

Also with this problem enter s<1,0>+t<0,2>. Prior to #1004 this displayed correctly. With #1004 it is displayed as s*<1,0>+t*<0,2>. With this pull request is is displayed correctly.

DOCUMENT();

loadMacros('PGstandard.pl', 'PGML.pl');

Context('Matrix');
$A = Matrix([ [ 1, 0 ], [ 0, -1 ] ]);

BEGIN_PGML
Enter the matrix [`[$A]`]:

[_]*{$A}
END_PGML

ENDDOCUMENT();

With this problem enter S, -1, 1, 0 for the matrix entries. Prior to #1004 this displayed correctly. With #1004 this is escaped twice, and so you see the HTML code displayed. With this pull request this is again displayed correctly.

[pstaabp]

This looks like it fixes everything. 👍

[Alex-Jordan]

To my knowledge this was the last (most recent, not necessarily final) change that was made on this topic. My production server has this change. And yet we are seeing an HTML escaping issue in certain answers. I cannot reproduce it on develop, so I wanted to ask if off the top of your head, have there been other merged commits in the past week or two that would affect HTML escaping in student answers?

What we are seeing is that with an answer blank where MathQuill is not in use (in our case, because mathQuillOpts => 'disabled' is set) then if a student submits an answer like {x|x<1}, when the page reloads it now has {x|x<1}. (If the submitted answer was correct, it is correctly assessed as correct. But not then if you submit again, since now the answer has an unexpected character.)

[Alex-Jordan]

Nevermind about my question, I found the issue. My production server still had the very firs attempt that we made about this, uncommitted, that was cooked up during a meeting when we first found this issue. I removed that and now things seem fine.