From 4a374d755e991e9cc53d419af5035ab588c7f574 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dandelion=20Man=C3=A9?= Date: Thu, 27 Sep 2018 20:32:43 -0700 Subject: [PATCH] Hyperlink Git commits to GitHub (#887) This modifies the `nodeDescription` code for the Git plugin so that when given a Git commit, it will hyperlink to that commit on GitHub. It does this by looking up the corresponding `RepoId`s from the newly-added `commitToRepoId` field in the `Repository` (#884). Per a [suggestion in review], rather than hardcoding the GitHub url logic in the Git plugin, we provide them via a `GitGateway`. [suggestion in review]: https://github.com/sourcecred/sourcecred/pull/887#issuecomment-424059649 When no `RepoId` is found, it errors to console and does not include a hyperlink. When multiple `RepoId`s are available, it chooses to link to one arbitrarily. (In the future, we could amend this behavior to add links to every valid repo). This behavior is tested. Test plan: I ran the application on newly-generated data and verified that it sets up commit hyperlinks appropriately. Also, see unit tests. --- CHANGELOG.md | 1 + src/app/adapters/defaultPlugins.js | 6 +- .../git/__snapshots__/render.test.js.snap | 3 - src/plugins/git/gitGateway.js | 12 ++ src/plugins/git/pluginAdapter.js | 20 ++- src/plugins/git/render.js | 41 +++++- src/plugins/git/render.test.js | 136 +++++++++++++++--- src/plugins/github/githubGitGateway.js | 11 ++ src/plugins/github/githubGitGateway.test.js | 17 +++ src/plugins/github/pluginAdapter.js | 2 +- src/plugins/github/render.js | 1 - 11 files changed, 214 insertions(+), 36 deletions(-) delete mode 100644 src/plugins/git/__snapshots__/render.test.js.snap create mode 100644 src/plugins/git/gitGateway.js create mode 100644 src/plugins/github/githubGitGateway.js create mode 100644 src/plugins/github/githubGitGateway.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d5eb28..173692b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ # Changelog ## [Unreleased] +- Hyperlink Git commits to GitHub (#887) - Relicense from MIT to MIT + Apache-2 (#812) - Display short hash + summary for commits (#879) - Hyperlink to GitHub entities (#860) diff --git a/src/app/adapters/defaultPlugins.js b/src/app/adapters/defaultPlugins.js index 4e1c11a..c036176 100644 --- a/src/app/adapters/defaultPlugins.js +++ b/src/app/adapters/defaultPlugins.js @@ -3,7 +3,11 @@ import {StaticAdapterSet} from "./adapterSet"; import {StaticPluginAdapter as GithubAdapter} from "../../plugins/github/pluginAdapter"; import {StaticPluginAdapter as GitAdapter} from "../../plugins/git/pluginAdapter"; +import {GithubGitGateway} from "../../plugins/github/githubGitGateway"; export function defaultStaticAdapters(): StaticAdapterSet { - return new StaticAdapterSet([new GithubAdapter(), new GitAdapter()]); + return new StaticAdapterSet([ + new GithubAdapter(), + new GitAdapter(new GithubGitGateway()), + ]); } diff --git a/src/plugins/git/__snapshots__/render.test.js.snap b/src/plugins/git/__snapshots__/render.test.js.snap deleted file mode 100644 index d362096..0000000 --- a/src/plugins/git/__snapshots__/render.test.js.snap +++ /dev/null @@ -1,3 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`plugins/git/render commit snapshots as expected 1`] = `"3715ddf: This is an example commit"`; diff --git a/src/plugins/git/gitGateway.js b/src/plugins/git/gitGateway.js new file mode 100644 index 0000000..d64c7c4 --- /dev/null +++ b/src/plugins/git/gitGateway.js @@ -0,0 +1,12 @@ +// @flow + +import type {RepoId} from "../../core/repoId"; +import type {Hash} from "./types"; + +export type URL = string; + +// Interface for adapting to a Git hosting provider, e.g. GitHub +export interface GitGateway { + // URL to permalink to an individual commit + commitUrl(repo: RepoId, hash: Hash): URL; +} diff --git a/src/plugins/git/pluginAdapter.js b/src/plugins/git/pluginAdapter.js index c11f7d6..0ee46de 100644 --- a/src/plugins/git/pluginAdapter.js +++ b/src/plugins/git/pluginAdapter.js @@ -10,8 +10,14 @@ import {description} from "./render"; import type {Assets} from "../../app/assets"; import type {RepoId} from "../../core/repoId"; import type {Repository} from "./types"; +import type {GitGateway} from "./gitGateway"; export class StaticPluginAdapter implements IStaticPluginAdapter { + _gitGateway: GitGateway; + + constructor(gg: GitGateway): void { + this._gitGateway = gg; + } name() { return "Git"; } @@ -65,16 +71,22 @@ export class StaticPluginAdapter implements IStaticPluginAdapter { loadGraph(), loadRepository(), ]); - return new DynamicPluginAdapter(graph, repository); + return new DynamicPluginAdapter(this._gitGateway, graph, repository); } } class DynamicPluginAdapter implements IDynamicPluginAdapter { +_graph: Graph; +_repository: Repository; - constructor(graph: Graph, repository: Repository) { + +_gitGateway: GitGateway; + constructor( + gitGateway: GitGateway, + graph: Graph, + repository: Repository + ): void { this._graph = graph; this._repository = repository; + this._gitGateway = gitGateway; } graph() { return this._graph; @@ -83,9 +95,9 @@ class DynamicPluginAdapter implements IDynamicPluginAdapter { // This cast is unsound, and might throw at runtime, but won't have // silent failures or cause problems down the road. const address = N.fromRaw((node: any)); - return description(address, this._repository); + return description(address, this._repository, this._gitGateway); } static() { - return new StaticPluginAdapter(); + return new StaticPluginAdapter(this._gitGateway); } } diff --git a/src/plugins/git/render.js b/src/plugins/git/render.js index 161fc2c..70e38c2 100644 --- a/src/plugins/git/render.js +++ b/src/plugins/git/render.js @@ -1,11 +1,16 @@ // @flow +import React from "react"; +import Link from "../../app/Link"; import * as N from "./nodes"; import type {Repository} from "./types"; +import {type RepoIdString, stringToRepoId} from "../../core/repoId"; +import type {GitGateway} from "./gitGateway"; export function description( address: N.StructuredAddress, - repository: Repository + repository: Repository, + gateway: GitGateway ) { switch (address.type) { case "COMMIT": { @@ -13,12 +18,40 @@ export function description( const commit = repository.commits[hash]; if (commit == null) { console.error(`Unable to find data for commit ${hash}`); - return hash; + return {hash}; } - const {shortHash, summary} = commit; - return `${shortHash}: ${summary}`; + // This `any`-cast courtesy of facebook/flow#6927. + const repoIdStrings: $ReadOnlyArray = (Object.keys( + repository.commitToRepoId[hash] || {} + ): any); + if (repoIdStrings.length === 0) { + console.error(`Unable to find repoIds for commit ${hash}`); + // TODO(@wchargin): This shortHash is unambiguous for a single repo, + // but might be ambiguous across many repositories. Consider disambiguating + return ( + + {commit.shortHash}: {commit.summary} + + ); + } + const repoId = stringToRepoId(repoIdStrings[0]); + const url = gateway.commitUrl(repoId, commit.hash); + const hyperlinkedHash = hyperlink(url, commit.shortHash); + return ( + + {hyperlinkedHash}: {commit.summary} + + ); } default: throw new Error(`unknown type: ${(address.type: empty)}`); } } + +function hyperlink(url, text) { + return ( + + {text} + + ); +} diff --git a/src/plugins/git/render.test.js b/src/plugins/git/render.test.js index 312cabd..77eee40 100644 --- a/src/plugins/git/render.test.js +++ b/src/plugins/git/render.test.js @@ -1,40 +1,132 @@ // @flow -import {makeRepoId} from "../../core/repoId"; -import * as GN from "./nodes"; +import * as N from "./nodes"; +import {shallow} from "enzyme"; import {description} from "./render"; -import type {Repository} from "./types"; +import {type RepoId, repoIdToString, makeRepoId} from "../../core/repoId"; +import type {Repository, Hash, Commit} from "./types"; +import type {GitGateway, URL} from "./gitGateway"; +import Link from "../../app/Link"; + +require("../../app/testUtil").configureEnzyme(); describe("plugins/git/render", () => { - const exampleHash = "3715ddfb8d4c4fd2a6f6af75488c82f84c92ec2f"; - const exampleCommit: GN.CommitAddress = Object.freeze({ - type: GN.COMMIT_TYPE, - hash: exampleHash, - }); + const repoId1 = makeRepoId("example-owner", "1"); + const repoId2 = makeRepoId("example-owner", "2"); + const singleRepoCommit = { + hash: "singleRepoCommit", + shortHash: "singleRepo", + summary: "A simple example commit", + parentHashes: [], + }; + const twoRepoCommit = { + hash: "twoRepoCommit", + shortHash: "twoRepo", + summary: "Two repos claim dominion over this commit", + parentHashes: [], + }; + const noRepoCommit = { + hash: "noRepoCommit", + shortHash: "noRepo", + summary: "commitToRepoId has no memory of this commit ", + parentHashes: [], + }; + const zeroRepoCommit = { + hash: "zeroRepoCommit", + shortHash: "zeroRepo", + summary: "This commit has exactly zero repoIds matching", + parentHashes: [], + }; + const unregisteredCommit = { + hash: "unregisteredCommit", + shortHash: "unregistered", + summary: "This commit isn't in the Repository", + parentHashes: [], + }; const exampleRepository: Repository = Object.freeze({ commits: { - [exampleHash]: { - hash: exampleHash, - shortHash: exampleHash.slice(0, 7), - summary: "This is an example commit", - parentHashes: [], - }, + zeroRepoCommit, + singleRepoCommit, + twoRepoCommit, + noRepoCommit, }, commitToRepoId: { - [exampleHash]: {[(makeRepoId("sourcecred", "example-git"): any)]: true}, + zeroRepoCommit: {}, + singleRepoCommit: {[(repoIdToString(repoId1): any)]: true}, + twoRepoCommit: { + [(repoIdToString(repoId1): any)]: true, + [(repoIdToString(repoId2): any)]: true, + }, }, }); - it("commit snapshots as expected", () => { - expect(description(exampleCommit, exampleRepository)).toMatchSnapshot(); + const exampleGitGateway: GitGateway = Object.freeze({ + commitUrl(repo: RepoId, hash: Hash): URL { + return repoIdToString(repo) + "/" + hash; + }, + }); + + function renderExample(commit: Commit) { + const commitAddress = {type: N.COMMIT_TYPE, hash: commit.hash}; + return shallow( + description(commitAddress, exampleRepository, exampleGitGateway) + ); + } + + it("handles a commit in exactly one repo", () => { + const el = renderExample(singleRepoCommit); + const link = el.find(Link); + const expectedUrl = exampleGitGateway.commitUrl( + repoId1, + singleRepoCommit.hash + ); + expect(link.props().href).toEqual(expectedUrl); + expect(link.props().children).toEqual(singleRepoCommit.shortHash); + expect(el.text()).toContain(singleRepoCommit.summary); + }); + + it("links to a single commit if it is in multiple repos", () => { + const el = renderExample(twoRepoCommit); + const link = el.find(Link); + const expectedUrl = exampleGitGateway.commitUrl( + repoId1, + twoRepoCommit.hash + ); + expect(link.props().href).toEqual(expectedUrl); + expect(link.props().children).toEqual(twoRepoCommit.shortHash); + expect(el.text()).toContain(twoRepoCommit.summary); + }); + + it("logs an error for a commit without any RepoIds", () => { + // noRepoCommit: It has no entry in the repo tracker + // zeroRepoCommit: it has an empty entry in the repo tracker + // (behavior should be the same) + for (const commit of [noRepoCommit, zeroRepoCommit]) { + const el = renderExample(commit); + + expect(console.error).toHaveBeenCalledWith( + `Unable to find repoIds for commit ${commit.hash}` + ); + // $ExpectFlowError + console.error = jest.fn(); + + expect(el.find(Link)).toHaveLength(0); + expect(el.find("code").text()).toEqual(commit.shortHash); + expect(el.text()).toContain(commit.summary); + } }); it("logs an error for a commit not in the repository", () => { - const badCommit = {type: GN.COMMIT_TYPE, hash: "1234"}; + const el = renderExample(unregisteredCommit); + expect(console.error).toHaveBeenCalledWith( + `Unable to find data for commit ${unregisteredCommit.hash}` + ); // $ExpectFlowError console.error = jest.fn(); - expect(description(badCommit, exampleRepository)).toBe("1234"); - expect(console.error).toHaveBeenCalledWith( - "Unable to find data for commit 1234" - ); + + expect(el.find(Link)).toHaveLength(0); + // Has the full hash, b.c. short hash couldn't be found + expect(el.find("code").text()).toEqual(unregisteredCommit.hash); + // No summary, as the data wasnt available + expect(el.text()).not.toContain(unregisteredCommit.summary); }); }); diff --git a/src/plugins/github/githubGitGateway.js b/src/plugins/github/githubGitGateway.js new file mode 100644 index 0000000..0157592 --- /dev/null +++ b/src/plugins/github/githubGitGateway.js @@ -0,0 +1,11 @@ +// @flow + +import {type RepoId} from "../../core/repoId"; +import type {Hash} from "../git/types"; +import type {GitGateway, URL} from "../git/gitGateway"; + +export class GithubGitGateway implements GitGateway { + commitUrl(repoId: RepoId, hash: Hash): URL { + return `https://github.com/${repoId.owner}/${repoId.name}/commit/${hash}`; + } +} diff --git a/src/plugins/github/githubGitGateway.test.js b/src/plugins/github/githubGitGateway.test.js new file mode 100644 index 0000000..9c50ce3 --- /dev/null +++ b/src/plugins/github/githubGitGateway.test.js @@ -0,0 +1,17 @@ +// @flow + +import {makeRepoId} from "../../core/repoId"; +import {GithubGitGateway} from "./githubGitGateway"; + +describe("src/plugins/github/githubGitGateway", () => { + describe("commitUrl", () => { + it("works for a simple example", () => { + const repoId = makeRepoId("sourcecred", "example-github"); + const hash = "ec91adb718a6"; + const url = new GithubGitGateway().commitUrl(repoId, hash); + expect(url).toMatchInlineSnapshot( + `"https://github.com/sourcecred/example-github/commit/ec91adb718a6"` + ); + }); + }); +}); diff --git a/src/plugins/github/pluginAdapter.js b/src/plugins/github/pluginAdapter.js index bb6736d..353b1d8 100644 --- a/src/plugins/github/pluginAdapter.js +++ b/src/plugins/github/pluginAdapter.js @@ -154,7 +154,7 @@ export class StaticPluginAdapter implements IStaticPluginAdapter { class DynamicPluginAdapter implements IDynamicPluginAdapater { +_view: RelationalView; +_graph: Graph; - constructor(view: RelationalView, graph: Graph) { + constructor(view: RelationalView, graph: Graph): void { this._view = view; this._graph = graph; } diff --git a/src/plugins/github/render.js b/src/plugins/github/render.js index 153702a..3b64148 100644 --- a/src/plugins/github/render.js +++ b/src/plugins/github/render.js @@ -75,7 +75,6 @@ function userlike(x: R.Userlike) { // because the commit has a Git plugin prefix and will therefore by // handled by the git plugin adapter function commit(x: R.Commit) { - // TODO(@wchargin): Ensure this hash is unambiguous const shortHash = x.address().hash.slice(0, 7); return (