Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Client Validation] Use emptyWidgetsFunctional in scoring #2083

Open
wants to merge 9 commits into
base: feature/client-validation
Choose a base branch
from

Conversation

jeremywiebe
Copy link
Collaborator

@jeremywiebe jeremywiebe commented Jan 8, 2025

This PR completes the work of extracting validation logic from scoring logic. This retains most of the validation that used to be intermingled with scoring.

This means that even when we strip scoring data from the widget options, we'll still be able to check if an answer is scorable.

Notably: input-number and numeric-input missed the train here. Both of these widgets use KhanAnswerTypes right at the beginning of scoring. Further, the coefficient answer type allows an empty value ("") and bare negative ("-") to be treated as answers by coercing them to 1 and -1 respectively. This means that we cannot do any validation/empty checking for these widgets because we need the full KhanAnswerTypes logic (which requires scoring data to work).

Issue: LEMS-2561

Test plan:

yarn test
yarn typecheck

@jeremywiebe jeremywiebe self-assigned this Jan 8, 2025
Copy link
Contributor

github-actions bot commented Jan 8, 2025

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR2083

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

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

Copy link
Contributor

github-actions bot commented Jan 8, 2025

Size Change: +86 B (+0.01%)

Total Size: 1.28 MB

Filename Size Change
packages/perseus/dist/es/index.js 420 kB +86 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 78 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 688 kB
packages/perseus-linter/dist/es/index.js 22.2 kB
packages/perseus/dist/es/strings.js 5.03 kB
packages/pure-markdown/dist/es/index.js 3.67 kB
packages/simple-markdown/dist/es/index.js 12.5 kB

compressed-size-action

@jeremywiebe jeremywiebe changed the title [Client Validation] Hook empty widgets check into validation [Client Validation] Use emptyWidgetsFunctional in scoring Jan 8, 2025
@jeremywiebe jeremywiebe marked this pull request as ready for review January 8, 2025 19:02
Copy link
Contributor

@handeyeco handeyeco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love seeing a PR that's mostly tests. Thanks!

})
// Not empty
.mockReturnValueOnce(null);
const scoringSpy = jest.spyOn(DropdownWidgetExport, "scorer");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the perspective of an outsider, I like this strategy. From the perspective of someone actively working on splitting up WidgetExports, I wonder if there's a way to do these tests without spying on WidgetExports; for example we could get 90% there by creating an empty user input object and that would keep me from having to refactor this test when I make my changes.

No action needed, just something that came to mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants