From 0dd393a15da31932cfd36178d4122102cf7e9c7a Mon Sep 17 00:00:00 2001 From: Jeremy Wiebe Date: Wed, 11 Dec 2024 17:39:46 -0800 Subject: [PATCH 01/12] Improve types for validation --- packages/perseus/src/validation.types.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/perseus/src/validation.types.ts b/packages/perseus/src/validation.types.ts index 0388756cc3..4205ceeddd 100644 --- a/packages/perseus/src/validation.types.ts +++ b/packages/perseus/src/validation.types.ts @@ -1,7 +1,7 @@ /** * This file contains types used for validation and scoring. The types abide by * a naming convention so that they're easy to follow and that we remain - * consistent across all of the widgets. + * consistency across all of the widgets. * * These types are: * @@ -281,6 +281,11 @@ export type ScoringDataMap = { export type ScoringData = ScoringDataRegistry[keyof ScoringDataRegistry]; +export type ScoringRenderer = { + content: string; + widgets: ScoringDataMap; +}; + /** * This is an interface so that it can be extended if a widget is created * outside of this Perseus package. See `PerseusWidgetTypes` for a full @@ -320,7 +325,7 @@ export type UserInput = UserInputRegistry[keyof UserInputRegistry]; export type UserInputMap = MakeWidgetMap; /** - * deprecated prefer using UserInputMap + * @deprecated prefer using UserInputMap */ export type UserInputArray = ReadonlyArray< UserInputArray | UserInput | null | undefined From ca62e4a85a8e260f51066ab88cbbbac2ed9b7333 Mon Sep 17 00:00:00 2001 From: Jeremy Wiebe Date: Thu, 12 Dec 2024 18:38:13 -0800 Subject: [PATCH 02/12] Revert @deprecated change to just deprecated --- packages/perseus/src/renderer.tsx | 1 + packages/perseus/src/validation.types.ts | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/perseus/src/renderer.tsx b/packages/perseus/src/renderer.tsx index 3259bdc3b0..76a473d32d 100644 --- a/packages/perseus/src/renderer.tsx +++ b/packages/perseus/src/renderer.tsx @@ -48,6 +48,7 @@ import type { Widget, WidgetProps, } from "./types"; +// eslint-disable-next-line import/no-deprecated import type {UserInputArray, UserInputMap} from "./validation.types"; import type { GetPromptJSONInterface, diff --git a/packages/perseus/src/validation.types.ts b/packages/perseus/src/validation.types.ts index 4205ceeddd..b041f21b7a 100644 --- a/packages/perseus/src/validation.types.ts +++ b/packages/perseus/src/validation.types.ts @@ -325,7 +325,7 @@ export type UserInput = UserInputRegistry[keyof UserInputRegistry]; export type UserInputMap = MakeWidgetMap; /** - * @deprecated prefer using UserInputMap + * deprecated prefer using UserInputMap */ export type UserInputArray = ReadonlyArray< UserInputArray | UserInput | null | undefined From 36cc175b1c540178accc943824296045c710d6dd Mon Sep 17 00:00:00 2001 From: Jeremy Wiebe Date: Mon, 16 Dec 2024 11:24:44 -0800 Subject: [PATCH 03/12] REmove unused eslint suppression --- packages/perseus/src/renderer.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/perseus/src/renderer.tsx b/packages/perseus/src/renderer.tsx index 76a473d32d..3259bdc3b0 100644 --- a/packages/perseus/src/renderer.tsx +++ b/packages/perseus/src/renderer.tsx @@ -48,7 +48,6 @@ import type { Widget, WidgetProps, } from "./types"; -// eslint-disable-next-line import/no-deprecated import type {UserInputArray, UserInputMap} from "./validation.types"; import type { GetPromptJSONInterface, From 13d07d56fae3bb8458ffdff0e14cbd1f4cecc135 Mon Sep 17 00:00:00 2001 From: Jeremy Wiebe Date: Mon, 16 Dec 2024 11:26:43 -0800 Subject: [PATCH 04/12] Fix comment --- packages/perseus/src/validation.types.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/perseus/src/validation.types.ts b/packages/perseus/src/validation.types.ts index b041f21b7a..610ee53025 100644 --- a/packages/perseus/src/validation.types.ts +++ b/packages/perseus/src/validation.types.ts @@ -1,7 +1,7 @@ /** * This file contains types used for validation and scoring. The types abide by * a naming convention so that they're easy to follow and that we remain - * consistency across all of the widgets. + * consistent across all of the widgets. * * These types are: * From 095e0e435ad155944cea89599371ac373c97de1c Mon Sep 17 00:00:00 2001 From: Jeremy Wiebe Date: Mon, 16 Dec 2024 17:38:57 -0800 Subject: [PATCH 05/12] Add missing (and now needed) version 1.0 to expression widget config --- packages/perseus/src/__testdata__/renderer.testdata.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/perseus/src/__testdata__/renderer.testdata.ts b/packages/perseus/src/__testdata__/renderer.testdata.ts index c95e1271de..19b1dd0410 100644 --- a/packages/perseus/src/__testdata__/renderer.testdata.ts +++ b/packages/perseus/src/__testdata__/renderer.testdata.ts @@ -23,6 +23,7 @@ export const expressionWidget: ExpressionWidget = { functions: [], times: true, }, + version: {major: 1, minor: 0}, }; export const dropdownWidget: DropdownWidget = { From 3f6cd2339eaa668565fe224ffc785faa9e37c4e7 Mon Sep 17 00:00:00 2001 From: Jeremy Wiebe Date: Wed, 18 Dec 2024 16:39:29 -0800 Subject: [PATCH 06/12] Hook empty widgets check into scoring --- packages/perseus/src/renderer-util.test.ts | 163 ++++++++++++++++++--- packages/perseus/src/renderer-util.ts | 24 ++- 2 files changed, 165 insertions(+), 22 deletions(-) diff --git a/packages/perseus/src/renderer-util.test.ts b/packages/perseus/src/renderer-util.test.ts index 2bdfc26fca..306d4ca9f3 100644 --- a/packages/perseus/src/renderer-util.test.ts +++ b/packages/perseus/src/renderer-util.test.ts @@ -1,4 +1,4 @@ -import {screen} from "@testing-library/react"; +import {act, screen} from "@testing-library/react"; import {userEvent as userEventLib} from "@testing-library/user-event"; import { @@ -6,6 +6,7 @@ import { 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,148 @@ describe("renderer utils", () => { }); }); + it("should return empty if any validator returns empty", () => { + // Act + const validatorSpy = jest + .spyOn(DropdownWidgetExport, "validator") + // Empty + .mockReturnValueOnce({ + type: "invalid", + message: null, + }) + // Not empty + .mockReturnValueOnce(null); + const scoringSpy = jest.spyOn(DropdownWidgetExport, "scorer"); + + // 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); + expect(scoringSpy).not.toHaveBeenCalled(); + 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).toHaveBeenCalled(); + 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 +897,6 @@ describe("renderer utils", () => { // Assert expect(score).toHaveBeenAnsweredCorrectly(); - expect(score).toEqual({ - earned: 3, - message: null, - total: 3, - type: "points", - }); }); }); }); diff --git a/packages/perseus/src/renderer-util.ts b/packages/perseus/src/renderer-util.ts index 9b7826a6aa..01e6759415 100644 --- a/packages/perseus/src/renderer-util.ts +++ b/packages/perseus/src/renderer-util.ts @@ -20,6 +20,16 @@ import type { PerseusWidgetsMap, } from "@khanacademy/perseus-core"; +function mapWidgetIdsToEmptyScore(widgetIds: ReadonlyArray): { + [widgetId: string]: PerseusScore; +} { + const emptyResult: {[widgetId: string]: PerseusScore} = {}; + widgetIds.forEach((widgetId) => { + emptyResult[widgetId] = {type: "invalid", message: null}; + }); + return emptyResult; +} + export function getUpgradedWidgetOptions( oldWidgetOptions: PerseusWidgetsMap, ): PerseusWidgetsMap { @@ -54,7 +64,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, + widgetIds: ReadonlyArray, userInputMap: UserInputMap, strings: PerseusStrings, locale: string, @@ -112,6 +122,18 @@ export function scoreWidgetsFunctional( ): {[widgetId: string]: PerseusScore} { const upgradedWidgets = getUpgradedWidgetOptions(widgets); + // Do empty check first + const emptyWidgets = emptyWidgetsFunctional( + widgets, + widgetIds, + userInputMap, + strings, + locale, + ); + if (emptyWidgets.length > 0) { + return mapWidgetIdsToEmptyScore(emptyWidgets); + } + const gradedWidgetIds = widgetIds.filter((id) => { const props = upgradedWidgets[id]; const widgetIsGraded: boolean = props?.graded == null || props.graded; From 36eed10662965eba0863d2adbc73dad1b0872e6d Mon Sep 17 00:00:00 2001 From: Jeremy Wiebe Date: Wed, 8 Jan 2025 07:54:52 -0800 Subject: [PATCH 07/12] Remove unused type --- packages/perseus/src/validation.types.ts | 5 ----- 1 file changed, 5 deletions(-) diff --git a/packages/perseus/src/validation.types.ts b/packages/perseus/src/validation.types.ts index 610ee53025..0388756cc3 100644 --- a/packages/perseus/src/validation.types.ts +++ b/packages/perseus/src/validation.types.ts @@ -281,11 +281,6 @@ export type ScoringDataMap = { export type ScoringData = ScoringDataRegistry[keyof ScoringDataRegistry]; -export type ScoringRenderer = { - content: string; - widgets: ScoringDataMap; -}; - /** * This is an interface so that it can be extended if a widget is created * outside of this Perseus package. See `PerseusWidgetTypes` for a full From bf5902d47530edf0ffce3da4cd70f70269ecfe66 Mon Sep 17 00:00:00 2001 From: Jeremy Wiebe Date: Wed, 8 Jan 2025 08:21:13 -0800 Subject: [PATCH 08/12] Remove explicit version that isn't actually needed --- packages/perseus/src/__testdata__/renderer.testdata.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/perseus/src/__testdata__/renderer.testdata.ts b/packages/perseus/src/__testdata__/renderer.testdata.ts index 19b1dd0410..c95e1271de 100644 --- a/packages/perseus/src/__testdata__/renderer.testdata.ts +++ b/packages/perseus/src/__testdata__/renderer.testdata.ts @@ -23,7 +23,6 @@ export const expressionWidget: ExpressionWidget = { functions: [], times: true, }, - version: {major: 1, minor: 0}, }; export const dropdownWidget: DropdownWidget = { From 305a53e0efd53d2eee6e533421b0cb274d9ca948 Mon Sep 17 00:00:00 2001 From: Jeremy Wiebe Date: Wed, 8 Jan 2025 08:59:24 -0800 Subject: [PATCH 09/12] docs(changeset): Use empty widgets check in scoring function --- .changeset/pink-pumas-hug.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/pink-pumas-hug.md diff --git a/.changeset/pink-pumas-hug.md b/.changeset/pink-pumas-hug.md new file mode 100644 index 0000000000..d37b81ea10 --- /dev/null +++ b/.changeset/pink-pumas-hug.md @@ -0,0 +1,5 @@ +--- +"@khanacademy/perseus": minor +--- + +Use empty widgets check in scoring function From 3f7dade6868db844065724c6277401c01fc03a9c Mon Sep 17 00:00:00 2001 From: Jeremy Wiebe Date: Fri, 17 Jan 2025 14:43:17 -0800 Subject: [PATCH 10/12] Fix assertion - thanks Tamara --- packages/perseus/src/renderer-util.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/perseus/src/renderer-util.test.ts b/packages/perseus/src/renderer-util.test.ts index 306d4ca9f3..0f4e1ff3ad 100644 --- a/packages/perseus/src/renderer-util.test.ts +++ b/packages/perseus/src/renderer-util.test.ts @@ -814,7 +814,7 @@ describe("renderer utils", () => { // Assert expect(validatorSpy).toHaveBeenCalledTimes(2); - expect(scoreSpy).toHaveBeenCalled(); + expect(scoreSpy).toHaveBeenCalledTimes(2); expect(score).toEqual({ type: "points", total: 2, From a11fa1a9b5d839d8703693e6e2e945df8d50c59c Mon Sep 17 00:00:00 2001 From: Jeremy Wiebe Date: Fri, 17 Jan 2025 18:24:14 -0800 Subject: [PATCH 11/12] Re-work where validation is called in scoring --- packages/perseus/src/renderer-util.test.ts | 11 ++++--- packages/perseus/src/renderer-util.ts | 35 +++++----------------- 2 files changed, 14 insertions(+), 32 deletions(-) diff --git a/packages/perseus/src/renderer-util.test.ts b/packages/perseus/src/renderer-util.test.ts index 0f4e1ff3ad..0a5fe26905 100644 --- a/packages/perseus/src/renderer-util.test.ts +++ b/packages/perseus/src/renderer-util.test.ts @@ -749,14 +749,16 @@ describe("renderer utils", () => { // Act const validatorSpy = jest .spyOn(DropdownWidgetExport, "validator") - // Empty + // 1st call - Empty .mockReturnValueOnce({ type: "invalid", message: null, }) - // Not empty + // 2nd call - Not empty .mockReturnValueOnce(null); - const scoringSpy = jest.spyOn(DropdownWidgetExport, "scorer"); + const scoringSpy = jest + .spyOn(DropdownWidgetExport, "scorer") + .mockReturnValueOnce({type: "points", total: 1, earned: 1}); // Act const score = scorePerseusItem( @@ -777,7 +779,8 @@ describe("renderer utils", () => { // Assert expect(validatorSpy).toHaveBeenCalledTimes(2); - expect(scoringSpy).not.toHaveBeenCalled(); + // Scoring is only called if validation passes + expect(scoringSpy).toHaveBeenCalledTimes(1); expect(score).toEqual({type: "invalid", message: null}); }); diff --git a/packages/perseus/src/renderer-util.ts b/packages/perseus/src/renderer-util.ts index 01e6759415..409626c614 100644 --- a/packages/perseus/src/renderer-util.ts +++ b/packages/perseus/src/renderer-util.ts @@ -20,16 +20,6 @@ import type { PerseusWidgetsMap, } from "@khanacademy/perseus-core"; -function mapWidgetIdsToEmptyScore(widgetIds: ReadonlyArray): { - [widgetId: string]: PerseusScore; -} { - const emptyResult: {[widgetId: string]: PerseusScore} = {}; - widgetIds.forEach((widgetId) => { - emptyResult[widgetId] = {type: "invalid", message: null}; - }); - return emptyResult; -} - export function getUpgradedWidgetOptions( oldWidgetOptions: PerseusWidgetsMap, ): PerseusWidgetsMap { @@ -122,18 +112,6 @@ export function scoreWidgetsFunctional( ): {[widgetId: string]: PerseusScore} { const upgradedWidgets = getUpgradedWidgetOptions(widgets); - // Do empty check first - const emptyWidgets = emptyWidgetsFunctional( - widgets, - widgetIds, - userInputMap, - strings, - locale, - ); - if (emptyWidgets.length > 0) { - return mapWidgetIdsToEmptyScore(emptyWidgets); - } - const gradedWidgetIds = widgetIds.filter((id) => { const props = upgradedWidgets[id]; const widgetIsGraded: boolean = props?.graded == null || props.graded; @@ -150,13 +128,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; } From fc63b15acf25c55b0b628659ca91369394f50587 Mon Sep 17 00:00:00 2001 From: Jeremy Wiebe Date: Fri, 17 Jan 2025 18:36:01 -0800 Subject: [PATCH 12/12] Remove unused import --- .../src/__testdata__/renderer.testdata.ts | 1 - packages/perseus/src/renderer-util.ts | 16 ++++++---------- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/packages/perseus/src/__testdata__/renderer.testdata.ts b/packages/perseus/src/__testdata__/renderer.testdata.ts index c95e1271de..279a82a5fd 100644 --- a/packages/perseus/src/__testdata__/renderer.testdata.ts +++ b/packages/perseus/src/__testdata__/renderer.testdata.ts @@ -3,7 +3,6 @@ import type { DropdownWidget, ExpressionWidget, ImageWidget, - NumericInputWidget, MockWidget, PerseusRenderer, } from "@khanacademy/perseus-core"; diff --git a/packages/perseus/src/renderer-util.ts b/packages/perseus/src/renderer-util.ts index 409626c614..b0a059042c 100644 --- a/packages/perseus/src/renderer-util.ts +++ b/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,