Refactor LabelImage to separate out answers from userInput into scoringData #1965

Myranae · 2024-12-06T20:35:15Z

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

PerseusLabelImageMarker was a duplicate type of InteractiveMarkerType. Copy over some of the comments that are still useful.

…ithout-rubric

github-actions · 2024-12-06T20:37:55Z

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (415f32a) and published it to npm. You
can install it using the tag PR1965.

Example:

yarn add @khanacademy/perseus@PR1965

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR1965

github-actions · 2024-12-06T20:38:14Z

Size Change: +77 B (+0.01%)

Total Size: 1.27 MB

Filename	Size	Change
`packages/perseus/dist/es/index.js`	416 kB	+77 B (+0.02%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	39 kB
`packages/keypad-context/dist/es/index.js`	760 B
`packages/kmath/dist/es/index.js`	4.27 kB
`packages/math-input/dist/es/index.js`	77.9 kB
`packages/math-input/dist/es/strings.js`	1.79 kB
`packages/perseus-core/dist/es/index.js`	1.48 kB
`packages/perseus-editor/dist/es/index.js`	688 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/perseus/dist/es/strings.js`	4.12 kB
`packages/pure-markdown/dist/es/index.js`	3.66 kB
`packages/simple-markdown/dist/es/index.js`	12.5 kB

_{compressed-size-action}

Myranae · 2024-12-06T21:27:07Z

packages/perseus/src/validation.types.ts

+export type PerseusLabelImageScoringData = {
+    markers: ReadonlyArray<MarkerAnswers>;
+};

 export type PerseusLabelImageUserInput = {
-    markers: ReadonlyArray<InteractiveMarkerType>;
+    markers: ReadonlyArray<Omit<InteractiveMarkerType, "answers">>;


These are the main changes. Changed PerseusLabelImageUserInput so it no longer includes answers in the array. Then I moved the parts of InteractiveMarkerType needed for scoring to PerseusLabelImageScoringData.

Myranae · 2024-12-06T22:22:52Z

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

-    // TODO: there's some weirdness in our types between
-    // PerseusLabelImageMarker and InteractiveMarkerType


Was able to resolve this by removing PerseusLabelImageMarker

Myranae · 2024-12-06T22:37:57Z

packages/perseus/src/widgets/label-image/score-label-image.ts

+    const combinedData = userInput.markers.map((marker, index) => {
+        return {
+            ...marker,
+            answers: scoringData.markers[index].answers,
+        };
+    });
+    const allMarkerData = {markers: combinedData};

-    for (const marker of userInput.markers) {
+    for (const marker of allMarkerData.markers) {


Data was combined here so that marker can have all the available information as scoreMarker requires an object of InteractiveMarkerType, which includes answers.

No action needed, but my gut feeling is that this is needless misdirection. I kind of feel we should just change scoreMarker:

function scoreMarker( userInput: APieceOfUserInputType, scoringData: APieceOfScoringDataType, ): InteractiveMarkerScore;

I'm fine punting on this though.

Myranae · 2024-12-06T22:39:12Z

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

-    // TODO(LEMS-2544): Investigate impact on scoring; possibly pull out &/or remove rubric parameter.
-    showRationalesForCurrentlySelectedChoices(rubric: PerseusLabelImageRubric) {
+    // TODO(LEMS-2544): Investigate impact on scoring; possibly pull out &/or remove scoringData parameter.
+    showRationalesForCurrentlySelectedChoices(


This is a bit of a problem as it requires scoringData on the client-side it seems like. Since this has a TODO associated with it, I left it for now.

This is a known issue: https://khanacademy.atlassian.net/browse/LEMS-2544

Luckily AX won't use rationales so we can punt on this for now.

khan-actions-bot · 2024-12-06T22:46:21Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/many-penguins-hug.md, packages/perseus/src/index.ts, packages/perseus/src/perseus-types.ts, packages/perseus/src/validation.types.ts, packages/perseus-editor/src/widgets/label-image-editor.tsx, packages/perseus/src/widgets/label-image/label-image.tsx, packages/perseus/src/widgets/label-image/score-label-image.test.ts, packages/perseus/src/widgets/label-image/score-label-image.ts, packages/perseus/src/widgets/label-image/types.ts, packages/perseus-editor/src/widgets/__stories__/label-image-editor.stories.tsx, packages/perseus-editor/src/widgets/label-image/marker.tsx, packages/perseus-editor/src/widgets/label-image/question-markers.tsx, packages/perseus-editor/src/widgets/label-image/__stories__/question-markers.stories.tsx

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

handeyeco

I think this is the right direction. I'm wondering though:

Will LabelImage render if we provide it with PerseusData sans-answers?
Will getUserInput work sans-answers?

Knowing how good you are about manual testing, I'm guessing the answer is "yes." I'd just like to see some automated tests to confirm.

handeyeco · 2024-12-09T16:18:26Z

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

-    // TODO(LEMS-2544): Investigate impact on scoring; possibly pull out &/or remove rubric parameter.
-    showRationalesForCurrentlySelectedChoices(rubric: PerseusLabelImageRubric) {
+    // TODO(LEMS-2544): Investigate impact on scoring; possibly pull out &/or remove scoringData parameter.
+    showRationalesForCurrentlySelectedChoices(


This is a known issue: https://khanacademy.atlassian.net/browse/LEMS-2544

Luckily AX won't use rationales so we can punt on this for now.

handeyeco · 2024-12-09T16:45:53Z

packages/perseus/src/validation.types.ts


 export type PerseusLabelImageUserInput = {
-    markers: ReadonlyArray<InteractiveMarkerType>;
+    markers: ReadonlyArray<Omit<InteractiveMarkerType, "answers">>;


It's a little higher-risk, but I think we should make this test case pass:

// label-image.test.ts describe("getUserInput", () => { it("doesn't include answer in getUserInput", async () => { // render component const {renderer} = renderQuestion(textQuestion); const userInput = renderer.getUserInputMap(); expect(userInput).toEqual({ "label-image 1": { markers: [ { label: "The fourth unlabeled bar line.", x: 25, y: 17.7, }, { label: "The third unlabeled bar line.", x: 25, y: 35.3, }, { label: "The second unlabeled bar line.", x: 25, y: 53, }, { label: "The first unlabeled bar line.", x: 25, y: 70.3, }, ], }, }); }); });

I think there should be another set of tests too:

Render a LabelImage widget using ItemData that has no answers (to simulate how it will be on the FE)

Answer the LabelImage (both wrong and right)

Use getUserInput on the Renderer to get the user input

Use scorePerseusItem to score the user input

More than anything, I'm just surprised that this ticket ended up being more of a types change whereas I was thinking it would end up being more of a logic change. Not saying you're wrong, it just makes me worried we're missing something.

handeyeco · 2024-12-09T16:46:43Z

.changeset/many-penguins-hug.md

@@ -0,0 +1,6 @@
+---
+"@khanacademy/perseus": minor


Since you changed PerseusTypes in a way that's not backwards compatible, I'd call this a major/breaking change.

Updated to major. Thanks for pointing this out!

handeyeco · 2024-12-09T16:55:01Z

packages/perseus/src/widgets/label-image/score-label-image.ts

+    const combinedData = userInput.markers.map((marker, index) => {
+        return {
+            ...marker,
+            answers: scoringData.markers[index].answers,
+        };
+    });
+    const allMarkerData = {markers: combinedData};

-    for (const marker of userInput.markers) {
+    for (const marker of allMarkerData.markers) {


No action needed, but my gut feeling is that this is needless misdirection. I kind of feel we should just change scoreMarker:

function scoreMarker( userInput: APieceOfUserInputType, scoringData: APieceOfScoringDataType, ): InteractiveMarkerScore;

I'm fine punting on this though.

handeyeco · 2024-12-09T16:55:52Z

packages/perseus/src/perseus-types.ts

@@ -1059,17 +1060,6 @@ export type PerseusLabelImageWidgetOptions = {
    static: boolean;
 };

-export type PerseusLabelImageMarker = {


@jeremywiebe is particular when it comes to changing perseus-types.ts, I would have him sign off first.

handeyeco · 2024-12-09T17:09:11Z

packages/perseus/src/widgets/label-image/types.ts

@@ -1,19 +1,21 @@
 // Base marker, with the props that are set by the editor.
-export type MarkerType = {
-    // The list of correct answers expected for the marker.
+export type MarkerAnswers = {


These types feel off to me. I'm wondering if it's actually something like:

// stuff to render type LabelImageMarker = { // Reveal the correctness state of the user selected answers for the marker. showCorrectness: "correct" | "incorrect"; focused: boolean; // Translatable Text; The text to show for the marker. Not displayed directly to the user label: string; // The marker coordinates on the question image as percent of image size. x: number; y: number; } type LabelImageMarkerScoringData = { // The list of correct answers expected for the marker. Often only one but can have multiple answers: ReadonlyArray<string>; } type LabelImageMarkerUserInput = { // The user selected list of answers, used to grade the question. selected: ReadonlyArray<string>; } type LabelImageFullMarker = LabelImageMarker & LabelImageMarkerScoringData & LabelImageMarkerUserInput;

Updated! Still need to add recommended tests and do additional manual testing though.

Based on PR feedback

jeremywiebe

Overall I like the direction of this PR. One change I'd like to see is that we keep the type defined in perseus-types.ts and then define the ScoringData for it in validation.types.ts.

jeremywiebe · 2024-12-07T01:32:13Z

packages/perseus/src/perseus-types.ts

@@ -1059,17 +1060,6 @@ export type PerseusLabelImageWidgetOptions = {
    static: boolean;
 };

-export type PerseusLabelImageMarker = {


I'm assuming you got rid of this type because it was a duplicate of what's in the widget? Could you instead, make this type the source of truth? perseus-types.ts is the source of truth for the data that we serialize out of Perseus and I've been working to keep all non-trivial type definitions for widget options here instead of in the widget folders.

Further, because TypeScript types match if they are the same shape, I was thinking about ScoringData and ValidationData for each widget as a matching subset of the widget options in this file.

Do you mean use the types in this file (perseus-types.ts) to define the ScoringData and ValidationData types (in validation.types.ts)? Also, with Matthew's suggestion, I ended up modifying the types in the label-image/types.ts file. I'll review what I did and what was here to see if I can make them work together still. Or maybe I can just leave this file alone 😂

catandthemachines

The code looks good to me! Though I would encourage you to get an approval from Matthew/Jeremy as they have more context into the scoring code 😉

catandthemachines · 2024-12-11T16:42:59Z

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

@@ -28,7 +31,7 @@ type Props = {
    imageWidth: number;
    imageHeight: number;
    // The list of label markers on the question image.
-    markers: ReadonlyArray<MarkerType>;
+    markers: ReadonlyArray<LabelImageMarker & LabelImageMarkerScoringData>;


Nit: I notice your creating this union in a lot of files. Can you combine them in the types object so you don't have to keep joining them in every file?

Great suggestion! I'll set that up. :)

We are moving scoring data and user input to validation data only

We build the wanted object instead of remove the unneeded property. The removal method was used previously to change as little as possible from the incoming markers object.

Don't want to use the ScoringData type here. Would be widget options type if the parameter returns

…ithout-rubric # Conflicts: # packages/perseus/src/index.ts

Add tests for scoring (invalid, correct, and incorrect), test for rendering without answers, and a test to confirm getUserInput works

Just confirms text from the question has been rendered on the page

jeremywiebe · 2024-12-12T23:52:30Z

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

@@ -709,4 +714,110 @@ describe("LabelImage", function () {
            });
        });
    });
+
+    describe("getUserInput", () => {
+        it("doesn't include answer in getUserInput from getUserInputMap", () => {


🤔 Can we make this test slightly more generic? I think we're less concerned about answer specifically and more concerned about getUserInputMap() returning the right data overall. So if I were to take a stab at naming this test, I would do something like:

Suggested change

it("doesn't include answer in getUserInput from getUserInputMap", () => {

it("should return the current user input on initial render", () => {

I think the focus was supposed to be making sure answer was not included, but it's also correct to confirm it's just returning the correct userInput, especially since I didn't write any expect statements specifically checking that answers are absent. I do think it's important to emphasize that the goal is to have answers out of user input, but I guess that will be obvious from the shape of UserInput. Updated :)

jeremywiebe · 2024-12-12T23:58:41Z

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

+});
+
+describe("textWithoutAnswersQuestion", () => {
+    it("should render a text question without answers", () => {


Nice! Great idea for a test as we get ready to start splitting things!!

jeremywiebe · 2024-12-13T00:00:57Z

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

+        return {
+            markers: this.props.markers.map((marker) => ({
+                selected: marker.selected,
+                label: marker.label,
+            })),
+        };


@handeyeco for visibility, Tamara and I re-wrote this function to build a return value by explicitly picking the pieces we need instead of simply return a value out of props. This help stop guarantee we're really only using values in the type and not depending elsewhere on keys that may get stripped when we do the Scoring Data stripping stuff.

jeremywiebe · 2024-12-13T00:03:01Z

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

+    x: 0,
+    y: 0,


I think we dropped x and y from this type, didn't we?

jeremywiebe · 2024-12-13T00:03:06Z

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

+    x: 0,
+    y: 0,


I think we dropped x and y from this type, didn't we?

jeremywiebe · 2024-12-13T00:04:39Z

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

            markers: [
                {
-                    ...emptyMarker,
+                    ...emptyUserInput,


I think once we update this object to only have what's in the user input type, we might not need it. It might be more clear to inline the single selected: [] bit here instead of spreading this object at that point. I'll leave it to you to decide which way you want to go. :)

Updated the tests to remove these empty types.

Myranae added 8 commits December 6, 2024 11:49

Rename MarkerType to MarkerAnswers

ee7ad47

Rename rubric to scoringData

f36f767

Remove duplicate type

8a3db65

PerseusLabelImageMarker was a duplicate type of InteractiveMarkerType. Copy over some of the comments that are still useful.

Separate answers out to scoringData

4164ebc

Combine marker data to use with scoreMarker

7fc9de8

Update tests for scoring

e694b90

Merge branch 'main' into tb/LEMS-2440/label-image-review-validation-w…

c2d75b2

…ithout-rubric

Add changeset

6ae620f

Myranae self-assigned this Dec 6, 2024

Myranae added 2 commits December 6, 2024 15:11

Update changeset

e6e2d26

Update type

95467c8

Myranae commented Dec 6, 2024

View reviewed changes

Myranae added 2 commits December 6, 2024 16:20

Fix a type change that was incorrect

d6870b8

Return order to original order

b0d5329

Myranae commented Dec 6, 2024

View reviewed changes

Myranae added 2 commits December 6, 2024 16:41

Update type to not require answers

96af0c4

Update comment to look into how scoreMarker is used

883f9bc

Myranae marked this pull request as ready for review December 6, 2024 22:46

Myranae requested review from handeyeco and jeremywiebe December 6, 2024 22:46

khan-actions-bot requested a review from a team December 6, 2024 22:46

handeyeco reviewed Dec 9, 2024

View reviewed changes

Myranae added 2 commits December 10, 2024 09:47

Update changeset

c6f7d86

Update types based on PR feedback

1579702

Update scoreMarker to keep selected and answers separate

35c610b

Based on PR feedback

jeremywiebe reviewed Dec 10, 2024

View reviewed changes

Myranae added 4 commits December 10, 2024 15:20

Add test confirming userInput does not contain answers

b7955c2

Update getUserInput to remove answers

7d8b0fe

Revert changes to perseus-types.ts

fb52876

Fixing linting

aae57e0

catandthemachines approved these changes Dec 11, 2024

View reviewed changes

Myranae added 17 commits December 12, 2024 09:26

Revert all changes to widget types

52e7d26

We are moving scoring data and user input to validation data only

Expand types and remove old widget types

5fe1979

Change types from old widget types to validation types

64e7d4b

Change from old widget types to widget options

c83fef5

Refer to props type not validation type

0dd5f68

Remove reference to ScoringData type

b0ccaf3

Update getUserInput to build wanted object

e5b0d26

We build the wanted object instead of remove the unneeded property. The removal method was used previously to change as little as possible from the incoming markers object.

Removed parameter and updated comment

78a37df

Don't want to use the ScoringData type here. Would be widget options type if the parameter returns

Merge branch 'main' into tb/LEMS-2440/label-image-review-validation-w…

2925dc8

…ithout-rubric # Conflicts: # packages/perseus/src/index.ts

Remove unnecessary export

3569bc0

Update test to reflect new type

b7b84ff

Add new tests

a433df6

Add tests for scoring (invalid, correct, and incorrect), test for rendering without answers, and a test to confirm getUserInput works

Update snapshot

aa6842e

Fix test syntax

188b6f5

Update tests to not need a snapshot

0dc8936

Just confirms text from the question has been rendered on the page

Add a todo

42a5dc3

Revert unneeded const removal

9829693

Myranae requested review from handeyeco, catandthemachines and jeremywiebe December 12, 2024 20:27

jeremywiebe approved these changes Dec 13, 2024

View reviewed changes

Myranae added 2 commits December 13, 2024 08:29

Make test name more generic

a342a49

Update tests to reflect updated type

415f32a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor LabelImage to separate out answers from userInput into scoringData #1965

Refactor LabelImage to separate out answers from userInput into scoringData #1965

Myranae commented Dec 6, 2024 •

edited

Loading

github-actions bot commented Dec 6, 2024 •

edited

Loading

github-actions bot commented Dec 6, 2024 •

edited

Loading

Myranae Dec 6, 2024 •

edited

Loading

Myranae Dec 6, 2024

Myranae Dec 6, 2024

handeyeco Dec 9, 2024

Myranae Dec 10, 2024

Myranae Dec 6, 2024

handeyeco Dec 9, 2024

khan-actions-bot commented Dec 6, 2024

handeyeco left a comment •

edited

Loading

handeyeco Dec 9, 2024

handeyeco Dec 9, 2024

handeyeco Dec 9, 2024

Myranae Dec 10, 2024

handeyeco Dec 9, 2024

handeyeco Dec 9, 2024

handeyeco Dec 9, 2024

Myranae Dec 10, 2024

jeremywiebe left a comment

jeremywiebe Dec 7, 2024

jeremywiebe Dec 10, 2024

Myranae Dec 10, 2024 •

edited

Loading

catandthemachines left a comment

catandthemachines Dec 11, 2024

Myranae Dec 11, 2024

jeremywiebe Dec 12, 2024

Myranae Dec 13, 2024 •

edited

Loading

jeremywiebe Dec 12, 2024

jeremywiebe Dec 13, 2024

jeremywiebe Dec 13, 2024

jeremywiebe Dec 13, 2024

jeremywiebe Dec 13, 2024

Myranae Dec 13, 2024

		// TODO: there's some weirdness in our types between
		// PerseusLabelImageMarker and InteractiveMarkerType

	it("doesn't include answer in getUserInput from getUserInputMap", () => {
	it("should return the current user input on initial render", () => {

Refactor LabelImage to separate out answers from userInput into scoringData #1965

Are you sure you want to change the base?

Refactor LabelImage to separate out answers from userInput into scoringData #1965

Conversation

Myranae commented Dec 6, 2024 • edited Loading

Summary:

Test plan:

github-actions bot commented Dec 6, 2024 • edited Loading

npm Snapshot: Published

github-actions bot commented Dec 6, 2024 • edited Loading

Myranae Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khan-actions-bot commented Dec 6, 2024

Gerald

handeyeco left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremywiebe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myranae Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

catandthemachines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myranae Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myranae commented Dec 6, 2024 •

edited

Loading

github-actions bot commented Dec 6, 2024 •

edited

Loading

github-actions bot commented Dec 6, 2024 •

edited

Loading

Myranae Dec 6, 2024 •

edited

Loading

handeyeco left a comment •

edited

Loading

Myranae Dec 10, 2024 •

edited

Loading

Myranae Dec 13, 2024 •

edited

Loading