Skip to content

Commit

Permalink
Ensured that we're still validating against pi when it is provided in…
Browse files Browse the repository at this point in the history
… the answerForms. (#750)

## Summary:
This PR fixes a bug where we were no longer validating against pi when `answer.strict=false` when pi was provided in the answerForms. It looks like this is a side effect that came about with a more recent bug fix that exposed a hard-to-find historical bug. Previously we were always validating against pi by default, but we changed this after having issue with several questions being incorrectly tagged as approximations of pi. 

I updated tests to make sure that we're still validating against pi when these conditions are met.

Issue: LC-1331

## Test plan:
- new test

Author: SonicScrewdriver

Reviewers: jeremywiebe, handeyeco

Required Reviewers:

Approved By: jeremywiebe

Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage, ✅ Publish npm snapshot (ubuntu-latest, 16.x), ✅ Cypress (ubuntu-latest, 16.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 16.x), ✅ Extract i18n strings (ubuntu-latest, 16.x), ✅ Jest Coverage (ubuntu-latest, 16.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 16.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 16.x), ✅ gerald, ✅ Check builds for changes in size (ubuntu-latest, 16.x)

Pull Request URL: #750
  • Loading branch information
SonicScrewdriver authored Oct 3, 2023
1 parent 8359a9e commit 0761377
Show file tree
Hide file tree
Showing 4 changed files with 64 additions and 11 deletions.
5 changes: 5 additions & 0 deletions .changeset/eleven-jokes-wait.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": minor
---

Ensured we're still validating against pi when strict is set to false and pi is in the answerforms.
3 changes: 2 additions & 1 deletion packages/perseus/src/perseus-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -762,7 +762,8 @@ export type PerseusNumericInputAnswer = {
// The forms available for this answer. Options: "integer, ""decimal", "proper", "improper", "mixed", or "pi"
// NOTE: perseus_data.go says this is required even though it isn't necessary.
answerForms?: ReadonlyArray<MathFormat>;
// Whether the answerForms should be strictly matched
// Whether we should check the answer strictly against the the configured answerForms (strict = true)
// or include the set of default answerForms (strict = false).
strict: boolean;
// A range of error +/- the value
// NOTE: perseus_data.go says this is non-nullable even though we handle null values.
Expand Down
35 changes: 35 additions & 0 deletions packages/perseus/src/widgets/__tests__/numeric-input.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,41 @@ describe("static function validate", () => {
expect(score.message?.includes("pi")).toBeFalsy();
});

it("still validates against pi if provided in answerForms", () => {
const rubric: Rubric = {
answers: [
{
maxError: null,
message: "",
simplify: "required",
status: "correct",
strict: false,
value: 311.01767270538954,
answerForms: ["pi"],
},
],
labelText: "",
size: "normal",
static: false,
coefficient: false,
};

const userInput = {
currentValue: "99 pi",
} as const;

const score = NumericInput.validate(userInput, rubric);

expect(score).toMatchInlineSnapshot(`
{
"earned": 1,
"message": null,
"total": 1,
"type": "points",
}
`);
});

it("with a strict answer", () => {
const rubric: Rubric = {
answers: [
Expand Down
32 changes: 22 additions & 10 deletions packages/perseus/src/widgets/numeric-input.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import type {
PerseusNumericInputAnswer,
PerseusNumericInputWidgetOptions,
PerseusNumericInputAnswerForm,
MathFormat,
} from "../perseus-types";
import type {
FocusPath,
Expand All @@ -25,7 +26,11 @@ import type {

const ParseTex = TexWrangler.parseTex;

const answerFormButtons = [
const answerFormButtons: ReadonlyArray<{
title: string;
value: MathFormat;
content: string;
}> = [
{title: "Integers", value: "integer", content: "6"},
{title: "Decimals", value: "decimal", content: "0.75"},
{title: "Proper fractions", value: "proper", content: "\u2157"},
Expand Down Expand Up @@ -149,8 +154,8 @@ export class NumericInput extends React.Component<Props, State> {
return answerStrings[0];
}

static validate(useInput: UserInput, rubric: Rubric): PerseusScore {
const allAnswerForms = answerFormButtons
static validate(userInput: UserInput, rubric: Rubric): PerseusScore {
const defaultAnswerForms = answerFormButtons
.map((e) => e["value"])
// Don't default to validating the answer as a pi answer
// if answerForm isn't set on the answer
Expand All @@ -159,6 +164,18 @@ export class NumericInput extends React.Component<Props, State> {

const createValidator = (answer: PerseusNumericInputAnswer) => {
const stringAnswer = `${answer.value}`;

// Always validate against the provided answer forms (pi, decimal, etc.)
const validatorForms = [...(answer.answerForms ?? [])];

// When an answer is set to strict, we validate using ONLY
// the provided answerForms. If strict is false, or if there
// were no provided answer forms, we will include all
// of the default answer forms in our validator.
if (!answer.strict || validatorForms.length === 0) {
validatorForms.push(...defaultAnswerForms);
}

return KhanAnswerTypes.number.createValidatorFunctional(
stringAnswer,
{
Expand All @@ -169,19 +186,14 @@ export class NumericInput extends React.Component<Props, State> {
: "optional",
inexact: true, // TODO(merlob) backfill / delete
maxError: answer.maxError,
forms:
answer.strict &&
answer.answerForms &&
answer.answerForms.length !== 0
? answer.answerForms
: allAnswerForms,
forms: validatorForms,
},
);
};

// We may have received TeX; try to parse it before grading.
// If `currentValue` is not TeX, this should be a no-op.
const currentValue = ParseTex(useInput.currentValue);
const currentValue = ParseTex(userInput.currentValue);
const correctAnswers = rubric.answers.filter(
(answer) => answer.status === "correct",
);
Expand Down

0 comments on commit 0761377

Please sign in to comment.