Expose a way to get user input from ServerItemRenderer #1753

handeyeco · 2024-10-16T15:06:39Z

Summary:

I'm trying to tread lightly, maybe being overly cautious at the expense of keeping too much old code around. I tried to make everything we can get rid of after the move with @deprecated.

Basically this should expose everything we need to move the actual scoring process out of ServerItemRenderer and the React tree. We now have getUserInput on ServerItemRenderer and scorePerseusItem which is a non-React, pure function that returns a score.

Next step is to replace uses of scoreInput in Webapp with scorePerseusItem; then we can come back and delete a lot of this legacy code 🤞

Issue: LEMS-2389

Test plan:

After the swap in Webapp, we should be able to complete an exercise with scorePerseusItem and everything else will work the same.

github-actions · 2024-10-16T15:10:20Z

Size Change: +581 B (+0.05%)

Total Size: 1.29 MB

Filename	Size	Change
`packages/perseus/dist/es/index.js`	421 kB	+581 B (+0.14%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	38.9 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.8 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`	699 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/perseus/dist/es/strings.js`	3.54 kB
`packages/pure-markdown/dist/es/index.js`	3.66 kB
`packages/simple-markdown/dist/es/index.js`	12.4 kB

_{compressed-size-action}

github-actions · 2024-10-16T15:10:43Z

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1753

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

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

packages/perseus/src/server-item-renderer.tsx

khan-actions-bot · 2024-11-07T16:05:50Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/rotten-starfishes-provide.md, dev/flipbook.tsx, testing/renderer-with-debug-ui.tsx, packages/perseus/src/index.ts, packages/perseus/src/renderer-util.test.ts, packages/perseus/src/renderer-util.ts, packages/perseus/src/renderer.tsx, packages/perseus/src/server-item-renderer.tsx, packages/perseus/src/components/__tests__/sorter.test.tsx, packages/perseus/src/widgets/categorizer/categorizer.test.ts, packages/perseus/src/widgets/definition/definition.test.ts, packages/perseus/src/widgets/expression/expression.test.tsx, packages/perseus/src/widgets/group/group.test.tsx, packages/perseus/src/widgets/group/group.tsx, packages/perseus/src/widgets/matcher/matcher.test.tsx, packages/perseus/src/widgets/matrix/matrix.test.ts, packages/perseus/src/widgets/deprecated-standin/__tests__/deprecated-standin.test.ts, packages/perseus/src/widgets/passage/__tests__/passage.test.tsx

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

jeremywiebe

Yay!

jeremywiebe · 2024-11-07T17:07:11Z

packages/perseus/src/renderer-util.test.ts

+        const userInput = renderer.getUserInputMap();
+        const score = scorePerseusItem(question1, userInput, mockStrings, "en");


Scoring split from user input! A red-letter day in history! 🎉

jeremywiebe · 2024-11-07T17:09:40Z

packages/perseus/src/renderer-util.ts

+    // TODO(LEMS-2461,LEMS-2391): these probably
+    // need to be removed before we move this to the server
+    strings: PerseusStrings,
+    locale: string,


I suspect we'll always need the locale for scoring, because of things like decimal separators. But localized strings I agree with you on.

jeremywiebe · 2024-11-07T17:10:42Z

packages/perseus/src/renderer-util.ts

+    // There seems to be a chance that PerseusRenderer.widgets might include
+    // widget data for widgets that are not in PerseusRenderer.content,
+    // so this checks that the widgets are being used before scoring them


Yes. Our editor doesn't (or hasn't always) cleared out widget configs when they are removed from content... so there is quite a bit of content that's been published that has unused widget configs in them.

jeremywiebe · 2024-11-07T17:13:05Z

packages/perseus/src/renderer-util.ts

 export function scoreWidgetsFunctional(
    widgets: PerseusWidgetsMap,
    // This is a port of old code, I'm not sure why
    // we need widgetIds vs the keys of the widgets object
-    widgetIds: Array<string>,
+    widgetIds: ReadonlyArray<string>,


👍 Thanks.

jeremywiebe · 2024-11-07T17:27:26Z

packages/perseus/src/renderer.tsx

-     * Returns an object mapping from widget ID to perseus-style score.
-     * The keys of this object are the values of the array returned
-     * from `getWidgetIds`.
+     * Grades the content.


nit: We've been asked to use "score" over "grade". :)

EDIT: Oh, I see that was the original comment from below. Would you mind updating it still?

Suggested change

* Grades the content.

* Scores the content.

jeremywiebe · 2024-11-13T20:51:07Z

packages/perseus/src/server-item-renderer.tsx

@@ -136,6 +139,16 @@ export class ServerItemRenderer
                this.props.onRendered(true);
            }
        }
+
+        if (this.props.score && this.props.score !== prevProps.score) {


question: Should we use a deep equality check here instead of simply instance comparison? 🤔 If the score is identical, I guess it could still mean that there are different widgets that are empty and need to be highlighted. I think I answered my own question. 😖

jeremywiebe · 2024-11-15T18:14:53Z

packages/perseus/src/server-item-renderer.tsx

+     * questionCompleted is used by Radio and LabelImage to change what
+     * it renders after a learner has checked the correctness of an answer.
+     * For instance LabelImage disables inputs for correct answers and marks
+     * incorrect answers so learners can update what they submitted.


I like referencing widgets that use the flag as an example, but I think we can word this more generically (like, what should this flag be used for). :)

So this flag is used by the renderer to signal to widgets that the question is done and they can alter what they render to reveal explanations or other information about why the learner's input was wrong, etc.

Your comment is basically good, I guess I would just suggest that we remove the wording that implies that only Radio and LabelImage use this (they do! but in the future they may not be the only ones). Sorry, I'm probably splitting hairs.

Also, it might be good to add a comment something like this to the WidgetProps<T> type as it has this flag too and it'd be helpful within widgets to be able to see that prop's intended usage.

jeremywiebe · 2024-11-15T18:18:31Z

packages/perseus/src/server-item-renderer.tsx

+     * after a learner has clicked the "check" button. I don't think this could
+     * still be used though, because the "check" button is disabled while there
+     * are empty widgets.
+     */
    questionHighlightedWidgets: ReadonlyArray<any>;


That's correct. And to be really clear, it's an array of widget IDs (strings). Could you try changing this to ReadonlyArray<string>

expose a way to get user input from SIR

09b98b3

handeyeco self-assigned this Oct 16, 2024

handeyeco changed the title ~~expose a way to get user input from SIR~~ Expose a way to get user input from SIR Oct 16, 2024

handeyeco changed the title ~~Expose a way to get user input from SIR~~ Expose a way to get user input from ServerItemRenderer Oct 16, 2024

handeyeco added 2 commits October 30, 2024 14:21

some pre-work cleanup

7eb8573

deprecate Renderer scoring methods

1d6d8e8

jeremywiebe reviewed Oct 31, 2024

View reviewed changes

packages/perseus/src/server-item-renderer.tsx Outdated Show resolved Hide resolved

handeyeco added 4 commits November 1, 2024 10:17

fix merge conflict

1739781

unfocus test

bc9bd82

add back tests I didn't need to delete

395a798

expose legacy user input

4103af1

handeyeco marked this pull request as ready for review November 7, 2024 16:05

khan-actions-bot requested a review from a team November 7, 2024 16:05

handeyeco requested a review from jeremywiebe November 7, 2024 16:10

handeyeco added 2 commits November 7, 2024 10:25

changeset

4bfc547

export scorePerseusItem

c0d6dfd

jeremywiebe approved these changes Nov 7, 2024

View reviewed changes

handeyeco added 4 commits November 7, 2024 16:37

add score as a prop I guess

0e921b8

merge main, fix conflict

159bf19

Merge branch 'main' into sir-get-user-input

d772fbe

export UserInputMap type

8f704e4

handeyeco requested review from jeremywiebe and a team November 13, 2024 19:29

jeremywiebe approved these changes Nov 13, 2024

View reviewed changes

handeyeco added 2 commits November 13, 2024 15:56

fix conflict

028ffe0

add some comments after digging in more

9482abe

jeremywiebe reviewed Nov 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose a way to get user input from ServerItemRenderer #1753

Expose a way to get user input from ServerItemRenderer #1753

handeyeco commented Oct 16, 2024 •

edited by jira bot

Loading

github-actions bot commented Oct 16, 2024 •

edited

Loading

github-actions bot commented Oct 16, 2024 •

edited

Loading

khan-actions-bot commented Nov 7, 2024 •

edited

Loading

jeremywiebe left a comment

jeremywiebe Nov 7, 2024

jeremywiebe Nov 7, 2024

jeremywiebe Nov 7, 2024

jeremywiebe Nov 7, 2024

jeremywiebe Nov 7, 2024

jeremywiebe Nov 13, 2024

jeremywiebe Nov 15, 2024

jeremywiebe Nov 15, 2024

jeremywiebe Nov 15, 2024

		const userInput = renderer.getUserInputMap();
		const score = scorePerseusItem(question1, userInput, mockStrings, "en");

Expose a way to get user input from ServerItemRenderer #1753

Are you sure you want to change the base?

Expose a way to get user input from ServerItemRenderer #1753

Conversation

handeyeco commented Oct 16, 2024 • edited by jira bot Loading

Summary:

Test plan:

github-actions bot commented Oct 16, 2024 • edited Loading

github-actions bot commented Oct 16, 2024 • edited Loading

npm Snapshot: Published

khan-actions-bot commented Nov 7, 2024 • edited Loading

Gerald

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

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

handeyeco commented Oct 16, 2024 •

edited by jira bot

Loading

github-actions bot commented Oct 16, 2024 •

edited

Loading

github-actions bot commented Oct 16, 2024 •

edited

Loading

khan-actions-bot commented Nov 7, 2024 •

edited

Loading