Skip to content

Commit

Permalink
This is another attempt at fixing student_ans HTML escaping issues.
Browse files Browse the repository at this point in the history
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*&lt;1,0&gt;+t*&lt;0,2&gt;`.  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.
  • Loading branch information
drgrice1 committed Feb 29, 2024
1 parent a9d9280 commit 27e15c8
Showing 1 changed file with 1 addition and 2 deletions.
3 changes: 1 addition & 2 deletions lib/Value/AnswerChecker.pm
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ sub cmp_parse {
$self->cmp_diagnostics($ans);
}
} else {
$ans->{student_ans} = protectHTML($ans->{student_ans});
$self->cmp_collect($ans);
$self->cmp_error($ans);
}
Expand Down Expand Up @@ -337,8 +338,6 @@ sub cmp_error {
. protectHTML(substr($string, $s, $e - $s))
. '</SPAN>'
. protectHTML(substr($string, $e));
} else {
$ans->{student_ans} = protectHTML($ans->{student_ans});
}
$self->cmp_Error($ans, $message);
}
Expand Down

0 comments on commit 27e15c8

Please sign in to comment.