From 73cf4b50f563191f1aafc675ff884108f4b3f2d7 Mon Sep 17 00:00:00 2001 From: Dennis Sivia Date: Fri, 30 Aug 2019 12:42:58 +0200 Subject: [PATCH 1/2] 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](https://github.com/octokit/rest.js/commit/c62b2bcb2c4f66d93ecff1563df7f7d8e8431788) from `id` to `_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. --- lib/unfurls/comment.js | 4 +- lib/unfurls/index.js | 3 +- test/fixtures/comment-rust.json | 31 +++++ test/fixtures/index.js | 3 + test/fixtures/repo-hub.json | 122 ++++++++++++++++++ test/fixtures/repo-rust.json | 122 ++++++++++++++++++ .../__snapshots__/unfurl.test.js.snap | 15 +++ test/integration/unfurl.test.js | 41 ++++++ 8 files changed, 338 insertions(+), 3 deletions(-) create mode 100644 test/fixtures/comment-rust.json create mode 100644 test/fixtures/repo-hub.json create mode 100644 test/fixtures/repo-rust.json diff --git a/lib/unfurls/comment.js b/lib/unfurls/comment.js index 5a69188df..cf7ab6d39 100644 --- 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 index a61b61fdf..e41e8f9eb 100644 --- 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 new file mode 100644 index 000000000..d7089ec64 --- /dev/null +++ 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);\r\ntrait Bar { const F: const fn(Self) -> Self; }\r\n\r\nimpl Foo {\r\n const fn new(x: T) -> Self { Foo(::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 index 222fb3a26..8949853ff 100644 --- 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 new file mode 100644 index 000000000..030a6299e --- /dev/null +++ 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 new file mode 100644 index 000000000..f67598435 --- /dev/null +++ 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 index 454660fe6..686c1c108 100644 --- 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 index f31f35968..37cc3defc 100644 --- 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, From dce6de270308e4e5089470a18d992f631b3785b1 Mon Sep 17 00:00:00 2001 From: Dennis Sivia Date: Fri, 30 Aug 2019 13:37:21 +0200 Subject: [PATCH 2/2] Adjust log level for unsupported unfurl urls Instead of logging with `warn`, I decided to make sure we are not creating lots of noise at our log provider and instead use `debug` which is excluded by default. We can adjust the level via ENV vars to analyse the data in a short time window. --- lib/unfurls/index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/unfurls/index.js b/lib/unfurls/index.js index e41e8f9eb..9139423f3 100644 --- a/lib/unfurls/index.js +++ b/lib/unfurls/index.js @@ -56,11 +56,11 @@ async function linkShared(req, res) { }); } catch (err) { if (err.name === 'UnsupportedResource' || err.code === 404) { - req.log.warn(err, 'Could not get unfurl attachment'); + req.log.debug(err, 'Could not get unfurl attachment'); return; } if (err.name === 'ResourceNotFound') { - req.log.warn(err, 'Resource not found during unfurl.'); + req.log.debug(err, 'Resource not found during unfurl.'); return; } if (err.name === 'UnfurlsAreDisabled') {