Categorizer: Extract validation out of scoring #1862
Conversation
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (9437cfd) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1862If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1862 |
|
Size Change: +161 B (+0.01%) Total Size: 1.29 MB
ℹ️ View Unchanged
|
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
| // Translatable text; a list of items to categorize. e.g. ["banana", "yellow", "apple", "purple", "shirt"] | ||
| items: ReadonlyArray<string>; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
We chatted on Slack, but we agreed to model the Rubric like this:
export type PerseusCategorizerRubric = {
// The correct answers where index relates to the items and value relates
// to the category. e.g. [0, 1, 0, 1, 2]
values: ReadonlyArray<number>;
} PerseusCategorizerValidationData; We also agreed to rename the term Rubric to ScoringData in the Server-Side Scoring area in a future task. :)
There was a problem hiding this comment.
The above comment I made was from an older version of the code. I updated Rubric to be the following:
export type PerseusCategorizerScoringData = {
// The correct answers where index relates to the items and value relates
// to the category. e.g. [0, 1, 0, 1, 2]
values: ReadonlyArray<number>;
} & PerseusCategorizerValidationData;
Is it also okay to rename Rubric as we go and to use the ticket for renaming any we missed? It helps my mental model to have the names updated while I'm working, but I can totally leave the name change to the very end if it's preferred to do them all at once. And if it's preferred to do them all at once, should I revert the name change in this PR?
There was a problem hiding this comment.
I started renaming things to ScoringData in one of my PRs. Then as I thought about it, I thought it might be more confusing to have a partial migration state in main. I was going to leave things as "Rubric" until we're done this pass of validation splitting and then we could do a single pass over everything to update our terms from Rubric to ScoringData. But I don't think it's a huge deal either way. :)
There was a problem hiding this comment.
I think this is good to ship as-is, but could you also get @jeremywiebe to look at this? Since this is one of the earlier ones we're doing, I want to make sure we're on the right track.
| const validationResult = validateCategorizer(userInput, rubric, strings); | ||
| if (validationResult) { | ||
| return validationResult; | ||
| } |
There was a problem hiding this comment.
I think this might make things more clear
| const validationResult = validateCategorizer(userInput, rubric, strings); | |
| if (validationResult) { | |
| return validationResult; | |
| } | |
| const validationError = validateCategorizer(userInput, rubric, strings); | |
| if (validationError) { | |
| return validationError; | |
| } |
There was a problem hiding this comment.
Updated! We might want to update the language in the table widget to be consistent with this if this is preferred.
| import type {PerseusCategorizerRubric} from "../../validation.types"; | ||
|
|
||
| describe("validateCategorizer", () => { | ||
| it("tells the learner its not complete if not selected", () => { |
There was a problem hiding this comment.
Can we add a test case for the null return value?
| let completed = true; | ||
| rubric.items.forEach((_, i) => { | ||
| if (userInput.values[i] == null) { | ||
| completed = false; | ||
| } | ||
| }); | ||
| if (!completed) { | ||
| return { | ||
| type: "invalid", | ||
| message: strings.invalidSelection, | ||
| }; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
I know this is just moved code, but what about something like:
| let completed = true; | |
| rubric.items.forEach((_, i) => { | |
| if (userInput.values[i] == null) { | |
| completed = false; | |
| } | |
| }); | |
| if (!completed) { | |
| return { | |
| type: "invalid", | |
| message: strings.invalidSelection, | |
| }; | |
| } | |
| return null; | |
| let incomplete = rubric.items.some((_, i) => userInput.values[i] == null); | |
| if (incomplete) { | |
| return { | |
| type: "invalid", | |
| message: strings.invalidSelection, | |
| }; | |
| } | |
| return null; |
There was a problem hiding this comment.
I'm all for simplifying. I'll try to simplify the validation logic in the future if it seems like a straightforward update. Was trying to leave as close to original as possible as we've mentioned not changing the logic, but I think just simplifying what's there is probably fine. Updated :)
|
|
||
| function validateCategorizer( | ||
| userInput: PerseusCategorizerUserInput, | ||
| rubric: PerseusCategorizerRubric, |
There was a problem hiding this comment.
CC @jeremywiebe
I think this highlights a possible need for more even more clarification of types (or maybe just a helper):
(In the examples I use feRubric as the stuff we need to validate and beRubric as the stuff we need to validate and score.)
There was a problem hiding this comment.
I love this! We just have to make sure whatever the info on rubric is that it doesn't convey the answer in any way. This definitely clarifies things.
|
@jeremywiebe @handeyeco This should now follow the format we discussed, with |
| // Translatable text; a list of items to categorize. e.g. ["banana", "yellow", "apple", "purple", "shirt"] | ||
| items: ReadonlyArray<string>; |
There was a problem hiding this comment.
We chatted on Slack, but we agreed to model the Rubric like this:
export type PerseusCategorizerRubric = {
// The correct answers where index relates to the items and value relates
// to the category. e.g. [0, 1, 0, 1, 2]
values: ReadonlyArray<number>;
} PerseusCategorizerValidationData; We also agreed to rename the term Rubric to ScoringData in the Server-Side Scoring area in a future task. :)
Summary:
In order to complete scoring server-side, we need to separate out the validation logic from the scoring logic. This work separates those two functions and updates associated tests.
Issue: LEMS-2596
Test plan: