Skip to content

Commit

Permalink
Revert expression-editor changes to answer form key handling (#827)
Browse files Browse the repository at this point in the history
## Summary:

In #711 we added some protective parsing code for AnswerForm `key`s. However, we discovered ([Slack](https://khanacademy.slack.com/archives/C3SLM30AW/p1701199431143119)) that there are instances of data where the key is entirely missing. 

Reverting the `throw` for now to unblock our content authors. I'll continue working on this in a separate PR/deploy to fix this more correctly as Ned was onto something!

Issue: LC-1502

## Test plan:

Run the tests. They should pass.

Author: jeremywiebe

Reviewers: nedredmond

Required Reviewers:

Approved By: nedredmond

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

Pull Request URL: #827
  • Loading branch information
jeremywiebe authored Nov 30, 2023
1 parent 7cb40e4 commit c7410cc
Show file tree
Hide file tree
Showing 4 changed files with 73 additions and 7 deletions.
5 changes: 5 additions & 0 deletions .changeset/rare-books-eat.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus-editor": minor
---

Revert defensive code in ExpressionEditor that caused a crash when an expression config's answer form was missing a `key` property.
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import {Dependencies} from "@khanacademy/perseus";
import {render, screen} from "@testing-library/react";
import * as React from "react";

import "@testing-library/jest-dom";

import {testDependencies} from "../../../../../testing/test-dependencies";
import ExpressionEditor from "../expression-editor";

import type {PropsFor} from "@khanacademy/wonder-blocks-core";

describe("expression-editor", () => {
beforeEach(() => {
jest.spyOn(Dependencies, "getDependencies").mockReturnValue(
testDependencies,
);
});

it("should render", async () => {
render(<ExpressionEditor onChange={() => undefined} />);

expect(await screen.findByText(/Add new answer/)).toBeInTheDocument();
});

it("should render answerForms missing a key", async () => {
const answerForms: PropsFor<typeof ExpressionEditor>["answerForms"] = [
{
value: "x\\cdot3=y",
form: true,
simplify: true,
considered: "correct",
},
{
value: "x^2=y",
form: true,
simplify: true,
considered: "wrong",
},
{
value: "x=y\\cdotπ",
form: true,
simplify: true,
considered: "wrong",
},
];

render(
<ExpressionEditor
onChange={() => undefined}
answerForms={answerForms}
/>,
);

expect(await screen.findByText(/π/)).toBeInTheDocument();
});
});
14 changes: 7 additions & 7 deletions packages/perseus-editor/src/widgets/expression-editor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import SortableArea from "../components/sortable";
import type {PerseusExpressionWidgetOptions} from "@khanacademy/perseus";

const {InfoTip, PropCheckBox, TexButtons} = components;
const {getDependencies} = Dependencies;

type Props = {
widgetId?: any;
Expand All @@ -38,11 +37,12 @@ type DefaultProps = {
};

const parseAnswerKey = ({key}: AnswerForm): number => {
const parsedKey = Number.parseInt(key ?? "");
if (Number.isNaN(parsedKey)) {
throw new Error(`Invalid answer key: ${key}`);
}
return parsedKey;
// We don't throw here because there is data stored in some
// exercises/articles where the answer forms don't have a key. If we throw,
// it blocks content editors from loading the page at all.
// TODO(Jeremy): find a way to handle these answer forms that are missing
// keys more gracefully.
return Number.parseInt(key ?? "");
};

// Pick a key that isn't currently used by an answer in answerForms
Expand Down Expand Up @@ -182,7 +182,7 @@ class ExpressionEditor extends React.Component<Props, State> {
},
);

const {TeX} = getDependencies(); // OldExpression only
const {TeX} = Dependencies.getDependencies(); // OldExpression only

buttonSetChoices.splice(
1,
Expand Down
5 changes: 5 additions & 0 deletions tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,10 @@
"@khanacademy/pure-markdown": ["./packages/pure-markdown/src"],
"@khanacademy/simple-markdown": ["./packages/simple-markdown/src"],
},

/* Preference */
// If this is disabled, TS will sometimes just delete dead code, which
// can be confusing!
"allowUnreachableCode": true
},
}

0 comments on commit c7410cc

Please sign in to comment.