openwebwork / webwork2

Course management front end for WeBWorK
http://webwork.maa.org/wiki/Main_Page
Other
143 stars 165 forks source link

Problem score set to emptystring when entering in non-empty set as answer for an emptyset. #2300

Closed somiaj closed 7 months ago

somiaj commented 7 months ago

There is an error in the AttemptsTable in 2.18 that causes a perl warning when entering in a non-empty set as an answer for an empty set. Consider the following example problem.

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

Context('Interval');
$empty = Set("{}");

BEGIN_PGML
Enter the emptyset: [_]{$empty}
END_PGML
ENDDOCUMENT();

Enter in any non-empty set as an answer, like {1}. AttemptsTable.pm gives the following warning if viewing the problem in Problem.pm after it has been assigned to a set (this warning does not appear in the PGProblemEditor).

Argument "" isn't numeric in numeric ge (>=) at /opt/webwork/webwork2-develop/lib/WeBWorK/HTML/AttemptsTable.pm line 230.

The error is due to the previous line:

    my $answerScore = $rh_answer->{score} // 0;

So for some reason in this case with a non-empty set entered in as an answer for an emptyset, somewhere in pg/webwork, the answer hash $rh_answer->{score} gets set to the empty string, "", and not zero.

A quick fix for this issue was to also default $answerScore to zero if this is the empty string with:

    my $answerScore = $rh_answer->{score} || 0;

Though I think the issue is something else before this point setting $rh_answer->{score} to the empty string and not zero, since this warning doesn't seem to appear in other problems.

Since the AnswerTable.pm is not being used anymore, this is no longer an issue in develop, only 2.18 (though maybe a hotfix might be useful), but I think there might be a deeper issue that should be addressed instead (unsure if there might be other places that would fail of the score in this case was the empty string instead of 0 or undefined).

drgrice1 commented 7 months ago

Your conclusion is not quite correct. $rh_answer->{score} is not being set to the empty string. It is being set to the return value of a numeric comparison. In other words it is a boolean false. That is an interesting creature. Playing with such boolean values in scripts shows that this is very odd. It prints the same as the empty string. It does register as being numeric via ($val ^ $val) eq '0' and does work in numeric comparisons. I suspect this changes to the empty string in the conversion that occurs when returning from the sub-process in lib/WeBWorK/Utils/Rendering.pm (via the JSON encoding/decoding involved), since if you look at this value in the renderPG method before the sub-process ends it is the numeric boolean false, but in the AttemptsTable.pm file it is the empty string.

The issue is caused by line 1711 (line 1699 in 2.18) of pg/lib/Value/AnswerChecker.pm which sets the score to the boolean return value of $m == 0. If you change that to $m == 0 ? 1 : 0, then the warning goes away.

somiaj commented 7 months ago

I can confirm that @drgrice1 suggestion for editing pg/lib/Value/AnswerChecker.pm also works to fix this issue.

Should such a fix be done in develop to avoid other possible issues due to how this boolean is set? What about an hotfix for 2.18?

drgrice1 commented 7 months ago

It is debatable if the change should be made in pg/lib/Value/AnswerChecker.pm, or if instead line 255 of pg/lib/AnswerHash.pm should be changed to $self->{score} = $score || 0 if defined($score);. Making the change in AnswerChecker.pm fixes this issue, but making the change in AnswerHash.pm fixes this issue as well as other similar cases that might exist elsewhere.

A hot fix is fine. Of course the fix should also go into develop.

somiaj commented 7 months ago

Any reason to not fix both locations?

I can make the PRs if you would like, let me know what you prefer.

drgrice1 commented 7 months ago

You can make the pull requests.

It doesn't hurt to make the change in both locations.

somiaj commented 7 months ago

This has been fixed in 2.18 via openwebwork/pg#993 and even though the attempts table is not in develop any more, to avoid other possible issues the same fix is openwebwork/pg#992.

taniwallach commented 1 month ago

Just for the record, for people using WW 2.18 (without the fix from https://github.com/openwebwork/pg/pull/993), the same sort of issue occurred when a MultiAnswer with singleResult => 0 had "parts" returning a score of 0. It was also apparently being turned into what appeared to be an empty string before it got to AnswerTable, and a student making a real incorrect submission also got the same warning.

This one was certainly fixed by the change to pg/lib/AnswerHash.pm included in the PR.

There was a second warning students encountered about line 330 from lib/WeBWorK/Utils/ProblemProcessing.pm which I expect is also fixed by this same PR.