Skip to content

Commit

Permalink
Add a feature flag for Mafs point graphs with fixed numPoints (#1381)
Browse files Browse the repository at this point in the history
## Summary:
The point graph has two separate user experiences:

- fixed number of points, where you can drag points around, but not add
  or delete them
- unlimited points, where you can add and delete points

We have built the first of these experiences in Mafs, but not the
second. We want to release Mafs graphs with fixed-numPoints before
implementing unlimited points.

This commit adds logic to make that possible with feature flags. It also
moves the existing (similar) logic for polygon graphs so all decisions
about which graph to render are made in one, testable place.

Issue: LEMS-2126

Test plan:

Deploy a webapp ZND. Visit
`/internal-courses/test-everything/test-everything-1/te-interactive-graph/a/survey-of-graph-types`
and verify that only the graphs that should use Mafs are doing so.

Author: benchristel

Reviewers: nishasy, #perseus, Myranae, SonicScrewdriver, mark-fitzgerald, nicolecomputer

Required Reviewers:

Approved By: nishasy

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), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ gerald, ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x)

Pull Request URL: #1381
  • Loading branch information
benchristel authored Jul 1, 2024
1 parent 93eeda1 commit 26dceb8
Show file tree
Hide file tree
Showing 3 changed files with 186 additions and 19 deletions.
5 changes: 5 additions & 0 deletions .changeset/tricky-rice-nail.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": minor
---

Make the `mafs.point` flag control whether point graphs with fixed numbers of points should use Mafs. Previously, turning on the `mafs.point` flag would enable Mafs for point graphs with unlimited points as well.
144 changes: 143 additions & 1 deletion packages/perseus/src/widgets/interactive-graph.test.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,15 @@
import invariant from "tiny-invariant";

import InteractiveGraph, {type Rubric} from "./interactive-graph";
import InteractiveGraph, {
type Rubric,
shouldUseMafs,
} from "./interactive-graph";

import type {
PerseusGraphTypeLinear,
PerseusGraphTypePoint,
PerseusGraphTypePolygon,
} from "../perseus-types";
import type {PerseusGraphType} from "@khanacademy/perseus";

function createRubric(graph: PerseusGraphType): Rubric {
Expand Down Expand Up @@ -182,3 +190,137 @@ describe("InteractiveGraph.validate on a segment question", () => {
]);
});
});

describe("shouldUseMafs", () => {
it("is false given no mafs flags", () => {
const graph: PerseusGraphTypeLinear = {
type: "linear",
};
const mafsFlags = undefined;

expect(shouldUseMafs(mafsFlags, graph)).toBe(false);
});

it("is false when mafs flags is a boolean", () => {
// boolean values aren't valid; we expect the mafs flags to be an
// object.
const graph: PerseusGraphTypeLinear = {
type: "linear",
};
const mafsFlags = true;

expect(shouldUseMafs(mafsFlags, graph)).toBe(false);
});

it("is false for a point graph when the feature flag is off", () => {
const graph: PerseusGraphTypePoint = {
type: "point",
numPoints: 42,
};
const mafsFlags = {};

expect(shouldUseMafs(mafsFlags, graph)).toBe(false);
});

it("is true for a point graph when the `point` feature flag is on", () => {
const graph: PerseusGraphTypePoint = {
type: "point",
numPoints: 42,
};
const mafsFlags = {
point: true,
};

expect(shouldUseMafs(mafsFlags, graph)).toBe(true);
});

it("is false for a point graph with numPoints = 'unlimited'", () => {
const graph: PerseusGraphTypePoint = {
type: "point",
numPoints: "unlimited",
};
const mafsFlags = {
point: true,
};

expect(shouldUseMafs(mafsFlags, graph)).toBe(false);
});

it("is true for a point graph without numPoints set when the feature flag is on", () => {
// numPoints defaults to 1
const graph: PerseusGraphTypePoint = {
type: "point",
};
const mafsFlags = {
point: true,
};

expect(shouldUseMafs(mafsFlags, graph)).toBe(true);
});

it("is false for a polygon graph when the feature flag is off", () => {
const graph: PerseusGraphTypePolygon = {
type: "polygon",
numSides: 3,
};
const mafsFlags = {};

expect(shouldUseMafs(mafsFlags, graph)).toBe(false);
});

it("is true for a polygon graph when the feature flag is on", () => {
const graph: PerseusGraphTypePolygon = {
type: "polygon",
numSides: 3,
};
const mafsFlags = {
polygon: true,
};

expect(shouldUseMafs(mafsFlags, graph)).toBe(true);
});

it("is false for a polygon graph when numSides is 'unlimited'", () => {
const graph: PerseusGraphTypePolygon = {
type: "polygon",
numSides: "unlimited",
};
const mafsFlags = {
polygon: true,
};

expect(shouldUseMafs(mafsFlags, graph)).toBe(false);
});

it("is true for a polygon graph when numSides is not set", () => {
// numSides defaults to 3
const graph: PerseusGraphTypePolygon = {
type: "polygon",
};
const mafsFlags = {
polygon: true,
};

expect(shouldUseMafs(mafsFlags, graph)).toBe(true);
});

it("is false for a linear graph when the feature flag is off", () => {
const graph: PerseusGraphTypeLinear = {
type: "linear",
};
const mafsFlags = {};

expect(shouldUseMafs(mafsFlags, graph)).toBe(false);
});

it("is true for a linear graph when the feature flag is on", () => {
const graph: PerseusGraphTypeLinear = {
type: "linear",
};
const mafsFlags = {
linear: true,
};

expect(shouldUseMafs(mafsFlags, graph)).toBe(true);
});
});
56 changes: 38 additions & 18 deletions packages/perseus/src/widgets/interactive-graph.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,18 @@ import Util from "../util";
import KhanColors from "../util/colors";
import {
angleMeasures,
canonicalSineCoefficients,
ccw,
collinear,
getLineEquation,
getLineIntersection,
intersects,
lawOfCosines,
magnitude,
rotate,
vector,
sign,
getLineEquation,
getLineIntersection,
canonicalSineCoefficients,
similar,
vector,
} from "../util/geometry";
import GraphUtils from "../util/graph-utils";
import {polar} from "../util/graphie";
Expand All @@ -53,8 +53,8 @@ import type {
} from "../types";
import type {
QuadraticCoefficient,
SineCoefficient,
Range,
SineCoefficient,
} from "../util/geometry";

const {DeprecationMixin} = Util;
Expand Down Expand Up @@ -1815,20 +1815,9 @@ class InteractiveGraph extends React.Component<Props, State> {
}

render() {
if (
this.props.graph.type === "polygon" &&
this.props.graph.numSides === "unlimited"
) {
return (
<LegacyInteractiveGraph
ref={this.legacyGraphRef}
{...this.props}
/>
);
}

// Mafs shim
if (this.props.apiOptions?.flags?.["mafs"]?.[this.props.graph.type]) {
const mafsFlags = this.props.apiOptions?.flags?.["mafs"];
if (shouldUseMafs(mafsFlags, this.props.graph)) {
const box = getInteractiveBoxFromSizeClass(
this.props.containerSizeClass,
);
Expand Down Expand Up @@ -2675,6 +2664,37 @@ class InteractiveGraph extends React.Component<Props, State> {
}
}

// exported for testing
export function shouldUseMafs(
mafsFlags: Record<string, boolean> | undefined | boolean,
graph: PerseusGraphType,
): boolean {
if (typeof mafsFlags === "boolean" || typeof mafsFlags === "undefined") {
return false;
}

switch (graph.type) {
case "point":
if (graph.numPoints === UNLIMITED) {
// TODO(benchristel): add a feature flag for the "unlimited"
// case once we've implemented point graphs with unlimited
// points
return false;
}
return Boolean(mafsFlags["point"]);
case "polygon":
if (graph.numSides === UNLIMITED) {
// TODO(benchristel): add a feature flag for the "unlimited"
// case once we've implemented polygon graphs with unlimited
// sides
return false;
}
return Boolean(mafsFlags["polygon"]);
default:
return Boolean(mafsFlags[graph.type]);
}
}

export default {
name: "interactive-graph",
displayName: "Interactive graph",
Expand Down

0 comments on commit 26dceb8

Please sign in to comment.