From adad642ab0ae95de6600e7018f0aff836acc5911 Mon Sep 17 00:00:00 2001 From: Jeremy Wiebe Date: Thu, 21 Nov 2024 15:19:37 -0800 Subject: [PATCH] Matrix: Extract validation from scorer (#1883) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary: This PR extracts validation from the `matrix`'s scorer function. In reality, it's an empty function, but it follows our conventions for having the scorer call a validator first. I've created the standard tests to ensure that scorer calls the validator. Issue: LEMS-2602 ## Test plan: `yarn test` `yarn typecheck` Author: jeremywiebe Reviewers: Myranae, handeyeco, jeremywiebe Required Reviewers: Approved By: Myranae, handeyeco Checks: ✅ Publish npm snapshot (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), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: https://github.com/Khan/perseus/pull/1883 --- .changeset/giant-vans-notice.md | 5 ++ packages/perseus/src/validation.types.ts | 15 +++-- .../src/widgets/matrix/score-matrix.test.ts | 61 +++++++++++++++++++ .../src/widgets/matrix/score-matrix.ts | 20 ++---- .../widgets/matrix/validate-matrix.test.ts | 50 +++++++++++++++ .../src/widgets/matrix/validate-matrix.ts | 45 ++++++++++++++ 6 files changed, 176 insertions(+), 20 deletions(-) create mode 100644 .changeset/giant-vans-notice.md create mode 100644 packages/perseus/src/widgets/matrix/validate-matrix.test.ts create mode 100644 packages/perseus/src/widgets/matrix/validate-matrix.ts diff --git a/.changeset/giant-vans-notice.md b/.changeset/giant-vans-notice.md new file mode 100644 index 0000000000..1745100a25 --- /dev/null +++ b/.changeset/giant-vans-notice.md @@ -0,0 +1,5 @@ +--- +"@khanacademy/perseus": minor +--- + +Introduces a validation function for the matrix widget (extracted from matrix scoring function). diff --git a/packages/perseus/src/validation.types.ts b/packages/perseus/src/validation.types.ts index 68674223c7..82f5b58062 100644 --- a/packages/perseus/src/validation.types.ts +++ b/packages/perseus/src/validation.types.ts @@ -9,10 +9,10 @@ * entered. This is referred to as the 'guess' in some older parts of Perseus. * * `PerseusValidationData`: 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. + * user input. Validation refers to the different checks that 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. * * `PerseusScoringData` (nee `PerseusRubric`): the data needed * to score the user input. By convention, this type is defined as the set of @@ -22,7 +22,8 @@ * For example: * ``` * type PerseusScoringData = { - * correct: string; + * correct: string; // Used _only_ for scoring + * size: number; // Used _only_ for scoring * } & PerseusValidationData; * ``` */ @@ -150,7 +151,9 @@ export type PerseusMatcherUserInput = { export type PerseusMatrixRubric = { // A data matrix representing the "correct" answers to be entered into the matrix answers: PerseusMatrixWidgetAnswers; -}; +} & PerseusMatrixValidationData; + +export type PerseusMatrixValidationData = Empty; export type PerseusMatrixUserInput = { answers: PerseusMatrixRubric["answers"]; diff --git a/packages/perseus/src/widgets/matrix/score-matrix.test.ts b/packages/perseus/src/widgets/matrix/score-matrix.test.ts index 4bcf0bc28b..8a4d5f7d59 100644 --- a/packages/perseus/src/widgets/matrix/score-matrix.test.ts +++ b/packages/perseus/src/widgets/matrix/score-matrix.test.ts @@ -1,6 +1,7 @@ import {mockStrings} from "../../strings"; import scoreMatrix from "./score-matrix"; +import * as MatrixValidator from "./validate-matrix"; import type { PerseusMatrixRubric, @@ -8,6 +9,66 @@ import type { } from "../../validation.types"; describe("scoreMatrix", () => { + it("should be correctly answerable if validation passes", function () { + // Arrange + const mockValidator = jest + .spyOn(MatrixValidator, "default") + .mockReturnValue(null); + + const rubric: PerseusMatrixRubric = { + answers: [ + [0, 1, 2], + [3, 4, 5], + [6, 7, 8], + ], + }; + + const userInput: PerseusMatrixUserInput = { + answers: rubric.answers, + }; + + // Act + const score = scoreMatrix(userInput, rubric, mockStrings); + + // Assert + expect(mockValidator).toHaveBeenCalledWith( + userInput, + rubric, + mockStrings, + ); + expect(score).toHaveBeenAnsweredCorrectly(); + }); + + it("should return 'empty' result if validation fails", function () { + // Arrange + const mockValidator = jest + .spyOn(MatrixValidator, "default") + .mockReturnValue({type: "invalid", message: null}); + + const rubric: PerseusMatrixRubric = { + answers: [ + [0, 1, 2], + [3, 4, 5], + [6, 7, 8], + ], + }; + + const userInput: PerseusMatrixUserInput = { + answers: rubric.answers, + }; + + // Act + const score = scoreMatrix(userInput, rubric, mockStrings); + + // Assert + expect(mockValidator).toHaveBeenCalledWith( + userInput, + rubric, + mockStrings, + ); + expect(score).toHaveInvalidInput(); + }); + it("can be answered correctly", () => { // Arrange const rubric: PerseusMatrixRubric = { diff --git a/packages/perseus/src/widgets/matrix/score-matrix.ts b/packages/perseus/src/widgets/matrix/score-matrix.ts index bb43e78739..ef78b3b5b7 100644 --- a/packages/perseus/src/widgets/matrix/score-matrix.ts +++ b/packages/perseus/src/widgets/matrix/score-matrix.ts @@ -3,6 +3,7 @@ import _ from "underscore"; import KhanAnswerTypes from "../../util/answer-types"; import {getMatrixSize} from "./matrix"; +import validateMatrix from "./validate-matrix"; import type {PerseusStrings} from "../../strings"; import type {PerseusScore} from "../../types"; @@ -16,6 +17,11 @@ function scoreMatrix( rubric: PerseusMatrixRubric, strings: PerseusStrings, ): PerseusScore { + const validationResult = validateMatrix(userInput, rubric, strings); + if (validationResult != null) { + return validationResult; + } + const solution = rubric.answers; const supplied = userInput.answers; const solutionSize = getMatrixSize(solution); @@ -27,16 +33,9 @@ function scoreMatrix( const createValidator = KhanAnswerTypes.number.createValidatorFunctional; let message = null; - let hasEmptyCell = false; let incorrect = false; _(suppliedSize[0]).times((row) => { _(suppliedSize[1]).times((col) => { - if ( - supplied[row][col] == null || - supplied[row][col].toString().length === 0 - ) { - hasEmptyCell = true; - } if (!incorrectSize) { const validator = createValidator( // @ts-expect-error - TS2345 - Argument of type 'number' is not assignable to parameter of type 'string'. @@ -58,13 +57,6 @@ function scoreMatrix( }); }); - if (hasEmptyCell) { - return { - type: "invalid", - message: strings.fillAllCells, - }; - } - if (incorrectSize) { return { type: "points", diff --git a/packages/perseus/src/widgets/matrix/validate-matrix.test.ts b/packages/perseus/src/widgets/matrix/validate-matrix.test.ts new file mode 100644 index 0000000000..f7583fafad --- /dev/null +++ b/packages/perseus/src/widgets/matrix/validate-matrix.test.ts @@ -0,0 +1,50 @@ +import {mockStrings} from "../../strings"; + +import validateMatrix from "./validate-matrix"; + +import type {PerseusMatrixUserInput} from "../../validation.types"; + +describe("matrixValidator", () => { + it("should return invalid when answers is completely empty", () => { + // Arrange + const userInput: PerseusMatrixUserInput = { + answers: [[]], + }; + + // Act + const result = validateMatrix(userInput, {}, mockStrings); + + // Assert + expect(result).toHaveInvalidInput(); + }); + + it("should return invalid when any answer row is empty", () => { + // Arrange + const userInput: PerseusMatrixUserInput = { + answers: [[1, 2, 3], [], [7, 8, 9]], + }; + + // Act + const result = validateMatrix(userInput, {}, mockStrings); + + // Assert + expect(result).toHaveInvalidInput(); + }); + + it("should return null for non-empty user input", () => { + // Arrange + const userInput: PerseusMatrixUserInput = { + answers: [ + [1, 2, 3], + [4, 5, 6], + [7, 8, 9], + ], + }; + + // Act + const result = validateMatrix(userInput, {}, mockStrings); + + // Assert + expect(result).toBeNull(); + }); +}); diff --git a/packages/perseus/src/widgets/matrix/validate-matrix.ts b/packages/perseus/src/widgets/matrix/validate-matrix.ts new file mode 100644 index 0000000000..a07886a16e --- /dev/null +++ b/packages/perseus/src/widgets/matrix/validate-matrix.ts @@ -0,0 +1,45 @@ +import _ from "underscore"; + +import {getMatrixSize} from "./matrix"; + +import type {PerseusStrings} from "../../strings"; +import type {PerseusScore} from "../../types"; +import type { + PerseusMatrixUserInput, + PerseusMatrixValidationData, +} from "../../validation.types"; + +/** + * Checks user input from the matrix widget to see if it is scorable. + * + * Note: The matrix widget cannot do much validation without the Scoring + * Data because of its use of KhanAnswerTypes as a core part of scoring. + * + * @see `scoreMatrix()` for more details. + */ +function validateMatrix( + userInput: PerseusMatrixUserInput, + validationData: PerseusMatrixValidationData, + strings: PerseusStrings, +): Extract | null { + const supplied = userInput.answers; + const suppliedSize = getMatrixSize(supplied); + + for (let row = 0; row < suppliedSize[0]; row++) { + for (let col = 0; col < suppliedSize[1]; col++) { + if ( + supplied[row][col] == null || + supplied[row][col].toString().length === 0 + ) { + return { + type: "invalid", + message: strings.fillAllCells, + }; + } + } + } + + return null; +} + +export default validateMatrix;