From dfcfa18cf310c8ebb0d6f6ed536867cdbfcee442 Mon Sep 17 00:00:00 2001 From: erwinmombay Date: Mon, 20 Mar 2017 14:29:01 -0700 Subject: [PATCH 1/7] temp --- .eslintrc | 4 +- config.js | 1 + declarations/all.js | 17 +- payload2.json | 472 ++++++++++++++++++++++++++++++++++++++++ routes/owners/api.js | 57 +++-- src/github.js | 39 +++- src/owner.js | 41 ---- src/repo-file.js | 31 ++- tests/test-owner.js | 78 +------ tests/test-repo-file.js | 61 ++++++ 10 files changed, 639 insertions(+), 162 deletions(-) create mode 100644 payload2.json create mode 100644 tests/test-repo-file.js diff --git a/.eslintrc b/.eslintrc index d8d7899..86d20b2 100644 --- a/.eslintrc +++ b/.eslintrc @@ -28,11 +28,13 @@ "context": false, "global": false, "describes": true, + "Owner": false, "OwnersMap": false, "GitHubStatusPost": false, "FileOwner": false, "FileOwners": false, - "PullRequestInfo": false + "PullRequestInfo": false, + "RepoFileOwner": false }, "rules": { "array-bracket-spacing": [2, "never"], diff --git a/config.js b/config.js index 149ab4f..76d78fa 100644 --- a/config.js +++ b/config.js @@ -63,6 +63,7 @@ nconf SECRET: 'keyboardcat', SECRET_TOKEN: process.env.SECRET_TOKEN, + NODE_ENV: process.env.NODE_ENV || 'development', GITHUB_ACCESS_TOKEN: process.env.GITHUB_ACCESS_TOKEN || 'default', GITHUB_REPO_DIR: process.env.GITHUB_REPO_DIR || 'default', GITHUB_BOT_USERNAME: process.env.GITHUB_BOT_USERNAME || 'default', diff --git a/declarations/all.js b/declarations/all.js index ac24046..7c79a38 100644 --- a/declarations/all.js +++ b/declarations/all.js @@ -27,15 +27,6 @@ type OwnersMap = { [key: string]: Owner } -type FileOwner = { - owner: Owner, - files: RepoFile[] -} - -type FileOwners = { - [key: string]: FileOwner -} - type GitHubStatusPost = { state: 'success' | 'error' | 'failure', target_url: string, @@ -45,7 +36,13 @@ type GitHubStatusPost = { type PullRequestInfo = { pr: PullRequest, - fileOwners: FileOwners, + repoFiles:RepoFile[], reviews: Review[], approvalsMet: boolean } + +type RepoFileOwner = { + id: string, + type: 'file' | 'dir', + usernames: string[] +} diff --git a/payload2.json b/payload2.json new file mode 100644 index 0000000..bdd009c --- /dev/null +++ b/payload2.json @@ -0,0 +1,472 @@ +{ + "action": "opened", + "number": 8270, + "pull_request": { + "url": "https://api.github.com/repos/ampproject/amphtml/pulls/8270", + "id": 111643522, + "html_url": "https://github.com/ampproject/amphtml/pull/8270", + "diff_url": "https://github.com/ampproject/amphtml/pull/8270.diff", + "patch_url": "https://github.com/ampproject/amphtml/pull/8270.patch", + "issue_url": "https://api.github.com/repos/ampproject/amphtml/issues/8270", + "number": 8270, + "state": "open", + "locked": false, + "title": "amp-bind: Support amp-iframe", + "user": { + "login": "choumx", + "id": 20525523, + "avatar_url": "https://avatars2.githubusercontent.com/u/20525523?v=3", + "gravatar_id": "", + "url": "https://api.github.com/users/choumx", + "html_url": "https://github.com/choumx", + "followers_url": "https://api.github.com/users/choumx/followers", + "following_url": "https://api.github.com/users/choumx/following{/other_user}", + "gists_url": "https://api.github.com/users/choumx/gists{/gist_id}", + "starred_url": "https://api.github.com/users/choumx/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/choumx/subscriptions", + "organizations_url": "https://api.github.com/users/choumx/orgs", + "repos_url": "https://api.github.com/users/choumx/repos", + "events_url": "https://api.github.com/users/choumx/events{/privacy}", + "received_events_url": "https://api.github.com/users/choumx/received_events", + "type": "User", + "site_admin": false + }, + "body": "Fixes #7837.\r\n\r\n- Support binding to `src` attribute of `amp-iframe`.\r\n\r\n/to @aghassemi @kmh287 PTAL", + "created_at": "2017-03-20T20:28:51Z", + "updated_at": "2017-03-20T20:28:51Z", + "closed_at": null, + "merged_at": null, + "merge_commit_sha": "31857d6cc6770fb1ff44510ada0b71884ad6420e", + "assignee": null, + "assignees": [ + + ], + "milestone": null, + "commits_url": "https://api.github.com/repos/ampproject/amphtml/pulls/8270/commits", + "review_comments_url": "https://api.github.com/repos/ampproject/amphtml/pulls/8270/comments", + "review_comment_url": "https://api.github.com/repos/ampproject/amphtml/pulls/comments{/number}", + "comments_url": "https://api.github.com/repos/ampproject/amphtml/issues/8270/comments", + "statuses_url": "https://api.github.com/repos/ampproject/amphtml/statuses/8918d0bf34a1f43f3b50cc91a78a2ad99009a916", + "head": { + "label": "choumx:bind-iframe", + "ref": "bind-iframe", + "sha": "8918d0bf34a1f43f3b50cc91a78a2ad99009a916", + "user": { + "login": "choumx", + "id": 20525523, + "avatar_url": "https://avatars2.githubusercontent.com/u/20525523?v=3", + "gravatar_id": "", + "url": "https://api.github.com/users/choumx", + "html_url": "https://github.com/choumx", + "followers_url": "https://api.github.com/users/choumx/followers", + "following_url": "https://api.github.com/users/choumx/following{/other_user}", + "gists_url": "https://api.github.com/users/choumx/gists{/gist_id}", + "starred_url": "https://api.github.com/users/choumx/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/choumx/subscriptions", + "organizations_url": "https://api.github.com/users/choumx/orgs", + "repos_url": "https://api.github.com/users/choumx/repos", + "events_url": "https://api.github.com/users/choumx/events{/privacy}", + "received_events_url": "https://api.github.com/users/choumx/received_events", + "type": "User", + "site_admin": false + }, + "repo": { + "id": 63805137, + "name": "amphtml", + "full_name": "choumx/amphtml", + "owner": { + "login": "choumx", + "id": 20525523, + "avatar_url": "https://avatars2.githubusercontent.com/u/20525523?v=3", + "gravatar_id": "", + "url": "https://api.github.com/users/choumx", + "html_url": "https://github.com/choumx", + "followers_url": "https://api.github.com/users/choumx/followers", + "following_url": "https://api.github.com/users/choumx/following{/other_user}", + "gists_url": "https://api.github.com/users/choumx/gists{/gist_id}", + "starred_url": "https://api.github.com/users/choumx/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/choumx/subscriptions", + "organizations_url": "https://api.github.com/users/choumx/orgs", + "repos_url": "https://api.github.com/users/choumx/repos", + "events_url": "https://api.github.com/users/choumx/events{/privacy}", + "received_events_url": "https://api.github.com/users/choumx/received_events", + "type": "User", + "site_admin": false + }, + "private": false, + "html_url": "https://github.com/choumx/amphtml", + "description": "AMP HTML source code, samples, and documentation. See below for more info.", + "fork": true, + "url": "https://api.github.com/repos/choumx/amphtml", + "forks_url": "https://api.github.com/repos/choumx/amphtml/forks", + "keys_url": "https://api.github.com/repos/choumx/amphtml/keys{/key_id}", + "collaborators_url": "https://api.github.com/repos/choumx/amphtml/collaborators{/collaborator}", + "teams_url": "https://api.github.com/repos/choumx/amphtml/teams", + "hooks_url": "https://api.github.com/repos/choumx/amphtml/hooks", + "issue_events_url": "https://api.github.com/repos/choumx/amphtml/issues/events{/number}", + "events_url": "https://api.github.com/repos/choumx/amphtml/events", + "assignees_url": "https://api.github.com/repos/choumx/amphtml/assignees{/user}", + "branches_url": "https://api.github.com/repos/choumx/amphtml/branches{/branch}", + "tags_url": "https://api.github.com/repos/choumx/amphtml/tags", + "blobs_url": "https://api.github.com/repos/choumx/amphtml/git/blobs{/sha}", + "git_tags_url": "https://api.github.com/repos/choumx/amphtml/git/tags{/sha}", + "git_refs_url": "https://api.github.com/repos/choumx/amphtml/git/refs{/sha}", + "trees_url": "https://api.github.com/repos/choumx/amphtml/git/trees{/sha}", + "statuses_url": "https://api.github.com/repos/choumx/amphtml/statuses/{sha}", + "languages_url": "https://api.github.com/repos/choumx/amphtml/languages", + "stargazers_url": "https://api.github.com/repos/choumx/amphtml/stargazers", + "contributors_url": "https://api.github.com/repos/choumx/amphtml/contributors", + "subscribers_url": "https://api.github.com/repos/choumx/amphtml/subscribers", + "subscription_url": "https://api.github.com/repos/choumx/amphtml/subscription", + "commits_url": "https://api.github.com/repos/choumx/amphtml/commits{/sha}", + "git_commits_url": "https://api.github.com/repos/choumx/amphtml/git/commits{/sha}", + "comments_url": "https://api.github.com/repos/choumx/amphtml/comments{/number}", + "issue_comment_url": "https://api.github.com/repos/choumx/amphtml/issues/comments{/number}", + "contents_url": "https://api.github.com/repos/choumx/amphtml/contents/{+path}", + "compare_url": "https://api.github.com/repos/choumx/amphtml/compare/{base}...{head}", + "merges_url": "https://api.github.com/repos/choumx/amphtml/merges", + "archive_url": "https://api.github.com/repos/choumx/amphtml/{archive_format}{/ref}", + "downloads_url": "https://api.github.com/repos/choumx/amphtml/downloads", + "issues_url": "https://api.github.com/repos/choumx/amphtml/issues{/number}", + "pulls_url": "https://api.github.com/repos/choumx/amphtml/pulls{/number}", + "milestones_url": "https://api.github.com/repos/choumx/amphtml/milestones{/number}", + "notifications_url": "https://api.github.com/repos/choumx/amphtml/notifications{?since,all,participating}", + "labels_url": "https://api.github.com/repos/choumx/amphtml/labels{/name}", + "releases_url": "https://api.github.com/repos/choumx/amphtml/releases{/id}", + "deployments_url": "https://api.github.com/repos/choumx/amphtml/deployments", + "created_at": "2016-07-20T18:33:46Z", + "updated_at": "2016-07-20T18:33:50Z", + "pushed_at": "2017-03-20T20:28:41Z", + "git_url": "git://github.com/choumx/amphtml.git", + "ssh_url": "git@github.com:choumx/amphtml.git", + "clone_url": "https://github.com/choumx/amphtml.git", + "svn_url": "https://github.com/choumx/amphtml", + "homepage": "https://ampproject.org", + "size": 236476, + "stargazers_count": 0, + "watchers_count": 0, + "language": "JavaScript", + "has_issues": false, + "has_downloads": false, + "has_wiki": false, + "has_pages": false, + "forks_count": 0, + "mirror_url": null, + "open_issues_count": 0, + "forks": 0, + "open_issues": 0, + "watchers": 0, + "default_branch": "master" + } + }, + "base": { + "label": "ampproject:master", + "ref": "master", + "sha": "6f9b0d405ec733faf807b6fd88a66ab9b57efec0", + "user": { + "login": "ampproject", + "id": 14114390, + "avatar_url": "https://avatars0.githubusercontent.com/u/14114390?v=3", + "gravatar_id": "", + "url": "https://api.github.com/users/ampproject", + "html_url": "https://github.com/ampproject", + "followers_url": "https://api.github.com/users/ampproject/followers", + "following_url": "https://api.github.com/users/ampproject/following{/other_user}", + "gists_url": "https://api.github.com/users/ampproject/gists{/gist_id}", + "starred_url": "https://api.github.com/users/ampproject/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/ampproject/subscriptions", + "organizations_url": "https://api.github.com/users/ampproject/orgs", + "repos_url": "https://api.github.com/users/ampproject/repos", + "events_url": "https://api.github.com/users/ampproject/events{/privacy}", + "received_events_url": "https://api.github.com/users/ampproject/received_events", + "type": "Organization", + "site_admin": false + }, + "repo": { + "id": 41766002, + "name": "amphtml", + "full_name": "ampproject/amphtml", + "owner": { + "login": "ampproject", + "id": 14114390, + "avatar_url": "https://avatars0.githubusercontent.com/u/14114390?v=3", + "gravatar_id": "", + "url": "https://api.github.com/users/ampproject", + "html_url": "https://github.com/ampproject", + "followers_url": "https://api.github.com/users/ampproject/followers", + "following_url": "https://api.github.com/users/ampproject/following{/other_user}", + "gists_url": "https://api.github.com/users/ampproject/gists{/gist_id}", + "starred_url": "https://api.github.com/users/ampproject/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/ampproject/subscriptions", + "organizations_url": "https://api.github.com/users/ampproject/orgs", + "repos_url": "https://api.github.com/users/ampproject/repos", + "events_url": "https://api.github.com/users/ampproject/events{/privacy}", + "received_events_url": "https://api.github.com/users/ampproject/received_events", + "type": "Organization", + "site_admin": false + }, + "private": false, + "html_url": "https://github.com/ampproject/amphtml", + "description": "AMP HTML source code, samples, and documentation. See below for more info.", + "fork": false, + "url": "https://api.github.com/repos/ampproject/amphtml", + "forks_url": "https://api.github.com/repos/ampproject/amphtml/forks", + "keys_url": "https://api.github.com/repos/ampproject/amphtml/keys{/key_id}", + "collaborators_url": "https://api.github.com/repos/ampproject/amphtml/collaborators{/collaborator}", + "teams_url": "https://api.github.com/repos/ampproject/amphtml/teams", + "hooks_url": "https://api.github.com/repos/ampproject/amphtml/hooks", + "issue_events_url": "https://api.github.com/repos/ampproject/amphtml/issues/events{/number}", + "events_url": "https://api.github.com/repos/ampproject/amphtml/events", + "assignees_url": "https://api.github.com/repos/ampproject/amphtml/assignees{/user}", + "branches_url": "https://api.github.com/repos/ampproject/amphtml/branches{/branch}", + "tags_url": "https://api.github.com/repos/ampproject/amphtml/tags", + "blobs_url": "https://api.github.com/repos/ampproject/amphtml/git/blobs{/sha}", + "git_tags_url": "https://api.github.com/repos/ampproject/amphtml/git/tags{/sha}", + "git_refs_url": "https://api.github.com/repos/ampproject/amphtml/git/refs{/sha}", + "trees_url": "https://api.github.com/repos/ampproject/amphtml/git/trees{/sha}", + "statuses_url": "https://api.github.com/repos/ampproject/amphtml/statuses/{sha}", + "languages_url": "https://api.github.com/repos/ampproject/amphtml/languages", + "stargazers_url": "https://api.github.com/repos/ampproject/amphtml/stargazers", + "contributors_url": "https://api.github.com/repos/ampproject/amphtml/contributors", + "subscribers_url": "https://api.github.com/repos/ampproject/amphtml/subscribers", + "subscription_url": "https://api.github.com/repos/ampproject/amphtml/subscription", + "commits_url": "https://api.github.com/repos/ampproject/amphtml/commits{/sha}", + "git_commits_url": "https://api.github.com/repos/ampproject/amphtml/git/commits{/sha}", + "comments_url": "https://api.github.com/repos/ampproject/amphtml/comments{/number}", + "issue_comment_url": "https://api.github.com/repos/ampproject/amphtml/issues/comments{/number}", + "contents_url": "https://api.github.com/repos/ampproject/amphtml/contents/{+path}", + "compare_url": "https://api.github.com/repos/ampproject/amphtml/compare/{base}...{head}", + "merges_url": "https://api.github.com/repos/ampproject/amphtml/merges", + "archive_url": "https://api.github.com/repos/ampproject/amphtml/{archive_format}{/ref}", + "downloads_url": "https://api.github.com/repos/ampproject/amphtml/downloads", + "issues_url": "https://api.github.com/repos/ampproject/amphtml/issues{/number}", + "pulls_url": "https://api.github.com/repos/ampproject/amphtml/pulls{/number}", + "milestones_url": "https://api.github.com/repos/ampproject/amphtml/milestones{/number}", + "notifications_url": "https://api.github.com/repos/ampproject/amphtml/notifications{?since,all,participating}", + "labels_url": "https://api.github.com/repos/ampproject/amphtml/labels{/name}", + "releases_url": "https://api.github.com/repos/ampproject/amphtml/releases{/id}", + "deployments_url": "https://api.github.com/repos/ampproject/amphtml/deployments", + "created_at": "2015-09-01T22:10:53Z", + "updated_at": "2017-03-20T18:44:25Z", + "pushed_at": "2017-03-20T20:28:52Z", + "git_url": "git://github.com/ampproject/amphtml.git", + "ssh_url": "git@github.com:ampproject/amphtml.git", + "clone_url": "https://github.com/ampproject/amphtml.git", + "svn_url": "https://github.com/ampproject/amphtml", + "homepage": "https://www.ampproject.org", + "size": 236614, + "stargazers_count": 9787, + "watchers_count": 9787, + "language": "JavaScript", + "has_issues": true, + "has_downloads": false, + "has_wiki": false, + "has_pages": false, + "forks_count": 1642, + "mirror_url": null, + "open_issues_count": 761, + "forks": 1642, + "open_issues": 761, + "watchers": 9787, + "default_branch": "master" + } + }, + "_links": { + "self": { + "href": "https://api.github.com/repos/ampproject/amphtml/pulls/8270" + }, + "html": { + "href": "https://github.com/ampproject/amphtml/pull/8270" + }, + "issue": { + "href": "https://api.github.com/repos/ampproject/amphtml/issues/8270" + }, + "comments": { + "href": "https://api.github.com/repos/ampproject/amphtml/issues/8270/comments" + }, + "review_comments": { + "href": "https://api.github.com/repos/ampproject/amphtml/pulls/8270/comments" + }, + "review_comment": { + "href": "https://api.github.com/repos/ampproject/amphtml/pulls/comments{/number}" + }, + "commits": { + "href": "https://api.github.com/repos/ampproject/amphtml/pulls/8270/commits" + }, + "statuses": { + "href": "https://api.github.com/repos/ampproject/amphtml/statuses/8918d0bf34a1f43f3b50cc91a78a2ad99009a916" + } + }, + "requested_reviewers": [ + { + "login": "aghassemi", + "id": 2099009, + "avatar_url": "https://avatars0.githubusercontent.com/u/2099009?v=3", + "gravatar_id": "", + "url": "https://api.github.com/users/aghassemi", + "html_url": "https://github.com/aghassemi", + "followers_url": "https://api.github.com/users/aghassemi/followers", + "following_url": "https://api.github.com/users/aghassemi/following{/other_user}", + "gists_url": "https://api.github.com/users/aghassemi/gists{/gist_id}", + "starred_url": "https://api.github.com/users/aghassemi/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/aghassemi/subscriptions", + "organizations_url": "https://api.github.com/users/aghassemi/orgs", + "repos_url": "https://api.github.com/users/aghassemi/repos", + "events_url": "https://api.github.com/users/aghassemi/events{/privacy}", + "received_events_url": "https://api.github.com/users/aghassemi/received_events", + "type": "User", + "site_admin": false + }, + { + "login": "kmh287", + "id": 7199374, + "avatar_url": "https://avatars0.githubusercontent.com/u/7199374?v=3", + "gravatar_id": "", + "url": "https://api.github.com/users/kmh287", + "html_url": "https://github.com/kmh287", + "followers_url": "https://api.github.com/users/kmh287/followers", + "following_url": "https://api.github.com/users/kmh287/following{/other_user}", + "gists_url": "https://api.github.com/users/kmh287/gists{/gist_id}", + "starred_url": "https://api.github.com/users/kmh287/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/kmh287/subscriptions", + "organizations_url": "https://api.github.com/users/kmh287/orgs", + "repos_url": "https://api.github.com/users/kmh287/repos", + "events_url": "https://api.github.com/users/kmh287/events{/privacy}", + "received_events_url": "https://api.github.com/users/kmh287/received_events", + "type": "User", + "site_admin": false + } + ], + "merged": false, + "mergeable": true, + "mergeable_state": "unstable", + "merged_by": null, + "comments": 0, + "review_comments": 0, + "maintainer_can_modify": true, + "commits": 4, + "additions": 90, + "deletions": 0, + "changed_files": 5 + }, + "repository": { + "id": 41766002, + "name": "amphtml", + "full_name": "ampproject/amphtml", + "owner": { + "login": "ampproject", + "id": 14114390, + "avatar_url": "https://avatars0.githubusercontent.com/u/14114390?v=3", + "gravatar_id": "", + "url": "https://api.github.com/users/ampproject", + "html_url": "https://github.com/ampproject", + "followers_url": "https://api.github.com/users/ampproject/followers", + "following_url": "https://api.github.com/users/ampproject/following{/other_user}", + "gists_url": "https://api.github.com/users/ampproject/gists{/gist_id}", + "starred_url": "https://api.github.com/users/ampproject/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/ampproject/subscriptions", + "organizations_url": "https://api.github.com/users/ampproject/orgs", + "repos_url": "https://api.github.com/users/ampproject/repos", + "events_url": "https://api.github.com/users/ampproject/events{/privacy}", + "received_events_url": "https://api.github.com/users/ampproject/received_events", + "type": "Organization", + "site_admin": false + }, + "private": false, + "html_url": "https://github.com/ampproject/amphtml", + "description": "AMP HTML source code, samples, and documentation. See below for more info.", + "fork": false, + "url": "https://api.github.com/repos/ampproject/amphtml", + "forks_url": "https://api.github.com/repos/ampproject/amphtml/forks", + "keys_url": "https://api.github.com/repos/ampproject/amphtml/keys{/key_id}", + "collaborators_url": "https://api.github.com/repos/ampproject/amphtml/collaborators{/collaborator}", + "teams_url": "https://api.github.com/repos/ampproject/amphtml/teams", + "hooks_url": "https://api.github.com/repos/ampproject/amphtml/hooks", + "issue_events_url": "https://api.github.com/repos/ampproject/amphtml/issues/events{/number}", + "events_url": "https://api.github.com/repos/ampproject/amphtml/events", + "assignees_url": "https://api.github.com/repos/ampproject/amphtml/assignees{/user}", + "branches_url": "https://api.github.com/repos/ampproject/amphtml/branches{/branch}", + "tags_url": "https://api.github.com/repos/ampproject/amphtml/tags", + "blobs_url": "https://api.github.com/repos/ampproject/amphtml/git/blobs{/sha}", + "git_tags_url": "https://api.github.com/repos/ampproject/amphtml/git/tags{/sha}", + "git_refs_url": "https://api.github.com/repos/ampproject/amphtml/git/refs{/sha}", + "trees_url": "https://api.github.com/repos/ampproject/amphtml/git/trees{/sha}", + "statuses_url": "https://api.github.com/repos/ampproject/amphtml/statuses/{sha}", + "languages_url": "https://api.github.com/repos/ampproject/amphtml/languages", + "stargazers_url": "https://api.github.com/repos/ampproject/amphtml/stargazers", + "contributors_url": "https://api.github.com/repos/ampproject/amphtml/contributors", + "subscribers_url": "https://api.github.com/repos/ampproject/amphtml/subscribers", + "subscription_url": "https://api.github.com/repos/ampproject/amphtml/subscription", + "commits_url": "https://api.github.com/repos/ampproject/amphtml/commits{/sha}", + "git_commits_url": "https://api.github.com/repos/ampproject/amphtml/git/commits{/sha}", + "comments_url": "https://api.github.com/repos/ampproject/amphtml/comments{/number}", + "issue_comment_url": "https://api.github.com/repos/ampproject/amphtml/issues/comments{/number}", + "contents_url": "https://api.github.com/repos/ampproject/amphtml/contents/{+path}", + "compare_url": "https://api.github.com/repos/ampproject/amphtml/compare/{base}...{head}", + "merges_url": "https://api.github.com/repos/ampproject/amphtml/merges", + "archive_url": "https://api.github.com/repos/ampproject/amphtml/{archive_format}{/ref}", + "downloads_url": "https://api.github.com/repos/ampproject/amphtml/downloads", + "issues_url": "https://api.github.com/repos/ampproject/amphtml/issues{/number}", + "pulls_url": "https://api.github.com/repos/ampproject/amphtml/pulls{/number}", + "milestones_url": "https://api.github.com/repos/ampproject/amphtml/milestones{/number}", + "notifications_url": "https://api.github.com/repos/ampproject/amphtml/notifications{?since,all,participating}", + "labels_url": "https://api.github.com/repos/ampproject/amphtml/labels{/name}", + "releases_url": "https://api.github.com/repos/ampproject/amphtml/releases{/id}", + "deployments_url": "https://api.github.com/repos/ampproject/amphtml/deployments", + "created_at": "2015-09-01T22:10:53Z", + "updated_at": "2017-03-20T18:44:25Z", + "pushed_at": "2017-03-20T20:28:52Z", + "git_url": "git://github.com/ampproject/amphtml.git", + "ssh_url": "git@github.com:ampproject/amphtml.git", + "clone_url": "https://github.com/ampproject/amphtml.git", + "svn_url": "https://github.com/ampproject/amphtml", + "homepage": "https://www.ampproject.org", + "size": 236614, + "stargazers_count": 9787, + "watchers_count": 9787, + "language": "JavaScript", + "has_issues": true, + "has_downloads": false, + "has_wiki": false, + "has_pages": false, + "forks_count": 1642, + "mirror_url": null, + "open_issues_count": 761, + "forks": 1642, + "open_issues": 761, + "watchers": 9787, + "default_branch": "master" + }, + "organization": { + "login": "ampproject", + "id": 14114390, + "url": "https://api.github.com/orgs/ampproject", + "repos_url": "https://api.github.com/orgs/ampproject/repos", + "events_url": "https://api.github.com/orgs/ampproject/events", + "hooks_url": "https://api.github.com/orgs/ampproject/hooks", + "issues_url": "https://api.github.com/orgs/ampproject/issues", + "members_url": "https://api.github.com/orgs/ampproject/members{/member}", + "public_members_url": "https://api.github.com/orgs/ampproject/public_members{/member}", + "avatar_url": "https://avatars0.githubusercontent.com/u/14114390?v=3", + "description": "" + }, + "sender": { + "login": "choumx", + "id": 20525523, + "avatar_url": "https://avatars2.githubusercontent.com/u/20525523?v=3", + "gravatar_id": "", + "url": "https://api.github.com/users/choumx", + "html_url": "https://github.com/choumx", + "followers_url": "https://api.github.com/users/choumx/followers", + "following_url": "https://api.github.com/users/choumx/following{/other_user}", + "gists_url": "https://api.github.com/users/choumx/gists{/gist_id}", + "starred_url": "https://api.github.com/users/choumx/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/choumx/subscriptions", + "organizations_url": "https://api.github.com/users/choumx/orgs", + "repos_url": "https://api.github.com/users/choumx/repos", + "events_url": "https://api.github.com/users/choumx/events{/privacy}", + "received_events_url": "https://api.github.com/users/choumx/received_events", + "type": "User", + "site_admin": false + } +} diff --git a/routes/owners/api.js b/routes/owners/api.js index d478071..5b2ee00 100644 --- a/routes/owners/api.js +++ b/routes/owners/api.js @@ -16,11 +16,11 @@ /* @flow */ -import * as bb from 'bluebird'; import * as _ from 'lodash'; import {Git} from '../../src/git'; import {PullRequest} from '../../src/github'; -import {findOwners} from '../../src/owner'; +import {createOwnersMap} from '../../src/owner'; +import {RepoFile} from '../../src/repo-file'; import * as express from 'express'; const config = require('../../config'); const GITHUB_BOT_USERNAME = config.get('GITHUB_BOT_USERNAME'); @@ -41,17 +41,10 @@ const prActionTypes: string[] = [ 'synchronize', ]; -function getOwners(pr: PullRequest) : Promise { +function getOwners(pr: PullRequest) : Promise { return git.pullLatestForRepo(GITHUB_REPO_DIR, 'origin', 'master').then(() => { - const promises = bb.all([ - pr.getFiles(), - git.getOwnersFilesForBranch(pr.author, GITHUB_REPO_DIR, 'master'), - ]); - return promises.then(function(res) { - const files = res[0]; - const ownersMap = res[1]; - return findOwners(files, ownersMap); - }); + return git.getOwnersFilesForBranch(pr.author, GITHUB_REPO_DIR, 'master') + .then(ownersMap => pr.getFiles(ownersMap)); }); } @@ -112,19 +105,37 @@ export function index(req: Object, res: Object) { .then(processPullRequest.bind(null, body)); } - return promise.then(ok).catch(() => { - res.status(500).send('Something went wrong!'); + return promise.then(ok).catch(e => { + if (process.env.NODE_ENV == 'production') { + res.status(500).send('Something went wrong!'); + } else { + res.status(500).send(e.stack); + } }); } function maybePostComment(prInfo: PullRequestInfo): Promise<*> { - const {pr, fileOwners} = prInfo; + const {pr, repoFiles} = prInfo; // If all approvals are still not met, do we need to submit a new post? return pr.getLastApproversList(GITHUB_BOT_USERNAME).then(reviewers => { - const allFileOwnersUsernames = Object.keys(fileOwners).sort() + + const aggregatedOwners = Object.create(null); + + repoFiles.forEach(repoFile => { + const id = repoFile.findRepoFileOwner().id; + if (!aggregatedOwners[id]) { + aggregatedOwners[id] = { + owner: repoFile.dirOwner, + files: [repoFile], + }; + } else { + aggregatedOwners[id].files.push(repoFile); + } + }); + const allFileOwnersUsernames = Object.keys(aggregatedOwners).sort() .map(key => { - const fileOwner = fileOwners[key]; + const fileOwner = aggregatedOwners[key]; const owner = fileOwner.owner; return owner.dirOwners; }); @@ -135,7 +146,7 @@ function maybePostComment(prInfo: PullRequestInfo): Promise<*> { // of previously required reviewers or less, but in any case we need to // post the list again) if (!_.isEqual(reviewers, allFileOwnersUsernames)) { - return pr.postIssuesComment(pr.composeBotComment(fileOwners)) + return pr.postIssuesComment(pr.composeBotComment(repoFiles)) .then(() => { return pr.setFailureStatus(); }); @@ -145,10 +156,10 @@ function maybePostComment(prInfo: PullRequestInfo): Promise<*> { } function getPullRequestInfo(pr: PullRequest): Promise { - return getOwners(pr).then(fileOwners => { + return getOwners(pr).then(repoFiles => { return pr.getUniqueReviews().then(reviews => { - const approvalsMet = pr.areAllApprovalsMet(fileOwners, reviews); - return {pr, fileOwners, reviews, approvalsMet}; + const approvalsMet = pr.areAllApprovalsMet(repoFiles, reviews); + return {pr, repoFiles, reviews, approvalsMet}; }); }); } @@ -171,11 +182,11 @@ function processPullRequest(body: Object, } function openedAction(prInfo: PullRequestInfo): Promise<*> { - const {pr, fileOwners, approvalsMet} = prInfo; + const {pr, repoFiles, approvalsMet} = prInfo; if (approvalsMet) { return prInfo.pr.setApprovedStatus(); } - return pr.postIssuesComment(pr.composeBotComment(fileOwners)) + return pr.postIssuesComment(pr.composeBotComment(repoFiles)) .then(() => { return pr.setFailureStatus(); }); diff --git a/src/github.js b/src/github.js index 3a53d46..2514834 100644 --- a/src/github.js +++ b/src/github.js @@ -87,7 +87,7 @@ export class PullRequest { * and pulls out the files that have been changed in any way * and returns type RepoFile[]. */ - getFiles(): Promise { + getFiles(ownersMap: OwnersMap): Promise { return request({ url: `https://api.github.com/repos/${this.project}/${this.repo}/pulls/` + `${this.id}/files`, @@ -96,7 +96,7 @@ export class PullRequest { headers, }).then(function(res: any) { const body = JSON.parse(res.body); - return body.map(item => new RepoFile(item.filename)); + return body.map(item => new RepoFile(item.filename, ownersMap)); }); } @@ -139,6 +139,10 @@ export class PullRequest { } postIssuesComment(body: string): Promise<*> { + if (process.env.NODE_ENV != 'production') { + console.log(body); + return Promise.resolve(); + } return request({ url: `https://api.github.com/repos/${this.project}/${this.repo}/issues/` + `${this.id}/comments`, @@ -197,7 +201,7 @@ export class PullRequest { }); } - areAllApprovalsMet(fileOwners: FileOwners, reviews: Review[]): boolean { + areAllApprovalsMet(repoFiles: RepoFile[], reviews: Review[]): boolean { const reviewersWhoApproved = reviews.filter(x => { return x.state == 'approved'; }).map(x => x.username); @@ -205,11 +209,9 @@ export class PullRequest { // PR. reviewersWhoApproved.push(this.author); - return Object.keys(fileOwners).every(path => { - const fileOwner = fileOwners[path]; - const owner = fileOwner.owner; - _.intersection(owner.dirOwners, reviewersWhoApproved); - return _.intersection(owner.dirOwners, reviewersWhoApproved).length > 0; + return repoFiles.every(repoFile => { + return _.intersection(repoFile.findRepoFileOwner().usernames, + reviewersWhoApproved).length > 0; }); } @@ -242,19 +244,34 @@ export class PullRequest { }); } - composeBotComment(fileOwners: FileOwners) { + composeBotComment(repoFiles: RepoFile[]): string { let comment = 'Hi, ampproject bot here! Here are a list of the owners ' + 'that can approve your files.\n\nYou may leave an issue comment ' + `stating "@${GITHUB_BOT_USERNAME} retry!" to force me to re-evaluate ` + 'this Pull Request\'s status\n\n'; - Object.keys(fileOwners).sort().forEach(key => { - const fileOwner = fileOwners[key]; + const aggregatedOwners = Object.create(null); + + repoFiles.forEach(repoFile => { + const id = repoFile.findRepoFileOwner().id; + if (!aggregatedOwners[id]) { + aggregatedOwners[id] = { + owner: repoFile.dirOwner, + files: [repoFile], + }; + } else { + aggregatedOwners[id].files.push(repoFile); + } + }); + + Object.keys(aggregatedOwners).sort().forEach(key => { + const fileOwner = aggregatedOwners[key]; const owner = fileOwner.owner; // Slice from char 2 to remove the ./ prefix normalization const files = fileOwner.files.map(x => `- ${x.path.slice(2)}`).join('\n'); const usernames = '/to ' + owner.dirOwners.join(' ') + '\n'; comment += usernames + files + '\n\n'; }); + comment += '\n\nFor any issues please file a bug at ' + 'https://github.com/google/github-owners-bot/issues'; return comment; diff --git a/src/owner.js b/src/owner.js index 8a52b2c..0334124 100644 --- a/src/owner.js +++ b/src/owner.js @@ -17,7 +17,6 @@ /* @flow */ import * as path from 'path'; -import {RepoFile} from './repo-file'; /** * @fileoverview Contains classes and functions in relation to "OWNER" files @@ -60,46 +59,6 @@ export class Owner { } } -/** - * Returns a list of github usernames that can be "approvers" for the set - * of files. It first tries to find the interection across the files and if - * there are none it will return the union across usernames. - */ -export function findOwners(files: RepoFile[], - ownersMap: OwnersMap): FileOwners { - const fileOwners: FileOwners = Object.create(null); - files.forEach((file : RepoFile) => { - const owner = findClosestOwnersFile(file, ownersMap); - if (!fileOwners[owner.dirname]) { - fileOwners[owner.dirname] = ({ - owner, - files: [file], - } : FileOwner); - } else { - fileOwners[owner.dirname].files.push(file); - } - }); - return fileOwners; -} - -/** - * Using the `ownersMap` key which is the path to the actual OWNER file - * in the repo, we simulate a folder traversal by splitting the path and - * finding the closest OWNER file for a RepoFile. - */ -export function findClosestOwnersFile(file: RepoFile, - ownersMap: OwnersMap): Owner { - let dirname = file.dirname; - let owner = ownersMap[dirname]; - const dirs = dirname.split(path.sep); - - while (!owner && dirs.pop() && dirs.length) { - dirname = dirs.join(path.sep); - owner = ownersMap[dirname]; - } - return owner; -} - export function createOwnersMap(owners: Owner[]): OwnersMap { return owners.reduce((ownersMap: OwnersMap, owner: Owner) => { if (owner.dirOwners.length) { diff --git a/src/repo-file.js b/src/repo-file.js index 3d5ecff..71ed92c 100644 --- a/src/repo-file.js +++ b/src/repo-file.js @@ -15,7 +15,7 @@ */ /* @flow */ - +import {Owner} from './owner'; import * as path from 'path'; /** @@ -23,13 +23,38 @@ import * as path from 'path'; * This is hydrated from the github pull request api. */ export class RepoFile { + path: string; dirname: string; + ownersMap: OwnersMap; + dirOwner: Owner; - constructor(filePath: string) { + constructor(filePath: string, ownersMap: OwnersMap) { // We want it have the leading ./ to evaluate `.` later on - /** @type {string} */ this.path = /^\./.test(filePath) ? filePath : `.${path.sep}${filePath}`; this.dirname = path.dirname(this.path); + this.ownersMap = ownersMap; + + this.dirOwner = this.findOwnerFile_(); + } + + findOwnerFile_(): Owner { + let dirname = this.dirname; + let owner = this.ownersMap[dirname]; + const dirs = dirname.split(path.sep); + + while (!owner && dirs.pop() && dirs.length) { + dirname = dirs.join(path.sep); + owner = this.ownersMap[dirname]; + } + return owner; + } + + findRepoFileOwner(): RepoFileOwner { + return { + id: this.dirOwner.path, + type: 'dir', + usernames: this.dirOwner.dirOwners, + }; } } diff --git a/tests/test-owner.js b/tests/test-owner.js index 4947ab0..8251497 100644 --- a/tests/test-owner.js +++ b/tests/test-owner.js @@ -16,81 +16,13 @@ import {RepoFile} from '../src/repo-file'; import test from 'ava'; -import {findOwners, createOwnersMap, Owner} from '../src/owner'; +import {createOwnersMap, Owner} from '../src/owner'; - -const pathToRepo = '/path/to/repo'; - -const defaultStruct = [ - new Owner(['person-0'], pathToRepo, 'OWNERS.yaml'), - new Owner(['person-0', 'build-system-person-0'], - pathToRepo, 'build-system/OWNERS.yaml'), - new Owner(['some-extension-owner-0'], - pathToRepo, 'extensions/0.1/some-extension/OWNERS.yaml'), - new Owner(['person-1'], - pathToRepo, - 'extensions/0.1/some-extension-with-only-person-0/OWNERS.yaml'), -]; - -function getOwnersMap() { - return createOwnersMap(defaultStruct); -} - -test('find single top level owner for top level file', t => { - t.plan(3); - const ownerFiles = findOwners([new RepoFile('README.md')], - getOwnersMap()); - const numOfOwnerFiles = Object.keys(ownerFiles).length; - t.is(numOfOwnerFiles, 1, 'number of found owner files should be 1'); - const ownerTuple = ownerFiles['.']; - t.deepEqual(ownerTuple.owner, defaultStruct[0]); - t.is(ownerTuple.files[0].path, './README.md'); -}); - -test('find top level owner for deep file with no dir owner', t => { - t.plan(3); - const ownerFiles = findOwners([ - new RepoFile('extensions/0.1/some-other-extension/some-other-extension.js'), - ], getOwnersMap()); - const numOfOwnerFiles = Object.keys(ownerFiles).length; - t.is(numOfOwnerFiles, 1, 'number of found owner files should be 1'); - - const ownerTuple = ownerFiles['.']; - t.deepEqual(ownerTuple.owner, defaultStruct[0]); - t.is(ownerTuple.files[0].path, - './extensions/0.1/some-other-extension/some-other-extension.js'); -}); - -test('find owner for deep level dir', t => { - t.plan(3); - var ownerFiles = findOwners([ - new RepoFile('extensions/0.1/some-extension/some-extension.js'), - ], getOwnersMap()); - const numOfOwnerFiles = Object.keys(ownerFiles).length; - t.is(numOfOwnerFiles, 1, 'number of found owner files should be 1'); - const someExtensionOwnerTuple = ownerFiles['./extensions/0.1/some-extension']; - t.deepEqual(someExtensionOwnerTuple.owner, defaultStruct[2]); - t.is(someExtensionOwnerTuple.files[0].path, - './extensions/0.1/some-extension/some-extension.js'); +test('create an Owners Map', t => { + }); -test('find owners for deep level dir owner and top level file', t => { - t.plan(6); - var ownerFiles = findOwners([ - new RepoFile('README.md'), - new RepoFile('extensions/0.1/some-extension/some-extension.js'), - new RepoFile('extensions/0.1/some-extension/some-helper-for-extension.js'), - ], getOwnersMap()); - const numOfOwnerFiles = Object.keys(ownerFiles).length; - t.is(numOfOwnerFiles, 2, 'number of found owner files should be 2'); - const ownerTuple = ownerFiles['.']; - t.deepEqual(ownerTuple.owner, defaultStruct[0]); - t.is(ownerTuple.files[0].path, './README.md'); +test('aggregate RepoFiles who share the same Owner into an ' + + 'OwnerFileTuple', t => { - const someExtensionOwnerTuple = ownerFiles['./extensions/0.1/some-extension']; - t.deepEqual(someExtensionOwnerTuple.owner, defaultStruct[2]); - t.is(someExtensionOwnerTuple.files[0].path, - './extensions/0.1/some-extension/some-extension.js'); - t.is(someExtensionOwnerTuple.files[1].path, - './extensions/0.1/some-extension/some-helper-for-extension.js'); }); diff --git a/tests/test-repo-file.js b/tests/test-repo-file.js new file mode 100644 index 0000000..3f0769a --- /dev/null +++ b/tests/test-repo-file.js @@ -0,0 +1,61 @@ +/** + * Copyright 2017 The AMP HTML Authors. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS-IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + + +import {RepoFile} from '../src/repo-file'; +import {Owner, createOwnersMap} from '../src/owner'; +import test from 'ava'; + +const pathToRepo = '/path/to/repo'; + +const defaultStruct = [ + new Owner(['person-0'], pathToRepo, 'OWNERS.yaml'), + new Owner(['person-0', 'build-system-person-0'], + pathToRepo, 'build-system/OWNERS.yaml'), + new Owner(['some-extension-owner-0'], + pathToRepo, 'extensions/0.1/some-extension/OWNERS.yaml'), + new Owner(['person-1'], + pathToRepo, + 'extensions/0.1/some-extension-with-only-person-0/OWNERS.yaml'), +]; + +const ownersMap = createOwnersMap(defaultStruct); + +test('find top level owner for top level file', t => { + t.plan(2); + const repoFile = new RepoFile('README.md', ownersMap); + const ownerFile = repoFile.findRepoFileOwner(); + t.deepEqual(ownerFile.id, defaultStruct[0].path); + t.deepEqual(ownerFile.usernames, defaultStruct[0].dirOwners); +}); + +test('find top level owner for deep file with no dir owner', t => { + t.plan(2); + const repoFile = new RepoFile( + 'extensions/0.1/some-other-extension/some-other-extension.js', ownersMap); + const ownerFile = repoFile.findRepoFileOwner(); + t.deepEqual(ownerFile.id, defaultStruct[0].path); + t.deepEqual(ownerFile.usernames, defaultStruct[0].dirOwners); +}); + +test('find owner for deep level dir', t => { + t.plan(2); + const repoFile = new RepoFile( + 'extensions/0.1/some-extension/some-extension.js', ownersMap); + const ownerFile = repoFile.findRepoFileOwner(); + t.deepEqual(ownerFile.id, defaultStruct[2].path); + t.deepEqual(ownerFile.usernames, defaultStruct[2].dirOwners); +}); From b80898d0f36967e40f792146de090ed9415c11ab Mon Sep 17 00:00:00 2001 From: erwinmombay Date: Mon, 20 Mar 2017 16:04:19 -0700 Subject: [PATCH 2/7] test2 --- .eslintrc | 1 + declarations/all.js | 10 +++++++++- routes/owners/api.js | 33 +++++++++------------------------ src/github.js | 19 ++----------------- src/owner.js | 21 +++++++++++++++++++++ tests/test-api.js | 2 +- 6 files changed, 43 insertions(+), 43 deletions(-) diff --git a/.eslintrc b/.eslintrc index 86d20b2..6334082 100644 --- a/.eslintrc +++ b/.eslintrc @@ -33,6 +33,7 @@ "GitHubStatusPost": false, "FileOwner": false, "FileOwners": false, + "OwnerTuples": false, "PullRequestInfo": false, "RepoFileOwner": false }, diff --git a/declarations/all.js b/declarations/all.js index 7c79a38..fb1a3ab 100644 --- a/declarations/all.js +++ b/declarations/all.js @@ -38,7 +38,8 @@ type PullRequestInfo = { pr: PullRequest, repoFiles:RepoFile[], reviews: Review[], - approvalsMet: boolean + approvalsMet: boolean, + ownerTuples: OwnerTuples } type RepoFileOwner = { @@ -46,3 +47,10 @@ type RepoFileOwner = { type: 'file' | 'dir', usernames: string[] } + +type OwnerTuple = { + owner: Owner, + files: RepoFile[] +} + +type OwnerTuples = OwnerTuple[]; diff --git a/routes/owners/api.js b/routes/owners/api.js index 5b2ee00..a44a099 100644 --- a/routes/owners/api.js +++ b/routes/owners/api.js @@ -19,7 +19,7 @@ import * as _ from 'lodash'; import {Git} from '../../src/git'; import {PullRequest} from '../../src/github'; -import {createOwnersMap} from '../../src/owner'; +import {createAggregatedOwnersTuple} from '../../src/owner'; import {RepoFile} from '../../src/repo-file'; import * as express from 'express'; const config = require('../../config'); @@ -115,30 +115,14 @@ export function index(req: Object, res: Object) { } function maybePostComment(prInfo: PullRequestInfo): Promise<*> { - const {pr, repoFiles} = prInfo; + const {pr, repoFiles, ownerTuples} = prInfo; // If all approvals are still not met, do we need to submit a new post? return pr.getLastApproversList(GITHUB_BOT_USERNAME).then(reviewers => { - - const aggregatedOwners = Object.create(null); - - repoFiles.forEach(repoFile => { - const id = repoFile.findRepoFileOwner().id; - if (!aggregatedOwners[id]) { - aggregatedOwners[id] = { - owner: repoFile.dirOwner, - files: [repoFile], - }; - } else { - aggregatedOwners[id].files.push(repoFile); - } + const allFileOwnersUsernames = ownerTuples.map(fileOwner => { + return fileOwner.owner.dirOwners; }); - const allFileOwnersUsernames = Object.keys(aggregatedOwners).sort() - .map(key => { - const fileOwner = aggregatedOwners[key]; - const owner = fileOwner.owner; - return owner.dirOwners; - }); + console.log(reviewers, allFileOwnersUsernames); // If the list of reviewers from the last bot's comment is different // from the current evaluation of required reviewers then we need to @@ -159,7 +143,8 @@ function getPullRequestInfo(pr: PullRequest): Promise { return getOwners(pr).then(repoFiles => { return pr.getUniqueReviews().then(reviews => { const approvalsMet = pr.areAllApprovalsMet(repoFiles, reviews); - return {pr, repoFiles, reviews, approvalsMet}; + const ownerTuples = createAggregatedOwnersTuple(repoFiles); + return {pr, repoFiles, reviews, approvalsMet, ownerTuples}; }); }); } @@ -182,11 +167,11 @@ function processPullRequest(body: Object, } function openedAction(prInfo: PullRequestInfo): Promise<*> { - const {pr, repoFiles, approvalsMet} = prInfo; + const {pr, approvalsMet, ownerTuples} = prInfo; if (approvalsMet) { return prInfo.pr.setApprovedStatus(); } - return pr.postIssuesComment(pr.composeBotComment(repoFiles)) + return pr.postIssuesComment(pr.composeBotComment(ownerTuples)) .then(() => { return pr.setFailureStatus(); }); diff --git a/src/github.js b/src/github.js index 2514834..d86a445 100644 --- a/src/github.js +++ b/src/github.js @@ -244,27 +244,12 @@ export class PullRequest { }); } - composeBotComment(repoFiles: RepoFile[]): string { + composeBotComment(aggregatedOwners: OwnerTuples): string { let comment = 'Hi, ampproject bot here! Here are a list of the owners ' + 'that can approve your files.\n\nYou may leave an issue comment ' + `stating "@${GITHUB_BOT_USERNAME} retry!" to force me to re-evaluate ` + 'this Pull Request\'s status\n\n'; - const aggregatedOwners = Object.create(null); - - repoFiles.forEach(repoFile => { - const id = repoFile.findRepoFileOwner().id; - if (!aggregatedOwners[id]) { - aggregatedOwners[id] = { - owner: repoFile.dirOwner, - files: [repoFile], - }; - } else { - aggregatedOwners[id].files.push(repoFile); - } - }); - - Object.keys(aggregatedOwners).sort().forEach(key => { - const fileOwner = aggregatedOwners[key]; + aggregatedOwners.forEach(fileOwner => { const owner = fileOwner.owner; // Slice from char 2 to remove the ./ prefix normalization const files = fileOwner.files.map(x => `- ${x.path.slice(2)}`).join('\n'); diff --git a/src/owner.js b/src/owner.js index 0334124..e857d4c 100644 --- a/src/owner.js +++ b/src/owner.js @@ -17,6 +17,7 @@ /* @flow */ import * as path from 'path'; +import {RepoFile} from './repo-file'; /** * @fileoverview Contains classes and functions in relation to "OWNER" files @@ -67,3 +68,23 @@ export function createOwnersMap(owners: Owner[]): OwnersMap { return ownersMap; }, Object.create(null)); } + +export function createAggregatedOwnersTuple( + repoFiles: RepoFile[]): OwnerTuples { + const aggregatedOwners = Object.create(null); + + repoFiles.forEach(repoFile => { + const id = repoFile.findRepoFileOwner().id; + if (!aggregatedOwners[id]) { + aggregatedOwners[id] = { + owner: repoFile.dirOwner, + files: [repoFile], + }; + } else { + aggregatedOwners[id].files.push(repoFile); + } + }); + return Object.keys(aggregatedOwners).sort().map(key => { + return aggregatedOwners[key]; + });; +} diff --git a/tests/test-api.js b/tests/test-api.js index 5d5d864..88a3228 100644 --- a/tests/test-api.js +++ b/tests/test-api.js @@ -98,7 +98,7 @@ test.serial('on a synchronize action that is not fully approved yet, if ' + PullRequest.prototype, 'setFailureStatus').returns(Promise.resolve()); const lastApproversListStub = sandbox .stub(PullRequest.prototype, 'getLastApproversList') - .returns(Promise.resolve([])); + .returns(Promise.resolve(['erwin-test'])); return request(app).post('/api/get-owners') .set('Content-Type', 'application/json') From 84fa2388f7249bd90aa1b75c8ecf63184a8c8db9 Mon Sep 17 00:00:00 2001 From: erwinmombay Date: Mon, 20 Mar 2017 17:15:58 -0700 Subject: [PATCH 3/7] make sure to mock out repository Owners --- routes/owners/api.js | 5 ++--- src/git.js | 2 +- src/github.js | 3 +-- tests/test-api.js | 16 +++++++++++++++- 4 files changed, 19 insertions(+), 7 deletions(-) diff --git a/routes/owners/api.js b/routes/owners/api.js index a44a099..2958e24 100644 --- a/routes/owners/api.js +++ b/routes/owners/api.js @@ -115,14 +115,13 @@ export function index(req: Object, res: Object) { } function maybePostComment(prInfo: PullRequestInfo): Promise<*> { - const {pr, repoFiles, ownerTuples} = prInfo; + const {pr, ownerTuples} = prInfo; // If all approvals are still not met, do we need to submit a new post? return pr.getLastApproversList(GITHUB_BOT_USERNAME).then(reviewers => { const allFileOwnersUsernames = ownerTuples.map(fileOwner => { return fileOwner.owner.dirOwners; }); - console.log(reviewers, allFileOwnersUsernames); // If the list of reviewers from the last bot's comment is different // from the current evaluation of required reviewers then we need to @@ -130,7 +129,7 @@ function maybePostComment(prInfo: PullRequestInfo): Promise<*> { // of previously required reviewers or less, but in any case we need to // post the list again) if (!_.isEqual(reviewers, allFileOwnersUsernames)) { - return pr.postIssuesComment(pr.composeBotComment(repoFiles)) + return pr.postIssuesComment(pr.composeBotComment(ownerTuples)) .then(() => { return pr.setFailureStatus(); }); diff --git a/src/git.js b/src/git.js index 0a41f21..8a6dbc6 100644 --- a/src/git.js +++ b/src/git.js @@ -49,7 +49,7 @@ export class Git { * Retrieves all the OWNERS paths inside a repository. */ getOwnersFilesForBranch(author: string, dirPath: string, - targetBranch: string): OwnersMap { + targetBranch: string): Promise { // NOTE: for some reason `git ls-tree --full-tree -r HEAD **/OWNERS* // doesn't work from here. return exec(`cd ${dirPath} && git checkout ${targetBranch} ` + diff --git a/src/github.js b/src/github.js index d86a445..6addda6 100644 --- a/src/github.js +++ b/src/github.js @@ -140,7 +140,6 @@ export class PullRequest { postIssuesComment(body: string): Promise<*> { if (process.env.NODE_ENV != 'production') { - console.log(body); return Promise.resolve(); } return request({ @@ -155,7 +154,7 @@ export class PullRequest { getCommentsByAuthor(author: string): Promise { return this.getComments().then(comments => { - return comments.filter(x => x.author === author); + return comments.filter(x => x.author == author); }); } diff --git a/tests/test-api.js b/tests/test-api.js index 88a3228..1b42311 100644 --- a/tests/test-api.js +++ b/tests/test-api.js @@ -1,6 +1,8 @@ import test from 'ava'; import * as sinon from 'sinon'; import {PullRequest, Review} from '../src/github'; +import {Owner} from '../src/owner'; +import {Git} from '../src/git'; const request = require('supertest'); const config = require('../config'); const fs = require('fs'); @@ -62,6 +64,10 @@ test.serial('on an opened pull request, if author is not part of owner ' + test.serial('on an opened pull request, if author is also part of owner ' + 'list it should set approved right away', t => { t.plan(2); + sandbox.stub(Git.prototype, 'getOwnersFilesForBranch') + .returns(Promise.resolve({ + '.': new Owner(['donttrustthisbot'], '/path/to/repo', 'OWNERS.yaml') + })); sandbox.stub(PullRequest.prototype, 'getReviews') .returns(Promise.resolve([])); const openedPayload = JSON.parse( @@ -98,7 +104,7 @@ test.serial('on a synchronize action that is not fully approved yet, if ' + PullRequest.prototype, 'setFailureStatus').returns(Promise.resolve()); const lastApproversListStub = sandbox .stub(PullRequest.prototype, 'getLastApproversList') - .returns(Promise.resolve(['erwin-test'])); + .returns(Promise.resolve([])); return request(app).post('/api/get-owners') .set('Content-Type', 'application/json') @@ -111,6 +117,10 @@ test.serial('on a synchronize action that is not fully approved yet, if ' + test.serial('on a comment issue where the retry command is invoked and ' + 'approvals are met, set approval status', t => { + sandbox.stub(Git.prototype, 'getOwnersFilesForBranch') + .returns(Promise.resolve({ + '.': new Owner(['donttrustthisbot'], '/path/to/repo', 'OWNERS.yaml') + })); const retryPayload = JSON.parse( fs.readFileSync( 'fixtures/retry_comment.json')); @@ -171,6 +181,10 @@ test.serial('on a comment issue where the retry command is invoked and ' + test.serial('it should not post a new comment if the old reviewers list ' + 'is equal to the new reviewers list', t => { + sandbox.stub(Git.prototype, 'getOwnersFilesForBranch') + .returns(Promise.resolve({ + '.': new Owner(['donttrustthisbot'], '/path/to/repo', 'OWNERS.yaml') + })); sandbox.stub(PullRequest.prototype, 'getLastApproversList') .returns(Promise.resolve([['donttrustthisbot']])); From bca405a9d2bed3142954084845c2175543e67711 Mon Sep 17 00:00:00 2001 From: erwinmombay Date: Mon, 20 Mar 2017 17:29:24 -0700 Subject: [PATCH 4/7] more tests --- tests/test-api.js | 12 ------------ tests/test-owner.js | 48 ++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 45 insertions(+), 15 deletions(-) diff --git a/tests/test-api.js b/tests/test-api.js index 1b42311..f5f9df3 100644 --- a/tests/test-api.js +++ b/tests/test-api.js @@ -11,10 +11,6 @@ import {app} from '../app'; const GITHUB_BOT_USERNAME = config.get('GITHUB_BOT_USERNAME'); -// Figure out how to get this to work on travis since we need to have -// the target repo -if (!process.env.TRAVIS) { - const reviewSubmittedFailedPayload = JSON.parse( fs.readFileSync( 'fixtures/review_submitted_failure.json')); @@ -252,11 +248,3 @@ test.serial('it should post a new comment if the old reviewers list is ' + 'Should call setFailureStatusSpy'); }); }); - -} else { - - test('to appease ava', t => { - t.plan(1); - t.is(1, 1); - }); -} diff --git a/tests/test-owner.js b/tests/test-owner.js index 8251497..c4c32bf 100644 --- a/tests/test-owner.js +++ b/tests/test-owner.js @@ -16,13 +16,55 @@ import {RepoFile} from '../src/repo-file'; import test from 'ava'; -import {createOwnersMap, Owner} from '../src/owner'; +import { + createOwnersMap, + createAggregatedOwnersTuple, + Owner, +} from '../src/owner'; + +const pathToRepo = '/path/to/repo'; + +const defaultStruct = [ + new Owner(['person-0'], pathToRepo, 'OWNERS.yaml'), + new Owner(['person-0', 'build-system-person-0'], + pathToRepo, 'build-system/OWNERS.yaml'), + new Owner(['some-extension-owner-0'], + pathToRepo, 'extensions/0.1/some-extension/OWNERS.yaml'), + new Owner(['person-1'], + pathToRepo, + 'extensions/0.1/some-extension-with-only-person-0/OWNERS.yaml'), +]; test('create an Owners Map', t => { - + t.plan(1); + const ownersMap = createOwnersMap(defaultStruct); + t.deepEqual(ownersMap, { + '.': defaultStruct[0], + './build-system': defaultStruct[1], + './extensions/0.1/some-extension': defaultStruct[2], + './extensions/0.1/some-extension-with-only-person-0': defaultStruct[3], + }); }); test('aggregate RepoFiles who share the same Owner into an ' + 'OwnerFileTuple', t => { - + t.plan(1); + const ownersMap = createOwnersMap(defaultStruct); + const repoFile0 = new RepoFile('README.md', ownersMap); + const repoFile1 = new RepoFile('some-src-file.js', ownersMap); + const repoFile2 = new RepoFile('build-system/some-src-file.js', ownersMap); + const repoFile3 = new RepoFile('build-system/some-src-file-1.js', ownersMap); + const repoFiles = [repoFile0, repoFile1, repoFile2, repoFile3]; + const aggregatedOwners = createAggregatedOwnersTuple(repoFiles); + console.log(aggregatedOwners); + t.deepEqual(aggregatedOwners, [ + { + owner: defaultStruct[0], + files: [repoFile0, repoFile1], + }, + { + owner: defaultStruct[1], + files: [repoFile2, repoFile3], + } + ]); }); From 3e914e4fcaa0f7e98304fc0af64310cf22afe405 Mon Sep 17 00:00:00 2001 From: erwinmombay Date: Mon, 20 Mar 2017 17:39:30 -0700 Subject: [PATCH 5/7] first stage --- src/owner.js | 14 ++++++++++++-- tests/test-api.js | 4 ++++ tests/test-repo-file.js | 10 +++++++++- 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/owner.js b/src/owner.js index e857d4c..a289410 100644 --- a/src/owner.js +++ b/src/owner.js @@ -52,8 +52,18 @@ export class Owner { config.forEach(entry => { if (typeof entry === 'string') { this.dirOwners.push(entry); - } else if (entry && entry['file-only']) { - // TODO(erwin): support file level entries. Finalize spec for it. + } else if (typeof entry === 'object') { + const username = Object.keys(entry)[0]; + const files = entry[username]; + files.forEach(filepath => { + const path = `${this.dirname}/${filepath}`; + const fileOwners = this.fileOwners[path]; + if (!fileOwners) { + this.fileOwners[path] = [username]; + } else if (fileOwners.indexOf(username) == -1) { + fileOwners.push(username); + } + }); } }); this.dirOwners.sort(); diff --git a/tests/test-api.js b/tests/test-api.js index f5f9df3..0d8aa6e 100644 --- a/tests/test-api.js +++ b/tests/test-api.js @@ -31,6 +31,10 @@ test.afterEach(() => { test.serial('on an opened pull request, if author is not part of owner ' + 'list and not full appproved it should set initial comment', t => { t.plan(2); + sandbox.stub(Git.prototype, 'getOwnersFilesForBranch') + .returns(Promise.resolve({ + '.': new Owner(['donttrustthisbot'], '/path/to/repo', 'OWNERS.yaml') + })); const openedPayload = JSON.parse( fs.readFileSync( 'fixtures/opened.json')); diff --git a/tests/test-repo-file.js b/tests/test-repo-file.js index 3f0769a..c3100fd 100644 --- a/tests/test-repo-file.js +++ b/tests/test-repo-file.js @@ -22,7 +22,11 @@ import test from 'ava'; const pathToRepo = '/path/to/repo'; const defaultStruct = [ - new Owner(['person-0'], pathToRepo, 'OWNERS.yaml'), + new Owner(['person-0', { + 'file-level-owner-0': [ + 'file-with-file-level-owner.js', + ] + }], pathToRepo, 'OWNERS.yaml'), new Owner(['person-0', 'build-system-person-0'], pathToRepo, 'build-system/OWNERS.yaml'), new Owner(['some-extension-owner-0'], @@ -59,3 +63,7 @@ test('find owner for deep level dir', t => { t.deepEqual(ownerFile.id, defaultStruct[2].path); t.deepEqual(ownerFile.usernames, defaultStruct[2].dirOwners); }); + +test('find file level owner', t => { + +}); From ed4d9e5a2008cfdac55aec4bb70a4bf0b4541e90 Mon Sep 17 00:00:00 2001 From: erwinmombay Date: Tue, 21 Mar 2017 01:30:52 -0700 Subject: [PATCH 6/7] add more tests --- .eslintrc | 2 -- declarations/all.js | 2 +- gulpfile.js | 4 +-- src/owner.js | 35 ++++++++++++++++--------- src/repo-file.js | 58 ++++++++++++++++++++++++++++++----------- tests/test-owner.js | 1 - tests/test-repo-file.js | 54 +++++++++++++++++++++++++++++++------- 7 files changed, 114 insertions(+), 42 deletions(-) diff --git a/.eslintrc b/.eslintrc index 6334082..80a787d 100644 --- a/.eslintrc +++ b/.eslintrc @@ -31,8 +31,6 @@ "Owner": false, "OwnersMap": false, "GitHubStatusPost": false, - "FileOwner": false, - "FileOwners": false, "OwnerTuples": false, "PullRequestInfo": false, "RepoFileOwner": false diff --git a/declarations/all.js b/declarations/all.js index fb1a3ab..5efd230 100644 --- a/declarations/all.js +++ b/declarations/all.js @@ -43,7 +43,7 @@ type PullRequestInfo = { } type RepoFileOwner = { - id: string, + id: ?string, type: 'file' | 'dir', usernames: string[] } diff --git a/gulpfile.js b/gulpfile.js index e30ccda..bfc67bf 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -32,7 +32,7 @@ gulp.task('babel', () => { }); gulp.task('test', () => { - return gulp.src(tests).pipe($$.ava({verbose: true, timeout: '15s'})); + return gulp.src(tests).pipe($$.ava({verbose: true, timeout: '20s'})); }); gulp.task('flow', () => { @@ -66,7 +66,7 @@ gulp.task('watch', function() { }); gulp.task('watch:test', function() { - return $$.watch(tests, {ignoreInitial: false}, + return $$.watch(sources.concat(tests), {ignoreInitial: false}, $$.batch(function(events, done) { gulp.start('test', done); })); diff --git a/src/owner.js b/src/owner.js index a289410..3dbcc80 100644 --- a/src/owner.js +++ b/src/owner.js @@ -34,7 +34,7 @@ export class Owner { fullpath: string; score: number; dirOwners: string[]; - fileOwners: Object; + fileOwners: {[key: string]: Set}; constructor(config: any, pathToRepoDir: string, filePath: string) { // We want it have the leading ./ to evaluate `.` later on @@ -59,15 +59,23 @@ export class Owner { const path = `${this.dirname}/${filepath}`; const fileOwners = this.fileOwners[path]; if (!fileOwners) { - this.fileOwners[path] = [username]; - } else if (fileOwners.indexOf(username) == -1) { - fileOwners.push(username); + this.fileOwners[path] = new Set([username]); + } else { + fileOwners.add(username); } }); } }); this.dirOwners.sort(); } + + getFileLevelOwners(repoFile: RepoFile): ?string[] { + const fileOwners = this.fileOwners[repoFile.path]; + if (fileOwners) { + return Array.from(fileOwners).sort(); + } + return null; + } } export function createOwnersMap(owners: Owner[]): OwnersMap { @@ -84,14 +92,17 @@ export function createAggregatedOwnersTuple( const aggregatedOwners = Object.create(null); repoFiles.forEach(repoFile => { - const id = repoFile.findRepoFileOwner().id; - if (!aggregatedOwners[id]) { - aggregatedOwners[id] = { - owner: repoFile.dirOwner, - files: [repoFile], - }; - } else { - aggregatedOwners[id].files.push(repoFile); + const repoFileOwner = repoFile.findRepoFileOwner(); + const id = repoFileOwner.id; + if (repoFileOwner.type == 'dir' && id) { + if (!aggregatedOwners[id]) { + aggregatedOwners[id] = { + owner: repoFile.ownersMap[id], + files: [repoFile], + }; + } else { + aggregatedOwners[id].files.push(repoFile); + } } }); return Object.keys(aggregatedOwners).sort().map(key => { diff --git a/src/repo-file.js b/src/repo-file.js index 71ed92c..bf8aae2 100644 --- a/src/repo-file.js +++ b/src/repo-file.js @@ -27,34 +27,62 @@ export class RepoFile { path: string; dirname: string; ownersMap: OwnersMap; - dirOwner: Owner; + dirOwner: ?Owner; + repoFileOwner: RepoFileOwner; + fileOwners: ?string[]; constructor(filePath: string, ownersMap: OwnersMap) { // We want it have the leading ./ to evaluate `.` later on this.path = /^\./.test(filePath) ? filePath : `.${path.sep}${filePath}`; this.dirname = path.dirname(this.path); this.ownersMap = ownersMap; + this.dirOwner = null; + this.fileOwners = null; - this.dirOwner = this.findOwnerFile_(); + const maybeOwner = this._findOwnerFile(); + if (maybeOwner instanceof Owner) { + this.dirOwner = maybeOwner; + this.repoFileOwner = { + id: this.dirOwner.dirname, + type: 'dir', + usernames: this.dirOwner.dirOwners, + }; + } else { + this.fileOwners = maybeOwner; + this.repoFileOwner = { + id: null, + type: 'file', + usernames: this.fileOwners, + }; + } } - findOwnerFile_(): Owner { - let dirname = this.dirname; - let owner = this.ownersMap[dirname]; - const dirs = dirname.split(path.sep); + _findOwnerFile(): Owner | string[] { + let ownerCandidate = null; + const fileOwners = new Set([]); + const dirs = this.dirname.split(path.sep); - while (!owner && dirs.pop() && dirs.length) { - dirname = dirs.join(path.sep); - owner = this.ownersMap[dirname]; + while (dirs.length) { + const dirname = dirs.join(path.sep); + const curOwner = this.ownersMap[dirname]; + if (curOwner) { + const curOwnerFileOwners = curOwner.getFileLevelOwners(this); + if (curOwnerFileOwners) { + curOwnerFileOwners.forEach(x => fileOwners.add(x)); + } + } + if (!ownerCandidate) { + ownerCandidate = curOwner; + } + dirs.pop(); + } + if (fileOwners.size > 0) { + return Array.from(fileOwners).sort(); } - return owner; + return ownerCandidate; } findRepoFileOwner(): RepoFileOwner { - return { - id: this.dirOwner.path, - type: 'dir', - usernames: this.dirOwner.dirOwners, - }; + return this.repoFileOwner; } } diff --git a/tests/test-owner.js b/tests/test-owner.js index c4c32bf..5a6db27 100644 --- a/tests/test-owner.js +++ b/tests/test-owner.js @@ -56,7 +56,6 @@ test('aggregate RepoFiles who share the same Owner into an ' + const repoFile3 = new RepoFile('build-system/some-src-file-1.js', ownersMap); const repoFiles = [repoFile0, repoFile1, repoFile2, repoFile3]; const aggregatedOwners = createAggregatedOwnersTuple(repoFiles); - console.log(aggregatedOwners); t.deepEqual(aggregatedOwners, [ { owner: defaultStruct[0], diff --git a/tests/test-repo-file.js b/tests/test-repo-file.js index c3100fd..55efdb6 100644 --- a/tests/test-repo-file.js +++ b/tests/test-repo-file.js @@ -22,13 +22,29 @@ import test from 'ava'; const pathToRepo = '/path/to/repo'; const defaultStruct = [ - new Owner(['person-0', { - 'file-level-owner-0': [ - 'file-with-file-level-owner.js', + new Owner([ + 'person-0', + { + 'file-level-owner-0': [ + 'file-with-file-level-owner.js', + ], + }, + { + 'file-level-owner-2': [ + 'build-system/some-file-in-build-system.js', + ], + }, + { + 'file-level-owner-3': [ + 'build-system/some-other-file-in-build-system.js', + ] + } + ], pathToRepo, 'OWNERS.yaml'), + new Owner(['person-0', 'build-system-person-0', { + 'file-level-owner-1': [ + 'some-file-in-build-system.js', ] - }], pathToRepo, 'OWNERS.yaml'), - new Owner(['person-0', 'build-system-person-0'], - pathToRepo, 'build-system/OWNERS.yaml'), + }], pathToRepo, 'build-system/OWNERS.yaml'), new Owner(['some-extension-owner-0'], pathToRepo, 'extensions/0.1/some-extension/OWNERS.yaml'), new Owner(['person-1'], @@ -42,7 +58,7 @@ test('find top level owner for top level file', t => { t.plan(2); const repoFile = new RepoFile('README.md', ownersMap); const ownerFile = repoFile.findRepoFileOwner(); - t.deepEqual(ownerFile.id, defaultStruct[0].path); + t.deepEqual(ownerFile.id, defaultStruct[0].dirname); t.deepEqual(ownerFile.usernames, defaultStruct[0].dirOwners); }); @@ -51,7 +67,7 @@ test('find top level owner for deep file with no dir owner', t => { const repoFile = new RepoFile( 'extensions/0.1/some-other-extension/some-other-extension.js', ownersMap); const ownerFile = repoFile.findRepoFileOwner(); - t.deepEqual(ownerFile.id, defaultStruct[0].path); + t.deepEqual(ownerFile.id, defaultStruct[0].dirname); t.deepEqual(ownerFile.usernames, defaultStruct[0].dirOwners); }); @@ -60,10 +76,30 @@ test('find owner for deep level dir', t => { const repoFile = new RepoFile( 'extensions/0.1/some-extension/some-extension.js', ownersMap); const ownerFile = repoFile.findRepoFileOwner(); - t.deepEqual(ownerFile.id, defaultStruct[2].path); + t.deepEqual(ownerFile.id, defaultStruct[2].dirname); t.deepEqual(ownerFile.usernames, defaultStruct[2].dirOwners); }); test('find file level owner', t => { + t.plan(1); + const repoFile = new RepoFile('file-with-file-level-owner.js', ownersMap); + const ownerFile = repoFile.findRepoFileOwner(); + t.deepEqual(ownerFile.usernames, ['file-level-owner-0']); +}); +test('find top file level owner in a deeper file', t => { + t.plan(1); + const repoFile = new RepoFile( + 'build-system/some-other-file-in-build-system.js', ownersMap); + const ownerFile = repoFile.findRepoFileOwner(); + t.deepEqual(ownerFile.usernames, ['file-level-owner-3']); +}); + +test('find accumulation of file level owners', t => { + t.plan(1); + const repoFile = new RepoFile('build-system/some-file-in-build-system.js', + ownersMap); + const ownerFile = repoFile.findRepoFileOwner(); + t.deepEqual(ownerFile.usernames, + ['file-level-owner-1', 'file-level-owner-2']); }); From 10ca00662f241ccfd0250e9e1df7203aaf71cc81 Mon Sep 17 00:00:00 2001 From: erwinmombay Date: Tue, 21 Mar 2017 01:49:43 -0700 Subject: [PATCH 7/7] debug travis --- declarations/all.js | 5 +++-- routes/owners/api.js | 1 + src/owner.js | 18 +++++++++--------- src/repo-file.js | 2 +- tests/test-api.js | 5 ++++- tests/test-owner.js | 26 ++++++++++++++++++++++++++ 6 files changed, 44 insertions(+), 13 deletions(-) diff --git a/declarations/all.js b/declarations/all.js index 5efd230..75a636a 100644 --- a/declarations/all.js +++ b/declarations/all.js @@ -43,13 +43,14 @@ type PullRequestInfo = { } type RepoFileOwner = { - id: ?string, + id: string, type: 'file' | 'dir', usernames: string[] } type OwnerTuple = { - owner: Owner, + type: 'file' | 'dir', + owner: Owner | string[], files: RepoFile[] } diff --git a/routes/owners/api.js b/routes/owners/api.js index 2958e24..0d41321 100644 --- a/routes/owners/api.js +++ b/routes/owners/api.js @@ -109,6 +109,7 @@ export function index(req: Object, res: Object) { if (process.env.NODE_ENV == 'production') { res.status(500).send('Something went wrong!'); } else { + console.log(e.stack); res.status(500).send(e.stack); } }); diff --git a/src/owner.js b/src/owner.js index 3dbcc80..2871905 100644 --- a/src/owner.js +++ b/src/owner.js @@ -94,15 +94,15 @@ export function createAggregatedOwnersTuple( repoFiles.forEach(repoFile => { const repoFileOwner = repoFile.findRepoFileOwner(); const id = repoFileOwner.id; - if (repoFileOwner.type == 'dir' && id) { - if (!aggregatedOwners[id]) { - aggregatedOwners[id] = { - owner: repoFile.ownersMap[id], - files: [repoFile], - }; - } else { - aggregatedOwners[id].files.push(repoFile); - } + + if (!aggregatedOwners[id]) { + aggregatedOwners[id] = { + type: repoFileOwner.type, + owner: repoFile.ownersMap[id], + files: [repoFile], + }; + } else { + aggregatedOwners[id].files.push(repoFile); } }); return Object.keys(aggregatedOwners).sort().map(key => { diff --git a/src/repo-file.js b/src/repo-file.js index bf8aae2..5c0db0c 100644 --- a/src/repo-file.js +++ b/src/repo-file.js @@ -50,7 +50,7 @@ export class RepoFile { } else { this.fileOwners = maybeOwner; this.repoFileOwner = { - id: null, + id: this.fileOwners.join(','), type: 'file', usernames: this.fileOwners, }; diff --git a/tests/test-api.js b/tests/test-api.js index 0d8aa6e..9615201 100644 --- a/tests/test-api.js +++ b/tests/test-api.js @@ -18,6 +18,7 @@ const reviewSubmittedFailedPayload = JSON.parse( let req, res, sandbox; test.beforeEach(() => { sandbox = sinon.sandbox.create(); + sandbox.stub(Git.prototype, 'pullLatestForRepo').returns(Promise.resolve()); }); test.afterEach(() => { @@ -25,7 +26,7 @@ test.afterEach(() => { }); // Note: Need to run these tests serially because of the shared "PullRequest" -// stubbing state. If we don't since ava runs everything conccurently the +// stubbing state. If we don't and since ava runs everything concurrently the // `afterEach` might not have ran yet when the next test does run. test.serial('on an opened pull request, if author is not part of owner ' + @@ -58,6 +59,8 @@ test.serial('on an opened pull request, if author is not part of owner ' + .then(() => { t.is(postCommentSpy.callCount, 1, 'Should call postIssuesComment'); t.is(setFailureStatusSpy.callCount, 1, 'Should call setFailureStatus'); + }).catch(e => { + console.log(e); }); }); diff --git a/tests/test-owner.js b/tests/test-owner.js index 5a6db27..600bd2c 100644 --- a/tests/test-owner.js +++ b/tests/test-owner.js @@ -59,10 +59,36 @@ test('aggregate RepoFiles who share the same Owner into an ' + t.deepEqual(aggregatedOwners, [ { owner: defaultStruct[0], + type: 'dir', files: [repoFile0, repoFile1], }, { owner: defaultStruct[1], + type: 'dir', + files: [repoFile2, repoFile3], + } + ]); +}); + +test('aggregate RepoFiles who share the same usernames list into an ' + + 'OwnerFileTuple for file level owners', t => { + t.plan(1); + const ownersMap = createOwnersMap(defaultStruct); + const repoFile0 = new RepoFile('README.md', ownersMap); + const repoFile1 = new RepoFile('some-src-file.js', ownersMap); + const repoFile2 = new RepoFile('build-system/some-src-file.js', ownersMap); + const repoFile3 = new RepoFile('build-system/some-src-file-1.js', ownersMap); + const repoFiles = [repoFile0, repoFile1, repoFile2, repoFile3]; + const aggregatedOwners = createAggregatedOwnersTuple(repoFiles); + t.deepEqual(aggregatedOwners, [ + { + owner: defaultStruct[0], + type: 'dir', + files: [repoFile0, repoFile1], + }, + { + owner: defaultStruct[1], + type: 'dir', files: [repoFile2, repoFile3], } ]);