-
Notifications
You must be signed in to change notification settings - Fork 351
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
BUGFIX - [Numeric Input] - Check for wrong answers when scoring #1743
base: main
Are you sure you want to change the base?
BUGFIX - [Numeric Input] - Check for wrong answers when scoring #1743
Conversation
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (25de050) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1743 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1743 |
Size Change: -144 B (-0.02%) Total Size: 867 kB
ℹ️ View Unchanged
|
…the validator when the answer is "correct".
Add unit test for empty/incomplete user input.
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
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.
Works well from my own testing on the ZND -- thank you for providing an easy link! The bug seems to be solved to me, great work!
I do wonder about how we can better explain / illustrate the importance of the "answer" orders for Content Creators, but that's a separate issue. :)
.every((answer) => Math.abs(answer.value) <= 1); | ||
|
||
// The coefficient is an attribute of the widget | ||
let localValue: string | number = currentValue; |
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.
Ignorable Nit: We could possibly explain a little more about the coefficient here, but perhaps that's best handled in the widget options.
.find((answer) => { | ||
// NOTE: "answer.score.correct" indicates a match via the validate function. | ||
// It does NOT indicate that the answer itself is correct. | ||
return ( |
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.
This is a very helpful comment, thank you! I suspect the validators were made before Expression/Numeric were updated to allow for the inclusion of "wrong" or "ungraded" answers.
correct: matchedAnswer?.status === "correct", | ||
message: matchedAnswer?.message ?? null, | ||
guess: localValue, | ||
}; | ||
|
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.
Thank you for getting rid of all of these ts expect errors!
Summary:
On occasion, answer options within the Numeric Input can overlap. In cases where the wrong/ungraded answer comes before the correct answer, it is important to honor this order when validating a learner's answer. Currently, the Numeric Input widget checks all correct answers for a match before looking at the remaining answer options. This means that an incorrect answer would be marked as correct. This bugfix changes the validation logic to look for the first match regardless of the correctness of the answer itself, and then reports the validation based upon the match.
Issue: LEMS-2238
Test plan:
No Storybook option exists for testing validation of a widget - must be tested in a ZND