Fix issue comment unfurl

Closes #928 This commmit fixes unfurling of private and public issue comments. When the `octokit/rest.js` changed [their rounting arguments](octokit/rest.js@c62b2bc) from `id` to `<resource>_id`, unfurling issue comments stopped working. This PR adjusts the argument name and thus fixes the problem. In addition I added new fixtures to allow tests that have less context. This should make it clear, that for public unfurls, the user of the repo and the comment does not matter and it decouples the fixture and subscription setup. This was mainly helpful to myself to make sure the tests only require, what they should require.
integrations · Aug 30, 2019 · 73cf4b5 · 73cf4b5
1 parent 8aa8e7b
commit 73cf4b5
Show file tree

Hide file tree

Showing 8 changed files with 338 additions and 3 deletions.
diff --git a/lib/unfurls/comment.js b/lib/unfurls/comment.js
@@ -4,11 +4,11 @@ module.exports = async (params, github, unfurlType) => {
   const {
     owner, repo, number, id,
   } = params;
-  const issue = (await github.issues.get({ owner, repo, number })).data;
+  const issue = (await github.issues.get({ owner, repo, issue_number: number })).data;
   const comment = (await github.issues.getComment({
     owner,
     repo,
-    id,
+    comment_id: id,
     headers: { accept: 'application/vnd.github.html+json' },
   })).data;
   const repository = (await github.repos.get({ owner, repo })).data;

diff --git a/lib/unfurls/index.js b/lib/unfurls/index.js
@@ -56,10 +56,11 @@ async function linkShared(req, res) {
       });
     } catch (err) {
       if (err.name === 'UnsupportedResource' || err.code === 404) {
-        req.log.debug(err, 'Could not get unfurl attachment');
+        req.log.warn(err, 'Could not get unfurl attachment');
         return;
       }
       if (err.name === 'ResourceNotFound') {
+        req.log.warn(err, 'Resource not found during unfurl.');
         return;
       }
       if (err.name === 'UnfurlsAreDisabled') {

diff --git a/test/fixtures/comment-rust.json b/test/fixtures/comment-rust.json
@@ -0,0 +1,31 @@
+{
+  "url": "https://api.github.com/repos/rust-lang/rust/issues/comments/526118914",
+  "html_url": "https://github.com/rust-lang/rust/issues/63997#issuecomment-526118914",
+  "issue_url": "https://api.github.com/repos/rust-lang/rust/issues/63997",
+  "id": 526118914,
+  "node_id": "MDEyOklzc3VlQ29tbWVudDUyNjExODkxNA==",
+  "user": {
+    "login": "gnzlbg",
+    "id": 904614,
+    "node_id": "MDQ6VXNlcjkwNDYxNA==",
+    "avatar_url": "https://avatars0.githubusercontent.com/u/904614?v=4",
+    "gravatar_id": "",
+    "url": "https://api.github.com/users/gnzlbg",
+    "html_url": "https://github.com/gnzlbg",
+    "followers_url": "https://api.github.com/users/gnzlbg/followers",
+    "following_url": "https://api.github.com/users/gnzlbg/following{/other_user}",
+    "gists_url": "https://api.github.com/users/gnzlbg/gists{/gist_id}",
+    "starred_url": "https://api.github.com/users/gnzlbg/starred{/owner}{/repo}",
+    "subscriptions_url": "https://api.github.com/users/gnzlbg/subscriptions",
+    "organizations_url": "https://api.github.com/users/gnzlbg/orgs",
+    "repos_url": "https://api.github.com/users/gnzlbg/repos",
+    "events_url": "https://api.github.com/users/gnzlbg/events{/privacy}",
+    "received_events_url": "https://api.github.com/users/gnzlbg/received_events",
+    "type": "User",
+    "site_admin": false
+  },
+  "created_at": "2019-08-29T10:07:04Z",
+  "updated_at": "2019-08-29T10:14:44Z",
+  "author_association": "CONTRIBUTOR",
+  "body": "I think that, at the very least, this should work:\r\n\r\n```rust\r\nconst fn foo() {}\r\nconst FOO: const fn() = foo;\r\nconst fn bar() { FOO() }\r\nconst fn baz(x: const fn()) { x() }\r\nconst fn bazz() { baz(FOO) }\r\n```\r\n\r\nFor this to work:\r\n\r\n* `const` must be part of `fn` types (just like `unsafe`, the `extern \"ABI\"`, etc.)\r\n* we should allow calling `const fn` types from const fn\r\n\r\nCurrently, `const fn`s already coerce to `fn`s, so `const fn` types should too:\r\n\r\n```rust\r\nconst fn foo() {}\r\nlet x: const fn() = foo;\r\nlet y: fn() = x; // OK: const fn => fn coercion\r\n```\r\n\r\nI don't see any problems with supporting this. The RFC mentions some issues, but I don't see anything against just supporting this restricted subset.\r\n\r\nThis subset would be **super** useful. For example, you could do:\r\n\r\n```rust\r\nstruct Foo<T>(T);\r\ntrait Bar { const F: const fn(Self) -> Self; }\r\n\r\nimpl<T: Bar> Foo<T> {\r\n    const fn new(x: T) -> Self { Foo(<T as Bar>::F(x)) }\r\n}\r\n\r\nconst fn map_i32(x: i32) -> i32 { x * 2 }\r\nimpl Bar for i32 { const F: const fn(Self) -> Self = map_i32; } \r\nconst fn map_u32(x: i32) -> i32 { x * 3 }\r\nimpl Bar for u32 { const F: const fn(Self) -> Self = map_u32; } \r\n```\r\n\r\nwhich is a quite awesome work around for the lack of `const` trait methods, but much simpler since dynamic dispatch isn't an issue, as opposed to: \r\n\r\n```rust\r\ntrait Bar { const fn map(self) -> Self; } \r\nimpl Bar for i32 { ... }\r\nimpl Bar for u32 { ... }\r\n// or const impl Bar for i32 { ... {\r\n```\r\n\r\nThis is also a way to avoid having to use `if`/`match` etc. in `const fn`s, since you can create a trait with a const, and just dispatch on it to achieve \"conditional\" control-flow at least at compile-time."
+}
diff --git a/test/fixtures/index.js b/test/fixtures/index.js
@@ -7,11 +7,14 @@ module.exports = {
   installation: require('./installation'),
   pull: require('./pull'),
   comment: require('./comment'),
+  commentRust: require('./comment-rust'),
   release: require('./release'),
   contents: require('./contents'),
   user: require('./user'),
   org: require('./org'),
   repo: require('./repo'),
+  repoRust: require('./repo-rust'),
+  repoHub: require('./repo-hub'),
   atomRepo: require('./atom-repo'),
   kubernetesRepo: require('./kubernetes-repo'),
   combinedStatus: require('./combined_status_some_passing'),

diff --git a/test/fixtures/repo-hub.json b/test/fixtures/repo-hub.json
@@ -0,0 +1,122 @@
+{
+  "id": 401025,
+  "node_id": "MDEwOlJlcG9zaXRvcnk0MDEwMjU=",
+  "name": "hub",
+  "full_name": "github/hub",
+  "private": false,
+  "owner": {
+    "login": "github",
+    "id": 9919,
+    "node_id": "MDEyOk9yZ2FuaXphdGlvbjk5MTk=",
+    "avatar_url": "https://avatars1.githubusercontent.com/u/9919?v=4",
+    "gravatar_id": "",
+    "url": "https://api.github.com/users/github",
+    "html_url": "https://github.com/github",
+    "followers_url": "https://api.github.com/users/github/followers",
+    "following_url": "https://api.github.com/users/github/following{/other_user}",
+    "gists_url": "https://api.github.com/users/github/gists{/gist_id}",
+    "starred_url": "https://api.github.com/users/github/starred{/owner}{/repo}",
+    "subscriptions_url": "https://api.github.com/users/github/subscriptions",
+    "organizations_url": "https://api.github.com/users/github/orgs",
+    "repos_url": "https://api.github.com/users/github/repos",
+    "events_url": "https://api.github.com/users/github/events{/privacy}",
+    "received_events_url": "https://api.github.com/users/github/received_events",
+    "type": "Organization",
+    "site_admin": false
+  },
+  "html_url": "https://github.com/github/hub",
+  "description": "A command-line tool that makes git easier to use with GitHub.",
+  "fork": false,
+  "url": "https://api.github.com/repos/github/hub",
+  "forks_url": "https://api.github.com/repos/github/hub/forks",
+  "keys_url": "https://api.github.com/repos/github/hub/keys{/key_id}",
+  "collaborators_url": "https://api.github.com/repos/github/hub/collaborators{/collaborator}",
+  "teams_url": "https://api.github.com/repos/github/hub/teams",
+  "hooks_url": "https://api.github.com/repos/github/hub/hooks",
+  "issue_events_url": "https://api.github.com/repos/github/hub/issues/events{/number}",
+  "events_url": "https://api.github.com/repos/github/hub/events",
+  "assignees_url": "https://api.github.com/repos/github/hub/assignees{/user}",
+  "branches_url": "https://api.github.com/repos/github/hub/branches{/branch}",
+  "tags_url": "https://api.github.com/repos/github/hub/tags",
+  "blobs_url": "https://api.github.com/repos/github/hub/git/blobs{/sha}",
+  "git_tags_url": "https://api.github.com/repos/github/hub/git/tags{/sha}",
+  "git_refs_url": "https://api.github.com/repos/github/hub/git/refs{/sha}",
+  "trees_url": "https://api.github.com/repos/github/hub/git/trees{/sha}",
+  "statuses_url": "https://api.github.com/repos/github/hub/statuses/{sha}",
+  "languages_url": "https://api.github.com/repos/github/hub/languages",
+  "stargazers_url": "https://api.github.com/repos/github/hub/stargazers",
+  "contributors_url": "https://api.github.com/repos/github/hub/contributors",
+  "subscribers_url": "https://api.github.com/repos/github/hub/subscribers",
+  "subscription_url": "https://api.github.com/repos/github/hub/subscription",
+  "commits_url": "https://api.github.com/repos/github/hub/commits{/sha}",
+  "git_commits_url": "https://api.github.com/repos/github/hub/git/commits{/sha}",
+  "comments_url": "https://api.github.com/repos/github/hub/comments{/number}",
+  "issue_comment_url": "https://api.github.com/repos/github/hub/issues/comments{/number}",
+  "contents_url": "https://api.github.com/repos/github/hub/contents/{+path}",
+  "compare_url": "https://api.github.com/repos/github/hub/compare/{base}...{head}",
+  "merges_url": "https://api.github.com/repos/github/hub/merges",
+  "archive_url": "https://api.github.com/repos/github/hub/{archive_format}{/ref}",
+  "downloads_url": "https://api.github.com/repos/github/hub/downloads",
+  "issues_url": "https://api.github.com/repos/github/hub/issues{/number}",
+  "pulls_url": "https://api.github.com/repos/github/hub/pulls{/number}",
+  "milestones_url": "https://api.github.com/repos/github/hub/milestones{/number}",
+  "notifications_url": "https://api.github.com/repos/github/hub/notifications{?since,all,participating}",
+  "labels_url": "https://api.github.com/repos/github/hub/labels{/name}",
+  "releases_url": "https://api.github.com/repos/github/hub/releases{/id}",
+  "deployments_url": "https://api.github.com/repos/github/hub/deployments",
+  "created_at": "2009-12-05T22:15:25Z",
+  "updated_at": "2019-08-30T08:57:14Z",
+  "pushed_at": "2019-08-27T10:56:22Z",
+  "git_url": "git://github.com/github/hub.git",
+  "ssh_url": "git@github.com:github/hub.git",
+  "clone_url": "https://github.com/github/hub.git",
+  "svn_url": "https://github.com/github/hub",
+  "homepage": "https://hub.github.com/",
+  "size": 6319,
+  "stargazers_count": 17223,
+  "watchers_count": 17223,
+  "language": "Go",
+  "has_issues": true,
+  "has_projects": true,
+  "has_downloads": false,
+  "has_wiki": false,
+  "has_pages": true,
+  "forks_count": 1758,
+  "mirror_url": null,
+  "archived": false,
+  "disabled": false,
+  "open_issues_count": 201,
+  "license": {
+    "key": "mit",
+    "name": "MIT License",
+    "spdx_id": "MIT",
+    "url": "https://api.github.com/licenses/mit",
+    "node_id": "MDc6TGljZW5zZTEz"
+  },
+  "forks": 1758,
+  "open_issues": 201,
+  "watchers": 17223,
+  "default_branch": "master",
+  "organization": {
+    "login": "github",
+    "id": 9919,
+    "node_id": "MDEyOk9yZ2FuaXphdGlvbjk5MTk=",
+    "avatar_url": "https://avatars1.githubusercontent.com/u/9919?v=4",
+    "gravatar_id": "",
+    "url": "https://api.github.com/users/github",
+    "html_url": "https://github.com/github",
+    "followers_url": "https://api.github.com/users/github/followers",
+    "following_url": "https://api.github.com/users/github/following{/other_user}",
+    "gists_url": "https://api.github.com/users/github/gists{/gist_id}",
+    "starred_url": "https://api.github.com/users/github/starred{/owner}{/repo}",
+    "subscriptions_url": "https://api.github.com/users/github/subscriptions",
+    "organizations_url": "https://api.github.com/users/github/orgs",
+    "repos_url": "https://api.github.com/users/github/repos",
+    "events_url": "https://api.github.com/users/github/events{/privacy}",
+    "received_events_url": "https://api.github.com/users/github/received_events",
+    "type": "Organization",
+    "site_admin": false
+  },
+  "network_count": 1758,
+  "subscribers_count": 335
+}
diff --git a/test/fixtures/repo-rust.json b/test/fixtures/repo-rust.json
@@ -0,0 +1,122 @@
+{
+  "id": 724712,
+  "node_id": "MDEwOlJlcG9zaXRvcnk3MjQ3MTI=",
+  "name": "rust",
+  "full_name": "rust-lang/rust",
+  "private": false,
+  "owner": {
+    "login": "rust-lang",
+    "id": 5430905,
+    "node_id": "MDEyOk9yZ2FuaXphdGlvbjU0MzA5MDU=",
+    "avatar_url": "https://avatars1.githubusercontent.com/u/5430905?v=4",
+    "gravatar_id": "",
+    "url": "https://api.github.com/users/rust-lang",
+    "html_url": "https://github.com/rust-lang",
+    "followers_url": "https://api.github.com/users/rust-lang/followers",
+    "following_url": "https://api.github.com/users/rust-lang/following{/other_user}",
+    "gists_url": "https://api.github.com/users/rust-lang/gists{/gist_id}",
+    "starred_url": "https://api.github.com/users/rust-lang/starred{/owner}{/repo}",
+    "subscriptions_url": "https://api.github.com/users/rust-lang/subscriptions",
+    "organizations_url": "https://api.github.com/users/rust-lang/orgs",
+    "repos_url": "https://api.github.com/users/rust-lang/repos",
+    "events_url": "https://api.github.com/users/rust-lang/events{/privacy}",
+    "received_events_url": "https://api.github.com/users/rust-lang/received_events",
+    "type": "Organization",
+    "site_admin": false
+  },
+  "html_url": "https://github.com/rust-lang/rust",
+  "description": "Empowering everyone to build reliable and efficient software.",
+  "fork": false,
+  "url": "https://api.github.com/repos/rust-lang/rust",
+  "forks_url": "https://api.github.com/repos/rust-lang/rust/forks",
+  "keys_url": "https://api.github.com/repos/rust-lang/rust/keys{/key_id}",
+  "collaborators_url": "https://api.github.com/repos/rust-lang/rust/collaborators{/collaborator}",
+  "teams_url": "https://api.github.com/repos/rust-lang/rust/teams",
+  "hooks_url": "https://api.github.com/repos/rust-lang/rust/hooks",
+  "issue_events_url": "https://api.github.com/repos/rust-lang/rust/issues/events{/number}",
+  "events_url": "https://api.github.com/repos/rust-lang/rust/events",
+  "assignees_url": "https://api.github.com/repos/rust-lang/rust/assignees{/user}",
+  "branches_url": "https://api.github.com/repos/rust-lang/rust/branches{/branch}",
+  "tags_url": "https://api.github.com/repos/rust-lang/rust/tags",
+  "blobs_url": "https://api.github.com/repos/rust-lang/rust/git/blobs{/sha}",
+  "git_tags_url": "https://api.github.com/repos/rust-lang/rust/git/tags{/sha}",
+  "git_refs_url": "https://api.github.com/repos/rust-lang/rust/git/refs{/sha}",
+  "trees_url": "https://api.github.com/repos/rust-lang/rust/git/trees{/sha}",
+  "statuses_url": "https://api.github.com/repos/rust-lang/rust/statuses/{sha}",
+  "languages_url": "https://api.github.com/repos/rust-lang/rust/languages",
+  "stargazers_url": "https://api.github.com/repos/rust-lang/rust/stargazers",
+  "contributors_url": "https://api.github.com/repos/rust-lang/rust/contributors",
+  "subscribers_url": "https://api.github.com/repos/rust-lang/rust/subscribers",
+  "subscription_url": "https://api.github.com/repos/rust-lang/rust/subscription",
+  "commits_url": "https://api.github.com/repos/rust-lang/rust/commits{/sha}",
+  "git_commits_url": "https://api.github.com/repos/rust-lang/rust/git/commits{/sha}",
+  "comments_url": "https://api.github.com/repos/rust-lang/rust/comments{/number}",
+  "issue_comment_url": "https://api.github.com/repos/rust-lang/rust/issues/comments{/number}",
+  "contents_url": "https://api.github.com/repos/rust-lang/rust/contents/{+path}",
+  "compare_url": "https://api.github.com/repos/rust-lang/rust/compare/{base}...{head}",
+  "merges_url": "https://api.github.com/repos/rust-lang/rust/merges",
+  "archive_url": "https://api.github.com/repos/rust-lang/rust/{archive_format}{/ref}",
+  "downloads_url": "https://api.github.com/repos/rust-lang/rust/downloads",
+  "issues_url": "https://api.github.com/repos/rust-lang/rust/issues{/number}",
+  "pulls_url": "https://api.github.com/repos/rust-lang/rust/pulls{/number}",
+  "milestones_url": "https://api.github.com/repos/rust-lang/rust/milestones{/number}",
+  "notifications_url": "https://api.github.com/repos/rust-lang/rust/notifications{?since,all,participating}",
+  "labels_url": "https://api.github.com/repos/rust-lang/rust/labels{/name}",
+  "releases_url": "https://api.github.com/repos/rust-lang/rust/releases{/id}",
+  "deployments_url": "https://api.github.com/repos/rust-lang/rust/deployments",
+  "created_at": "2010-06-16T20:39:03Z",
+  "updated_at": "2019-08-30T09:39:24Z",
+  "pushed_at": "2019-08-30T09:31:46Z",
+  "git_url": "git://github.com/rust-lang/rust.git",
+  "ssh_url": "git@github.com:rust-lang/rust.git",
+  "clone_url": "https://github.com/rust-lang/rust.git",
+  "svn_url": "https://github.com/rust-lang/rust",
+  "homepage": "https://www.rust-lang.org",
+  "size": 455465,
+  "stargazers_count": 38763,
+  "watchers_count": 38763,
+  "language": "Rust",
+  "has_issues": true,
+  "has_projects": true,
+  "has_downloads": true,
+  "has_wiki": false,
+  "has_pages": false,
+  "forks_count": 6026,
+  "mirror_url": null,
+  "archived": false,
+  "disabled": false,
+  "open_issues_count": 5180,
+  "license": {
+    "key": "other",
+    "name": "Other",
+    "spdx_id": "NOASSERTION",
+    "url": null,
+    "node_id": "MDc6TGljZW5zZTA="
+  },
+  "forks": 6026,
+  "open_issues": 5180,
+  "watchers": 38763,
+  "default_branch": "master",
+  "organization": {
+    "login": "rust-lang",
+    "id": 5430905,
+    "node_id": "MDEyOk9yZ2FuaXphdGlvbjU0MzA5MDU=",
+    "avatar_url": "https://avatars1.githubusercontent.com/u/5430905?v=4",
+    "gravatar_id": "",
+    "url": "https://api.github.com/users/rust-lang",
+    "html_url": "https://github.com/rust-lang",
+    "followers_url": "https://api.github.com/users/rust-lang/followers",
+    "following_url": "https://api.github.com/users/rust-lang/following{/other_user}",
+    "gists_url": "https://api.github.com/users/rust-lang/gists{/gist_id}",
+    "starred_url": "https://api.github.com/users/rust-lang/starred{/owner}{/repo}",
+    "subscriptions_url": "https://api.github.com/users/rust-lang/subscriptions",
+    "organizations_url": "https://api.github.com/users/rust-lang/orgs",
+    "repos_url": "https://api.github.com/users/rust-lang/repos",
+    "events_url": "https://api.github.com/users/rust-lang/events{/privacy}",
+    "received_events_url": "https://api.github.com/users/rust-lang/received_events",
+    "type": "Organization",
+    "site_admin": false
+  },
+  "network_count": 6026,
+  "subscribers_count": 1420
+}
diff --git a/test/integration/__snapshots__/unfurl.test.js.snap b/test/integration/__snapshots__/unfurl.test.js.snap
@@ -643,6 +643,21 @@ Array [
 ]
 `;
 
+exports[`Integration: unfurls public unfurls issue comments 1`] = `
+Array [
+  "unfurl-eligibility#T000A:C74M:https://github.com/rust-lang/rust/issues/63997#issuecomment-526118914",
+]
+`;
+
+exports[`Integration: unfurls public unfurls issue comments 2`] = `
+Array [
+  Array [
+    "unfurl-eligibility#T000A:C74M:https://github.com/rust-lang/rust/issues/63997#issuecomment-526118914",
+    true,
+  ],
+]
+`;
+
 exports[`Integration: unfurls public unfurls only unfurls link first time a link is shared 1`] = `
 Object {
   "channel": "C74M",

diff --git a/test/integration/unfurl.test.js b/test/integration/unfurl.test.js
@@ -53,6 +53,47 @@ describe('Integration: unfurls', () => {
         .expect(200);
     });
 
+    test('issue comments', async () => {
+      // nock the repo for the isPrivate check
+      nock('https://api.github.com').get('/repos/rust-lang/rust')
+        .reply(200, { ...fixtures.repoRust });
+
+      // nock the repo's issue for the fetching comments
+      nock('https://api.github.com').get('/repos/rust-lang/rust/issues/63997')
+        .reply(200, {});
+
+      // nock fetching the actual comment
+      nock('https://api.github.com')
+        .get('/repos/rust-lang/rust/issues/comments/526118914')
+        .reply(200, { ...fixtures.commentRust });
+
+      // nock the repo again, becasue we re-fetch it in the issueComment :/
+      nock('https://api.github.com').get('/repos/rust-lang/rust')
+        .reply(200, { ...fixtures.repo_rust });
+
+      nock('https://slack.com')
+        .post('/api/chat.unfurl')
+        .reply(200, { ok: true });
+
+
+      const issueCommentEvent = {
+        ...fixtures.slack.link_shared().event,
+        links: [
+          {
+            url: 'https://github.com/rust-lang/rust/issues/63997#issuecomment-526118914',
+            domain: 'github.com',
+          },
+        ],
+      };
+
+      const response = await request(probot.server).post('/slack/events').send({
+        ...fixtures.slack.link_shared(),
+        event: issueCommentEvent,
+      });
+      expect(response.status).toEqual(200);
+    });
+
+
     test('only unfurls link first time a link is shared', async () => {
       nock('https://api.github.com').get('/repos/bkeepers/dotenv').times(2).reply(
         200,