Skip to content

Commit

Permalink
Ensure the new Mafs-based Angle Graphs score correctly (#1432)
Browse files Browse the repository at this point in the history
## Summary:
The new Mafs-based Angle graphs were being marked as incorrect if the user moves the bottom point clockwise, which was not caught earlier as there was a bug in the implementation of the legacy Angle graphs that allowed users to create reflexive angles when they were not supposed to be able to. 

The new Mafs-based Angle graph solves this issue, but it uncovered a bug in our scoring that fails to handle this particular state. 

This ticket adds the logic necessary to reverse the coordinates (for scoring purposes) when they're clockwise and reflexive graphs are not allowed. This will be the approach used until we are able to remove/deprecate the legacy Angle graphs. 

When the legacy Angle graph is removed, it's highly recommended that this logic be moved directly to the scoring function in `interactive-graph.tsx`, so that there's no future confusion about how the scoring is being calculated.  Upgrading this graph was very difficult due to the previous logic being split across many different files, so this will save us a lot of headaches down the road. The ticket to house that work can be found [here](https://khanacademy.atlassian.net/browse/LEMS-2190). 

Issue: LEMS-2165

## Test plan:
- Manual testing using Flipbook
- Creation of new regression tests to test all 3 states to ensure that there's no future regressions during the transition (These should be moved along with the logic during LEMS-2190) 

## Video: 
https://github.com/user-attachments/assets/de76c0be-085d-4090-9ba8-598de27de0b4

Author: SonicScrewdriver

Reviewers: jeremywiebe, SonicScrewdriver

Required Reviewers:

Approved By: jeremywiebe

Checks: ✅ codecov/project, ❌ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald

Pull Request URL: #1432
  • Loading branch information
SonicScrewdriver authored Jul 31, 2024
1 parent af68a9e commit ed67370
Show file tree
Hide file tree
Showing 4 changed files with 117 additions and 2 deletions.
5 changes: 5 additions & 0 deletions .changeset/sweet-seas-dance.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": patch
---

Bug fix to ensure that new angle graphs are scored correctly.
4 changes: 4 additions & 0 deletions packages/perseus/src/widgets/interactive-graph.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,10 @@ type DefaultProps = {
graph: Props["graph"];
};

// (LEMS-2190): Move the Mafs Angle Graph coordinate reversal logic in interactive-graph-state.ts
// to this file when we remove the legacy graph. This logic allows us to support bi-directional angles
// for the new (non-reflexive) Mafs graphs, while maintaining the same scoring behaviour as the legacy graph.
// Once the legacy graph is removed, we should move this logic directly into the validate function below.
class LegacyInteractiveGraph extends React.Component<Props, State> {
static contextType = PerseusI18nContext;
declare context: React.ContextType<typeof PerseusI18nContext>;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,27 @@
import invariant from "tiny-invariant";

import {getGradableGraph} from "./interactive-graph-state";

import type {InteractiveGraphState} from "../types";
import type {PerseusGraphType} from "@khanacademy/perseus";

const defaultAngleState: InteractiveGraphState = {
type: "angle",
coords: [
[5, 0],
[0, 0],
[5, 5],
],
hasBeenInteractedWith: true,
range: [
[-10, 10],
[-10, 10],
],
snapStep: [1, 1],
showAngles: true,
allowReflexAngles: false,
};

describe("getGradableGraph", () => {
/**
* Originally `getGradableGraph` was returning a PerseusGraphType with just a
Expand All @@ -28,4 +47,74 @@ describe("getGradableGraph", () => {

expect(result).toEqual(initialGraph);
});

// (LEMS-2190): This test is to ensure that the new Mafs graph is reversing coordinates when the angle graph
// is reflexive in a clockwise direction in order to temporarily maintain the same angle scoring with the
// legacy graph. This logic (& the tests) should be moved to the scoring function after removing the legacy graph.
it("returns reversed coordinates if the angle graph is reflexive when not allowed", () => {
const state: InteractiveGraphState = {
...defaultAngleState,
coords: [
[-5, 0],
[0, 0],
[5, 5],
],
hasBeenInteractedWith: true,
};
const initialGraph: PerseusGraphType = {
type: "angle",
};
const result = getGradableGraph(state, initialGraph);
invariant(result.type === "angle");
expect(result.coords).toEqual([
[5, 5],
[0, 0],
[-5, 0],
]);
});

it("does not reverse coordinates if the angle graph is not reflexive", () => {
const state: InteractiveGraphState = {
...defaultAngleState,
coords: [
[5, 0],
[0, 0],
[5, 5],
],
hasBeenInteractedWith: true,
};
const initialGraph: PerseusGraphType = {
type: "angle",
};
const result = getGradableGraph(state, initialGraph);
invariant(result.type === "angle");
expect(result.coords).toEqual([
[5, 0],
[0, 0],
[5, 5],
]);
});

it("does not reverse coordinates if the angle graph is allowed to be reflexive", () => {
const state: InteractiveGraphState = {
...defaultAngleState,
coords: [
[5, 0],
[0, 0],
[-5, -5],
],
hasBeenInteractedWith: true,
allowReflexAngles: true,
};
const initialGraph: PerseusGraphType = {
type: "angle",
};
const result = getGradableGraph(state, initialGraph);
invariant(result.type === "angle");
expect(result.coords).toEqual([
[5, 0],
[0, 0],
[-5, -5],
]);
});
});
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import {clockwise} from "../../../util/geometry";

import type {CircleGraphState, InteractiveGraphState} from "../types";
import type {PerseusGraphType} from "@khanacademy/perseus";
import type {Coord, PerseusGraphType} from "@khanacademy/perseus";

export function getGradableGraph(
state: InteractiveGraphState,
Expand Down Expand Up @@ -77,9 +79,24 @@ export function getGradableGraph(
}

if (state.type === "angle" && initialGraph.type === "angle") {
// We're going to reverse the coords for scoring if the angle is clockwise and
// we don't allow reflex angles. This is because the angle is largely defined
// by the order of the points in the coords array, and we want to maintain
// the same angle scoring with the legacy graph (which had a bug).
// (LEMS-2190): When we remove the legacy graph, move this logic to the scoring function.
const areClockwise = clockwise([
state.coords[0],
state.coords[2],
state.coords[1],
]);
const shouldReverseCoords = areClockwise && !state.allowReflexAngles;
const coords: [Coord, Coord, Coord] = shouldReverseCoords
? (state.coords.slice().reverse() as [Coord, Coord, Coord])
: state.coords;

return {
...initialGraph,
coords: state.coords,
coords,
allowReflexAngles: state.allowReflexAngles,
};
}
Expand Down

0 comments on commit ed67370

Please sign in to comment.