-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
new context flag to require constant entries #990
Conversation
@Alex-Jordan Can you show an example problem that would use this? |
f997873
to
053cfdf
Compare
Here is a problem that uses this. For example, the answer might be
|
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.
I don't have any problem with this, though I left a comment that should allow you to make it more efficient.
You may also want to consider a more specific flag name, like requireConstantVectorEntries
if you are also planning to do this for Matrices, so that you can control them individually.
lib/Parser/List/Vector.pm
Outdated
if ($self->context->flag("requireConstantEntries") && !($x->{isConstant})) { | ||
$self->{equation}->Error("Coordinates of Vectors must be constant"); | ||
} |
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.
You don't need to check each coordinate separately, as the vector's isConstant
flag will already be set by the time the _check
subroutine is called, so I think you can just do
$self->{equation}->Error("Coordinates of Vectors must be constant");
if $self->context->flag("requireConstantEntries") && !($self->{isConstant};
after the foreach
loop.
Testing your problem above, the correct answer was supposed to be: |
I also see the issue that @pstaabp found. And I see it after applying @dpvc 's suggestion too. This calls |
Does the feedback mechanism properly escape HTML special characters like |
So now I'm not able to reproduce the issue. I am wondering if what happened was Davide's suggested change actually did incidentally fix that issue, but while I was testing I forgot to restart webwork2. So it was still running off the old .pm. @pstaabp can you try this now? If the same issue arises, can you look into Davide's idea? In the HTML source for the page, do you see the angle brackets? |
This is an issue with HTML characters. The DOM shows that the entire answer is there, but it is not rendered because of the starting |
It is odd that this does not happen with the develop branch though. It only happens with this pull request. I haven't figured out why that is the case. |
Well, I'm thoroughly confused and wondering if the behavior is not consistent. I know for sure that prior to my Feb 3 comment here, I tested this on both 2.18 (where I made a local uncommitted edit) and on develop. And I saw the issue both places (in the results table in 2.18 and in the popover with develop). I specifically confirmed that before I made that comment because I was wondering if the results table was a piece of the puzzle. Earlier today on develop I could not repeat the issue, which made me think that in earlier testing I hadn't restarted webwork2. But now with what you (@drgrice1 ) say, I'm just really confused. |
Oh! I get it... In my testing today, the answer I tried was So is this issue actually independent of this PR? Would any vector that starts with a letter after the |
I am able to get this to happen on develop as well. I just had to get an error to occur in the student answer (but not the one from this pull request). Testing this with 2.18 and the attempts table, I see that this also happens there. So this is not an issue with this pull request, and is also not an issue with the new feedback mechanism. The issue is that |
A note to anyone who might look at this PR: I will extend this to Matrices and Points, and do as Davide suggested and use different flag names for each. So this can wait for a new review until I add those. And the bug @pstaabp found is an independent issue that Glenn has a handle on. |
Testing this in a script from the command line, if the input for the answer has the value I also tested this with webwork and pg version 2.17, and this issue already occurred with that version. So this isn't an issue with the Mojolicious change either. |
…s occur. This addresses the issue found in openwebwork#990. This issue occurs at least as far back as PG 2.17 (but probably occured before that as well. If an answer has an error that prevents it from being parsed into a MathObject value, and yet the error `pos` is not set, then the answer is not HTML escaped as it should be. This can result in the answer in the "You Entered" feedback entry (or the old attempts table "Entered" column) not being displayed correctly. You can test with the problem ```perl DOCUMENT(); loadMacros('PGstandard.pl', 'PGML.pl'); Context('Matrix'); $v = Vector(1, 0, 0); BEGIN_PGML Enter the vector [`[$v]`]: [_]{$v} END_PGML ENDDOCUMENT(); ``` If you enter the answer `<x(1,0),0,0>`, then on the develop branch the "You Entered" section of feedback will appear to be empty. Using the element inspector of the developer tools you will see that it is there. With this pull request it will appear as it should. Note that in the cases that the answer does parse into a MatHObject value or the error `pos` is set, then the answer is HTML escaped already.
…s occur. This addresses the issue found in openwebwork#990. This issue occurs at least as far back as PG 2.17 (but probably occured before that as well). If an answer has an error that prevents it from being parsed into a MathObject value, and yet the error `pos` is not set, then the answer is not HTML escaped as it should be. This can result in the answer in the "You Entered" feedback entry (or the old attempts table "Entered" column) not being displayed correctly. You can test with the problem ```perl DOCUMENT(); loadMacros('PGstandard.pl', 'PGML.pl'); Context('Matrix'); $v = Vector(1, 0, 0); BEGIN_PGML Enter the vector [`[$v]`]: [_]{$v} END_PGML ENDDOCUMENT(); ``` If you enter the answer `<x(1,0),0,0>`, then on the develop branch the "You Entered" section of feedback will appear to be empty. Using the element inspector of the developer tools you will see that it is there. With this pull request it will appear as it should. Note that in the cases that the answer does parse into a MatHObject value or the error `pos` is set, then the answer is HTML escaped already.
One of the sources of this problem is that it is not clear what |
7da067c
to
0a2d16f
Compare
This now has three new flags: Note that a Point, Vector, or Matrix object would be required to have constant entries, including when used in a Formula. But a Formula that returns Points, Vectors, or Matrices is not necessarily required to be constant. For example, So I considered having more clear names for these flags, to make it more clear that the entries need to be constant but nothing is required outside of the object: Whenever this is resolved and merged, I need to update wiki documentation or I'm pretty sure documentation for this will be forgotten about later. |
…s occur. This addresses the issue found in openwebwork#990. This issue occurs at least as far back as PG 2.17 (but probably occured before that as well). If an answer has an error that prevents it from being parsed into a MathObject value, and yet the error `pos` is not set, then the answer is not HTML escaped as it should be. This can result in the answer in the "You Entered" feedback entry (or the old attempts table "Entered" column) not being displayed correctly. You can test with the problem ```perl DOCUMENT(); loadMacros('PGstandard.pl', 'PGML.pl'); Context('Matrix'); $v = Vector(1, 0, 0); BEGIN_PGML Enter the vector [`[$v]`]: [_]{$v} END_PGML ENDDOCUMENT(); ``` If you enter the answer `<x(1,0),0,0>`, then on the develop branch the "You Entered" section of feedback will appear to be empty. Using the element inspector of the developer tools you will see that it is there. With this pull request it will appear as it should. Note that in the cases that the answer does parse into a MatHObject value or the error `pos` is set, then the answer is HTML escaped already.
@dpvc This still has a block on it that you set. I think that the issue that floated up here has been determined to be an independent issue. Do you think what is there now looks good, at least as far as the scope of this PR? |
This looks good now. Tested both constant vectors and constant points. Although it make sense to use constantMatrix as well, I'm trying to figure out how to test. Since the standard is to use a matrix input, there wouldn't be a way to have a variable times that. @Alex-Jordan do you have an idea about using this? |
@pstaabp: Matrices can also be asked to be entered in a single text input in the format |
By the way, here is a very simple example of how to ask for a matrix in the format I mentioned (not utilizing the new flag): DOCUMENT();
loadMacros('PGstandard.pl', 'MathObjects.pl', 'PGML.pl');
Context('Matrix');
$matrix1 = Matrix('[[1,0],[0,1]]');
BEGIN_PGML
What is the [`2 \times 2`] identity matrix?
[_]{$matrix1}{10}
END_PGML
ENDDOCUMENT(); |
Actually, something doesn't seem to be working with the matrix flag. With the following example:
I can enter |
Nevermind. It turns out you need to check out this pull request for the flag to work. |
This backs up one step from what was done in openwebwork#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 openwebwork#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 definece, 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 openwebwork#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 openwebwork#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 openwebwork#1004 and after. I think that something along the lines of @dpvc's suggestion in openwebwork#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.
This backs up one step from what was done in openwebwork#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 openwebwork#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 openwebwork#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 openwebwork#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 openwebwork#1004 and after. I think that something along the lines of @dpvc's suggestion in openwebwork#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 openwebwork#1004 the first vector would disappear from the displayed answer because `student_ans` was not escaped. After openwebwork#1004 and still with this pull request this is displayed correctly. Also with this problem enter `s<1,0>+t<0,2>`. Prior to openwebwork#1004 this displayed correctly. With openwebwork#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 openwebwork#1004 this displayed correctly. With openwebwork#1004 this is escaped twice, and so you see the HTML code displayed. With this pull request this is again displayed correctly.
This backs up one step from what was done in openwebwork#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 openwebwork#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 openwebwork#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 openwebwork#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 openwebwork#1004 and after. I think that something along the lines of @dpvc's suggestion in openwebwork#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 openwebwork#1004 the first vector would disappear from the displayed answer because `student_ans` was not escaped. After openwebwork#1004 and still with this pull request this is displayed correctly. Also with this problem enter `s<1,0>+t<0,2>`. Prior to openwebwork#1004 this displayed correctly. With openwebwork#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 openwebwork#1004 this displayed correctly. With openwebwork#1004 this is escaped twice, and so you see the HTML code displayed. With this pull request this is again displayed correctly.
This backs up one step from what was done in openwebwork#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 openwebwork#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 openwebwork#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 openwebwork#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 openwebwork#1004 and after. I think that something along the lines of @dpvc's suggestion in openwebwork#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 openwebwork#1004 the first vector would disappear from the displayed answer because `student_ans` was not escaped. After openwebwork#1004 and still with this pull request this is displayed correctly. Also with this problem enter `s<1,0>+t<0,2>`. Prior to openwebwork#1004 this displayed correctly. With openwebwork#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 openwebwork#1004 this displayed correctly. With openwebwork#1004 this is escaped twice, and so you see the HTML code displayed. With this pull request this is again displayed correctly.
This backs up one step from what was done in openwebwork#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 openwebwork#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 openwebwork#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 openwebwork#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 openwebwork#1004 and after. I think that something along the lines of @dpvc's suggestion in openwebwork#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 openwebwork#1004 the first vector would disappear from the displayed answer because `student_ans` was not escaped. After openwebwork#1004 and still with this pull request this is displayed correctly. Also with this problem enter `s<1,0>+t<0,2>`. Prior to openwebwork#1004 this displayed correctly. With openwebwork#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 openwebwork#1004 this displayed correctly. With openwebwork#1004 this is escaped twice, and so you see the HTML code displayed. With this pull request this is again displayed correctly.
This makes it so that you can set a context flag
requireConstantEntries
, and if so, vectors cannot have variables in their entries. But for example something liket <1, 2, 3>
is still legal. This allows me to write a problem that requires students to "factor out" a parameter from vector answers.I want to check with @dpvc that this is an OK way to do this. If so I will extend this PR to do the same thing for Matrix objects. And I'll add a default value of
0
toDefault.pm
.