[LEMS-2849] Answerless Radio #2233
Conversation
|
Size Change: +710 B (+0.08%) Total Size: 871 kB
ℹ️ View Unchanged
|
handeyeco
changed the title
| } | ||
|
|
||
| const version0 = optional(object({major: constant(0), minor: number})); | ||
| const parseRadioWidgetV0: Parser<RadioWidget> = parseWidgetWithVersion( |
There was a problem hiding this comment.
So Radio version 0 and 1 already existed, but for some reason there wasn't an existing migration for Radio. The parser got upset with me when I tried to add a migration from 1 to 2 without having one for 0 to 1.
There was a problem hiding this comment.
I didn't write a migration for v0 -> v1 because it's fairly trivial and I don't think hasNoneOfTheAbove is even used anymore.
"1": (v0props: any): PerseusRadioWidgetOptions => {
const {noneOfTheAbove, ...rest} = v0props;
if (noneOfTheAbove) {
throw new Error(
"radio widget v0 no longer supports auto noneOfTheAbove",
);
}
return {
...rest,
hasNoneOfTheAbove: false,
};
},
| const numCorrect: number = _.reduce( | ||
| widgetOptions.choices, | ||
| function (memo, choice) { | ||
| return choice.correct ? memo + 1 : memo; | ||
| }, | ||
| 0, | ||
| ); |
There was a problem hiding this comment.
The main idea is to pull this logic out of transform and call it during the upgrade process. That way the widget can do all the pre-scoring things it needs to without answers (ie. render and be answerable).
| // assert that functionality previous based on answers still works | ||
| expect(screen.getByRole("group", {name: "Choose 2 answers:"})); | ||
|
|
||
| const userInput = renderer.getUserInputMap(); |
There was a problem hiding this comment.
This is important because we can do everything we need to do pre-scoring with the answerless data now as far as I can tell.
handeyeco
requested review from
jeremywiebe,
benchristel,
Myranae and
a team
| return { | ||
| ...widget, | ||
| version: {major: 1, minor: 0}, | ||
| options: { | ||
| ...options, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Do we even need to copy the options here?
| return { | |
| ...widget, | |
| version: {major: 1, minor: 0}, | |
| options: { | |
| ...options, | |
| }, | |
| }; | |
| return { | |
| ...widget, | |
| version: {major: 1, minor: 0}, | |
| }; |
Alternatively, maybe we should remove noneOfTheAbove and set hasNoneOfTheAbove to false, like in the original upgrade function?
| expect(score).toHaveBeenAnsweredCorrectly(); | ||
| }); | ||
|
|
||
| it("is interactive with stripped widget options", async () => { |
There was a problem hiding this comment.
We're using a few different terms for this concept:
- public options
- stripped options
- answerless options
I'd prefer to standardize on one, for the sake of future maintainers.
There was a problem hiding this comment.
The functions are all named getPublicWidgetOptions... so it feels like we've gone with "public". I'm a bit averse to "stripped" after starting with it. It feels aggressive. "Answerless" isn't always 100% correct because there are situations where we remove data that isn't the answer, but could reveal info about the answer. 🤷♂️
There was a problem hiding this comment.
I personally think "public" is the most incorrect. Answerless/answerful are both public, just at different times. But I can change it for this PR.
There was a problem hiding this comment.
+1 for keeping things consistent for now. A temporarily unclear name is better than temporary inconsistency. Find-replace is easy, but gets harder when naming isn't consistent.
| curr.correct ? acc + 1 : acc, | ||
| 0, | ||
| ) | ||
| ); |
There was a problem hiding this comment.
I see this algorithm was copied from the component code. I think there's a simpler way. I suspect the JIT can optimize this:
choices.filter((c) => c.correct).lengthto avoid creating the intermediate array. Even if it doesn't... 🤷
| constant("radio"), | ||
| object({ | ||
| numCorrect: optional(number), |
There was a problem hiding this comment.
This makes me slightly nervous. numCorrect only needs to be optional because we'll be running this parser on two different inputs:
- data loaded from datastore, where
numCorrectis usually not present (and maybe never needs to be present, since it can be derived?) - data sent to the the client, where
numCorrectmust be present if the data is answerless.
I worry that if the parsers accumulate a lot of irregularities of this kind, we'll be back in the position of not really understanding our data schema.
But I also think that it would be a pain in the butt to maintain separate parsers for answerless and answerful data.
One idea I'm toying with is letting the ParseContext keep track of whether we're parsing answerless or answerful data. Then we could write widget options parsers like this:
object({
numCorrect: ifAnswerful(optional(number)).ifAnswerless(number),
})
But maybe that's overkill? I guess one thing I don't understand is whether the plan is to persist numCorrect in the answerful data or not.
There was a problem hiding this comment.
For NumericInput v2 numCorrect will:
- always be in the data store
- always be in the full widget options
- will conditionally be in the "public" widget options
I added a commit since your comment that changes things. Maybe this is resolved now?
…rt of answerless Radio
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (70304ba) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR2233If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR2233 |
| // How many of the choices are correct, which is conditionally used to tell | ||
| // learners ahead of time how many options they'll need. | ||
| numCorrect?: number; |
There was a problem hiding this comment.
Does updating this file require a stakeholder update? Apologies for pointing this out each time, but the memories of what Ned went through and Jeremy always talking about protecting this file are strong 😅
| numCorrect: PerseusRadioWidgetOptions["numCorrect"], | ||
| ) { | ||
| return multipleSelect && countChoices && numCorrect; | ||
| } |
There was a problem hiding this comment.
It's kind of unfortunate that this code is coupled to logic in the widget. We're predicting when the widget is going to need EDIT: I see we're calling this function in the widget! In that case, I think it should be named something like numCorrect, which feels brittle.revealNumCorrect or showNumCorrectInInstructions.
It seems like we could leverage the type system's ability to model the different possibilities here. Something like:
type PerseusRadioWidgetOptions = {
responseFormat: SingleSelect | MultiSelect,
// ...
}
type SingleSelect = {type: "single-select"};
type MultiSelect = {
type: "multi-select",
/**
* If present, the number of choices the learner must select for
* their response to be accepted. Responses with a different number
* of selected choices are marked invalid rather than incorrect. The
* number of correct choices is displayed in the widget instructions.
*
* If null, the learner can submit a response with any number of
* selected options. Responses with the wrong number of options
* selected are marked incorrect.
*/
numCorrect: number | null,
}This would require a major version bump of the widget options format, so perhaps it's not something we want to take on right now. It might be worth migrating to this in the future. EDIT: ugh, sorry for the confusion. I was reading the diff of one commit thinking it was the whole PR. I see now that you are bumping the major version. In that case, I think changing to this more-structured format would be nice if we can make it work.
Summary:
So this focuses on
numCorrectwhich originally derived from the answers but now is its own widget option. I'm sure there's an argument that this also shouldn't be sent to the FE, but it's conditionally used to render help text:Select 2 answers. I don't know, I don't think it's that big of a deal to have it but I also made it optional so we can remove it from questions that don't need it:needsNumCorrect = options.multipleSelect === true && options.countChoices === true.The important thing is that we can render, answer, and score a Radio widget that's been stripped of answers. As far as I can tell, this supports all behavior pre-scoring.
Issue: LEMS-2849
Test plan:
Radio should continue to be renderable, answerable, and scorable.