[Client Validation] Use emptyWidgetsFunctional in scoring (#2083)

This PR completes the work of extracting validation logic from scoring logic. This retains most of the validation that used to be intermingled with scoring. 

This means that even when we strip scoring data from the widget options, we'll still be able to check if an answer is scorable. 

Notably: `input-number` and `numeric-input` missed the train here. Both of these widgets use `KhanAnswerTypes` right at the beginning of scoring. Further, the `coefficient` answer type [allows](https://github.com/Khan/perseus/blob/f7160d66f6e0185dd11d8b802ad88f94cf4b92dd/packages/perseus/src/util/answer-types.ts#L394) an empty value (`""`) and bare negative (`"-"`) to be treated as answers by coercing them to `1` and `-1` respectively. This means that we cannot do any validation/empty checking for these widgets because we need the full `KhanAnswerTypes` logic (which requires scoring data to work). 

Issue: LEMS-2561

# Test plan: 

`yarn test`
`yarn typecheck`

Author: jeremywiebe

Reviewers: handeyeco, Myranae, jeremywiebe

Required Reviewers:

Approved By: handeyeco, Myranae

Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (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: #2083

.changeset/pink-pumas-hug.md

@@ -0,0 +1,5 @@
+---
+"@khanacademy/perseus": minor
+---
+
+Use empty widgets check in scoring function

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

@@ -1,11 +1,12 @@
-import {screen} from "@testing-library/react";
+import {act, screen} from "@testing-library/react";
 import {userEvent as userEventLib} from "@testing-library/user-event";
 
 import {
     testDependencies,
     testDependenciesV2,
 } from "../../../testing/test-dependencies";
 
+import {question1} from "./__testdata__/renderer.testdata";
 import * as Dependencies from "./dependencies";
 import {
     emptyWidgetsFunctional,
@@ -15,7 +16,7 @@ import {
 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 DropdownWidgetExport from "./widgets/dropdown";
 
 import type {UserInputMap} from "./validation.types";
 import type {
@@ -78,6 +79,7 @@ function getLegacyExpressionWidget() {
         },
     };
 }
+
 describe("renderer utils", () => {
     beforeAll(() => {
         registerAllWidgetsForTesting();
@@ -743,23 +745,151 @@ describe("renderer utils", () => {
             });
         });
 
+        it("should return empty if any validator returns empty", () => {
+            // Act
+            const validatorSpy = jest
+                .spyOn(DropdownWidgetExport, "validator")
+                // 1st call - Empty
+                .mockReturnValueOnce({
+                    type: "invalid",
+                    message: null,
+                })
+                // 2nd call - Not empty
+                .mockReturnValueOnce(null);
+            const scoringSpy = jest
+                .spyOn(DropdownWidgetExport, "scorer")
+                .mockReturnValueOnce({type: "points", total: 1, earned: 1});
+
+            // Act
+            const score = scorePerseusItem(
+                {
+                    content:
+                        question1.content +
+                        question1.content.replace("dropdown 1", "dropdown 2"),
+                    widgets: {
+                        "dropdown 1": question1.widgets["dropdown 1"],
+                        "dropdown 2": question1.widgets["dropdown 1"],
+                    },
+                    images: {},
+                },
+                {},
+                mockStrings,
+                "en",
+            );
+
+            // Assert
+            expect(validatorSpy).toHaveBeenCalledTimes(2);
+            // Scoring is only called if validation passes
+            expect(scoringSpy).toHaveBeenCalledTimes(1);
+            expect(score).toEqual({type: "invalid", message: null});
+        });
+
+        it("should score item if all validators return null", () => {
+            // Arrange
+            const validatorSpy = jest
+                .spyOn(DropdownWidgetExport, "validator")
+                .mockReturnValue(null);
+            const scoreSpy = jest
+                .spyOn(DropdownWidgetExport, "scorer")
+                .mockReturnValue({
+                    type: "points",
+                    total: 1,
+                    earned: 1,
+                    message: null,
+                });
+
+            // Act
+            const score = scorePerseusItem(
+                {
+                    content:
+                        question1.content +
+                        question1.content.replace("dropdown 1", "dropdown 2"),
+                    widgets: {
+                        "dropdown 1": question1.widgets["dropdown 1"],
+                        "dropdown 2": question1.widgets["dropdown 1"],
+                    },
+                    images: {},
+                },
+                {"dropdown 1": {value: 0}},
+                mockStrings,
+                "en",
+            );
+
+            // Assert
+            expect(validatorSpy).toHaveBeenCalledTimes(2);
+            expect(scoreSpy).toHaveBeenCalledTimes(2);
+            expect(score).toEqual({
+                type: "points",
+                total: 2,
+                earned: 2,
+                message: null,
+            });
+        });
+
+        it("should return correct, with no points earned, if widget is static", () => {
+            const validatorSpy = jest.spyOn(DropdownWidgetExport, "validator");
+
+            const score = scorePerseusItem(
+                {
+                    ...question1,
+                    widgets: {
+                        "dropdown 1": {
+                            ...question1.widgets["dropdown 1"],
+                            static: true,
+                        },
+                    },
+                },
+                {"dropdown 1": {value: 1}},
+                mockStrings,
+                "en",
+            );
+
+            expect(validatorSpy).not.toHaveBeenCalled();
+            expect(score).toHaveBeenAnsweredCorrectly({
+                shouldHavePoints: false,
+            });
+        });
+
+        it("should ignore widgets that aren't referenced in content", () => {
+            const validatorSpy = jest.spyOn(DropdownWidgetExport, "validator");
+            const score = scorePerseusItem(
+                {
+                    content:
+                        "This content references [[☃ dropdown 1]] but not dropdown 2!",
+                    widgets: {
+                        ...question1.widgets,
+                        "dropdown 2": {
+                            ...question1.widgets["dropdown 1"],
+                        },
+                    },
+                    images: {},
+                },
+                {"dropdown 1": {value: 2}},
+                mockStrings,
+                "en",
+            );
+
+            expect(validatorSpy).toHaveBeenCalledTimes(1);
+            expect(score).toHaveBeenAnsweredCorrectly({
+                shouldHavePoints: true,
+            });
+        });
+
         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",
+
+            // Answer correctly
+            await userEvent.click(screen.getByRole("combobox"));
+            await act(() => jest.runOnlyPendingTimers());
+
+            await userEvent.click(
+                screen.getByRole("option", {
+                    name: "less than or equal to",
+                }),
             );
+
+            // Act
             const userInput = renderer.getUserInputMap();
             const score = scorePerseusItem(
                 question1,
@@ -770,12 +900,6 @@ describe("renderer utils", () => {
 
             // Assert
             expect(score).toHaveBeenAnsweredCorrectly();
-            expect(score).toEqual({
-                earned: 3,
-                message: null,
-                total: 3,
-                type: "points",
-            });
         });
     });
 });

packages/perseus/src/renderer-util.ts

@@ -1,4 +1,8 @@
-import {mapObject} from "@khanacademy/perseus-core";
+import {
+    mapObject,
+    type PerseusRenderer,
+    type PerseusWidgetsMap,
+} from "@khanacademy/perseus-core";
 
 import {scoreIsEmpty, flattenScores} from "./util/scoring";
 import {getWidgetIdsFromContent} from "./widget-type-utils";
@@ -10,15 +14,7 @@ import {
 
 import type {PerseusStrings} from "./strings";
 import type {PerseusScore} from "./types";
-import type {
-    UserInput,
-    UserInputMap,
-    ValidationDataMap,
-} from "./validation.types";
-import type {
-    PerseusRenderer,
-    PerseusWidgetsMap,
-} from "@khanacademy/perseus-core";
+import type {ValidationDataMap, UserInputMap} from "./validation.types";
 
 export function getUpgradedWidgetOptions(
     oldWidgetOptions: PerseusWidgetsMap,
@@ -54,7 +50,7 @@ export function emptyWidgetsFunctional(
     widgets: ValidationDataMap,
     // 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>,
     userInputMap: UserInputMap,
     strings: PerseusStrings,
     locale: string,
@@ -128,13 +124,14 @@ export function scoreWidgetsFunctional(
         }
 
         const userInput = userInputMap[id];
+        const validator = getWidgetValidator(widget.type);
         const scorer = getWidgetScorer(widget.type);
-        const score = scorer?.(
-            userInput as UserInput,
-            widget.options,
-            strings,
-            locale,
-        );
+
+        // We do validation (empty checks) first and then scoring. If
+        // validation fails, it's result is itself a PerseusScore.
+        const score =
+            validator?.(userInput, widget.options, strings, locale) ??
+            scorer?.(userInput, widget.options, strings, locale);
         if (score != null) {
             widgetScores[id] = score;
         }