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,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)

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()),
+  ]);
 }

src/plugins/git/__snapshots__/render.test.js.snap

@@ -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"`;

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;
+}

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);
   }
 }

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 <code>{hash}</code>;
       }
-      const {shortHash, summary} = commit;
-      return `${shortHash}: ${summary}`;
+      // This `any`-cast courtesy of facebook/flow#6927.
+      const repoIdStrings: $ReadOnlyArray<RepoIdString> = (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 (
+          <span>
+            <code>{commit.shortHash}</code>: {commit.summary}
+          </span>
+        );
+      }
+      const repoId = stringToRepoId(repoIdStrings[0]);
+      const url = gateway.commitUrl(repoId, commit.hash);
+      const hyperlinkedHash = hyperlink(url, commit.shortHash);
+      return (
+        <span>
+          <code>{hyperlinkedHash}</code>: {commit.summary}
+        </span>
+      );
     }
     default:
       throw new Error(`unknown type: ${(address.type: empty)}`);
   }
 }
+
+function hyperlink(url, text) {
+  return (
+    <Link href={url} target="_blank" rel="nofollow noopener">
+      {text}
+    </Link>
+  );
+}

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);
   });
 });

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}`;
+  }
+}

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"`
+      );
+    });
+  });
+});

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;
   }

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 (
     <span>