Skip to content

Commit

Permalink
Expression: Extract validation from scorer (#1878)
Browse files Browse the repository at this point in the history
## Summary:

Arguably, this validator is quite useless in that it does nothing. However, I was thinking that we'd be better off if all of our widgets had a validator, even if it does nothing, rather than have some widgets have a validator and some not. In my mind, we're better off with the same convention everywhere.

This PR introduces the concept of "validation data" that we discussed earlier today (November 18). This represents _only_ the data needed by the validator. It intersects that type into the ScoringData type (nee Rubric) to ensure that the scoring function has all the data it needs to score (but also validate).

  * `PerseusExpressionRubric` - the data needed to score the `expression` user input (Note: this will be renamed to `PerseusExpressionScoringData` in the near future)
  * `PerseusExpressionValidationData` - the data needed to validate the `expression` user input
  * `ExpressionWidgetOptions` - the data needed to render the `expression` widget 

In the `expression` widget case, the validation data is empty, but in other widgets, the data in this type would also need to be in both the rubric and widget options (as the widget will call the validator on the client with only the widget options in hand and the scorer will call the validator on the server with only the rubric in hand). 


Issue: LEMS-2598

## Test plan:

`yarn test`
`yarn typecheck`

Author: jeremywiebe

Reviewers: handeyeco, jeremywiebe, Myranae

Required Reviewers:

Approved By: handeyeco

Checks: ✅ Publish npm snapshot (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), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald

Pull Request URL: #1878
  • Loading branch information
jeremywiebe authored Nov 20, 2024
1 parent 0afb1a4 commit a27f23b
Show file tree
Hide file tree
Showing 6 changed files with 108 additions and 0 deletions.
5 changes: 5 additions & 0 deletions .changeset/warm-llamas-tease.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": minor
---

Add expression validator function
29 changes: 29 additions & 0 deletions packages/perseus/src/validation.types.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,32 @@
/**
* This file contains types used for validation and scoring. The types abide by
* a naming convention so that they're easy to follow and that we remain
* consistent across all of the widgets.
*
* These types are:
*
* `Perseus<Widget>UserInput`: the data returned by the widget that the user
* entered. This is referred to as the 'guess' in some older parts of Perseus.
*
* `Perseus<Widget>ValidationData`: the data needed to do validation of the
* user input. Validation is the checks we can do both on the client-side,
* before submitting user input for scoring, and on the server-side when we
* score it. As such, it cannot contain any of the sensitive scoring data
* that would reveal the answer.
*
* `Perseus<Widget>ScoringData` (nee `Perseus<Widget>Rubric`): the data needed
* to score the user input. By convention, this type is defined as the set of
* sensitive answer data and then intersected with
* `Perseus<Widget>ValidationData`.
*
* For example:
* ```
* type Perseus<Widget>ScoringData = {
* correct: string;
* } & Perseus<Widget>ValidationData;
* ```
*/

import type {
GrapherAnswerTypes,
PerseusDropdownChoice,
Expand Down
33 changes: 33 additions & 0 deletions packages/perseus/src/widgets/expression/score-expression.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,41 @@ import {mockStrings} from "../../strings";

import {expressionItem3Options} from "./expression.testdata";
import scoreExpression from "./score-expression";
import * as ExpressionValidator from "./validate-expression";

import type {PerseusExpressionRubric} from "../../validation.types";

describe("scoreExpression", () => {
it("should be correctly answerable if validation passes", function () {
// Arrange
const mockValidator = jest
.spyOn(ExpressionValidator, "default")
.mockReturnValue(null);
const rubric: PerseusExpressionRubric = expressionItem3Options;

// Act
const score = scoreExpression("z+1", rubric, mockStrings, "en");

// Assert
expect(mockValidator).toHaveBeenCalledWith("z+1");
expect(score).toHaveBeenAnsweredCorrectly();
});

it("should return 'empty' result if validation fails", function () {
// Arrange
const mockValidator = jest
.spyOn(ExpressionValidator, "default")
.mockReturnValue({type: "invalid", message: null});
const rubric: PerseusExpressionRubric = expressionItem3Options;

// Act
const score = scoreExpression("z+1", rubric, mockStrings, "en");

// Assert
expect(mockValidator).toHaveBeenCalledWith("z+1");
expect(score).toHaveInvalidInput();
});

it("should handle defined ungraded answer case with no error callback", function () {
const err = scoreExpression(
"x+1",
Expand Down
6 changes: 6 additions & 0 deletions packages/perseus/src/widgets/expression/score-expression.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {Log} from "../../logging/log";
import KhanAnswerTypes from "../../util/answer-types";

import getDecimalSeparator from "./get-decimal-separator";
import validateExpression from "./validate-expression";

import type {PerseusExpressionAnswerForm} from "../../perseus-types";
import type {PerseusStrings} from "../../strings";
Expand Down Expand Up @@ -40,6 +41,11 @@ function scoreExpression(
strings: PerseusStrings,
locale: string,
): PerseusScore {
const validationError = validateExpression(userInput);
if (validationError) {
return validationError;
}

const options = _.clone(rubric);
_.extend(options, {
decimal_separator: getDecimalSeparator(locale),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import validateExpression from "./validate-expression";

describe("expression validation", () => {
it("should return invalid for empty user input", () => {
const result = validateExpression("");
expect(result).toHaveInvalidInput();
});

it("should return null for non-empty user input", () => {
const result = validateExpression("x+1");
expect(result).toBeNull();
});
});
22 changes: 22 additions & 0 deletions packages/perseus/src/widgets/expression/validate-expression.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import type {PerseusScore} from "../../types";
import type {PerseusExpressionUserInput} from "../../validation.types";

/**
* Checks user input from the expression widget to see if it is scorable.
*
* Note: Most of the expression widget's validation requires the Rubric because
* of its use of KhanAnswerTypes as a core part of scoring.
*
* @see `scoreExpression()` for more details.
*/
function validateExpression(
userInput: PerseusExpressionUserInput,
): Extract<PerseusScore, {type: "invalid"}> | null {
if (userInput === "") {
return {type: "invalid", message: null};
}

return null;
}

export default validateExpression;

0 comments on commit a27f23b

Please sign in to comment.