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

Matrix: Extract validation from scorer #1883

Merged
merged 4 commits into from
Nov 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/giant-vans-notice.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": minor
---

Introduces a validation function for the matrix widget (extracted from matrix scoring function).
15 changes: 9 additions & 6 deletions packages/perseus/src/validation.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@
* 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.
* 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.
*
* `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
Expand All @@ -22,7 +22,8 @@
* For example:
* ```
* type Perseus<Widget>ScoringData = {
* correct: string;
* correct: string; // Used _only_ for scoring
* size: number; // Used _only_ for scoring
* } & Perseus<Widget>ValidationData;
* ```
*/
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

I mentioned this in the other PRs, but just to be extra annoying: I'm not sure we really need a bunch of aliases for Empty.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll remove it (and others that were Empty). I've added a doc comment to the top of this file in another PR.


export type PerseusMatrixUserInput = {
answers: PerseusMatrixRubric["answers"];
Expand Down
61 changes: 61 additions & 0 deletions packages/perseus/src/widgets/matrix/score-matrix.test.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,74 @@
import {mockStrings} from "../../strings";

import scoreMatrix from "./score-matrix";
import * as MatrixValidator from "./validate-matrix";
jeremywiebe marked this conversation as resolved.
Show resolved Hide resolved

import type {
PerseusMatrixRubric,
PerseusMatrixUserInput,
} from "../../validation.types";

describe("scoreMatrix", () => {
it("should be correctly answerable if validation passes", function () {
// Arrange
const mockValidator = jest
.spyOn(MatrixValidator, "default")
.mockReturnValue(null);
jeremywiebe marked this conversation as resolved.
Show resolved Hide resolved

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 = {
Expand Down
20 changes: 6 additions & 14 deletions packages/perseus/src/widgets/matrix/score-matrix.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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);
Expand All @@ -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'.
Expand All @@ -58,13 +57,6 @@ function scoreMatrix(
});
});

if (hasEmptyCell) {
return {
type: "invalid",
message: strings.fillAllCells,
};
}

if (incorrectSize) {
return {
type: "points",
Expand Down
50 changes: 50 additions & 0 deletions packages/perseus/src/widgets/matrix/validate-matrix.test.ts
Original file line number Diff line number Diff line change
@@ -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();
});
});
45 changes: 45 additions & 0 deletions packages/perseus/src/widgets/matrix/validate-matrix.ts
Original file line number Diff line number Diff line change
@@ -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.
*
jeremywiebe marked this conversation as resolved.
Show resolved Hide resolved
* 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(
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Did you decide just to leave all the createValidator logic in the scoring function? Do you think this is something we might want to change in the future? Looks like some of it is for scoring, but some is for validation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's all left in the scoring function because there is no logic that can be moved into the validator that doesn't require the scoring data.

Copy link
Contributor

Choose a reason for hiding this comment

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

It kind of looked like the hasEmptyCell check could possibly be moved out, but then you'd duplicate a lot of that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice catch. I need the solution size for part of these check but the hasEmptyCell check can definitely be raised up!

userInput: PerseusMatrixUserInput,
validationData: PerseusMatrixValidationData,
strings: PerseusStrings,
): Extract<PerseusScore, {type: "invalid"}> | 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;