From 26dceb8d7ada9b6f3c47893d8dfaccdbeb3df980 Mon Sep 17 00:00:00 2001 From: Ben Christel Date: Mon, 1 Jul 2024 10:34:01 -0700 Subject: [PATCH] Add a feature flag for Mafs point graphs with fixed numPoints (#1381) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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: https://github.com/Khan/perseus/pull/1381 --- .changeset/tricky-rice-nail.md | 5 + .../src/widgets/interactive-graph.test.tsx | 144 +++++++++++++++++- .../perseus/src/widgets/interactive-graph.tsx | 56 ++++--- 3 files changed, 186 insertions(+), 19 deletions(-) create mode 100644 .changeset/tricky-rice-nail.md diff --git a/.changeset/tricky-rice-nail.md b/.changeset/tricky-rice-nail.md new file mode 100644 index 0000000000..ff6b820670 --- /dev/null +++ b/.changeset/tricky-rice-nail.md @@ -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. diff --git a/packages/perseus/src/widgets/interactive-graph.test.tsx b/packages/perseus/src/widgets/interactive-graph.test.tsx index efa77e5479..9303a1b772 100644 --- a/packages/perseus/src/widgets/interactive-graph.test.tsx +++ b/packages/perseus/src/widgets/interactive-graph.test.tsx @@ -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 { @@ -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); + }); +}); diff --git a/packages/perseus/src/widgets/interactive-graph.tsx b/packages/perseus/src/widgets/interactive-graph.tsx index 00e6c9f8bb..2b4c8c39c7 100644 --- a/packages/perseus/src/widgets/interactive-graph.tsx +++ b/packages/perseus/src/widgets/interactive-graph.tsx @@ -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"; @@ -53,8 +53,8 @@ import type { } from "../types"; import type { QuadraticCoefficient, - SineCoefficient, Range, + SineCoefficient, } from "../util/geometry"; const {DeprecationMixin} = Util; @@ -1815,20 +1815,9 @@ class InteractiveGraph extends React.Component { } render() { - if ( - this.props.graph.type === "polygon" && - this.props.graph.numSides === "unlimited" - ) { - return ( - - ); - } - // 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, ); @@ -2675,6 +2664,37 @@ class InteractiveGraph extends React.Component { } } +// exported for testing +export function shouldUseMafs( + mafsFlags: Record | 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",