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

Categorizer: Extract validation out of scoring #1862

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
5 changes: 5 additions & 0 deletions .changeset/green-ghosts-burn.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": minor
---

Split out validation function for the `categorizer` widget. This can be used to check if the user selected an answer for every row, confirming the question is ready to be scored.
8 changes: 8 additions & 0 deletions packages/perseus/src/validation.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,19 @@ export type PerseusCategorizerRubric = {
// The correct answers where index relates to the items and value relates
// to the category. e.g. [0, 1, 0, 1, 2]
values: ReadonlyArray<number>;
// Translatable text; a list of items to categorize. e.g. ["banana", "yellow", "apple", "purple", "shirt"]
items: ReadonlyArray<string>;
Comment on lines +34 to +35

This comment was marked as outdated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We chatted on Slack, but we agreed to model the Rubric like this:

export type PerseusCategorizerRubric = {
     // The correct answers where index relates to the items and value relates
     // to the category.  e.g. [0, 1, 0, 1, 2]
     values: ReadonlyArray<number>;
} PerseusCategorizerValidationData; 

We also agreed to rename the term Rubric to ScoringData in the Server-Side Scoring area in a future task. :)

Copy link
Contributor Author

@Myranae Myranae Nov 20, 2024

Choose a reason for hiding this comment

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

The above comment I made was from an older version of the code. I updated Rubric to be the following:

export type PerseusCategorizerScoringData = {
    // The correct answers where index relates to the items and value relates
    // to the category.  e.g. [0, 1, 0, 1, 2]
    values: ReadonlyArray<number>;
} & PerseusCategorizerValidationData;

Is it also okay to rename Rubric as we go and to use the ticket for renaming any we missed? It helps my mental model to have the names updated while I'm working, but I can totally leave the name change to the very end if it's preferred to do them all at once. And if it's preferred to do them all at once, should I revert the name change in this PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I started renaming things to ScoringData in one of my PRs. Then as I thought about it, I thought it might be more confusing to have a partial migration state in main. I was going to leave things as "Rubric" until we're done this pass of validation splitting and then we could do a single pass over everything to update our terms from Rubric to ScoringData. But I don't think it's a huge deal either way. :)

};

export type PerseusCategorizerUserInput = {
values: PerseusCategorizerRubric["values"];
};

export type PerseusCategorizerValidationData = {
// Translatable text; a list of items to categorize. e.g. ["banana", "yellow", "apple", "purple", "shirt"]
items: ReadonlyArray<string>;
};

// TODO(LEMS-2440): Can possibly be removed during 2440?
// This is not used for grading at all. The only place it is used is to define
// Props type in cs-program.tsx, but RenderProps already contains WidgetOptions
Expand Down
17 changes: 2 additions & 15 deletions packages/perseus/src/widgets/categorizer/score-categorizer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ describe("scoreCategorizer", () => {
it("gives points when the answer is correct", () => {
const rubric: PerseusCategorizerRubric = {
values: [1, 3],
items: ["apples", "oranges"],
};

const userInput = {
Expand All @@ -21,6 +22,7 @@ describe("scoreCategorizer", () => {
it("does not give points when incorrectly answered", () => {
const rubric: PerseusCategorizerRubric = {
values: [1, 3],
items: ["apples", "oranges"],
};

const userInput = {
Expand All @@ -30,19 +32,4 @@ describe("scoreCategorizer", () => {

expect(score).toHaveBeenAnsweredIncorrectly();
});

it("tells the learner its not complete if not selected", () => {
const rubric: PerseusCategorizerRubric = {
values: [1, 3],
};

const userInput = {
values: [2],
} as const;
const score = scoreCategorizer(userInput, rubric, mockStrings);

expect(score).toHaveInvalidInput(
"Make sure you select something for every row.",
);
});
});
17 changes: 7 additions & 10 deletions packages/perseus/src/widgets/categorizer/score-categorizer.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import validateCategorizer from "./validate-categorizer";

import type {PerseusStrings} from "../../strings";
import type {PerseusScore} from "../../types";
import type {
Expand All @@ -10,22 +12,17 @@ function scoreCategorizer(
rubric: PerseusCategorizerRubric,
strings: PerseusStrings,
): PerseusScore {
let completed = true;
const validationError = validateCategorizer(userInput, rubric, strings);
if (validationError) {
return validationError;
}

let allCorrect = true;
rubric.values.forEach((value, i) => {
if (userInput.values[i] == null) {
completed = false;
}
if (userInput.values[i] !== value) {
allCorrect = false;
}
});
if (!completed) {
return {
type: "invalid",
message: strings.invalidSelection,
};
}
return {
type: "points",
earned: allCorrect ? 1 : 0,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import {mockStrings} from "../../strings";

import validateCategorizer from "./validate-categorizer";

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

describe("validateCategorizer", () => {
it("tells the learner its not complete if not selected", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a test case for the null return value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added!

const rubric: PerseusCategorizerRubric = {
values: [1, 3],
items: ["apples", "oranges"],
};

const userInput = {
values: [2],
} as const;
const score = validateCategorizer(userInput, rubric, mockStrings);

expect(score).toHaveInvalidInput(
"Make sure you select something for every row.",
);
});

it("returns null if the userInput is valid", () => {
const rubric: PerseusCategorizerRubric = {
values: [1, 3],
items: ["apples", "oranges"],
};

const userInput = {
values: [2, 4],
} as const;
const score = validateCategorizer(userInput, rubric, mockStrings);

expect(score).toBeNull();
});
});
26 changes: 26 additions & 0 deletions packages/perseus/src/widgets/categorizer/validate-categorizer.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import type {PerseusStrings} from "../../strings";
import type {PerseusScore} from "../../types";
import type {
PerseusCategorizerUserInput,
PerseusCategorizerValidationData,
} from "../../validation.types";

function validateCategorizer(
userInput: PerseusCategorizerUserInput,
validationData: PerseusCategorizerValidationData,
strings: PerseusStrings,
): Extract<PerseusScore, {type: "invalid"}> | null {
const incomplete = validationData.items.some(
(_, i) => userInput.values[i] == null,
);

if (incomplete) {
return {
type: "invalid",
message: strings.invalidSelection,
};
}
return null;
}

export default validateCategorizer;