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

…ng` to `main` (#1926) ## Summary: This PR includes the following commits: - Numeric-Input: Extract validation from scorer (#1882) - Orderer: Extract validation out of scoring (#1869) - Sorter: Extract validation out of scoring (#1876) - Dropdown: Extract validation out of scoring (#1898) - Plotter: Extract validation out of scoring (#1899) - Radio: Extract validation out of scoring (#1902) - Categorizer: Extract validation out of scoring (#1862) Issue: <none> ## Test plan: Author: jeremywiebe Reviewers: lillialexis, #perseus Required Reviewers: Approved By: lillialexis Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald, ✅ Publish npm snapshot (ubuntu-latest, 20.x), 🚫 Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), 🚫 Check builds for changes in size (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald, ✅ .github/dependabot.yml Pull Request URL: #1926

@Myranae

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @khanacademy/[email protected] ### Minor Changes - [#1898](#1898) [`3a9b5921b`](3a9b592) Thanks [@Myranae](https://github.com/Myranae)! - Introduces a validation function for the dropdown widget (extracted from dropdown scoring function). - [#1862](#1862) [`451de899f`](451de89) Thanks [@Myranae](https://github.com/Myranae)! - Split out validation function for the `categorizer` widget. This can be used to check if the user selected an answer for every row, confirming the question is ready to be scored. - [#1882](#1882) [`40d2ebb75`](40d2ebb) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Introduces a validation function for the numeric-input widget (extracted from numeric-input scoring function). - [#1899](#1899) [`2437ce61b`](2437ce6) Thanks [@Myranae](https://github.com/Myranae)! - Introduces a validation function for the plotter widget (extracted from the scoring function). - [#1869](#1869) [`f43edd42c`](f43edd4) Thanks [@Myranae](https://github.com/Myranae)! - Split out validation function for the `orderer` widget. This can be used to check if the user has ordered at least one option, confirming the question is ready to be scored. - [#1902](#1902) [`0cec7628c`](0cec762) Thanks [@Myranae](https://github.com/Myranae)! - Introduces a validation function for the radio widget (extracted from the scoring function), though not all validation logic can be extracted. - [#1876](#1876) [`0bd4270ad`](0bd4270) Thanks [@Myranae](https://github.com/Myranae)! - Split out validation function for the `sorter` widget. This can be used to check if the user has made any changes to the sorting order, confirming whether or not the question can be scored. ### Patch Changes - [#1907](#1907) [`3dbca965a`](3dbca96) Thanks [@benchristel](https://github.com/benchristel)! - Internal: add and pass regression tests for `PerseusItem` parser. - [#1915](#1915) [`ee09e9fc0`](ee09e9f) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove use of findDOMNode in number-input component - [#1920](#1920) [`88ba71bef`](88ba71b) Thanks [@handeyeco](https://github.com/handeyeco)! - Fix some file-wide error suppressions - [#1919](#1919) [`64ea2ee86`](64ea2ee) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove usage of findDOMNode in text-input component - [#1906](#1906) [`799ffe4a5`](799ffe4) Thanks [@anakaren-rojas](https://github.com/anakaren-rojas)! - update moveable point component and use control point method to have optional params - [#1895](#1895) [`841551a65`](841551a) Thanks [@benchristel](https://github.com/benchristel)! - Internal: remove unused fields from `answerArea` when parsing `PerseusItem`s. - [#1921](#1921) [`8ec06f444`](8ec06f4) Thanks [@benchristel](https://github.com/benchristel)! - Internal: Inline widget version into Expression widget parser. - [#1914](#1914) [`3e98b7cd3`](3e98b7c) Thanks [@handeyeco](https://github.com/handeyeco)! - Add tests for propUpgrades functions (and remove underscore usage) - [#1908](#1908) [`7f2866cf4`](7f2866c) Thanks [@benchristel](https://github.com/benchristel)! - Internal: Migrate expression widget options to the latest version in parseAndTypecheckPerseusItem (not yet used in production). - Updated dependencies \[[`88ba71bef`](88ba71b)]: - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - [#1920](#1920) [`88ba71bef`](88ba71b) Thanks [@handeyeco](https://github.com/handeyeco)! - Fix some file-wide error suppressions ## @khanacademy/[email protected] ### Patch Changes - [#1920](#1920) [`88ba71bef`](88ba71b) Thanks [@handeyeco](https://github.com/handeyeco)! - Fix some file-wide error suppressions - [#1863](#1863) [`584153588`](5841535) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Switch two corner usages of deprecated @khanacademy/wonder-blocks-spacing to @khanacademy/wonder-blocks-tokens - Updated dependencies \[[`3dbca965a`](3dbca96), [`ee09e9fc0`](ee09e9f), [`3a9b5921b`](3a9b592), [`88ba71bef`](88ba71b), [`64ea2ee86`](64ea2ee), [`451de899f`](451de89), [`40d2ebb75`](40d2ebb), [`799ffe4a5`](799ffe4), [`2437ce61b`](2437ce6), [`841551a65`](841551a), [`8ec06f444`](8ec06f4), [`f43edd42c`](f43edd4), [`0cec7628c`](0cec762), [`0bd4270ad`](0bd4270), [`3e98b7cd3`](3e98b7c), [`7f2866cf4`](7f2866c)]: - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - Updated dependencies \[[`88ba71bef`](88ba71b)]: - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - [#1920](#1920) [`88ba71bef`](88ba71b) Thanks [@handeyeco](https://github.com/handeyeco)! - Fix some file-wide error suppressions ## @khanacademy/[email protected] ### Patch Changes - Updated dependencies \[[`88ba71bef`](88ba71b)]: - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] Author: khan-actions-bot Reviewers: jeremywiebe Required Reviewers: Approved By: jeremywiebe Checks: ✅ Cypress (ubuntu-latest, 20.x), ⏭️ Publish npm snapshot, ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1918

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

jeremywiebe mentioned this pull request Nov 27, 2024

This is a feature-branch pull-request from release/server-side-scoring to main #1926

Merged

khan-actions-bot mentioned this pull request Nov 27, 2024

Version Packages #1918

Merged

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