Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose a way to get user input from ServerItemRenderer #1753

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
6 changes: 6 additions & 0 deletions .changeset/rotten-starfishes-provide.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@khanacademy/perseus": major
"@khanacademy/perseus-dev-ui": patch
---

Change ServerItemRenderer scoring APIs to externalize scoring
11 changes: 10 additions & 1 deletion dev/flipbook.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {useEffect, useMemo, useReducer, useRef, useState} from "react";

import {Renderer} from "../packages/perseus/src";
import {SvgImage} from "../packages/perseus/src/components";
import {scorePerseusItem} from "../packages/perseus/src/renderer-util";
import {mockStrings} from "../packages/perseus/src/strings";
import {isCorrect} from "../packages/perseus/src/util";
import {trueForAllMafsSupportedGraphTypes} from "../packages/perseus/src/widgets/interactive-graphs/mafs-supported-graph-types";
Expand Down Expand Up @@ -319,7 +320,15 @@ function GradableRenderer(props: QuestionRendererProps) {
leftContent={
<Button
onClick={() => {
setScore(rendererRef.current?.score());
if (rendererRef.current) {
const score = scorePerseusItem(
question,
rendererRef.current.getUserInputMap(),
mockStrings,
"en",
);
setScore(score);
}
clearScoreTimeout.set();
}}
>
Expand Down
12 changes: 4 additions & 8 deletions packages/perseus/src/components/__tests__/sorter.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -77,15 +77,13 @@ describe("sorter widget", () => {
const {renderer} = renderQuestion(question1, apiOptions);
const sorter = renderer.findWidgets("sorter 1")[0];

// Act
// Put the options in the correct order

["$0.005$ kilograms", "$15$ grams", "$55$ grams"].forEach((option) => {
act(() => sorter.moveOptionToIndex(option, 3));
});
// Act
renderer.guessAndScore();

// assert
// Assert
expect(renderer).toHaveBeenAnsweredCorrectly();
});
it("can be answered incorrectly", () => {
Expand All @@ -96,17 +94,15 @@ describe("sorter widget", () => {
const {renderer} = renderQuestion(question1, apiOptions);
const sorter = renderer.findWidgets("sorter 1")[0];

// Act
// Put the options in the reverse order
["$0.005$ kilograms", "$15$ grams", "$55$ grams"].forEach(
(option, index) => {
act(() => sorter.moveOptionToIndex(option, 0));
},
);

// Act
renderer.guessAndScore();

// assert
// Assert
expect(renderer).toHaveBeenAnsweredIncorrectly();
});
});
1 change: 1 addition & 0 deletions packages/perseus/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export {default as ServerItemRenderer} from "./server-item-renderer";
export {default as HintsRenderer} from "./hints-renderer";
export {default as HintRenderer} from "./hint-renderer";
export {default as Renderer} from "./renderer";
export {scorePerseusItem} from "./renderer-util";

/**
* Widgets
Expand Down
51 changes: 50 additions & 1 deletion packages/perseus/src/renderer-util.test.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,19 @@
import {emptyWidgetsFunctional, scoreWidgetsFunctional} from "./renderer-util";
import {screen} from "@testing-library/react";
import {userEvent as userEventLib} from "@testing-library/user-event";

import {
emptyWidgetsFunctional,
scorePerseusItem,
scoreWidgetsFunctional,
} from "./renderer-util";
import {mockStrings} from "./strings";
import {registerAllWidgetsForTesting} from "./util/register-all-widgets-for-testing";
import {renderQuestion} from "./widgets/__testutils__/renderQuestion";
import {question1} from "./widgets/group/group.testdata";

import type {DropdownWidget, PerseusWidgetsMap} from "./perseus-types";
import type {UserInputMap} from "./validation.types";
import type {UserEvent} from "@testing-library/user-event";

const testDropdownWidget: DropdownWidget = {
type: "dropdown",
Expand Down Expand Up @@ -469,3 +479,42 @@ describe("scoreWidgetsFunctional", () => {
expect(result["group 1"]).toHaveBeenAnsweredIncorrectly();
});
});

describe("scorePerseusItem", () => {
let userEvent: UserEvent;
beforeEach(() => {
userEvent = userEventLib.setup({
advanceTimers: jest.advanceTimersByTime,
});
});

it("should return score from contained Renderer", async () => {
// Arrange
const {renderer} = renderQuestion(question1);
// Answer all widgets correctly
await userEvent.click(screen.getAllByRole("radio")[4]);
// Note(jeremy): If we don't tab away from the radio button in this
// test, it seems like the userEvent typing doesn't land in the first
// text field.
await userEvent.tab();
await userEvent.type(
screen.getByRole("textbox", {name: /nearest ten/}),
"230",
);
await userEvent.type(
screen.getByRole("textbox", {name: /nearest hundred/}),
"200",
);
const userInput = renderer.getUserInputMap();
const score = scorePerseusItem(question1, userInput, mockStrings, "en");
Comment on lines +508 to +509
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Assert
expect(score).toHaveBeenAnsweredCorrectly();
expect(score).toEqual({
earned: 3,
message: null,
total: 3,
type: "points",
});
});
});
31 changes: 28 additions & 3 deletions packages/perseus/src/renderer-util.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import Util from "./util";
import {getWidgetIdsFromContent} from "./widget-type-utils";
import {getWidgetValidator} from "./widgets";

import type {PerseusWidgetsMap} from "./perseus-types";
import type {PerseusRenderer, PerseusWidgetsMap} from "./perseus-types";
import type {PerseusStrings} from "./strings";
import type {PerseusScore} from "./types";
import type {UserInput, UserInputMap} from "./validation.types";
Expand Down Expand Up @@ -50,11 +51,35 @@ export function emptyWidgetsFunctional(
});
}

// TODO: combine scorePerseusItem with scoreWidgetsFunctional
// once scorePerseusItem is the only one calling scoreWidgetsFunctional
export function scorePerseusItem(
perseusRenderData: PerseusRenderer,
userInputMap: UserInputMap,
// TODO(LEMS-2461,LEMS-2391): these probably
// need to be removed before we move this to the server
strings: PerseusStrings,
locale: string,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

): PerseusScore {
// 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
Comment on lines +64 to +66
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const usedWidgetIds = getWidgetIdsFromContent(perseusRenderData.content);
const scores = scoreWidgetsFunctional(
perseusRenderData.widgets,
usedWidgetIds,
userInputMap,
strings,
locale,
);
return Util.flattenScores(scores);
}

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>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks.

userInputMap: UserInputMap,
strings: PerseusStrings,
locale: string,
Expand All @@ -79,7 +104,7 @@ export function scoreWidgetsFunctional(
if (widget.type === "group") {
const scores = scoreWidgetsFunctional(
widget.options.widgets,
Object.keys(widget.options.widgets),
getWidgetIdsFromContent(widget.options.content),
userInputMap[id] as UserInputMap,
strings,
locale,
Expand Down
40 changes: 18 additions & 22 deletions packages/perseus/src/renderer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,7 @@ class Renderer extends React.Component<Props, State> {
// WidgetContainers don't update their widgets' props when
// they are re-rendered, so even if they've been
// re-rendered we need to call these methods on them.
_.each(this.widgetIds, (id) => {
this.widgetIds.forEach((id) => {
const container = this._widgetContainers.get(makeContainerId(id));
container && container.replaceWidgetProps(this.getWidgetProps(id));
});
Expand Down Expand Up @@ -1119,7 +1119,7 @@ class Renderer extends React.Component<Props, State> {
// /cry(aria)
this._foundTextNodes = true;

if (_.contains(this.widgetIds, node.id)) {
if (this.widgetIds.includes(node.id)) {
// We don't want to render a duplicate widget key/ref,
// as this causes problems with react (for obvious
// reasons). Instead we just notify the
Expand Down Expand Up @@ -1505,15 +1505,15 @@ class Renderer extends React.Component<Props, State> {

getInputPaths: () => ReadonlyArray<FocusPath> = () => {
const inputPaths: Array<FocusPath> = [];
_.each(this.widgetIds, (widgetId: string) => {
this.widgetIds.forEach((widgetId: string) => {
const widget = this.getWidgetInstance(widgetId);
if (widget && widget.getInputPaths) {
// Grab all input paths and add widgetID to the front
const widgetInputPaths = widget.getInputPaths();
// Prefix paths with their widgetID and add to collective
// list of paths.
// @ts-expect-error - TS2345 - Argument of type '(inputPath: string) => void' is not assignable to parameter of type 'CollectionIterator<FocusPath, void, readonly FocusPath[]>'.
_.each(widgetInputPaths, (inputPath: string) => {
widgetInputPaths.forEach((inputPath: string) => {
const relativeInputPath = [widgetId].concat(inputPath);
inputPaths.push(relativeInputPath);
});
Expand Down Expand Up @@ -1717,46 +1717,42 @@ class Renderer extends React.Component<Props, State> {
};

/**
* 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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*
* @deprecated use scorePerseusItem
*/
scoreWidgets(): {[widgetId: string]: PerseusScore} {
return scoreWidgetsFunctional(
score(): PerseusScore {
const scores = scoreWidgetsFunctional(
this.state.widgetInfo,
this.widgetIds,
this.getUserInputMap(),
this.props.strings,
this.context.locale,
);
}

/**
* Grades the content.
*/
score(): PerseusScore {
const scores = this.scoreWidgets();
const combinedScore = Util.flattenScores(scores);
return combinedScore;
}

guessAndScore(): [UserInputArray, PerseusScore] {
/**
* @deprecated use scorePerseusItem
*/
guessAndScore: () => [UserInputArray, PerseusScore] = () => {
const totalGuess = this.getUserInput();
const totalScore = this.score();

return [totalGuess, totalScore];
}
};

examples: () => ReadonlyArray<string> | null | undefined = () => {
const widgetIds = this.widgetIds;
const examples = _.compact(
_.map(widgetIds, (widgetId) => {
const examples = widgetIds
.map((widgetId) => {
const widget = this.getWidgetInstance(widgetId);
return widget != null && widget.examples
? widget.examples()
: null;
}),
);
})
.filter(Boolean);

// no widgets with examples
if (!examples.length) {
Expand Down
34 changes: 30 additions & 4 deletions packages/perseus/src/server-item-renderer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import HintsRenderer from "./hints-renderer";
import LoadingContext from "./loading-context";
import {ApiOptions} from "./perseus-api";
import Renderer from "./renderer";
import {scorePerseusItem} from "./renderer-util";
import Util from "./util";

import type {PerseusItem, ShowSolutions} from "./perseus-types";
Expand All @@ -27,6 +28,7 @@ import type {
PerseusDependenciesV2,
SharedRendererProps,
} from "./types";
import type {UserInputArray, UserInputMap} from "./validation.types";
import type {KeypadAPI} from "@khanacademy/math-input";
import type {
KeypadContextRendererInterface,
Expand Down Expand Up @@ -294,18 +296,42 @@ export class ServerItemRenderer
return this.props.item.hints.length;
}

/**
* Returns an array of the widget `.getUserInput()` results
*
* TODO: can we remove this? Seems to be just for backwards
* compatibility with old Perseus Chrome logging
* @deprecated use getUserInput
*/
getUserInputLegacy(): UserInputArray {
return this.questionRenderer.getUserInput();
}

/**
* Returns an object of the widget `.getUserInput()` results
*/
getUserInput(): UserInputMap {
return this.questionRenderer.getUserInputMap();
}

/**
* Grades the item.
*
* @deprecated use scorePerseusItem
*/
scoreInput(): KEScore {
const guessAndScore = this.questionRenderer.guessAndScore();
const guess = guessAndScore[0];
const score = guessAndScore[1];
const guess = this.getUserInput();
const score = scorePerseusItem(
this.props.item.question,
guess,
this.context.strings,
this.context.locale,
);

// Continue to include an empty guess for the now defunct answer area.
// TODO(alex): Check whether we rely on the format here for
// analyzing ProblemLogs. If not, remove this layer.
const maxCompatGuess = [guess, []];
const maxCompatGuess = [this.questionRenderer.getUserInput(), []];

const keScore = Util.keScoreFromPerseusScore(
score,
Expand Down
Loading