-
Notifications
You must be signed in to change notification settings - Fork 350
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
Number Line: Extract validation out of scoring #1901
Merged
+144
−96
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
2d30658
Split validate logic to own function
Myranae 09ae628
Split out validation tests
Myranae dfad0ec
Rename rubric type to scoringData and add validation type
Myranae d0083a4
Add changeset
Myranae 47f4c3f
Remove unneeded import
Myranae 859d732
Merge branch 'main' into tb/LEMS-2608/number-line-split-out-validation
Myranae 74b8043
Return parts of validation that would change flow
Myranae ce53e80
ValidationData no longer necessary
Myranae 05f3299
Remove unneeded import
Myranae c5767bb
Fix tests to account for returned logic
Myranae 0d48670
Remove unneeded import
Myranae File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@khanacademy/perseus": minor | ||
--- | ||
|
||
Introduces a validation function for the number line widget (extracted from the scoring function). |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
116 changes: 45 additions & 71 deletions
116
packages/perseus/src/widgets/number-line/score-number-line.test.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,109 +1,83 @@ | ||
import scoreNumberLine from "./score-number-line"; | ||
|
||
import type { | ||
PerseusNumberLineRubric, | ||
PerseusNumberLineScoringData, | ||
PerseusNumberLineUserInput, | ||
} from "../../validation.types"; | ||
|
||
const baseInput: PerseusNumberLineUserInput = { | ||
isTickCrtl: true, | ||
numLinePosition: 1, | ||
rel: "eq", | ||
numDivisions: 10, | ||
divisionRange: [-10, 10], | ||
}; | ||
|
||
const baseRubric: PerseusNumberLineRubric = { | ||
correctRel: "eq", | ||
correctX: -1.5, | ||
divisionRange: [1, 12], | ||
initialX: -1, | ||
labelRange: [null, null], | ||
labelStyle: "decimal", | ||
labelTicks: true, | ||
numDivisions: null, | ||
range: [-1.5, 1.5], | ||
snapDivisions: 2, | ||
static: false, | ||
tickStep: 0.5, | ||
isInequality: false, | ||
}; | ||
|
||
function generateInput( | ||
extend?: Partial<PerseusNumberLineUserInput>, | ||
): PerseusNumberLineUserInput { | ||
return {...baseInput, ...extend}; | ||
} | ||
|
||
function generateRubric( | ||
extend?: Partial<PerseusNumberLineRubric>, | ||
): PerseusNumberLineRubric { | ||
return {...baseRubric, ...extend}; | ||
} | ||
|
||
describe("scoreNumberLine", () => { | ||
it("is invalid when outside allowed range", () => { | ||
// Arrange | ||
const userInput = generateInput({ | ||
divisionRange: [-1, 1], | ||
numLinePosition: 10, | ||
}); | ||
|
||
const rubric = generateRubric(); | ||
|
||
// Act | ||
const result = scoreNumberLine(userInput, rubric); | ||
|
||
// Assert | ||
expect(result).toHaveInvalidInput( | ||
"Number of divisions is outside the allowed range.", | ||
); | ||
}); | ||
|
||
it("is invalid when end state is the same as beginning state", () => { | ||
// Arrange | ||
const userInput = generateInput({ | ||
const userInput: PerseusNumberLineUserInput = { | ||
isTickCrtl: true, | ||
rel: "eq", | ||
numDivisions: 10, | ||
divisionRange: [-10, 10], | ||
numLinePosition: 0, | ||
}); | ||
}; | ||
|
||
const rubric = generateRubric({ | ||
const scoringData: PerseusNumberLineScoringData = { | ||
correctRel: "eq", | ||
correctX: -1.5, | ||
initialX: 0, | ||
}); | ||
range: [-1.5, 1.5], | ||
isInequality: false, | ||
}; | ||
|
||
// Act | ||
const result = scoreNumberLine(userInput, rubric); | ||
const score = scoreNumberLine(userInput, scoringData); | ||
|
||
// Assert | ||
expect(result).toHaveInvalidInput(); | ||
expect(score).toHaveInvalidInput(); | ||
}); | ||
|
||
it("can be answered correctly", () => { | ||
// Arrange | ||
const userInput = generateInput({ | ||
const userInput: PerseusNumberLineUserInput = { | ||
isTickCrtl: true, | ||
rel: "eq", | ||
numDivisions: 10, | ||
divisionRange: [-10, 10], | ||
numLinePosition: -1.5, | ||
}); | ||
}; | ||
|
||
const rubric = generateRubric(); | ||
const scoringData: PerseusNumberLineScoringData = { | ||
correctRel: "eq", | ||
correctX: -1.5, | ||
initialX: -1, | ||
range: [-1.5, 1.5], | ||
isInequality: false, | ||
}; | ||
|
||
// Act | ||
const result = scoreNumberLine(userInput, rubric); | ||
const score = scoreNumberLine(userInput, scoringData); | ||
|
||
// Assert | ||
expect(result).toHaveBeenAnsweredCorrectly(); | ||
expect(score).toHaveBeenAnsweredCorrectly(); | ||
}); | ||
|
||
it("can be answered incorrectly", () => { | ||
// Arrange | ||
const userInput = generateInput({ | ||
const userInput: PerseusNumberLineUserInput = { | ||
isTickCrtl: true, | ||
rel: "eq", | ||
numDivisions: 10, | ||
divisionRange: [-10, 10], | ||
numLinePosition: 1.5, | ||
}); | ||
}; | ||
|
||
const rubric = generateRubric(); | ||
const scoringData: PerseusNumberLineScoringData = { | ||
correctRel: "eq", | ||
correctX: -1.5, | ||
initialX: -1, | ||
range: [-1.5, 1.5], | ||
isInequality: false, | ||
}; | ||
|
||
// Act | ||
const result = scoreNumberLine(userInput, rubric); | ||
const score = scoreNumberLine(userInput, scoringData); | ||
|
||
// Assert | ||
expect(result).toHaveBeenAnsweredIncorrectly(); | ||
expect(score).toHaveBeenAnsweredIncorrectly(); | ||
}); | ||
}); |
41 changes: 19 additions & 22 deletions
41
packages/perseus/src/widgets/number-line/score-number-line.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,44 +1,41 @@ | ||
import {number as knumber} from "@khanacademy/kmath"; | ||
|
||
import validateNumberLine from "./validate-number-line"; | ||
|
||
import type {PerseusScore} from "../../types"; | ||
import type { | ||
PerseusNumberLineRubric, | ||
PerseusNumberLineScoringData, | ||
PerseusNumberLineUserInput, | ||
} from "../../validation.types"; | ||
|
||
function scoreNumberLine( | ||
state: PerseusNumberLineUserInput, | ||
rubric: PerseusNumberLineRubric, | ||
userInput: PerseusNumberLineUserInput, | ||
scoringData: PerseusNumberLineScoringData, | ||
): PerseusScore { | ||
const range = rubric.range; | ||
const divisionRange = state.divisionRange; | ||
const start = rubric.initialX != null ? rubric.initialX : range[0]; | ||
const startRel = rubric.isInequality ? "ge" : "eq"; | ||
const correctRel = rubric.correctRel || "eq"; | ||
const validationError = validateNumberLine(userInput); | ||
if (validationError) { | ||
return validationError; | ||
} | ||
|
||
const range = scoringData.range; | ||
const start = | ||
scoringData.initialX != null ? scoringData.initialX : range[0]; | ||
const startRel = scoringData.isInequality ? "ge" : "eq"; | ||
const correctRel = scoringData.correctRel || "eq"; | ||
const correctPos = knumber.equal( | ||
state.numLinePosition, | ||
rubric.correctX || 0, | ||
userInput.numLinePosition, | ||
scoringData.correctX || 0, | ||
); | ||
const outsideAllowedRange = | ||
state.numDivisions > divisionRange[1] || | ||
state.numDivisions < divisionRange[0]; | ||
|
||
// TODO: I don't think isTickCrtl is a thing anymore | ||
if (state.isTickCrtl && outsideAllowedRange) { | ||
return { | ||
type: "invalid", | ||
message: "Number of divisions is outside the allowed range.", | ||
}; | ||
} | ||
if (correctPos && correctRel === state.rel) { | ||
if (correctPos && correctRel === userInput.rel) { | ||
return { | ||
type: "points", | ||
earned: 1, | ||
total: 1, | ||
message: null, | ||
}; | ||
} | ||
if (state.numLinePosition === start && state.rel === startRel) { | ||
if (userInput.numLinePosition === start && userInput.rel === startRel) { | ||
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 check could possibly be moved to the validation function at a later time if the flow can change so this check occurs first. Only possible if the answer is never the starting position though. |
||
// We're where we started. | ||
return { | ||
type: "invalid", | ||
|
41 changes: 41 additions & 0 deletions
41
packages/perseus/src/widgets/number-line/validate-number-line.test.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
import validateNumberLine from "./validate-number-line"; | ||
|
||
import type {PerseusNumberLineUserInput} from "../../validation.types"; | ||
|
||
describe("validateNumberLine", () => { | ||
it("is invalid when outside allowed range", () => { | ||
// Arrange | ||
const userInput: PerseusNumberLineUserInput = { | ||
isTickCrtl: true, | ||
rel: "eq", | ||
numDivisions: 10, | ||
divisionRange: [-1, 1], | ||
numLinePosition: 10, | ||
}; | ||
|
||
// Act | ||
const validationError = validateNumberLine(userInput); | ||
|
||
// Assert | ||
expect(validationError).toHaveInvalidInput( | ||
"Number of divisions is outside the allowed range.", | ||
); | ||
}); | ||
|
||
it("returns null when validation passes", () => { | ||
// Arrange | ||
const userInput: PerseusNumberLineUserInput = { | ||
isTickCrtl: true, | ||
rel: "eq", | ||
numDivisions: 10, | ||
divisionRange: [-10, 10], | ||
numLinePosition: -1.5, | ||
}; | ||
|
||
// Act | ||
const validationError = validateNumberLine(userInput); | ||
|
||
// Assert | ||
expect(validationError).toBeNull(); | ||
}); | ||
}); |
28 changes: 28 additions & 0 deletions
28
packages/perseus/src/widgets/number-line/validate-number-line.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
import type {PerseusScore} from "../../types"; | ||
import type {PerseusNumberLineUserInput} from "../../validation.types"; | ||
|
||
/** | ||
* Checks user input is within the allowed range and not the same as the initial | ||
* state. | ||
* @param userInput | ||
* @see 'scoreNumberLine' for the scoring logic. | ||
*/ | ||
function validateNumberLine( | ||
userInput: PerseusNumberLineUserInput, | ||
): Extract<PerseusScore, {type: "invalid"}> | null { | ||
const divisionRange = userInput.divisionRange; | ||
const outsideAllowedRange = | ||
userInput.numDivisions > divisionRange[1] || | ||
userInput.numDivisions < divisionRange[0]; | ||
|
||
// TODO: I don't think isTickCrtl is a thing anymore | ||
if (userInput.isTickCrtl && outsideAllowedRange) { | ||
return { | ||
type: "invalid", | ||
message: "Number of divisions is outside the allowed range.", | ||
}; | ||
} | ||
return null; | ||
} | ||
|
||
export default validateNumberLine; |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I'm guessing when exercises allow users to set their own interval, then this check was necessary in case they put in an invalid number of divisions.
I checked the number line editor, and it still has a checkbox for
isTickCrtl
and the json still has the field (though it defaults to not being there). I tried removingstate.isTickCrtl
from line 27 and all the tests still pass. It doesn't look like it is referenced anywhere other than tests and this line here. Wouldn't this always be false ifisTickCrtl
is not in use? Maybe this validation check hasn't been working?Or maybe this check is not necessary if we are no longer allowing the user to set their own tick interval. It is possible to turn this to
true
in the editor, so I think we should only remove it if that checkbox ever gets removed, just in case it has been set to true in the past or in case anyone sets it to true in the future.I don't think this property should be on userInput though. Should we move it to validationData? Should there be any tests making sure things work with or without this value?
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.
Maybe moving it can be a later fix if needed. Just left it for now as the only logic moved into the validation function.