Categorizer: Extract validation out of scoring #1862

Myranae · 2024-11-14T22:29:28Z

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:

Confirm all checks pass
Confirm the widget still behaves as it should.

github-actions · 2024-11-14T22:31:50Z

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (9437cfd) and published it to npm. You
can install it using the tag PR1862.

Example:

yarn add @khanacademy/perseus@PR1862

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR1862

github-actions · 2024-11-14T22:32:19Z

Size Change: +162 B (+0.01%)

Total Size: 1.29 MB

Filename	Size	Change
`packages/perseus/dist/es/index.js`	421 kB	+162 B (+0.04%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	39 kB
`packages/keypad-context/dist/es/index.js`	760 B
`packages/kmath/dist/es/index.js`	4.27 kB
`packages/math-input/dist/es/index.js`	77.9 kB
`packages/math-input/dist/es/strings.js`	1.79 kB
`packages/perseus-core/dist/es/index.js`	1.48 kB
`packages/perseus-editor/dist/es/index.js`	698 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/perseus/dist/es/strings.js`	3.68 kB
`packages/pure-markdown/dist/es/index.js`	3.66 kB
`packages/simple-markdown/dist/es/index.js`	12.5 kB

_{compressed-size-action}

khan-actions-bot · 2024-11-15T15:35:28Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/green-ghosts-burn.md, packages/perseus/src/validation.types.ts, packages/perseus/src/widgets/categorizer/categorizer.tsx, packages/perseus/src/widgets/categorizer/score-categorizer.test.ts, packages/perseus/src/widgets/categorizer/score-categorizer.ts, packages/perseus/src/widgets/categorizer/validate-categorizer.test.ts, packages/perseus/src/widgets/categorizer/validate-categorizer.ts

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

jeremywiebe · 2024-11-18T23:12:01Z

packages/perseus/src/validation.types.ts

+    // Translatable text; a list of items to categorize. e.g. ["banana", "yellow", "apple", "purple", "shirt"]
+    items: ReadonlyArray<string>;


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. :)

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?

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. :)

handeyeco

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.

handeyeco · 2024-11-15T15:58:14Z

packages/perseus/src/widgets/categorizer/score-categorizer.ts

+    const validationResult = validateCategorizer(userInput, rubric, strings);
+    if (validationResult) {
+        return validationResult;
+    }


I think this might make things more clear

Suggested change

const validationResult = validateCategorizer(userInput, rubric, strings);

if (validationResult) {

return validationResult;

}

const validationError = validateCategorizer(userInput, rubric, strings);

if (validationError) {

return validationError;

}

Updated! We might want to update the language in the table widget to be consistent with this if this is preferred.

handeyeco · 2024-11-15T15:59:24Z

packages/perseus/src/widgets/categorizer/validate-categorizer.test.ts

+import type {PerseusCategorizerRubric} from "../../validation.types";
+
+describe("validateCategorizer", () => {
+    it("tells the learner its not complete if not selected", () => {


Can we add a test case for the null return value?

handeyeco · 2024-11-15T16:03:26Z

packages/perseus/src/widgets/categorizer/validate-categorizer.ts

+    let completed = true;
+    rubric.items.forEach((_, i) => {
+        if (userInput.values[i] == null) {
+            completed = false;
+        }
+    });
+    if (!completed) {
+        return {
+            type: "invalid",
+            message: strings.invalidSelection,
+        };
+    }
+    return null;


I know this is just moved code, but what about something like:

Suggested change

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;

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 :)

handeyeco · 2024-11-15T16:18:32Z

packages/perseus/src/widgets/categorizer/validate-categorizer.ts

+
+function validateCategorizer(
+    userInput: PerseusCategorizerUserInput,
+    rubric: PerseusCategorizerRubric,


CC @jeremywiebe

I think this highlights a possible need for more even more clarification of types (or maybe just a helper):

Example using unions (seems cleaner)

Example using utilities

(In the examples I use feRubric as the stuff we need to validate and beRubric as the stuff we need to validate and score.)

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.

Myranae · 2024-11-18T23:00:16Z

@jeremywiebe @handeyeco This should now follow the format we discussed, with rubric now as scoringData and its type as the old rubric type unioned with the validationData type. Re-requested review for one more check.

jeremywiebe · 2024-11-18T23:12:01Z

packages/perseus/src/validation.types.ts

+    // Translatable text; a list of items to categorize. e.g. ["banana", "yellow", "apple", "purple", "shirt"]
+    items: ReadonlyArray<string>;


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. :)

Myranae added 3 commits November 14, 2024 16:01

Split validation tests from scoring tests

e6f0b85

Put validation logic in own function and call it

4e1f7b2

Add changeset

e2552af

Myranae self-assigned this Nov 14, 2024

Myranae marked this pull request as ready for review November 15, 2024 15:35

Myranae requested review from handeyeco and jeremywiebe November 15, 2024 15:35

khan-actions-bot requested a review from a team November 15, 2024 15:35

Pull values (the answers) out of the validation

52e6aad

Myranae commented Nov 15, 2024

View reviewed changes

handeyeco approved these changes Nov 15, 2024

View reviewed changes

Myranae added 6 commits November 15, 2024 11:15

Update validationResult to validationError

28766a3

Add test for null return

27c222e

Simplify validation check

c77806a

Add validation type

539b8cc

Update validator to use validateData instead of rubric

109df1b

Rename rubric + validation data to scoringData

261fcf1

Myranae requested a review from handeyeco November 18, 2024 22:56

jeremywiebe approved these changes Nov 18, 2024

View reviewed changes

Add documentation to validate function

9437cfd

jeremywiebe changed the base branch from main to release/server-side-scoring November 26, 2024 20:17

jeremywiebe merged commit 451de89 into release/server-side-scoring Nov 26, 2024
15 checks passed

jeremywiebe deleted the tb/LEMS-2596/categorizer-split-out-validation-with-rubric branch November 26, 2024 20:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Categorizer: Extract validation out of scoring #1862

Categorizer: Extract validation out of scoring #1862

Myranae commented Nov 14, 2024

github-actions bot commented Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 14, 2024 •

edited

Loading

khan-actions-bot commented Nov 15, 2024 •

edited

Loading

This comment was marked as outdated.

jeremywiebe Nov 18, 2024

Myranae Nov 20, 2024 •

edited

Loading

jeremywiebe Nov 20, 2024

handeyeco left a comment

handeyeco Nov 15, 2024

Myranae Nov 15, 2024 •

edited

Loading

handeyeco Nov 15, 2024

Myranae Nov 15, 2024

handeyeco Nov 15, 2024

Myranae Nov 15, 2024

handeyeco Nov 15, 2024 •

edited

Loading

Myranae Nov 15, 2024

Myranae commented Nov 18, 2024 •

edited

Loading

jeremywiebe Nov 18, 2024

		// Translatable text; a list of items to categorize. e.g. ["banana", "yellow", "apple", "purple", "shirt"]
		items: ReadonlyArray<string>;

Categorizer: Extract validation out of scoring #1862

Categorizer: Extract validation out of scoring #1862

Conversation

Myranae commented Nov 14, 2024

Summary:

Test plan:

github-actions bot commented Nov 14, 2024 • edited Loading

npm Snapshot: Published

github-actions bot commented Nov 14, 2024 • edited Loading

khan-actions-bot commented Nov 15, 2024 • edited Loading

Gerald

This comment was marked as outdated.

Choose a reason for hiding this comment

Myranae Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

handeyeco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myranae Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

handeyeco Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myranae commented Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 14, 2024 •

edited

Loading

khan-actions-bot commented Nov 15, 2024 •

edited

Loading

Myranae Nov 20, 2024 •

edited

Loading

Myranae Nov 15, 2024 •

edited

Loading

handeyeco Nov 15, 2024 •

edited

Loading

Myranae commented Nov 18, 2024 •

edited

Loading