Skip to content

Commit

Permalink
Table: Extract validation out of scoring (#1849)
Browse files Browse the repository at this point in the history
## Summary:

This PR is a test run for extracting validation logic from scoring (using `table` as the test subject). I've used a pattern where the validator still returns a `PerseusScore`. This means that we can call the validator as the first step in scoring and return the score if it's `invalid`.  

Issue: LEMS-2606

## Test plan:

All tests should still pass.

Author: jeremywiebe

Reviewers: jeremywiebe, Myranae, handeyeco

Required Reviewers:

Approved By: handeyeco, Myranae

Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald

Pull Request URL: #1849
  • Loading branch information
jeremywiebe authored Nov 14, 2024
1 parent 46dc13f commit f7160d6
Show file tree
Hide file tree
Showing 9 changed files with 103 additions and 48 deletions.
5 changes: 5 additions & 0 deletions .changeset/curvy-glasses-poke.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": minor
---

Introduce validation function for `table` widget - useful for checking if the learner has filled in the table sufficiently to score it (fully client-side)
2 changes: 1 addition & 1 deletion docs/architecture.md
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ For each widget in `.widgets` the `Renderer`:
1. Runs each widget's options through any upgrade transforms (see
`Widgets.upgradeWidgetInfoToLatestVersion()` and
`WidgetExports.propUpgrades`). See [Prop Upgrades](#Prop_Upgrades) for more info
1. Prepares upgraded opions for rendering by applying the widget's
1. Prepares upgraded options for rendering by applying the widget's
`transform()` or `staticTransform()` (if rendering with
`static: true`). These functions map the widget options to the widget's
render Props.
Expand Down
10 changes: 4 additions & 6 deletions packages/perseus/src/util/answer-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import type {PerseusStrings} from "../strings";
const MAXERROR_EPSILON = Math.pow(2, -42);

type Guess = any;
type Predicate = any;
type Predicate = (guess: number, maxError: number) => boolean;

// TOOD(kevinb): Figure out how this relates to KEScore in
// perseus-all-package/types.js and see if there's a way to
Expand All @@ -26,7 +26,7 @@ export type Score = {
ungraded?: boolean;
};

/*
/**
* Answer types
*
* Utility for creating answerable questions displayed in exercises
Expand Down Expand Up @@ -69,7 +69,6 @@ export type Score = {
* TODO(alpert): Think of a less-absurd name for createValidatorFunctional.
*
*/

const KhanAnswerTypes = {
/*
* predicate answer type
Expand Down Expand Up @@ -625,11 +624,11 @@ const KhanAnswerTypes = {
convertToPredicate: function (
correctAnswer: string,
options: any,
): Predicate {
): [predicate: Predicate, options: any] {
const correctFloat = parseFloat(correctAnswer);

return [
function (guess: Predicate, maxError: Predicate) {
function (guess, maxError) {
return Math.abs(guess - correctFloat) < maxError;
},
$.extend({}, options, {type: "predicate"}),
Expand All @@ -641,7 +640,6 @@ const KhanAnswerTypes = {
strings: PerseusStrings,
): (arg1: Guess) => Score {
return KhanAnswerTypes.predicate.createValidatorFunctional(
// @ts-expect-error - TS2556 - A spread argument must either have a tuple type or be passed to a rest parameter.
...KhanAnswerTypes.number.convertToPredicate(
correctAnswer,
options,
Expand Down
6 changes: 4 additions & 2 deletions packages/perseus/src/validation.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import type {
PerseusOrdererWidgetOptions,
PerseusPlotterWidgetOptions,
PerseusRadioChoice,
PerseusTableWidgetOptions,
PerseusGraphCorrectType,
} from "./perseus-types";
import type {InteractiveMarkerType} from "./widgets/label-image/types";
Expand Down Expand Up @@ -180,7 +179,10 @@ export type PerseusSorterUserInput = {
changed: boolean;
};

export type PerseusTableRubric = PerseusTableWidgetOptions;
export type PerseusTableRubric = {
// Translatable Text; A 2-dimensional array of text to populate the table with
answers: ReadonlyArray<ReadonlyArray<string>>;
};

export type PerseusTableUserInput = ReadonlyArray<ReadonlyArray<string>>;

Expand Down
15 changes: 0 additions & 15 deletions packages/perseus/src/widgets/table/score-table.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,6 @@ describe("scoreTable", () => {
];

const rubric: PerseusTableRubric = {
headers: ["Col 1", "Col 2"],
rows: 2,
columns: 2,
answers: [
["1", "2"],
["3", "4"],
Expand All @@ -41,9 +38,6 @@ describe("scoreTable", () => {
];

const rubric: PerseusTableRubric = {
headers: ["Col 1", "Col 2"],
rows: 2,
columns: 2,
answers: [
["1", "2"],
["3", "4"],
Expand All @@ -65,9 +59,6 @@ describe("scoreTable", () => {
];

const rubric: PerseusTableRubric = {
headers: ["Col 1", "Col 2"],
rows: 2,
columns: 2,
answers: [
["1", "2"],
["3", "4"],
Expand All @@ -89,9 +80,6 @@ describe("scoreTable", () => {
];

const rubric: PerseusTableRubric = {
headers: ["Col 1", "Col 2"],
rows: 2,
columns: 2,
answers: [
["1", "2"],
["3", "4"],
Expand All @@ -113,9 +101,6 @@ describe("scoreTable", () => {
];

const rubric: PerseusTableRubric = {
headers: ["Col 1", "Col 2"],
rows: 2,
columns: 2,
answers: [
["1", "2"],
["3", "4"],
Expand Down
39 changes: 15 additions & 24 deletions packages/perseus/src/widgets/table/score-table.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ import _ from "underscore";

import KhanAnswerTypes from "../../util/answer-types";

import {filterNonEmpty} from "./utils";
import validateTable from "./validate-table";

import type {PerseusStrings} from "../../strings";
import type {PerseusScore} from "../../types";
import type {
Expand All @@ -10,29 +13,17 @@ import type {
} from "../../validation.types";

function scoreTable(
state: PerseusTableUserInput,
userInput: PerseusTableUserInput,
rubric: PerseusTableRubric,
strings: PerseusStrings,
): PerseusScore {
const filterNonEmpty = function (table: any) {
return _.filter(table, function (row) {
// Check if row has a cell that is nonempty
return _.some(row, _.identity);
});
};
const solution = filterNonEmpty(rubric.answers);
const supplied = filterNonEmpty(state);
const hasEmptyCell = _.some(supplied, function (row) {
return _.some(row, function (cell) {
return cell === "";
});
});
if (hasEmptyCell || !supplied.length) {
return {
type: "invalid",
message: null,
};
const validationResult = validateTable(userInput);
if (validationResult != null) {
return validationResult;
}

const supplied = filterNonEmpty(userInput);
const solution = filterNonEmpty(rubric.answers);
if (supplied.length !== solution.length) {
return {
type: "points",
Expand All @@ -41,12 +32,13 @@ function scoreTable(
message: null,
};
}

const createValidator = KhanAnswerTypes.number.createValidatorFunctional;
let message = null;
const allCorrect = _.every(solution, function (rowSolution) {
let message: string | null = null;
const allCorrect = solution.every(function (rowSolution) {
for (let i = 0; i < supplied.length; i++) {
const rowSupplied = supplied[i];
const correct = _.every(rowSupplied, function (cellSupplied, i) {
const correct = rowSupplied.every(function (cellSupplied, i) {
const cellSolution = rowSolution[i];
const validator = createValidator(
cellSolution,
Expand All @@ -57,7 +49,6 @@ function scoreTable(
);
const result = validator(cellSupplied);
if (result.message) {
// @ts-expect-error - TS2322 - Type 'string' is not assignable to type 'null'.
message = result.message;
}
return result.correct;
Expand All @@ -73,7 +64,7 @@ function scoreTable(
type: "points",
earned: allCorrect ? 1 : 0,
total: 1,
message: message,
message,
};
}

Expand Down
14 changes: 14 additions & 0 deletions packages/perseus/src/widgets/table/utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/**
* Filters the given table (modelled as a 2D array) to remove any rows that are
* completely empty.
*
* @returns A new table with only non-empty rows.
*/
export const filterNonEmpty = function (
table: ReadonlyArray<ReadonlyArray<string>>,
) {
return table.filter(function (row) {
// Return only rows that are non-empty.
return row.some((cell) => cell);
});
};
33 changes: 33 additions & 0 deletions packages/perseus/src/widgets/table/validate-table.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import validateTable from "./validate-table";

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

describe("tableValidator", () => {
it("is invalid if there is an empty cell", () => {
// Arrange
const userInput: PerseusTableUserInput = [
["1", ""],
["3", "4"],
];

// Act
const result = validateTable(userInput);

// Assert
expect(result).toHaveInvalidInput();
});

it("is null if all cells are provided", () => {
// Arrange
const userInput: PerseusTableUserInput = [
["1", "2"],
["3", "4"],
];

// Act
const result = validateTable(userInput);

// Assert
expect(result).toBeNull();
});
});
27 changes: 27 additions & 0 deletions packages/perseus/src/widgets/table/validate-table.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import {filterNonEmpty} from "./utils";

import type {PerseusScore} from "../../types";
import type {PerseusTableUserInput} from "../../validation.types";

function validateTable(
userInput: PerseusTableUserInput,
): Extract<PerseusScore, {type: "invalid"}> | null {
const supplied = filterNonEmpty(userInput);

const hasEmptyCell = supplied.some(function (row) {
return row.some(function (cell) {
return cell === "";
});
});

if (hasEmptyCell || !supplied.length) {
return {
type: "invalid",
message: null,
};
}

return null;
}

export default validateTable;

0 comments on commit f7160d6

Please sign in to comment.