From bcb1e82eff2733166dafd146d0f4f62ec35878de Mon Sep 17 00:00:00 2001 From: Jeremy Wiebe Date: Mon, 18 Nov 2024 17:31:41 -0800 Subject: [PATCH 1/4] Extract validation from matrix scorer --- .changeset/giant-vans-notice.md | 5 ++ packages/perseus/src/validation.types.ts | 4 +- .../src/widgets/matrix/score-matrix.test.ts | 53 +++++++++++++++++++ .../src/widgets/matrix/score-matrix.ts | 6 +++ .../widgets/matrix/validate-matrix.test.ts | 22 ++++++++ .../src/widgets/matrix/validate-matrix.ts | 22 ++++++++ 6 files changed, 111 insertions(+), 1 deletion(-) 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..57b3e3a78e 100644 --- a/packages/perseus/src/validation.types.ts +++ b/packages/perseus/src/validation.types.ts @@ -150,7 +150,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..7658960270 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,58 @@ 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); + 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); + 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..07d806e3d2 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); + if (validationResult != null) { + return validationResult; + } + const solution = rubric.answers; const supplied = userInput.answers; const solutionSize = getMatrixSize(solution); 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..89e3350ec1 --- /dev/null +++ b/packages/perseus/src/widgets/matrix/validate-matrix.test.ts @@ -0,0 +1,22 @@ +import validateMatrix from "./validate-matrix"; + +import type {PerseusMatrixUserInput} from "../../validation.types"; + +describe("matrixValidator", () => { + it("is null always", () => { + // Arrange + const userInput: PerseusMatrixUserInput = { + answers: [ + [1, 2, 3], + [4, 5, 6], + [7, 8, 9], + ], + }; + + // Act + const result = validateMatrix(userInput, {}); + + // 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..09ea1e4a81 --- /dev/null +++ b/packages/perseus/src/widgets/matrix/validate-matrix.ts @@ -0,0 +1,22 @@ +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 any 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, + rubric: PerseusMatrixValidationData, +): Extract | null { + return null; +} + +export default validateMatrix; From eaa149247c6e6d7f0c1c6957b051d33b674132b3 Mon Sep 17 00:00:00 2001 From: Jeremy Wiebe Date: Tue, 19 Nov 2024 12:15:29 -0800 Subject: [PATCH 2/4] Add validation for empty user input --- .../widgets/matrix/validate-matrix.test.ts | 28 ++++++++++++++++++- .../src/widgets/matrix/validate-matrix.ts | 10 ++++++- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/packages/perseus/src/widgets/matrix/validate-matrix.test.ts b/packages/perseus/src/widgets/matrix/validate-matrix.test.ts index 89e3350ec1..d961f0b3a9 100644 --- a/packages/perseus/src/widgets/matrix/validate-matrix.test.ts +++ b/packages/perseus/src/widgets/matrix/validate-matrix.test.ts @@ -3,7 +3,33 @@ import validateMatrix from "./validate-matrix"; import type {PerseusMatrixUserInput} from "../../validation.types"; describe("matrixValidator", () => { - it("is null always", () => { + it("should return invalid when answers is completely empty", () => { + // Arrange + const userInput: PerseusMatrixUserInput = { + answers: [], + }; + + // Act + const result = validateMatrix(userInput, {}); + + // 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, {}); + + // Assert + expect(result).toHaveInvalidInput(); + }); + + it("should return null for non-empty user input", () => { // Arrange const userInput: PerseusMatrixUserInput = { answers: [ diff --git a/packages/perseus/src/widgets/matrix/validate-matrix.ts b/packages/perseus/src/widgets/matrix/validate-matrix.ts index 09ea1e4a81..4df26e85fd 100644 --- a/packages/perseus/src/widgets/matrix/validate-matrix.ts +++ b/packages/perseus/src/widgets/matrix/validate-matrix.ts @@ -7,7 +7,7 @@ import type { /** * Checks user input from the matrix widget to see if it is scorable. * - * Note: The matrix widget cannot do any validation without the Scoring + * 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. @@ -16,6 +16,14 @@ function validateMatrix( userInput: PerseusMatrixUserInput, rubric: PerseusMatrixValidationData, ): Extract | null { + // Very basic check: did we get 0 rows or any rows with 0 answers? If so, + // then the user input is not gradable. + if ( + userInput.answers.length === 0 || + userInput.answers.some((row) => row.length === 0) + ) { + return {type: "invalid", message: null}; + } return null; } From 718fc621e8cb0eb7e583450ecd0e5443373ee795 Mon Sep 17 00:00:00 2001 From: Jeremy Wiebe Date: Wed, 20 Nov 2024 15:08:30 -0800 Subject: [PATCH 3/4] Move some more validation logic out of matrix scorer into validator --- .../src/widgets/matrix/score-matrix.test.ts | 12 +++++-- .../src/widgets/matrix/score-matrix.ts | 16 +--------- .../widgets/matrix/validate-matrix.test.ts | 10 +++--- .../src/widgets/matrix/validate-matrix.ts | 31 ++++++++++++++----- 4 files changed, 40 insertions(+), 29 deletions(-) diff --git a/packages/perseus/src/widgets/matrix/score-matrix.test.ts b/packages/perseus/src/widgets/matrix/score-matrix.test.ts index 7658960270..8a4d5f7d59 100644 --- a/packages/perseus/src/widgets/matrix/score-matrix.test.ts +++ b/packages/perseus/src/widgets/matrix/score-matrix.test.ts @@ -31,7 +31,11 @@ describe("scoreMatrix", () => { const score = scoreMatrix(userInput, rubric, mockStrings); // Assert - expect(mockValidator).toHaveBeenCalledWith(userInput, rubric); + expect(mockValidator).toHaveBeenCalledWith( + userInput, + rubric, + mockStrings, + ); expect(score).toHaveBeenAnsweredCorrectly(); }); @@ -57,7 +61,11 @@ describe("scoreMatrix", () => { const score = scoreMatrix(userInput, rubric, mockStrings); // Assert - expect(mockValidator).toHaveBeenCalledWith(userInput, rubric); + expect(mockValidator).toHaveBeenCalledWith( + userInput, + rubric, + mockStrings, + ); expect(score).toHaveInvalidInput(); }); diff --git a/packages/perseus/src/widgets/matrix/score-matrix.ts b/packages/perseus/src/widgets/matrix/score-matrix.ts index 07d806e3d2..ef78b3b5b7 100644 --- a/packages/perseus/src/widgets/matrix/score-matrix.ts +++ b/packages/perseus/src/widgets/matrix/score-matrix.ts @@ -17,7 +17,7 @@ function scoreMatrix( rubric: PerseusMatrixRubric, strings: PerseusStrings, ): PerseusScore { - const validationResult = validateMatrix(userInput, rubric); + const validationResult = validateMatrix(userInput, rubric, strings); if (validationResult != null) { return validationResult; } @@ -33,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'. @@ -64,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 index d961f0b3a9..f7583fafad 100644 --- a/packages/perseus/src/widgets/matrix/validate-matrix.test.ts +++ b/packages/perseus/src/widgets/matrix/validate-matrix.test.ts @@ -1,3 +1,5 @@ +import {mockStrings} from "../../strings"; + import validateMatrix from "./validate-matrix"; import type {PerseusMatrixUserInput} from "../../validation.types"; @@ -6,11 +8,11 @@ describe("matrixValidator", () => { it("should return invalid when answers is completely empty", () => { // Arrange const userInput: PerseusMatrixUserInput = { - answers: [], + answers: [[]], }; // Act - const result = validateMatrix(userInput, {}); + const result = validateMatrix(userInput, {}, mockStrings); // Assert expect(result).toHaveInvalidInput(); @@ -23,7 +25,7 @@ describe("matrixValidator", () => { }; // Act - const result = validateMatrix(userInput, {}); + const result = validateMatrix(userInput, {}, mockStrings); // Assert expect(result).toHaveInvalidInput(); @@ -40,7 +42,7 @@ describe("matrixValidator", () => { }; // Act - const result = validateMatrix(userInput, {}); + 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 index 4df26e85fd..a07886a16e 100644 --- a/packages/perseus/src/widgets/matrix/validate-matrix.ts +++ b/packages/perseus/src/widgets/matrix/validate-matrix.ts @@ -1,3 +1,8 @@ +import _ from "underscore"; + +import {getMatrixSize} from "./matrix"; + +import type {PerseusStrings} from "../../strings"; import type {PerseusScore} from "../../types"; import type { PerseusMatrixUserInput, @@ -14,16 +19,26 @@ import type { */ function validateMatrix( userInput: PerseusMatrixUserInput, - rubric: PerseusMatrixValidationData, + validationData: PerseusMatrixValidationData, + strings: PerseusStrings, ): Extract | null { - // Very basic check: did we get 0 rows or any rows with 0 answers? If so, - // then the user input is not gradable. - if ( - userInput.answers.length === 0 || - userInput.answers.some((row) => row.length === 0) - ) { - return {type: "invalid", message: 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; } From 61094125afd35456bb14f5b779aaa469ccc8361c Mon Sep 17 00:00:00 2001 From: Jeremy Wiebe Date: Wed, 20 Nov 2024 15:12:06 -0800 Subject: [PATCH 4/4] Comment tweaks describing validation/scoring types --- packages/perseus/src/validation.types.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/perseus/src/validation.types.ts b/packages/perseus/src/validation.types.ts index 57b3e3a78e..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; * ``` */