Skip to content

Commit

Permalink
SSS: Move group scoring into group scorer (#1946)
Browse files Browse the repository at this point in the history
## Summary:

This PR refactors the scoring around `group` so that it follows the same pattern as all the other widgets. This means that our main `scoreWidgetsFunctional` can be simplified to remove the special case for the `group` widget. 

All tests still pass!

Issue: LEMS-2561

## Test plan:

`yarn test`
`yarn typecheck`

Author: jeremywiebe

Reviewers: Myranae, handeyeco, jeremywiebe

Required Reviewers:

Approved By: handeyeco

Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald

Pull Request URL: #1946
  • Loading branch information
jeremywiebe authored Dec 4, 2024
1 parent b8926e3 commit f355127
Show file tree
Hide file tree
Showing 7 changed files with 60 additions and 37 deletions.
5 changes: 5 additions & 0 deletions .changeset/metal-readers-impress.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": patch
---

Refactor scoring for `group` widget to follow the same pattern as all other widgets
5 changes: 5 additions & 0 deletions .changeset/stupid-apricots-retire.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus-editor": patch
---

Remove debugging call in GraphSettings component
1 change: 0 additions & 1 deletion packages/perseus-editor/src/components/graph-settings.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,6 @@ const GraphSettings = createReactClass({

// @ts-expect-error - TS2531 - Object is possibly 'null'. | TS2339 - Property 'value' does not exist on type 'Element | Text'.
const url = ReactDOM.findDOMNode(this.refs["bg-url"]).value; // eslint-disable-line react/no-string-refs
url; // ?
if (url) {
Util.getImageSize(url, (width, height) => {
if (this._isMounted) {
Expand Down
50 changes: 14 additions & 36 deletions packages/perseus/src/renderer-util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,27 +53,13 @@ export function emptyWidgetsFunctional(
return false;
}

let score: PerseusScore | null = null;
const userInput = userInputMap[id];
const scorer = getWidgetScorer(widget.type);

if (widget.type === "group") {
const scores = scoreWidgetsFunctional(
widget.options.widgets,
Object.keys(widget.options.widgets),
userInputMap[id] as UserInputMap,
strings,
locale,
);
score = Util.flattenScores(scores);
} else if (scorer) {
score = scorer(
userInput as UserInput,
widget.options,
strings,
locale,
);
}
const score = scorer?.(
userInputMap[id] as UserInput,
widget.options,
strings,
locale,
);

if (score) {
return Util.scoreIsEmpty(score);
Expand Down Expand Up @@ -133,22 +119,14 @@ export function scoreWidgetsFunctional(

const userInput = userInputMap[id];
const scorer = getWidgetScorer(widget.type);
if (widget.type === "group") {
const scores = scoreWidgetsFunctional(
widget.options.widgets,
getWidgetIdsFromContent(widget.options.content),
userInputMap[id] as UserInputMap,
strings,
locale,
);
widgetScores[id] = Util.flattenScores(scores);
} else if (scorer) {
widgetScores[id] = scorer(
userInput as UserInput,
widget.options,
strings,
locale,
);
const score = scorer?.(
userInput as UserInput,
widget.options,
strings,
locale,
);
if (score != null) {
widgetScores[id] = score;
}
});

Expand Down
1 change: 1 addition & 0 deletions packages/perseus/src/validation.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ export type PerseusExpressionRubric = {
export type PerseusExpressionUserInput = string;

export type PerseusGroupRubric = PerseusGroupWidgetOptions;
export type PerseusGroupUserInput = UserInputMap;

export type PerseusGradedGroupRubric = PerseusGradedGroupWidgetOptions;

Expand Down
5 changes: 5 additions & 0 deletions packages/perseus/src/widgets/group/group.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import {ApiOptions} from "../../perseus-api";
import Renderer from "../../renderer";
import {getPromptJSON as _getPromptJSON} from "../../widget-ai-utils/group/group-ai-utils";

import scoreGroup from "./score-group";

import type {PerseusGroupWidgetOptions} from "../../perseus-types";
import type {
APIOptions,
Expand Down Expand Up @@ -202,6 +204,9 @@ export default {
displayName: "Group (SAT only)",
widget: Group,
traverseChildWidgets: traverseChildWidgets,
// TODO(LEMS-2656): remove TS suppression
// @ts-expect-error: Type 'UserInput' is not assignable to type 'PerseusCSProgramUserInput'.
scorer: scoreGroup,
hidden: true,
isLintable: true,
} satisfies WidgetExports<typeof Group>;
30 changes: 30 additions & 0 deletions packages/perseus/src/widgets/group/score-group.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import {scoreWidgetsFunctional} from "../../renderer-util";
import Util from "../../util";

import type {PerseusStrings} from "../../strings";
import type {PerseusScore} from "../../types";
import type {
PerseusGroupRubric,
PerseusGroupUserInput,
} from "../../validation.types";

// The `group` widget is basically a widget hosting a full Perseus system in
// it. As such, scoring a group means scoring all widgets it contains.
function scoreGroup(
userInput: PerseusGroupUserInput,
options: PerseusGroupRubric,
strings: PerseusStrings,
locale: string,
): PerseusScore {
const scores = scoreWidgetsFunctional(
options.widgets,
Object.keys(options.widgets),
userInput,
strings,
locale,
);

return Util.flattenScores(scores);
}

export default scoreGroup;

0 comments on commit f355127

Please sign in to comment.