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.

Sign up for GitHub

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

Jump to bottom

Categorizer: Extract validation out of scoring #1862

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Categorizer: Extract validation out of scoring #1862

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e6f0b85
Split validation tests from scoring tests
Myranae Nov 14, 2024
4e1f7b2
Put validation logic in own function and call it
Myranae Nov 14, 2024
e2552af
Add changeset
Myranae Nov 14, 2024
52e6aad
Pull values (the answers) out of the validation
Myranae Nov 15, 2024
28766a3
Update validationResult to validationError
Myranae Nov 15, 2024
27c222e
Add test for null return
Myranae Nov 15, 2024
c77806a
Simplify validation check
Myranae Nov 15, 2024
539b8cc
Add validation type
Myranae Nov 18, 2024
109df1b
Update validator to use validateData instead of rubric
Myranae Nov 18, 2024
261fcf1
Rename rubric + validation data to scoringData
Myranae Nov 18, 2024
9437cfd
Add documentation to validate function
Myranae Nov 20, 2024
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/green-ghosts-burn.md
View file
Open in desktop
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.
2 changes: 2 additions & 0 deletions packages/perseus/src/validation.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ 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.

Sign in to view

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 •
edited
Loading

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 = {
Expand Down
17 changes: 2 additions & 15 deletions packages/perseus/src/widgets/categorizer/score-categorizer.test.ts
View file
Open in desktop
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
View file
Open in desktop
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 validationResult = validateCategorizer(userInput, rubric, strings);
if (validationResult) {
return validationResult;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might make things more clear

Suggested change
const validationResult = validateCategorizer(userInput, rubric, strings);
if (validationResult) {
return validationResult;
}
const validationError = validateCategorizer(userInput, rubric, strings);
if (validationError) {
return validationError;
}

Copy link
Contributor Author

@Myranae Myranae Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Updated! We might want to update the language in the table widget to be consistent with this if this is preferred.


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
23 changes: 23 additions & 0 deletions packages/perseus/src/widgets/categorizer/validate-categorizer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
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.",
);
});
});
28 changes: 28 additions & 0 deletions packages/perseus/src/widgets/categorizer/validate-categorizer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import type {PerseusStrings} from "../../strings";
import type {PerseusScore} from "../../types";
import type {
PerseusCategorizerRubric,
PerseusCategorizerUserInput,
} from "../../validation.types";

function validateCategorizer(
userInput: PerseusCategorizerUserInput,
rubric: PerseusCategorizerRubric,
Copy link
Contributor

@handeyeco handeyeco Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

CC @jeremywiebe

I think this highlights a possible need for more even more clarification of types (or maybe just a helper):

(In the examples I use feRubric as the stuff we need to validate and beRubric as the stuff we need to validate and score.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I love this! We just have to make sure whatever the info on rubric is that it doesn't convey the answer in any way. This definitely clarifies things.

strings: PerseusStrings,
): Extract<PerseusScore, {type: "invalid"}> | null {
let completed = true;
rubric.items.forEach((_, i) => {
if (userInput.values[i] == null) {
completed = false;
}
});
if (!completed) {
return {
type: "invalid",
message: strings.invalidSelection,
};
}
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is just moved code, but what about something like:

Suggested change
let completed = true;
rubric.items.forEach((_, i) => {
if (userInput.values[i] == null) {
completed = false;
}
});
if (!completed) {
return {
type: "invalid",
message: strings.invalidSelection,
};
}
return null;
let incomplete = rubric.items.some((_, i) => userInput.values[i] == null);
if (incomplete) {
return {
type: "invalid",
message: strings.invalidSelection,
};
}
return null;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm all for simplifying. I'll try to simplify the validation logic in the future if it seems like a straightforward update. Was trying to leave as close to original as possible as we've mentioned not changing the logic, but I think just simplifying what's there is probably fine. Updated :)

}

export default validateCategorizer;