Skip to content
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.

Sign up for GitHub

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

Udpate Radio's userInput and rubric to be as focused as possible #1844

Merged
merged 12 commits into from
Nov 12, 2024
Merged

Udpate Radio's userInput and rubric to be as focused as possible #1844

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
29d8c3f
Remove numCorrect from rubric and userinput
Myranae Nov 7, 2024
009bd41
Remove notaIndex from userInput
Myranae Nov 8, 2024
89b59e4
Remove notaSelected from UserInput
Myranae Nov 8, 2024
f3c7719
Merge branch 'main' into tb/LEMS-2541/clean-up-radio-user-input
Myranae Nov 8, 2024
7aae2f1
Remove new notaSelected references
Myranae Nov 8, 2024
992e9bd
Merge branch 'main' into tb/LEMS-2541/clean-up-radio-user-input
Myranae Nov 12, 2024
bea6a85
Add changeset
Myranae Nov 12, 2024
1f3a831
Remove properties no longer on radio's userInput
Myranae Nov 12, 2024
3dfaf4d
Add back info that was unrelated
Myranae Nov 12, 2024
5928467
Update based on PR review feedback
Myranae Nov 12, 2024
166f709
Remove unneeded import
Myranae Nov 12, 2024
3ada8da
Merge branch 'main' into tb/LEMS-2541/clean-up-radio-user-input
Myranae Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/bright-countries-peel.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": patch
---

Update Radio's userInput and rubric objects and types
3 changes: 0 additions & 3 deletions packages/perseus/src/__tests__/widgets.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,9 +170,6 @@ describe("Widget API support", () => {
const userInput = Widget.getUserInputFromProps(props);
expect(userInput).toEqual({
choicesSelected: [false, true, false],
noneOfTheAboveIndex: null,
noneOfTheAboveSelected: false,
numCorrect: undefined,
});
} else {
throw new Error("Widget does not have getUserInputFromProps");
Expand Down
6 changes: 0 additions & 6 deletions packages/perseus/src/multi-items/__tests__/multi-renderer.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -629,9 +629,6 @@ describe("multi-item renderer", () => {
true,
false,
],
"noneOfTheAboveIndex": null,
"noneOfTheAboveSelected": false,
"numCorrect": 1,
},
{
"currentValue": "-42",
Expand Down Expand Up @@ -784,9 +781,6 @@ describe("multi-item renderer", () => {
true,
false,
],
"noneOfTheAboveIndex": null,
"noneOfTheAboveSelected": false,
"numCorrect": 1,
},
{
"currentValue": "-42",
Expand Down
3 changes: 0 additions & 3 deletions packages/perseus/src/validation.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,9 +168,6 @@ export type PerseusRadioRubric = {

export type PerseusRadioUserInput = {
choicesSelected: ReadonlyArray<boolean>;
numCorrect?: number;
noneOfTheAboveIndex?: number | null | undefined;
noneOfTheAboveSelected?: boolean;
};

export type PerseusSorterRubric = {
Expand Down
1 change: 0 additions & 1 deletion packages/perseus/src/widget-ai-utils/group/group.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ describe("group widget", () => {
false,
false,
],
isNoneOfTheAboveSelected: false,
},
},
},
Expand Down
2 changes: 0 additions & 2 deletions packages/perseus/src/widget-ai-utils/group/prompt-utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ describe("Group getPromptJSON", () => {
],
userInput: {
selectedOptions: [false, false, false, false, false],
isNoneOfTheAboveSelected: false,
},
},
"matrix 1": {
Expand Down Expand Up @@ -72,7 +71,6 @@ describe("Group getPromptJSON", () => {
],
userInput: {
selectedOptions: [false, false, false, false, false],
isNoneOfTheAboveSelected: false,
},
},
"matrix 1": {
Expand Down
2 changes: 0 additions & 2 deletions packages/perseus/src/widget-ai-utils/radio/prompt-utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ describe("Radio getPromptJSON", () => {

const userInput: PerseusRadioUserInput = {
choicesSelected: [true, false, false, false],
noneOfTheAboveSelected: false,
};

const resultJSON = getPromptJSON(renderProps, userInput);
Expand All @@ -50,7 +49,6 @@ describe("Radio getPromptJSON", () => {
],
userInput: {
selectedOptions: [true, false, false, false],
isNoneOfTheAboveSelected: false,
},
});
});
Expand Down
2 changes: 0 additions & 2 deletions packages/perseus/src/widget-ai-utils/radio/prompt-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ export type RadioPromptJSON = {
options: BasicOption[];
userInput: {
selectedOptions: ReadonlyArray<boolean>;
isNoneOfTheAboveSelected: boolean;
};
};

Expand All @@ -34,7 +33,6 @@ export const getPromptJSON = (
options,
userInput: {
selectedOptions: userInput.choicesSelected.slice(),
isNoneOfTheAboveSelected: !!userInput.noneOfTheAboveSelected,
},
};
};
3 changes: 0 additions & 3 deletions packages/perseus/src/widgets/group/group.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,9 +429,6 @@ describe("group widget", () => {
false,
true,
],
"noneOfTheAboveIndex": null,
"noneOfTheAboveSelected": false,
"numCorrect": 1,
},
],
[
Expand Down
31 changes: 0 additions & 31 deletions packages/perseus/src/widgets/radio/radio-component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,63 +82,32 @@ class Radio extends React.Component<Props> implements Widget {
// future timeline implementers: this used to be {value: i} before
// multiple select was added)
if (props.choiceStates) {
let noneOfTheAboveIndex = null;
let noneOfTheAboveSelected = false;

const choiceStates = props.choiceStates;
const choicesSelected = choiceStates.map(() => false);
const numCorrect = props.numCorrect;

for (let i = 0; i < choicesSelected.length; i++) {
const index = unshuffle ? props.choices[i].originalIndex : i;

choicesSelected[index] = choiceStates[i].selected;

if (props.choices[i].isNoneOfTheAbove) {
// @ts-expect-error - TS2322 - Type 'number' is not assignable to type 'null'.
noneOfTheAboveIndex = index;

if (choicesSelected[i]) {
noneOfTheAboveSelected = true;
}
}
}

return {
choicesSelected,
numCorrect,
noneOfTheAboveIndex,
noneOfTheAboveSelected,
};
// Support legacy choiceState implementation
}
/* c8 ignore if - props.values is deprecated */
const {values} = props;
if (values) {
let noneOfTheAboveIndex = null;
let noneOfTheAboveSelected = false;

const choicesSelected = [...values];
const numCorrect = props.numCorrect;
const valuesLength = values.length;

for (let i = 0; i < valuesLength; i++) {
const index = unshuffle ? props.choices[i].originalIndex : i;
choicesSelected[index] = values[i];

if (props.choices[i].isNoneOfTheAbove) {
// @ts-expect-error - TS2322 - Type 'number' is not assignable to type 'null'.
noneOfTheAboveIndex = index;
if (choicesSelected[i]) {
noneOfTheAboveSelected = true;
}
}
}
return {
choicesSelected,
noneOfTheAboveIndex,
noneOfTheAboveSelected,
numCorrect,
};
}
// Nothing checked
Expand Down
14 changes: 7 additions & 7 deletions packages/perseus/src/widgets/radio/radio-validator.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ describe("radioValidator", () => {

it("is invalid when number selected does not match number correct", () => {
const userInput: PerseusRadioUserInput = {
numCorrect: 2,
choicesSelected: [true, false, false, false],
};

Expand All @@ -49,7 +48,6 @@ describe("radioValidator", () => {

it("is invalid when none of the above and an answer are both selected", () => {
const userInput: PerseusRadioUserInput = {
noneOfTheAboveSelected: true,
choicesSelected: [true, false, false, false, true],
};

Expand All @@ -59,7 +57,11 @@ describe("radioValidator", () => {
{content: "Choice 2", correct: false},
{content: "Choice 3", correct: false},
{content: "Choice 4", correct: false},
{content: "None of the above", correct: false},
{
content: "None of the above",
correct: false,
isNoneOfTheAbove: true,
},
],
};

Expand Down Expand Up @@ -147,8 +149,6 @@ describe("radioValidator", () => {
it("can handle none of the above correct answer", () => {
const userInput: PerseusRadioUserInput = {
choicesSelected: [false, false, false, false, true],
noneOfTheAboveSelected: true,
noneOfTheAboveIndex: 4,
};

const rubric: PerseusRadioRubric = {
Expand All @@ -157,6 +157,7 @@ describe("radioValidator", () => {
{content: "Choice 2", correct: false},
{content: "Choice 3", correct: false},
{content: "Choice 4", correct: false},
{content: "Choice 5", correct: true, isNoneOfTheAbove: true},
],
};

Expand All @@ -168,8 +169,6 @@ describe("radioValidator", () => {
it("can handle none of the above incorrect answer", () => {
const userInput: PerseusRadioUserInput = {
choicesSelected: [false, false, false, false, true],
noneOfTheAboveSelected: true,
noneOfTheAboveIndex: 4,
};

const rubric: PerseusRadioRubric = {
Expand All @@ -178,6 +177,7 @@ describe("radioValidator", () => {
{content: "Choice 2", correct: false},
{content: "Choice 3", correct: false},
{content: "Choice 4", correct: false},
{content: "Choice 5", correct: false, isNoneOfTheAbove: true},
],
};

Expand Down
36 changes: 23 additions & 13 deletions packages/perseus/src/widgets/radio/radio-validator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import _ from "underscore";

import type {PerseusStrings} from "../../strings";
import type {PerseusScore} from "../../types";
import type {
Expand All @@ -21,23 +23,33 @@ function radioValidator(
};
}

// TODO(LEMS-2541): should numCorrect actually be on the rubric
// instead of the userInput?
if (
userInput.numCorrect &&
userInput.numCorrect > 1 &&
numSelected !== userInput.numCorrect
) {
const numCorrect: number = _.reduce(
rubric.choices,
function (memo, choice) {
return choice.correct ? memo + 1 : memo;
},
0,
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When it's an option, I'd like to move away from underscore and use native JS; reduce is native now.

Suggested change
const numCorrect: number = _.reduce(
rubric.choices,
function (memo, choice) {
return choice.correct ? memo + 1 : memo;
},
0,
);
const numCorrect: number = rubric.choices.reduce(
(acc, curr) => {
return curr.correct ? acc + 1 : acc;
},
0,
);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also try to get rid of underscore stuff. Sometimes my brain refuses to realize I can update something I copy and pasted from another part of the codebase 😂 Thanks! Updated~


if (numCorrect && numCorrect > 1 && numSelected !== numCorrect) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (numCorrect && numCorrect > 1 && numSelected !== numCorrect) {
if (numCorrect > 1 && numSelected !== numCorrect) {

I know you're just moving existing code, but numCorrect is a number and the only falsy number is 0 which is not greater than 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

return {
type: "invalid",
message: strings.chooseCorrectNum,
};
// If NOTA and some other answer are checked, ...
}
let noneOfTheAboveSelected = false;
for (let i = 0; i < rubric.choices.length; i++) {
if (
rubric.choices[i].isNoneOfTheAbove &&
userInput.choicesSelected[i]
) {
noneOfTheAboveSelected = true;
break;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your code is clearer, but just because I like the new methods here is an alternative:

Suggested change
let noneOfTheAboveSelected = false;
for (let i = 0; i < rubric.choices.length; i++) {
if (
rubric.choices[i].isNoneOfTheAbove &&
userInput.choicesSelected[i]
) {
noneOfTheAboveSelected = true;
break;
}
}
let noneOfTheAboveSelected = rubric.choices.some((e, i) => (
e.isNoneOfTheAbove && userInput.choicesSelected[i]
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I love this. Definitely updated to your alternative, though I tried to make the arguments a bit more descriptive :)


// TODO(LEMS-2541): should noneOfTheAboveSelected be replaced with a
// combination of choicesSelected and noneOfTheAboveIndex?
if (userInput.noneOfTheAboveSelected && numSelected > 1) {
if (noneOfTheAboveSelected && numSelected > 1) {
return {
type: "invalid",
message: strings.notNoneOfTheAbove,
Expand All @@ -46,9 +58,7 @@ function radioValidator(

const correct = userInput.choicesSelected.every((selected, i) => {
let isCorrect: boolean;
// TODO(LEMS-2541): should noneOfTheAboveIndex actually be on the rubric
// instead of the userInput?
if (userInput.noneOfTheAboveIndex === i) {
if (rubric.choices[i].isNoneOfTheAbove) {
isCorrect = rubric.choices.every((choice, j) => {
return i === j || !choice.correct;
});
Expand Down