Skip to content

Commit 0f2bec3

Browse files
authored
Refactor LabelImage to separate out answers from userInput into scoringData (#1965)
## Summary: This PR updates LabelImage so that answers are no longer available in the userInput object. This allows the scoring function to have both a userInput parameter and a scoringData parameter to keep answers separate from the user's input with the goal of supporting server side scoring. PerseusLabelImageMarker and MarkerType contained the same properties. As such, I simplified the code a bit and removed PerseusLabelImageMarker. In addition, several locations were referencing the wrong types, so those were updated to reference the correct ones. Also, new tests were added confirming the output of `getUserInput` does not contain answers, that `scorePerseusItem` returns the correct results, and that the widget renders correctly if answers are not present in the JSON blob. Issue: LEMS-2440 ## Test plan: - Confirm all checks pass - Confirm widget still works as expected Author: Myranae Reviewers: Myranae, handeyeco, jeremywiebe, catandthemachines Required Reviewers: Approved By: catandthemachines, jeremywiebe Checks: ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x) Pull Request URL: #1965
1 parent b6623bb commit 0f2bec3

14 files changed

+378
-117
lines changed

.changeset/many-penguins-hug.md

+6
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
"@khanacademy/perseus": major
3+
"@khanacademy/perseus-editor": patch
4+
---
5+
6+
Refactor the LabelImage widget to separate out answers from userInput into scoringData

packages/perseus-editor/src/widgets/__stories__/label-image-editor.stories.tsx

+2-2
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import * as React from "react";
44

55
import LabelImageEditor from "../label-image-editor";
66

7-
import type {MarkerType} from "@khanacademy/perseus";
7+
import type {PerseusLabelImageWidgetOptions} from "@khanacademy/perseus";
88

99
type StoryArgs = Record<any, any>;
1010

@@ -29,7 +29,7 @@ type State = {
2929
imageUrl: string;
3030
imageWidth: number;
3131
imageHeight: number;
32-
markers: ReadonlyArray<MarkerType>;
32+
markers: PerseusLabelImageWidgetOptions["markers"];
3333
};
3434

3535
class WithState extends React.Component<Empty, State> {

packages/perseus-editor/src/widgets/label-image-editor.tsx

+5-5
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import Behavior from "./label-image/behavior";
1717
import QuestionMarkers from "./label-image/question-markers";
1818
import SelectImage from "./label-image/select-image";
1919

20-
import type {MarkerType} from "@khanacademy/perseus";
20+
import type {PerseusLabelImageWidgetOptions} from "@khanacademy/perseus";
2121

2222
type Props = {
2323
// List of answer choices to label question image with.
@@ -28,7 +28,7 @@ type Props = {
2828
imageWidth: number;
2929
imageHeight: number;
3030
// The list of label markers on the question image.
31-
markers: ReadonlyArray<MarkerType>;
31+
markers: PerseusLabelImageWidgetOptions["markers"];
3232
// Whether multiple answer choices may be selected for markers.
3333
multipleAnswers: boolean;
3434
// Whether to hide answer choices from user instructions.
@@ -176,9 +176,9 @@ class LabelImageEditor extends React.Component<Props> {
176176
this.props.onChange({choices});
177177
};
178178

179-
handleMarkersChange: (markers: ReadonlyArray<MarkerType>) => void = (
180-
markers: ReadonlyArray<MarkerType>,
181-
) => {
179+
handleMarkersChange: (
180+
markers: PerseusLabelImageWidgetOptions["markers"],
181+
) => void = (markers: PerseusLabelImageWidgetOptions["markers"]) => {
182182
this.props.onChange({markers});
183183
};
184184

packages/perseus-editor/src/widgets/label-image/__stories__/question-markers.stories.tsx

+2-2
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import * as React from "react";
33

44
import QuestionMarkers from "../question-markers";
55

6-
import type {MarkerType} from "@khanacademy/perseus";
6+
import type {PerseusLabelImageWidgetOptions} from "@khanacademy/perseus";
77

88
type StoryArgs = Record<any, any>;
99

@@ -31,7 +31,7 @@ const Wrapper = (props) => (
3131
class WithState extends React.Component<
3232
Record<any, any>,
3333
{
34-
markers: ReadonlyArray<MarkerType>;
34+
markers: PerseusLabelImageWidgetOptions["markers"];
3535
}
3636
> {
3737
state = {

packages/perseus-editor/src/widgets/label-image/marker.tsx

+6-4
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,15 @@ import Option, {OptionGroup} from "../../components/dropdown-option";
1414
import FormWrappedTextField from "../../components/form-wrapped-text-field";
1515
import {gray17, gray85, gray98} from "../../styles/global-colors";
1616

17-
import type {MarkerType} from "@khanacademy/perseus";
17+
import type {PerseusLabelImageWidgetOptions} from "@khanacademy/perseus";
1818

19-
type Props = MarkerType & {
19+
type Props = PerseusLabelImageWidgetOptions["markers"][number] & {
2020
// The list of possible answer choices.
21-
choices: ReadonlyArray<string>;
21+
choices: PerseusLabelImageWidgetOptions["choices"];
2222
// Callback for when any of the marker props are changed.
23-
onChange: (marker: MarkerType) => void;
23+
onChange: (
24+
marker: PerseusLabelImageWidgetOptions["markers"][number],
25+
) => void;
2426
// Callback to remove marker from the question image.
2527
onRemove: () => void;
2628
};

packages/perseus-editor/src/widgets/label-image/question-markers.tsx

+3-3
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import {gray17, gray68} from "../../styles/global-colors";
1111

1212
import Marker from "./marker";
1313

14-
import type {MarkerType} from "@khanacademy/perseus";
14+
import type {PerseusLabelImageWidgetOptions} from "@khanacademy/perseus";
1515

1616
type Props = {
1717
// The list of possible answers in a specific order.
@@ -21,9 +21,9 @@ type Props = {
2121
imageWidth: number;
2222
imageHeight: number;
2323
// The list of markers placed on the question image.
24-
markers: ReadonlyArray<MarkerType>;
24+
markers: PerseusLabelImageWidgetOptions["markers"];
2525
// Callback for when any of markers change.
26-
onChange: (markers: ReadonlyArray<MarkerType>) => void;
26+
onChange: (markers: PerseusLabelImageWidgetOptions["markers"]) => void;
2727
};
2828

2929
export default class QuestionMarkers extends React.Component<Props> {

packages/perseus/src/index.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -229,6 +229,7 @@ export type {
229229
PerseusInputNumberWidgetOptions,
230230
PerseusInteractiveGraphWidgetOptions,
231231
PerseusItem,
232+
PerseusLabelImageWidgetOptions,
232233
PerseusPhetSimulationWidgetOptions,
233234
PerseusPlotterWidgetOptions,
234235
PerseusPythonProgramWidgetOptions,
@@ -241,7 +242,6 @@ export type {
241242
} from "./perseus-types";
242243
export type {UserInputMap} from "./validation.types";
243244
export type {Coord} from "./interactive2/types";
244-
export type {MarkerType} from "./widgets/label-image/types";
245245
export type {
246246
RendererPromptJSON,
247247
WidgetPromptJSON,

packages/perseus/src/validation.types.ts

+11-6
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,6 @@ import type {
4343
PerseusRadioChoice,
4444
PerseusGraphCorrectType,
4545
} from "./perseus-types";
46-
import type {InteractiveMarkerType} from "./widgets/label-image/types";
4746
import type {Relationship} from "./widgets/number-line/number-line";
4847

4948
export type UserInputStatus = "correct" | "incorrect" | "incomplete";
@@ -130,12 +129,18 @@ export type PerseusInteractiveGraphRubric = {
130129

131130
export type PerseusInteractiveGraphUserInput = PerseusGraphType;
132131

133-
/* TODO(LEMS-2440): Should be removed or refactored. Grading info may need
134-
to be moved to the rubric from userInput. */
135-
export type PerseusLabelImageRubric = Empty;
132+
export type PerseusLabelImageScoringData = {
133+
markers: ReadonlyArray<{
134+
answers: ReadonlyArray<string>;
135+
label: string;
136+
}>;
137+
};
136138

137139
export type PerseusLabelImageUserInput = {
138-
markers: ReadonlyArray<InteractiveMarkerType>;
140+
markers: ReadonlyArray<{
141+
selected?: ReadonlyArray<string>;
142+
label: string;
143+
}>;
139144
};
140145

141146
export type PerseusMatcherRubric = PerseusMatcherWidgetOptions;
@@ -237,7 +242,7 @@ export type Rubric =
237242
| PerseusGrapherRubric
238243
| PerseusInputNumberRubric
239244
| PerseusInteractiveGraphRubric
240-
| PerseusLabelImageRubric
245+
| PerseusLabelImageScoringData
241246
| PerseusMatcherRubric
242247
| PerseusMatrixRubric
243248
| PerseusNumberLineScoringData

packages/perseus/src/widgets/label-image/__tests__/label-image.test.ts

+112-1
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,15 @@ import {
66
testDependenciesV2,
77
} from "../../../../../../testing/test-dependencies";
88
import * as Dependencies from "../../../dependencies";
9+
import {scorePerseusItemTesting} from "../../../util/test-utils";
910
import {renderQuestion} from "../../__testutils__/renderQuestion";
1011
import {LabelImage} from "../label-image";
1112

12-
import {textQuestion} from "./label-image.testdata";
13+
import {
14+
shortTextQuestion,
15+
textQuestion,
16+
textWithoutAnswersQuestion,
17+
} from "./label-image.testdata";
1318

1419
import type {UserEvent} from "@testing-library/user-event";
1520

@@ -717,4 +722,110 @@ describe("LabelImage", function () {
717722
});
718723
});
719724
});
725+
726+
describe("getUserInput", () => {
727+
it("should return the current user input on initial render", () => {
728+
// render component
729+
const {renderer} = renderQuestion(textQuestion);
730+
731+
const userInput = renderer.getUserInputMap();
732+
733+
expect(userInput).toEqual({
734+
"label-image 1": {
735+
markers: [
736+
{
737+
label: "The fourth unlabeled bar line.",
738+
selected: undefined,
739+
},
740+
{
741+
label: "The third unlabeled bar line.",
742+
selected: undefined,
743+
},
744+
{
745+
label: "The second unlabeled bar line.",
746+
selected: undefined,
747+
},
748+
{
749+
label: "The first unlabeled bar line.",
750+
selected: undefined,
751+
},
752+
],
753+
},
754+
});
755+
});
756+
});
757+
758+
describe("scorePerseusItem", () => {
759+
it("should be invalid on first render", () => {
760+
// Arrange
761+
const {renderer} = renderQuestion(textQuestion);
762+
763+
// Act
764+
const score = scorePerseusItemTesting(
765+
textQuestion,
766+
renderer.getUserInputMap(),
767+
);
768+
769+
// Assert
770+
expect(score).toHaveInvalidInput();
771+
});
772+
773+
it("can be answered correctly when correct option is picked for the marker", async () => {
774+
// Arrange
775+
const {renderer} = renderQuestion(shortTextQuestion);
776+
777+
// Act
778+
const markerButton = screen.getByRole("button", {
779+
name: "The fourth unlabeled bar line.",
780+
});
781+
await userEvent.click(markerButton);
782+
783+
const choice = screen.getByRole("option", {name: "SUVs"});
784+
await userEvent.click(choice);
785+
786+
const score = scorePerseusItemTesting(
787+
shortTextQuestion,
788+
renderer.getUserInputMap(),
789+
);
790+
791+
// Assert
792+
expect(score).toHaveBeenAnsweredCorrectly();
793+
});
794+
795+
it("can be answered incorrectly when incorrect option picked for the marker", async () => {
796+
// Arrange
797+
const {renderer} = renderQuestion(shortTextQuestion);
798+
799+
// Act
800+
const markerButton = screen.getByRole("button", {
801+
name: "The fourth unlabeled bar line.",
802+
});
803+
await userEvent.click(markerButton);
804+
805+
const choice = screen.getByRole("option", {name: "Trucks"});
806+
await userEvent.click(choice);
807+
808+
const score = scorePerseusItemTesting(
809+
shortTextQuestion,
810+
renderer.getUserInputMap(),
811+
);
812+
813+
// Assert
814+
expect(score).toHaveBeenAnsweredIncorrectly();
815+
});
816+
});
817+
818+
describe("textWithoutAnswersQuestion", () => {
819+
it("should render the widget without answers", async () => {
820+
// Arrange
821+
renderQuestion(textWithoutAnswersQuestion);
822+
823+
// Act and Assert
824+
const markerButton = screen.getByRole("button", {
825+
name: "The fourth unlabeled bar line.",
826+
});
827+
// Confirms the widget renders and that marker buttons are present
828+
await userEvent.click(markerButton);
829+
});
830+
});
720831
});

0 commit comments

Comments
 (0)