docs(changeset): Ensure that extra keys are considered when determining whether an Expression Widget answer is invalid.#3292
Conversation
…riableNames error when the variables are provided in the extra keys
…are considered when determining whether an Expression Widget answer is invalid.
🗄️ Schema Change: No Changes ✅ |
|
Size Change: +64 B (+0.01%) Total Size: 485 kB
ℹ️ View Unchanged
|
🛠️ Item Splitting: No Changes ✅ |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (86910eb) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3292If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3292If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3292 |
packages/kas/src/compare.ts
Outdated
| message = | ||
| // Variable checks | ||
| const vars = expr1.sameVars(expr2, optionsWithDefaults.extraKeys); | ||
| if (!vars.equal && vars.equalIgnoringCase) { |
There was a problem hiding this comment.
I opted to split this into two returns in order to improve clarity after the addition of hasUnexpectedVars. We could keep the logic consolidated, I just found the if statement(s) far easier to read this way.
| simplify: boolean; | ||
| // Variables from other answer forms (e.g. intentionally wrong answers) that | ||
| // are "known" — students using these should be considered to have valid answers | ||
| extraKeys?: ReadonlyArray<string>; |
There was a problem hiding this comment.
There's a specific type (KeypadKey) that ExpressionWidget uses for extraKeys, but there's notes in data-schema.ts that says we shouldn't be using it, so I've just opted for a simple array of strings.
packages/kas/src/nodes.ts
Outdated
| @@ -395,7 +397,13 @@ abstract class Expr { | |||
| var equal = same(vars1, vars2); | |||
| var equalIgnoringCase = same(lower(vars1), lower(vars2)); | |||
There was a problem hiding this comment.
I had a thought here, but I'm not sure the best approach.
I kind of like the idea of adding:
var hasWrongVarCase = !equal && same(lower(vars1), lower(vars2));
So that we only return hasWrongVarCase and hasUnexpectedVars. This lets us simplify the logic a little in compare.ts, and makes this code easier to read. I'd honestly prefer the name of the function to be clearer too, given the increased complexity, to something like verifyVars instead of sameVars.
Personally, that feels clearer to me. However, I'm not sure if we want to be making that many changes to this code.
There was a problem hiding this comment.
... you know what? Yolo :P I can always revert the next commit if we don't like it.
| ...options, | ||
| }; | ||
|
|
||
| // TODO(CP-1614): Figure out how to make these messages translatable |
There was a problem hiding this comment.
This ticket was closed as "WONT DO" a long time ago, so I figured it was safe to clean up the TODO. I do wonder if we should make a new ticket, however.
1 participant
Summary:
We ran into an issue where the Expression Widget was returning
invalidwhen the user used a variable that the Content Creators had explicitly used in a "wrong" answer.I've added new logic to KAS to handle this situation. I opted to add most of the logic to
sameVar, as that seemed like the most logical place for it to be due to it only being used for the Expression widget. However, I'm open to push back on this. :)Update Note: While re-reviewing, I took the opportunity to rename
sameVarstovalidateVarsand update its return type.sameVarswas really only being used to surface specific variable errors, so I'm not sure the name was ever quite right. With my changes, I thinkvalidateVarsimproves code readability. I also folded the!equalguard intohasWrongVarCasedirectly, which allowed me to make the return more self-explanatory while simplifying the logic in compare.ts. I'm fully open to push back on this one, however — I just got bold. ;)Issue: LEMS-3566
Test plan: