From aa3382a8b24d852db4944fc9125739f1eb8c6312 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dandelion=20Man=C3=A9?= Date: Fri, 27 Jul 2018 10:33:15 -0700 Subject: [PATCH] Default to selecting last loaded repository (#531) In #529, I made the cred explorer populate a dropdown with the list of repositories that are available to explore. That dropdown defaults to selecting the alphabetically first repository. This has an unfortunate consequence in that it makes it impossible for us to explicitly set a default - for example, we would like sourcecred.github.io/explorer to show sourcecred/sourcecred by default, but instead it shows example-git. So that we can choose the default, I've changed the logic so that it instead shows the most-recently-loaded data first. This required a breaking change to the repoRegistry serialized format, so I've also refactored the module to use compat, which I should have done from the beginning. Test plan: Unit tests for the repo selector are updated. The CLI load command unfortunately has no tests, so I manually tested that it always provides the lastest repository last, and appropriately handles the case where the same repository is loaded multiple times. --- src/app/credExplorer/App.js | 3 +- src/app/credExplorer/RepositorySelect.js | 12 ++- src/app/credExplorer/RepositorySelect.test.js | 76 ++++++++----------- src/app/credExplorer/repoRegistry.js | 32 ++++++++ src/app/credExplorer/repoRegistry.test.js | 53 +++++++++++++ src/cli/commands/load.js | 20 +++-- 6 files changed, 136 insertions(+), 60 deletions(-) create mode 100644 src/app/credExplorer/repoRegistry.js create mode 100644 src/app/credExplorer/repoRegistry.test.js diff --git a/src/app/credExplorer/App.js b/src/app/credExplorer/App.js index e4af73b..c5e20b9 100644 --- a/src/app/credExplorer/App.js +++ b/src/app/credExplorer/App.js @@ -16,7 +16,8 @@ import type {DynamicPluginAdapter} from "../pluginAdapter"; import {type EdgeEvaluator} from "../../core/attribution/pagerank"; import {WeightConfig} from "./WeightConfig"; import type {PagerankNodeDecomposition} from "../../core/attribution/pagerankNodeDecomposition"; -import RepositorySelect, {type Repo} from "./RepositorySelect"; +import RepositorySelect from "./RepositorySelect"; +import type {Repo} from "./repoRegistry"; import * as NullUtil from "../../util/null"; diff --git a/src/app/credExplorer/RepositorySelect.js b/src/app/credExplorer/RepositorySelect.js index e7096ac..792c628 100644 --- a/src/app/credExplorer/RepositorySelect.js +++ b/src/app/credExplorer/RepositorySelect.js @@ -7,11 +7,9 @@ import deepEqual from "lodash.isequal"; import * as NullUtil from "../../util/null"; import type {LocalStore} from "../localStore"; -export const REPO_REGISTRY_API = "/api/v1/data/repositoryRegistry.json"; +import {type Repo, fromJSON, REPO_REGISTRY_API} from "./repoRegistry"; export const REPO_KEY = "selectedRepository"; -export type Repo = {|+name: string, +owner: string|}; - export type Status = | {|+type: "LOADING"|} | {| @@ -95,17 +93,17 @@ export async function loadStatus(localStore: LocalStore): Promise { return {type: "FAILURE"}; } const json = await response.json(); - let availableRepos = Object.keys(json).map(repoStringToRepo); - availableRepos = sortBy(availableRepos, (r) => r.owner, (r) => r.name); + const availableRepos = fromJSON(json); if (availableRepos.length === 0) { return {type: "NO_REPOS"}; } const localStoreRepo = localStore.get(REPO_KEY, null); const selectedRepo = NullUtil.orElse( availableRepos.find((x) => deepEqual(x, localStoreRepo)), - availableRepos[0] + availableRepos[availableRepos.length - 1] ); - return {type: "VALID", availableRepos, selectedRepo}; + const sortedRepos = sortBy(availableRepos, (r) => r.owner, (r) => r.name); + return {type: "VALID", availableRepos: sortedRepos, selectedRepo}; } catch (e) { return {type: "FAILURE"}; } diff --git a/src/app/credExplorer/RepositorySelect.test.js b/src/app/credExplorer/RepositorySelect.test.js index e9942e5..2430b01 100644 --- a/src/app/credExplorer/RepositorySelect.test.js +++ b/src/app/credExplorer/RepositorySelect.test.js @@ -10,9 +10,10 @@ import RepositorySelect, { loadStatus, type Status, REPO_KEY, - REPO_REGISTRY_API, } from "./RepositorySelect"; +import {toJSON, type RepoRegistry, REPO_REGISTRY_API} from "./repoRegistry"; + require("../testUtil").configureEnzyme(); require("../testUtil").configureAphrodite(); @@ -20,6 +21,10 @@ describe("app/credExplorer/RepositorySelect", () => { beforeEach(() => { fetch.resetMocks(); }); + + function mockRegistry(registry: RepoRegistry) { + fetch.mockResponseOnce(JSON.stringify(toJSON(registry))); + } describe("PureRepositorySelect", () => { it("doesn't render a select while loading", () => { const e = shallow( @@ -100,12 +105,10 @@ describe("app/credExplorer/RepositorySelect", () => { describe("loadStatus", () => { function expectLoadValidStatus( - fetchReturn, localStore, expectedAvailableRepos, expectedSelectedRepo ) { - fetch.mockResponseOnce(fetchReturn); const result = loadStatus(localStore); expect(fetch).toHaveBeenCalledTimes(1); expect(fetch).toHaveBeenCalledWith(REPO_REGISTRY_API); @@ -120,27 +123,19 @@ describe("app/credExplorer/RepositorySelect", () => { }); } it("calls fetch and handles a simple success", () => { - const fetchResult = JSON.stringify({"foo/bar": true}); + mockRegistry([{owner: "foo", name: "bar"}]); const repo = {owner: "foo", name: "bar"}; - return expectLoadValidStatus(fetchResult, testLocalStore(), [repo], repo); + return expectLoadValidStatus(testLocalStore(), [repo], repo); }); - it("returns repos in sorted order, and selects the first repo", () => { - const fetchResult = JSON.stringify({ - "foo/bar": true, - "a/z": true, - "a/b": true, - }); + it("returns repos in sorted order, and selects the last repo", () => { const repos = [ {owner: "a", name: "b"}, {owner: "a", name: "z"}, {owner: "foo", name: "bar"}, ]; - return expectLoadValidStatus( - fetchResult, - testLocalStore(), - repos, - repos[0] - ); + const nonSortedRepos = [repos[2], repos[0], repos[1]]; + mockRegistry(nonSortedRepos); + return expectLoadValidStatus(testLocalStore(), repos, repos[1]); }); it("returns FAILURE on invalid fetch response", () => { fetch.mockResponseOnce(JSON.stringify(["hello"])); @@ -157,49 +152,37 @@ describe("app/credExplorer/RepositorySelect", () => { }); }); it("loads selectedRepo from localStore, if available", () => { - const fetchResult = JSON.stringify({ - "foo/bar": true, - "a/z": true, - "a/b": true, - }); const repos = [ {owner: "a", name: "b"}, {owner: "a", name: "z"}, {owner: "foo", name: "bar"}, ]; + mockRegistry(repos); const localStore = testLocalStore(); localStore.set(REPO_KEY, {owner: "a", name: "z"}); - return expectLoadValidStatus(fetchResult, localStore, repos, repos[1]); + return expectLoadValidStatus(localStore, repos, repos[1]); }); it("ignores selectedRepo from localStore, if not available", () => { - const fetchResult = JSON.stringify({ - "foo/bar": true, - "a/z": true, - "a/b": true, - }); const repos = [ {owner: "a", name: "b"}, {owner: "a", name: "z"}, {owner: "foo", name: "bar"}, ]; + mockRegistry(repos); const localStore = testLocalStore(); localStore.set(REPO_KEY, {owner: "non", name: "existent"}); - return expectLoadValidStatus(fetchResult, localStore, repos, repos[0]); + return expectLoadValidStatus(localStore, repos, repos[2]); }); it("ignores malformed value in localStore", () => { - const fetchResult = JSON.stringify({ - "foo/bar": true, - "a/z": true, - "a/b": true, - }); const repos = [ {owner: "a", name: "b"}, {owner: "a", name: "z"}, {owner: "foo", name: "bar"}, ]; + mockRegistry(repos); const localStore = testLocalStore(); localStore.set(REPO_KEY, 42); - return expectLoadValidStatus(fetchResult, localStore, repos, repos[0]); + return expectLoadValidStatus(localStore, repos, repos[2]); }); }); @@ -289,13 +272,13 @@ describe("app/credExplorer/RepositorySelect", () => { it("on successful load, sets the status on the child", async () => { const onChange = jest.fn(); - fetch.mockResponseOnce(JSON.stringify({"foo/bar": true})); + const selectedRepo = {owner: "foo", name: "bar"}; + mockRegistry([selectedRepo]); const e = shallow( ); await waitForUpdate(e); const childStatus = e.props().status; - const selectedRepo = {owner: "foo", name: "bar"}; const availableRepos = [selectedRepo]; expect(childStatus).toEqual({ type: "VALID", @@ -306,16 +289,17 @@ describe("app/credExplorer/RepositorySelect", () => { it("on successful load, passes the status to the onChange", async () => { const onChange = jest.fn(); - fetch.mockResponseOnce(JSON.stringify({"foo/bar": true})); + const repo = { + owner: "foo", + name: "bar", + }; + mockRegistry([repo]); const e = shallow( ); await waitForUpdate(e); expect(onChange).toHaveBeenCalledTimes(1); - expect(onChange).toHaveBeenCalledWith({ - owner: "foo", - name: "bar", - }); + expect(onChange).toHaveBeenCalledWith(repo); }); it("on failed load, onChange not called", async () => { @@ -343,20 +327,20 @@ describe("app/credExplorer/RepositorySelect", () => { it("selecting child option updates top-level state", async () => { const onChange = jest.fn(); - fetch.mockResponseOnce(JSON.stringify({"foo/bar": true, "z/a": true})); + const repos = [{owner: "foo", name: "bar"}, {owner: "z", name: "a"}]; + mockRegistry(repos); const e = mount( ); await waitForUpdate(e); const child = e.find(PureRepositorySelect); - const repo = {owner: "z", name: "a"}; - child.props().onChange(repo); + child.props().onChange(repos[0]); const status: Status = e.state().status; expect(status.type).toEqual("VALID"); if (status.type !== "VALID") { throw new Error("Impossible"); } - expect(status.selectedRepo).toEqual(repo); + expect(status.selectedRepo).toEqual(repos[0]); }); }); }); diff --git a/src/app/credExplorer/repoRegistry.js b/src/app/credExplorer/repoRegistry.js new file mode 100644 index 0000000..aed5692 --- /dev/null +++ b/src/app/credExplorer/repoRegistry.js @@ -0,0 +1,32 @@ +// @flow + +// The repoRegistry is written by the CLI load command +// (src/cli/commands/load.js) and is read by the RepositorySelect component +// (src/app/credExplorer/RepositorySelect.js) +import deepEqual from "lodash.isequal"; +import {toCompat, fromCompat, type Compatible} from "../../util/compat"; + +export const REPO_REGISTRY_FILE = "repositoryRegistry.json"; +export const REPO_REGISTRY_API = "/api/v1/data/repositoryRegistry.json"; + +const REPO_REGISTRY_COMPAT = {type: "REPO_REGISTRY", version: "0.1.0"}; + +export type Repo = {|+name: string, +owner: string|}; +export type RepoRegistry = $ReadOnlyArray; +export type RepoRegistryJSON = Compatible; + +export function toJSON(r: RepoRegistry): RepoRegistryJSON { + return toCompat(REPO_REGISTRY_COMPAT, r); +} + +export function fromJSON(j: RepoRegistryJSON): RepoRegistry { + return fromCompat(REPO_REGISTRY_COMPAT, j); +} + +export function addRepo(r: Repo, reg: RepoRegistry): RepoRegistry { + return [...reg.filter((x) => !deepEqual(x, r)), r]; +} + +export function emptyRegistry(): RepoRegistry { + return []; +} diff --git a/src/app/credExplorer/repoRegistry.test.js b/src/app/credExplorer/repoRegistry.test.js new file mode 100644 index 0000000..e56a36f --- /dev/null +++ b/src/app/credExplorer/repoRegistry.test.js @@ -0,0 +1,53 @@ +// @flow + +import { + toJSON, + fromJSON, + addRepo, + emptyRegistry, + type RepoRegistry, +} from "./repoRegistry"; + +describe("app/credExplorer/repoRegistry", () => { + const r = (owner, name) => ({owner, name}); + describe("to/fromJSON compose on", () => { + function checkExample(x: RepoRegistry) { + expect(fromJSON(toJSON(x))).toEqual(x); + expect(toJSON(fromJSON(toJSON(x)))).toEqual(toJSON(x)); + } + it("empty registry", () => { + checkExample(emptyRegistry()); + }); + it("nonempty registry", () => { + checkExample([r("foo", "bar"), r("zoo", "zod")]); + }); + }); + describe("addRepo", () => { + it("adds to empty registry", () => { + expect(addRepo(r("foo", "bar"), emptyRegistry())).toEqual([ + r("foo", "bar"), + ]); + }); + it("adds to nonempty registry", () => { + const registry = [r("foo", "bar")]; + expect(addRepo(r("zoo", "zod"), registry)).toEqual([ + r("foo", "bar"), + r("zoo", "zod"), + ]); + }); + it("adding repo that is already the last has no effect", () => { + const registry = [r("zoo", "zod"), r("foo", "bar")]; + expect(addRepo(r("foo", "bar"), registry)).toEqual(registry); + }); + it("adding already-existing repo shifts it to the end", () => { + const registry = [r("zoo", "zod"), r("foo", "bar")]; + expect(addRepo(r("zoo", "zod"), registry)).toEqual([ + r("foo", "bar"), + r("zoo", "zod"), + ]); + }); + }); + it("empty registry is empty", () => { + expect(emptyRegistry()).toEqual([]); + }); +}); diff --git a/src/cli/commands/load.js b/src/cli/commands/load.js index fc05e2f..4cd5933 100644 --- a/src/cli/commands/load.js +++ b/src/cli/commands/load.js @@ -14,6 +14,14 @@ import { sourcecredDirectoryFlag, } from "../common"; +import { + toJSON, + fromJSON, + addRepo, + emptyRegistry, + REPO_REGISTRY_FILE, +} from "../../app/credExplorer/repoRegistry"; + const execDependencyGraph = require("../../tools/execDependencyGraph").default; export default class PluginGraphCommand extends Command { @@ -152,21 +160,21 @@ function loadPlugin({basedir, plugin, repoOwner, repoName, githubToken}) { } } -const REPO_REGISTRY_FILE = "repositoryRegistry.json"; - function addToRepoRegistry(options) { // TODO: Make this function transactional before loading repositories in // parallel. const {basedir, repoOwner, repoName} = options; + const repo = {owner: repoOwner, name: repoName}; const outputFile = path.join(basedir, REPO_REGISTRY_FILE); let registry = null; if (fs.existsSync(outputFile)) { const contents = fs.readFileSync(outputFile); - registry = JSON.parse(contents.toString()); + const registryJSON = JSON.parse(contents.toString()); + registry = fromJSON(registryJSON); } else { - registry = {}; + registry = emptyRegistry(); } + registry = addRepo(repo, registry); - registry[`${repoOwner}/${repoName}`] = true; - fs.writeFileSync(outputFile, stringify(registry)); + fs.writeFileSync(outputFile, stringify(toJSON(registry))); }