-
Notifications
You must be signed in to change notification settings - Fork 351
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
SSS: Hook emptyWidgets() up to validation functions #2000
Changes from all commits
a0de84e
4e3dd54
510fa5d
ba94da8
b507fc5
5fdc3c7
c619dd8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@khanacademy/perseus": minor | ||
--- | ||
|
||
Change empty widgets check in Renderer to depend only on data available (and not on scoring data) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ import { | |
definitionItem, | ||
mockedRandomItem, | ||
mockedShuffledRadioProps, | ||
question3, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The changes in this file were needed because:
|
||
} from "../__testdata__/renderer.testdata"; | ||
import * as Dependencies from "../dependencies"; | ||
import {registerWidget} from "../widgets"; | ||
|
@@ -1605,35 +1606,36 @@ describe("renderer", () => { | |
it("should return all empty widgets", async () => { | ||
// Arrange | ||
const {renderer} = renderQuestion({ | ||
...question2, | ||
...question3, | ||
content: | ||
"Input 1: [[☃ numeric-input 1]]\n\n" + | ||
"Input 2: [[☃ numeric-input 2]]", | ||
"Input 1: [[☃ expression 1]]\n\n" + | ||
"Input 2: [[☃ expression 2]]", | ||
widgets: { | ||
...question2.widgets, | ||
"numeric-input 2": question2.widgets["numeric-input 1"], | ||
...question3.widgets, | ||
"expression 2": question3.widgets["expression 1"], | ||
}, | ||
}); | ||
await userEvent.type(screen.getAllByRole("textbox")[0], "150"); | ||
act(() => jest.runOnlyPendingTimers()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: What makes these lines necessary? (there's another one at L1683) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is because we are now using the |
||
|
||
// Act | ||
const emptyWidgets = renderer.emptyWidgets(); | ||
|
||
// Assert | ||
expect(emptyWidgets).toStrictEqual(["numeric-input 2"]); | ||
expect(emptyWidgets).toStrictEqual(["expression 2"]); | ||
}); | ||
|
||
it("should not return static widgets even if empty", () => { | ||
// Arrange | ||
const {renderer} = renderQuestion({ | ||
...question2, | ||
...question3, | ||
content: | ||
"Input 1: [[☃ numeric-input 1]]\n\n" + | ||
"Input 2: [[☃ numeric-input 2]]", | ||
"Input 1: [[☃ expression 1]]\n\n" + | ||
"Input 2: [[☃ expression 2]]", | ||
widgets: { | ||
...question2.widgets, | ||
"numeric-input 2": { | ||
...question2.widgets["numeric-input 1"], | ||
...question3.widgets, | ||
"expression 2": { | ||
...question3.widgets["expression 1"], | ||
static: true, | ||
}, | ||
}, | ||
|
@@ -1643,7 +1645,7 @@ describe("renderer", () => { | |
const emptyWidgets = renderer.emptyWidgets(); | ||
|
||
// Assert | ||
expect(emptyWidgets).toStrictEqual(["numeric-input 1"]); | ||
expect(emptyWidgets).toStrictEqual(["expression 1"]); | ||
}); | ||
|
||
it("should return widget ID for group with empty widget", () => { | ||
|
@@ -1663,7 +1665,7 @@ describe("renderer", () => { | |
JSON.stringify(simpleGroupQuestion), | ||
); | ||
simpleGroupQuestionCopy.widgets["group 1"].options.widgets[ | ||
"numeric-input 1" | ||
"expression 1" | ||
].static = true; | ||
const {renderer} = renderQuestion(simpleGroupQuestionCopy); | ||
|
||
|
@@ -1678,6 +1680,7 @@ describe("renderer", () => { | |
// Arrange | ||
const {renderer} = renderQuestion(simpleGroupQuestion); | ||
await userEvent.type(screen.getByRole("textbox"), "99"); | ||
act(() => jest.runOnlyPendingTimers()); | ||
|
||
// Act | ||
const emptyWidgets = renderer.emptyWidgets(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,20 @@ | ||
import {mapObject} from "./interactive2/objective_"; | ||
import {scoreIsEmpty, flattenScores} from "./util/scoring"; | ||
import {getWidgetIdsFromContent} from "./widget-type-utils"; | ||
import {getWidgetScorer, upgradeWidgetInfoToLatestVersion} from "./widgets"; | ||
import { | ||
getWidgetScorer, | ||
getWidgetValidator, | ||
upgradeWidgetInfoToLatestVersion, | ||
} from "./widgets"; | ||
|
||
import type {PerseusRenderer, PerseusWidgetsMap} from "./perseus-types"; | ||
import type {PerseusStrings} from "./strings"; | ||
import type {PerseusScore} from "./types"; | ||
import type {UserInput, UserInputMap} from "./validation.types"; | ||
import type { | ||
UserInput, | ||
UserInputMap, | ||
ValidationDataMap, | ||
} from "./validation.types"; | ||
|
||
export function getUpgradedWidgetOptions( | ||
oldWidgetOptions: PerseusWidgetsMap, | ||
|
@@ -33,31 +41,31 @@ export function getUpgradedWidgetOptions( | |
}); | ||
} | ||
|
||
/** | ||
* Checks the given user input to see if any answerable widgets have not been | ||
* "filled in" (ie. if they're empty). Another way to think about this | ||
* function is that its a check to see if we can score the provided input. | ||
*/ | ||
Comment on lines
+44
to
+48
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Praise: Thanks for this! 🤩 |
||
export function emptyWidgetsFunctional( | ||
widgets: PerseusWidgetsMap, | ||
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>, | ||
userInputMap: UserInputMap, | ||
strings: PerseusStrings, | ||
locale: string, | ||
): ReadonlyArray<string> { | ||
const upgradedWidgets = getUpgradedWidgetOptions(widgets); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: What makes this line no longer necessary? I looked through the PR, but not sure what's related There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not needed because this empty widgets call can (should?) only ever be called in a situation where the widget options have already been upgraded. When we are on the client, this data will be widget options from the Renderer which have already been upgraded. When we are scoring on the server, it will be scoring data that is already upgraded. Basically, I don't think this function should be exported outside of Perseus. |
||
|
||
return widgetIds.filter((id) => { | ||
const widget = upgradedWidgets[id]; | ||
if (!widget || widget.static) { | ||
const widget = widgets[id]; | ||
if (!widget || widget.static === true) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This use of |
||
// Static widgets shouldn't count as empty | ||
return false; | ||
} | ||
|
||
const scorer = getWidgetScorer(widget.type); | ||
const score = scorer?.( | ||
userInputMap[id] as UserInput, | ||
widget.options, | ||
strings, | ||
locale, | ||
); | ||
const validator = getWidgetValidator(widget.type); | ||
const userInput = userInputMap[id]; | ||
const validationData = widget.options; | ||
const score = validator?.(userInput, validationData, strings, locale); | ||
|
||
if (score) { | ||
return scoreIsEmpty(score); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -90,6 +90,7 @@ export type PerseusExpressionRubric = { | |
export type PerseusExpressionUserInput = string; | ||
|
||
export type PerseusGroupRubric = PerseusGroupWidgetOptions; | ||
export type PerseusGroupValidationData = {widgets: ValidationDataMap}; | ||
export type PerseusGroupUserInput = UserInputMap; | ||
|
||
export type PerseusGradedGroupRubric = PerseusGradedGroupWidgetOptions; | ||
|
@@ -264,6 +265,7 @@ export type UserInput = | |
| PerseusDropdownUserInput | ||
| PerseusExpressionUserInput | ||
| PerseusGrapherUserInput | ||
| PerseusGroupUserInput | ||
| PerseusIFrameUserInput | ||
| PerseusInputNumberUserInput | ||
| PerseusInteractiveGraphUserInput | ||
|
@@ -278,11 +280,73 @@ export type UserInput = | |
| PerseusSorterUserInput | ||
| PerseusTableUserInput; | ||
|
||
export type UserInputMap = {[widgetId: string]: UserInput | UserInputMap}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wasn't this here because some of the utility functions were recursive and might have nesting going on? Is that not the case anymore? May not have understood how it was used originally, so just checking :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The only widget that did that was the |
||
export type UserInputMap = {[widgetId: string]: UserInput}; | ||
|
||
/** | ||
* deprecated prefer using UserInputMap | ||
*/ | ||
export type UserInputArray = ReadonlyArray< | ||
UserInputArray | UserInput | null | undefined | ||
>; | ||
export interface ValidationDataTypes { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This uses the same construct as the new There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are the potential issues with the interface not having an entry for all widgets? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Today I'm not totally sure yet. At least, it would give us an inaccurate view of what the different types of validation data we support (we might have a type defined, but not in this "master list"). Ideally in future PRs we can derive some extra checks to ensure these interface type registries stay accurate. 🤞 |
||
categorizer: PerseusCategorizerValidationData; | ||
// "cs-program": PerseusCSProgramValidationData; | ||
// definition: PerseusDefinitionValidationData; | ||
// dropdown: PerseusDropdownRubric; | ||
// explanation: PerseusExplanationValidationData; | ||
// expression: PerseusExpressionValidationData; | ||
// grapher: PerseusGrapherValidationData; | ||
// "graded-group-set": PerseusGradedGroupSetValidationData; | ||
// "graded-group": PerseusGradedGroupValidationData; | ||
group: PerseusGroupValidationData; | ||
// iframe: PerseusIFrameValidationData; | ||
// image: PerseusImageValidationData; | ||
// "input-number": PerseusInputNumberValidationData; | ||
// interaction: PerseusInteractionValidationData; | ||
// "interactive-graph": PerseusInteractiveGraphValidationData; | ||
// "label-image": PerseusLabelImageValidationData; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm guessing you're going to do something with this commented code right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, just not sure what. |
||
// matcher: PerseusMatcherValidationData; | ||
// matrix: PerseusMatrixValidationData; | ||
// measurer: PerseusMeasurerValidationData; | ||
// "molecule-renderer": PerseusMoleculeRendererValidationData; | ||
// "number-line": PerseusNumberLineValidationData; | ||
// "numeric-input": PerseusNumericInputValidationData; | ||
// orderer: PerseusOrdererValidationData; | ||
// "passage-ref-target": PerseusRefTargetValidationData; | ||
// "passage-ref": PerseusPassageRefValidationData; | ||
// passage: PerseusPassageValidationData; | ||
// "phet-simulation": PerseusPhetSimulationValidationData; | ||
// "python-program": PerseusPythonProgramValidationData; | ||
plotter: PerseusPlotterValidationData; | ||
// radio: PerseusRadioValidationData; | ||
// sorter: PerseusSorterValidationData; | ||
// table: PerseusTableValidationData; | ||
// video: PerseusVideoValidationData; | ||
|
||
// Deprecated widgets | ||
// sequence: PerseusAutoCorrectValidationData; | ||
} | ||
|
||
/** | ||
* A map of validation data, keyed by `widgetId`. This data is used to check if | ||
* a question is answerable. This data represents the minimal intersection of | ||
* data that's available in the client (widget options) and server (scoring | ||
* data) and is represented by a group of types known as "validation data". | ||
* | ||
* NOTE: The value in this map is intentionally a subset of WidgetOptions<T>. | ||
* By using the same shape (minus any unneeded data), we are able to pass a | ||
* `PerseusWidgetsMap` or ` into any function that accepts a | ||
* `ValidationDataMap` without any mutation of data. | ||
*/ | ||
export type ValidationDataMap = { | ||
[Property in keyof ValidationDataTypes as `${Property} ${number}`]: { | ||
type: Property; | ||
static?: boolean; | ||
options: ValidationDataTypes[Property]; | ||
}; | ||
}; | ||
|
||
/** | ||
* A union type of all the different widget validation data types that exist. | ||
*/ | ||
export type ValidationData = ValidationDataTypes[keyof ValidationDataTypes]; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ import Util from "../../util"; | |
import {getPromptJSON as _getPromptJSON} from "../../widget-ai-utils/categorizer/categorizer-ai-utils"; | ||
|
||
import scoreCategorizer from "./score-categorizer"; | ||
import validateCategorizer from "./validate-categorizer"; | ||
|
||
import type {PerseusCategorizerWidgetOptions} from "../../perseus-types"; | ||
import type {Widget, WidgetExports, WidgetProps} from "../../types"; | ||
|
@@ -328,4 +329,7 @@ export default { | |
// TODO(LEMS-2656): remove TS suppression | ||
// @ts-expect-error: Type 'UserInput' is not assignable to type 'PerseusCSProgramUserInput'. | ||
scorer: scoreCategorizer, | ||
// TODO(LEMS-2656): remove TS suppression | ||
// @ts-expect-error: Type 'UserInput' is not assignable to type 'PerseusCSProgramUserInput'. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the same issue as with the The solution, I think, is to introduce more generic types on WidgetExports but that gets out of hand quickly each time I try. I'm going to leave this here for now and come back to it after this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be a fun "wind down" task next week. Might not. idk, I'm fine with error suppression for now. |
||
validator: validateCategorizer, | ||
} satisfies WidgetExports<typeof Categorizer>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I think this is right. Some of the TS is a little "big-brain" for me to understand and it looks like you might still be tidying up a little bit, but I think it's mostly good to go.
My concern/observation: this does change the behavior of
emptyWidgets
. Before we only enabled the "Check" button after a deep validation check (including the validation that requires the answers). Now we'll enable the "Check" button after a shallow validation check (only checking if the answer is scorable).This is inevitable and baked into the project. It just occurs to me that we haven't really shared this with content and support advocates. I think it'll hardly be noticeable, but what do I know.