From bf35bbbbdadd5953d2bae564e58ca39e5f533718 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dandelion=20Man=C3=A9?= Date: Thu, 6 Sep 2018 18:58:09 -0700 Subject: [PATCH] Move WeightConfig into PagerankTable (#798) Previously, the WeightConfig (and the button that expanded it) were in the credExplorer App. This was a little weird, as there's no reason to play with the weights before you have some Pagerank results to investigate; additionally, it risked confusing new users with a concept that was not yet applicable. Also, the implementation was wonky: the WeightConfig had responsibility for expanding/hiding itself, which gave poor ability to position the button and the WeightConfig separately. Finally, the codepath was untested (vestiges of #604). This commit fixes all three issues: - The WeightConfig and button have moved into PagerankTable - The WeightConfig is now a stateless component, and the parent takes responsibility for deciding when to mount it - Logic for showing/hiding the WeightConfig is now tested. --- src/app/credExplorer/App.js | 10 +- src/app/credExplorer/App.test.js | 22 ++-- src/app/credExplorer/pagerankTable/Table.js | 42 ++++++- .../credExplorer/pagerankTable/Table.test.js | 115 ++++++++++++------ .../pagerankTable/sharedTestUtils.js | 4 +- src/app/credExplorer/weights/WeightConfig.js | 33 ++--- .../credExplorer/weights/WeightConfig.test.js | 2 - 7 files changed, 138 insertions(+), 90 deletions(-) diff --git a/src/app/credExplorer/App.js b/src/app/credExplorer/App.js index d19c17e..052d34f 100644 --- a/src/app/credExplorer/App.js +++ b/src/app/credExplorer/App.js @@ -9,7 +9,6 @@ import BrowserLocalStore from "../browserLocalStore"; import {defaultStaticAdapters} from "../adapters/defaultPlugins"; import {PagerankTable} from "./pagerankTable/Table"; -import {WeightConfig} from "./weights/WeightConfig"; import { type WeightedTypes, defaultWeightsForAdapterSet, @@ -86,6 +85,10 @@ export function createApp( + this.setState({weightedTypes}) + } pnd={pnd} maxEntriesPerList={100} /> @@ -130,11 +133,6 @@ export function createApp( > Analyze cred - this.setState({weightedTypes})} - weightedTypes={this.state.weightedTypes} - adapters={this.props.adapters} - /> {pagerankTable} diff --git a/src/app/credExplorer/App.test.js b/src/app/credExplorer/App.test.js index 75d705e..6bd59b0 100644 --- a/src/app/credExplorer/App.test.js +++ b/src/app/credExplorer/App.test.js @@ -13,7 +13,6 @@ import {defaultWeightsForAdapter} from "./weights/weights"; import RepositorySelect from "./RepositorySelect"; import {PagerankTable} from "./pagerankTable/Table"; -import {WeightConfig} from "./weights/WeightConfig"; import {createApp, LoadingIndicator} from "./App"; import {uninitializedState} from "./state"; import {_Prefix as GithubPrefix} from "../../plugins/github/nodes"; @@ -126,18 +125,6 @@ describe("app/credExplorer/App", () => { }); } - function testWeightConfig(stateFn) { - it("creates a working WeightConfig", () => { - const {el, setState} = example(); - setState(stateFn()); - const wc = el.find(WeightConfig); - const wt = defaultWeightsForAdapter(new FactorioStaticAdapter()); - wc.props().onChange(wt); - expect(el.state().weightedTypes).toBe(wt); - expect(wc.props().adapters).toBe(el.instance().props.adapters); - }); - } - function testAnalyzeCredButton(stateFn, {disabled}) { const adjective = disabled ? "disabled" : "working"; it(`has a ${adjective} analyze cred button`, () => { @@ -178,8 +165,16 @@ describe("app/credExplorer/App", () => { } const adapters = state.graphWithAdapters.adapters; const pnd = state.pagerankNodeDecomposition; + const weightedTypes = el.instance().state.weightedTypes; expect(prt.props().adapters).toBe(adapters); expect(prt.props().pnd).toBe(pnd); + expect(prt.props().weightedTypes).toBe(weightedTypes); + const prtWeightedTypesChange = prt.props().onWeightedTypesChange; + const newTypes = defaultWeightsForAdapter( + new FactorioStaticAdapter() + ); + prtWeightedTypesChange(newTypes); + expect(el.instance().state.weightedTypes).toBe(newTypes); } else { expect(prt).toHaveLength(0); } @@ -203,7 +198,6 @@ describe("app/credExplorer/App", () => { {analyzeCredDisabled, hasPagerankTable} ) { describe(suiteName, () => { - testWeightConfig(stateFn); testRepositorySelect(stateFn); testAnalyzeCredButton(stateFn, {disabled: analyzeCredDisabled}); testPagerankTable(stateFn, hasPagerankTable); diff --git a/src/app/credExplorer/pagerankTable/Table.js b/src/app/credExplorer/pagerankTable/Table.js index 9f07ff3..cd7a82f 100644 --- a/src/app/credExplorer/pagerankTable/Table.js +++ b/src/app/credExplorer/pagerankTable/Table.js @@ -9,16 +9,23 @@ import type {PagerankNodeDecomposition} from "../../../core/attribution/pagerank import {DynamicAdapterSet} from "../../adapters/adapterSet"; import type {DynamicPluginAdapter} from "../../adapters/pluginAdapter"; import {FALLBACK_NAME} from "../../adapters/fallbackAdapter"; +import {type WeightedTypes} from "../weights/weights"; +import {WeightConfig} from "../weights/WeightConfig"; import {NodeRowList} from "./Node"; type PagerankTableProps = {| +pnd: PagerankNodeDecomposition, +adapters: DynamicAdapterSet, + +weightedTypes: WeightedTypes, + +onWeightedTypesChange: (WeightedTypes) => void, +maxEntriesPerList: number, +defaultNodeFilter: ?NodeAddressT, |}; -type PagerankTableState = {|topLevelFilter: NodeAddressT|}; +type PagerankTableState = {| + topLevelFilter: NodeAddressT, + showWeightConfig: boolean, +|}; export class PagerankTable extends React.PureComponent< PagerankTableProps, PagerankTableState @@ -38,13 +45,42 @@ export class PagerankTable extends React.PureComponent< props.defaultNodeFilter, NodeAddress.empty ); - this.state = {topLevelFilter}; + this.state = {topLevelFilter, showWeightConfig: false}; + } + + renderConfigurationRow() { + const {showWeightConfig} = this.state; + return ( +
+ {this.renderFilterSelect()} + + +
+ ); } render() { + const {showWeightConfig} = this.state; return (
- {this.renderFilterSelect()} + {this.renderConfigurationRow()} + {showWeightConfig && ( + this.props.onWeightedTypesChange(wt)} + /> + )} {this.renderTable()}
); diff --git a/src/app/credExplorer/pagerankTable/Table.test.js b/src/app/credExplorer/pagerankTable/Table.test.js index 399d2cd..a444d18 100644 --- a/src/app/credExplorer/pagerankTable/Table.test.js +++ b/src/app/credExplorer/pagerankTable/Table.test.js @@ -7,42 +7,87 @@ import {NodeAddress, type NodeAddressT} from "../../../core/graph"; import {PagerankTable} from "./Table"; import {example, COLUMNS} from "./sharedTestUtils"; +import {NodeRowList} from "./Node"; +import {WeightConfig} from "../weights/WeightConfig"; +import {defaultWeightsForAdapter} from "../weights/weights"; +import {FactorioStaticAdapter} from "../../adapters/demoAdapters"; require("../../testUtil").configureEnzyme(); describe("app/credExplorer/pagerankTable/Table", () => { describe("PagerankTable", () => { - it("renders thead column order properly", async () => { - const {pnd, adapters} = await example(); + async function setup(defaultNodeFilter?: NodeAddressT) { + const {pnd, adapters, weightedTypes} = await example(); + const onWeightedTypesChange = jest.fn(); + const maxEntriesPerList = 321; const element = shallow( ); + return {pnd, adapters, element, maxEntriesPerList, onWeightedTypesChange}; + } + it("renders thead column order properly", async () => { + const {element} = await setup(); const th = element.find("thead th"); const columnNames = th.map((t) => t.text()); expect(columnNames).toEqual(COLUMNS()); }); - describe("has a filter select", () => { - async function setup(defaultNodeFilter?: NodeAddressT) { - const {pnd, adapters} = await example(); - const element = shallow( - - ); - const label = element.find("label"); - const options = label.find("option"); - return {pnd, adapters, element, label, options}; + describe("has a WeightConfig", () => { + function findButton(element) { + const button = element + .findWhere( + (x) => + x.text() === "Show weight configuration" || + x.text() === "Hide weight configuration" + ) + .find("button"); + expect(button).toHaveLength(1); + return button; } + it("which is not present by default", async () => { + const {element} = await setup(); + expect(element.find(WeightConfig)).toHaveLength(0); + const button = findButton(element); + expect(button.text()).toEqual("Show weight configuration"); + }); + it("which is present when the WeightConfig button is pushed", async () => { + const {element, onWeightedTypesChange} = await setup(); + let button = findButton(element); + expect(button.text()).toEqual("Show weight configuration"); + button.simulate("click"); + button = findButton(element); + expect(button.text()).toEqual("Hide weight configuration"); + expect(button).toHaveLength(1); // Its text changed + const wc = element.find(WeightConfig); + expect(wc).toHaveLength(1); + expect(wc.props().weightedTypes).toBe( + element.instance().props.weightedTypes + ); + const wt = defaultWeightsForAdapter(new FactorioStaticAdapter()); + wc.props().onChange(wt); + expect(onWeightedTypesChange).toHaveBeenCalledWith(wt); + expect(onWeightedTypesChange).toHaveBeenCalledTimes(1); + }); + it("which is hidden when the WeightConfig button is pushed twice", async () => { + const {element} = await setup(); + findButton(element).simulate("click"); + findButton(element).simulate("click"); + let button = findButton(element); + expect(button.text()).toEqual("Show weight configuration"); + expect(element.find(WeightConfig)).toHaveLength(0); + }); + }); + + describe("has a filter select", () => { it("with expected label text", async () => { - const {label} = await setup(); + const {element} = await setup(); + const label = element.find("label"); const filterText = label .find("span") .first() @@ -50,7 +95,9 @@ describe("app/credExplorer/pagerankTable/Table", () => { expect(filterText).toMatchSnapshot(); }); it("with expected option groups", async () => { - const {options} = await setup(); + const {element} = await setup(); + const label = element.find("label"); + const options = label.find("option"); const optionsJSON = options.map((o) => ({ valueString: NodeAddress.toString(o.prop("value")), style: o.prop("style"), @@ -59,7 +106,9 @@ describe("app/credExplorer/pagerankTable/Table", () => { expect(optionsJSON).toMatchSnapshot(); }); it("with the ability to filter nodes passed to NodeRowList", async () => { - const {element, options} = await setup(); + const {element} = await setup(); + const label = element.find("label"); + const options = label.find("option"); const option = options.at(2); const value = option.prop("value"); @@ -93,29 +142,17 @@ describe("app/credExplorer/pagerankTable/Table", () => { }); describe("creates a NodeRowList", () => { - async function setup() { - const {adapters, pnd} = await example(); - const maxEntriesPerList = 1; - const element = shallow( - - ); - const nrl = element.find("NodeRowList"); - return {adapters, pnd, element, nrl, maxEntriesPerList}; - } it("with the correct SharedProps", async () => { - const {nrl, adapters, pnd, maxEntriesPerList} = await setup(); + const {element, adapters, pnd, maxEntriesPerList} = await setup(); + const nrl = element.find(NodeRowList); const expectedSharedProps = {adapters, pnd, maxEntriesPerList}; - expect(nrl.prop("sharedProps")).toEqual(expectedSharedProps); + expect(nrl.props().sharedProps).toEqual(expectedSharedProps); }); it("including all nodes by default", async () => { - const {nrl, pnd} = await setup(); + const {element, pnd} = await setup(); + const nrl = element.find(NodeRowList); const expectedNodes = Array.from(pnd.keys()); - expect(nrl.prop("nodes")).toEqual(expectedNodes); + expect(nrl.props().nodes).toEqual(expectedNodes); }); }); }); diff --git a/src/app/credExplorer/pagerankTable/sharedTestUtils.js b/src/app/credExplorer/pagerankTable/sharedTestUtils.js index ed5995f..b29256c 100644 --- a/src/app/credExplorer/pagerankTable/sharedTestUtils.js +++ b/src/app/credExplorer/pagerankTable/sharedTestUtils.js @@ -2,16 +2,18 @@ import {dynamicAdapterSet} from "../../adapters/demoAdapters"; import {pagerank} from "../../../core/attribution/pagerank"; +import {defaultWeightsForAdapterSet} from "../weights/weights"; export const COLUMNS = () => ["Description", "", "Cred"]; export async function example() { const adapters = await dynamicAdapterSet(); + const weightedTypes = defaultWeightsForAdapterSet(adapters.static()); const graph = adapters.graph(); const pnd = await pagerank(graph, (_unused_Edge) => ({ toWeight: 1, froWeight: 1, })); - return {adapters, pnd}; + return {adapters, pnd, weightedTypes}; } diff --git a/src/app/credExplorer/weights/WeightConfig.js b/src/app/credExplorer/weights/WeightConfig.js index 0da6af0..e8d1dbc 100644 --- a/src/app/credExplorer/weights/WeightConfig.js +++ b/src/app/credExplorer/weights/WeightConfig.js @@ -15,40 +15,23 @@ type Props = {| +onChange: (WeightedTypes) => void, |}; -type State = {| - expanded: boolean, -|}; - -export class WeightConfig extends React.Component { +export class WeightConfig extends React.Component { constructor(props: Props): void { super(props); - this.state = { - expanded: false, - }; } render() { - const {expanded} = this.state; return ( - - {expanded && ( -
- {this._renderPluginWeightConfigs()} -
- )} + {this._renderPluginWeightConfigs()} + ); } diff --git a/src/app/credExplorer/weights/WeightConfig.test.js b/src/app/credExplorer/weights/WeightConfig.test.js index 1402c95..4c37c73 100644 --- a/src/app/credExplorer/weights/WeightConfig.test.js +++ b/src/app/credExplorer/weights/WeightConfig.test.js @@ -31,8 +31,6 @@ describe("app/credExplorer/weights/WeightConfig", () => { onChange={onChange} /> ); - // TODO(@decentralion): WeightConfig stops expanding itself - el.setState({expanded: true}); return {el, adapters, types, onChange}; } it("creates a PluginWeightConfig for every non-fallback adapter", () => {