Number Line: Extract validation out of scoring #1901

Myranae · 2024-11-21T20:49:54Z

Summary:

To complete server-side scoring, we are separating out validation logic from scoring logic. This PR separates that logic and associated tests for the number line widget.

Issue: LEMS-2608

Test plan:

Confirm checks pass
Confirm widget still works as expected
Confirm changing the flow of validation and scoring does not affect user experience

Rename state to userInput Rename rubric to scoringData Rename rubric type

Add check for null return on validation pass Remove test helpers Rename rubric to scoringData

Update scoringData type to union with ValidationData

github-actions · 2024-11-21T20:52:38Z

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1901

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

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

github-actions · 2024-11-21T20:52:52Z

Size Change: +85 B (+0.01%)

Total Size: 1.29 MB

Filename	Size	Change
`packages/perseus/dist/es/index.js`	425 kB	+85 B (+0.02%)

ℹ️ 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`	697 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/perseus/dist/es/strings.js`	3.7 kB
`packages/pure-markdown/dist/es/index.js`	3.66 kB
`packages/simple-markdown/dist/es/index.js`	12.5 kB

_{compressed-size-action}

Myranae · 2024-11-21T21:16:11Z

packages/perseus/src/widgets/number-line/score-number-line.ts

-    // TODO: I don't think isTickCrtl is a thing anymore
-    if (state.isTickCrtl && outsideAllowedRange) {


I'm guessing when exercises allow users to set their own interval, then this check was necessary in case they put in an invalid number of divisions.

I checked the number line editor, and it still has a checkbox for isTickCrtl and the json still has the field (though it defaults to not being there). I tried removing state.isTickCrtl from line 27 and all the tests still pass. It doesn't look like it is referenced anywhere other than tests and this line here. Wouldn't this always be false if isTickCrtl is not in use? Maybe this validation check hasn't been working?

Or maybe this check is not necessary if we are no longer allowing the user to set their own tick interval. It is possible to turn this to true in the editor, so I think we should only remove it if that checkbox ever gets removed, just in case it has been set to true in the past or in case anyone sets it to true in the future.

I don't think this property should be on userInput though. Should we move it to validationData? Should there be any tests making sure things work with or without this value?

Maybe moving it can be a later fix if needed. Just left it for now as the only logic moved into the validation function.

Myranae · 2024-11-21T21:18:29Z

packages/perseus/src/widgets/number-line/score-number-line.ts

-    if (state.numLinePosition === start && state.rel === startRel) {
-        // We're where we started.
-        return {
-            type: "invalid",
-            message: null,
-        };
-    }


I don't think it's necessary to do this check after checking if the user got it right. If the user tries to check their answer and they have not moved the dot, then it should probably be invalid. This might not be the case if the first tick could possibly be the right answer. Might be good to check with content before landing this change since this validation check gets moved to before checking correctness.

Decided to leave this validation logic in the scoring function since it occurs after scoring logic.

Validation only occurs on the userInput, therefore no validationData object is needed. Logic previously in the validation function is being moved back to the scoring function, so more info is needed in scoringData

Myranae · 2024-12-04T18:43:30Z

packages/perseus/src/widgets/number-line/score-number-line.ts

        return {
            type: "points",
            earned: 1,
            total: 1,
            message: null,
        };
    }
-    if (state.numLinePosition === start && state.rel === startRel) {
+    if (userInput.numLinePosition === start && userInput.rel === startRel) {


This check could possibly be moved to the validation function at a later time if the flow can change so this check occurs first. Only possible if the answer is never the starting position though.

khan-actions-bot · 2024-12-04T19:58:20Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/sweet-ligers-arrive.md, packages/perseus/src/validation.types.ts, packages/perseus/src/widgets/number-line/score-number-line.test.ts, packages/perseus/src/widgets/number-line/score-number-line.ts, packages/perseus/src/widgets/number-line/validate-number-line.test.ts, packages/perseus/src/widgets/number-line/validate-number-line.ts

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

handeyeco

Thanks!

@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 - [#1901](#1901) [`051c50cf7`](051c50c) Thanks [@Myranae](https://github.com/Myranae)! - Introduces a validation function for the number line widget (extracted from the scoring function). - [#1936](#1936) [`d05272661`](d052726) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Changes the PerseusWidgetsMap to be extensible so that widgets can be registered outside of Perseus and still have full type safety. - [#1832](#1832) [`e3062b3c8`](e3062b3) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - [Numeric Input] - Update the UI to match Expression widget ### Patch Changes - [#1937](#1937) [`3cdabf1a3`](3cdabf1) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - TypeScript fixes - [#1948](#1948) [`e21a3a39b`](e21a3a3) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Refactor internally used object mapping utilities to use ES6 exports - [#1938](#1938) [`5e8d8468b`](5e8d846) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Type fixes - [#1942](#1942) [`1d2b4e7bf`](1d2b4e7) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Ensure that zoomed-in images retain alt text - [#1861](#1861) [`763a4ba38`](763a4ba) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - [Numeric Input] - Show format options as a list - [#1947](#1947) [`b8926e38a`](b8926e3) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Minor refactoring of ServerItemRenderer's componentDidUpdate to reduce duplication - [#1946](#1946) [`f35512786`](f355127) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Refactor scoring for `group` widget to follow the same pattern as all other widgets - [#1891](#1891) [`ef819ea95`](ef819ea) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - [Numeric Input] - Associate format options tooltip content with input field for assistive technologies - [#1945](#1945) [`e69ca3146`](e69ca31) Thanks [@nishasy](https://github.com/nishasy)! - Add global styles to reflect prod styling - [#1923](#1923) [`be8c06c75`](be8c06c) Thanks [@benchristel](https://github.com/benchristel)! - Internal: convert backgroundImage dimensions to numbers during parsing. - [#1934](#1934) [`129adebef`](129adeb) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Improve comments on some Perseus types - [#1924](#1924) [`2d89ef87d`](2d89ef8) Thanks [@benchristel](https://github.com/benchristel)! - Internal: add and pass regression tests for PerseusItem parser's handling of legacy data ## @khanacademy/[email protected] ### Patch Changes - [#1938](#1938) [`5e8d8468b`](5e8d846) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Type fixes - [#1937](#1937) [`3cdabf1a3`](3cdabf1) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Ensure links opening to style guide (Google Docs) set `rel="noreferrer"` - [#1946](#1946) [`f35512786`](f355127) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Remove debugging call in GraphSettings component - Updated dependencies \[[`3cdabf1a3`](3cdabf1), [`e21a3a39b`](e21a3a3), [`5e8d8468b`](5e8d846), [`1d2b4e7bf`](1d2b4e7), [`763a4ba38`](763a4ba), [`b8926e38a`](b8926e3), [`f35512786`](f355127), [`ef819ea95`](ef819ea), [`e69ca3146`](e69ca31), [`be8c06c75`](be8c06c), [`051c50cf7`](051c50c), [`129adebef`](129adeb), [`d05272661`](d052726), [`2d89ef87d`](2d89ef8), [`e3062b3c8`](e3062b3)]: - @khanacademy/[email protected] Author: khan-actions-bot Reviewers: mark-fitzgerald Required Reviewers: Approved By: mark-fitzgerald Checks: ⏭️ Publish npm snapshot, ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1944

Myranae added 5 commits November 21, 2024 14:43

Split validate logic to own function

2d30658

Rename state to userInput Rename rubric to scoringData Rename rubric type

Split out validation tests

09ae628

Add check for null return on validation pass Remove test helpers Rename rubric to scoringData

Rename rubric type to scoringData and add validation type

dfad0ec

Update scoringData type to union with ValidationData

Add changeset

d0083a4

Remove unneeded import

47f4c3f

Myranae self-assigned this Nov 21, 2024

Myranae commented Nov 21, 2024

View reviewed changes

Myranae requested review from handeyeco and jeremywiebe November 21, 2024 21:19

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

Base automatically changed from release/server-side-scoring to main November 27, 2024 22:17

Myranae added 4 commits December 4, 2024 08:41

Merge branch 'main' into tb/LEMS-2608/number-line-split-out-validation

859d732

Return parts of validation that would change flow

74b8043

ValidationData no longer necessary

ce53e80

Validation only occurs on the userInput, therefore no validationData object is needed. Logic previously in the validation function is being moved back to the scoring function, so more info is needed in scoringData

Remove unneeded import

05f3299

Myranae commented Dec 4, 2024

View reviewed changes

Myranae added 2 commits December 4, 2024 10:58

Fix tests to account for returned logic

c5767bb

Remove unneeded import

0d48670

Myranae marked this pull request as ready for review December 4, 2024 19:58

khan-actions-bot requested a review from a team December 4, 2024 19:58

handeyeco approved these changes Dec 4, 2024

View reviewed changes

Myranae merged commit 051c50c into main Dec 4, 2024
10 checks passed

Myranae deleted the tb/LEMS-2608/number-line-split-out-validation branch December 4, 2024 21:46

khan-actions-bot mentioned this pull request Dec 4, 2024

Version Packages #1944

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Number Line: Extract validation out of scoring #1901

Number Line: Extract validation out of scoring #1901

Myranae commented Nov 21, 2024

github-actions bot commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

Myranae Nov 21, 2024 •

edited

Loading

Myranae Dec 4, 2024

Myranae Nov 21, 2024 •

edited

Loading

Myranae Dec 4, 2024

Myranae Dec 4, 2024

khan-actions-bot commented Dec 4, 2024

handeyeco left a comment

		// TODO: I don't think isTickCrtl is a thing anymore
		if (state.isTickCrtl && outsideAllowedRange) {

Number Line: Extract validation out of scoring #1901

Number Line: Extract validation out of scoring #1901

Conversation

Myranae commented Nov 21, 2024

Summary:

Test plan:

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

npm Snapshot: Published

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

Myranae Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Myranae Dec 4, 2024

Choose a reason for hiding this comment

Myranae Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Myranae Dec 4, 2024

Choose a reason for hiding this comment

Myranae Dec 4, 2024

Choose a reason for hiding this comment

khan-actions-bot commented Dec 4, 2024

Gerald

handeyeco left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

Myranae Nov 21, 2024 •

edited

Loading

Myranae Nov 21, 2024 •

edited

Loading