[LEMS-2849] Answerless Radio

[handeyeco]

Summary:

So this focuses on numCorrect which 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.

[github-actions]

Size Change: +591 B (+0.07%)

Total Size: 873 kB

Filename	Size	Change
packages/perseus-core/dist/es/index.js	30.5 kB	+700 B (+2.35%)
packages/perseus-editor/dist/es/index.js	276 kB	-118 B (-0.04%)
packages/perseus/dist/es/index.js	367 kB	+9 B (0%)

ℹ️ View Unchanged

Filename	Size
packages/kas/dist/es/index.js	39.7 kB
packages/keypad-context/dist/es/index.js	760 B
packages/kmath/dist/es/index.js	11.1 kB
packages/math-input/dist/es/index.js	78.2 kB
packages/math-input/dist/es/strings.js	1.79 kB
packages/perseus-linter/dist/es/index.js	22.8 kB
packages/perseus-score/dist/es/index.js	20.6 kB
packages/perseus/dist/es/strings.js	6.74 kB
packages/pure-markdown/dist/es/index.js	4.14 kB
packages/simple-markdown/dist/es/index.js	13.1 kB

_{compressed-size-action}

[handeyeco]

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).

[handeyeco]

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.

[benchristel]

I left a few minor comments and suggestions inline, but nothing blocking. LGTM!

[benchristel]

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 numCorrect is usually not present (and maybe never needs to be present, since it can be derived?)
• data sent to the the client, where numCorrect must 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.

[handeyeco]

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?

[github-actions]

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (49819c4) and published it to npm. You
can install it using the tag PR2233.

Example:

pnpm add @khanacademy/perseus@PR2233

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR2233

[jeremywiebe]

Nice. I like the approach of removing countChoices in get getPublicWidgetOptions.

[Myranae]

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 😅

[handeyeco]

Thanks for bringing this up: https://khanacademy.slack.com/archives/C49296Q7P/p1740496295273589

[benchristel]

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 numCorrect, which feels brittle. EDIT: I see we're calling this function in the widget! In that case, I think it should be named something like 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.

[jeremywiebe]

Hopefully this can become alot simpler with the upcoming Radio rebasing on top of Wonder Blocks work.

[jeremywiebe]

Stylistically, I find using an onChange jest.fn() for these types of things simpler to grok. With a var we set in the callback you have a to trace up through the test to understand that the onChange callback sets it.

With an onChange mock, we could use:

expect(onChange).toHaveBeenCalledWith(expect.objectContaining({
    numCorrect: 3
});

In my mind that's less logic to support the test than having a let options we manage. 🤷‍♂️

[handeyeco]

Not a hill I'll die on, but I'm not totally sure I agree. I think...

expect(options?.numCorrect).toBe(3);

...is much more legible than...

expect(onChangeMock).toHaveBeenCalledWith(
    expect.objectContaining({
        numCorrect: 3,
    }),
);

I also think from a KISS perspective that a simple callback is easier to follow/debug than a Jest abstraction. But it's not a big deal in this particular case, so I made the change.