From fb630ec8e12d33263096865d7db22d0e699e5f42 Mon Sep 17 00:00:00 2001 From: Jonathan Burke Date: Fri, 19 Nov 2021 17:19:20 +0100 Subject: [PATCH 1/2] extend github api to send complete reviews Signed-off-by: Jonathan Burke --- source/platforms/FakePlatform.ts | 5 +++ source/platforms/github/GitHubAPI.ts | 25 +++++++++++++ .../github/_tests/_github_api.test.ts | 17 +++++++++ .../platforms/github/comms/issueCommenter.ts | 14 ++++++++ source/platforms/platform.ts | 2 ++ source/runner/Executor.ts | 36 ++++++++++++++++--- source/runner/_tests/_executor.test.ts | 19 +++++++++- 7 files changed, 112 insertions(+), 6 deletions(-) diff --git a/source/platforms/FakePlatform.ts b/source/platforms/FakePlatform.ts index 63d3f3425..fd89aff74 100644 --- a/source/platforms/FakePlatform.ts +++ b/source/platforms/FakePlatform.ts @@ -85,4 +85,9 @@ export class FakePlatform implements Platform { } getFileContents = (path: string) => new Promise(res => res(readFileSync(path, "utf8"))) + + createInlineReview?: ( + git: GitDSL, + comments: { comment: string; path: string; line: number }[] + ) => Promise = undefined } diff --git a/source/platforms/github/GitHubAPI.ts b/source/platforms/github/GitHubAPI.ts index 3d5862f4f..485fcf7fe 100644 --- a/source/platforms/github/GitHubAPI.ts +++ b/source/platforms/github/GitHubAPI.ts @@ -181,6 +181,31 @@ export class GitHubAPI { } } + postInlinePRReview = async (commitId: string, comments: { comment: string; path: string; position: number }[]) => { + const repo = this.repoMetadata.repoSlug + const prID = this.repoMetadata.pullRequestID + const res = await this.post( + `repos/${repo}/pulls/${prID}/reviews`, + {}, + { + body: "", + event: "COMMENT", + commit_id: commitId, + comments: comments.map(({ comment, path, position }) => ({ + body: comment, + path, + position, + })), + }, + false + ) + if (res.ok) { + return res.json() + } else { + throw await res.json() + } + } + updateInlinePRComment = async (comment: string, commentId: string) => { const repo = this.repoMetadata.repoSlug const res = await this.patch( diff --git a/source/platforms/github/_tests/_github_api.test.ts b/source/platforms/github/_tests/_github_api.test.ts index 79eb4d91f..f1931676e 100644 --- a/source/platforms/github/_tests/_github_api.test.ts +++ b/source/platforms/github/_tests/_github_api.test.ts @@ -156,6 +156,23 @@ describe("API testing", () => { await expect(api.postInlinePRComment("", "", "", 0)).rejects.toEqual(expectedJSON) }) + it("postInlinePRReview success", async () => { + api.fetch = fetchJSON + const expectedJSON = { + api: "https://api.github.com/repos/artsy/emission/pulls/1/reviews", + headers: { + Authorization: "token ABCDE", + "Content-Type": "application/json", + }, + method: "POST", + body: '{"body":"","event":"COMMENT","commit_id":"","comments":[{"body":"","path":"","position":0}]}', + } + expect.assertions(1) + await expect(api.postInlinePRReview("", [{ comment: "", path: "", position: 0 }])).resolves.toMatchObject( + expectedJSON + ) + }) + it("updateStatus('pending') success", async () => { api.fetch = jest.fn().mockReturnValue({ ok: true }) api.getPullRequestInfo = await requestWithFixturedJSON("github_pr.json") diff --git a/source/platforms/github/comms/issueCommenter.ts b/source/platforms/github/comms/issueCommenter.ts index d6c71d6a3..9ce271398 100644 --- a/source/platforms/github/comms/issueCommenter.ts +++ b/source/platforms/github/comms/issueCommenter.ts @@ -89,6 +89,20 @@ export const GitHubIssueCommenter = (api: GitHubAPI) => { }) }, + createInlineReview: (git: GitDSL, comments: { comment: string; path: string; line: number }[]) => { + let commitId = git.commits[git.commits.length - 1].sha + d("Finishing review. Commit: " + commitId) + return Promise.all( + comments.map(comment => + findPositionForInlineComment(git, comment.line, comment.path).then(position => ({ + comment: comment.comment, + path: comment.path, + position, + })) + ) + ).then(comments => api.postInlinePRReview(commitId, comments)) + }, + /** * Updates an inline comment if possible. If platform can't update an inline comment, * it returns a promise rejection. (e.g. platform doesn't support inline comments or line was out of diff). diff --git a/source/platforms/platform.ts b/source/platforms/platform.ts index 31a9270f7..9d776b822 100644 --- a/source/platforms/platform.ts +++ b/source/platforms/platform.ts @@ -81,6 +81,8 @@ export interface PlatformCommunicator { updateInlineComment: (comment: string, commentId: string) => Promise /** Delete an inline comment */ deleteInlineComment: (commentId: string) => Promise + /** Create a review with inline comments */ + createInlineReview?: (git: GitDSL, comments: { comment: string; path: string; line: number }[]) => Promise /** Delete the main Danger comment */ deleteMainComment: (dangerID: string) => Promise /** Replace the main Danger comment, returning the URL to the issue */ diff --git a/source/runner/Executor.ts b/source/runner/Executor.ts index d541a6011..6707abe62 100644 --- a/source/runner/Executor.ts +++ b/source/runner/Executor.ts @@ -381,32 +381,58 @@ export class Executor { // Leftovers in deleteComments array should all be deleted afterwards let deleteComments = Array.isArray(previousComments) ? previousComments.filter(c => c.ownedByDanger) : [] let commentPromises: Promise[] = [] + const inlineResultsForReview: DangerInlineResults[] = [] for (let inlineResult of sortedInlineResults) { const index = deleteComments.findIndex(p => p.body.includes(fileLineToString(inlineResult.file, inlineResult.line)) ) - let promise: Promise + let promise: Promise | undefined = undefined if (index != -1) { let previousComment = deleteComments[index] deleteComments.splice(index, 1) promise = this.updateInlineComment(inlineResult, previousComment) } else { - promise = this.sendInlineComment(git, inlineResult) + if (typeof this.platform.createInlineReview === "function") { + inlineResultsForReview.push(inlineResult) + } else { + promise = this.sendInlineComment(git, inlineResult) + } + } + if (promise) { + commentPromises.push(promise.then(_r => emptyDangerResults).catch(_e => inlineResultsIntoResults(inlineResult))) } - commentPromises.push(promise.then(_r => emptyDangerResults).catch(_e => inlineResultsIntoResults(inlineResult))) } deleteComments.forEach(comment => { let promise = this.deleteInlineComment(comment) commentPromises.push(promise.then(_r => emptyDangerResults).catch(_e => emptyDangerResults)) }) - return Promise.all(commentPromises).then(dangerResults => { + return Promise.all([ + this.sendInlineReview(git, inlineResultsForReview).catch(_e => + inlineResultsForReview.forEach(inlineResult => inlineResultsIntoResults(inlineResult)) + ), + ...commentPromises, + ]).then(dangerResults => { return new Promise(resolve => { - resolve(dangerResults.reduce((acc, r) => mergeResults(acc, r), emptyResult)) + resolve(dangerResults.slice(1).reduce((acc, r) => mergeResults(acc, r), emptyResult)) }) }) } + async sendInlineReview(git: GitDSL, inlineResultsForReview: DangerInlineResults[]): Promise { + if (inlineResultsForReview.length === 0 || typeof this.platform.createInlineReview !== "function") { + return emptyDangerResults + } + return await this.platform.createInlineReview( + git, + inlineResultsForReview.map(result => ({ + comment: this.inlineCommentTemplate(result), + path: result.file, + line: result.line, + })) + ) + } + async sendInlineComment(git: GitDSL, inlineResults: DangerInlineResults): Promise { const comment = this.inlineCommentTemplate(inlineResults) return await this.platform.createInlineComment(git, comment, inlineResults.file, inlineResults.line) diff --git a/source/runner/_tests/_executor.test.ts b/source/runner/_tests/_executor.test.ts index 427877332..865f419c3 100644 --- a/source/runner/_tests/_executor.test.ts +++ b/source/runner/_tests/_executor.test.ts @@ -233,6 +233,20 @@ describe("setup", () => { expect(platform.createInlineComment).toBeCalled() }) + it("Creates multiple inline comments as a review", async () => { + const platform = new FakePlatform() + const exec = new Executor(new FakeCI({}), platform, inlineRunner, defaultConfig, new FakeProcces()) + const dsl = await defaultDsl(platform) + platform.createInlineReview = jest.fn() + platform.createInlineComment = jest.fn() + platform.createComment = jest.fn() + platform.updateOrCreateComment = jest.fn() + + await exec.handleResults(inlineMultipleWarnResults, dsl.git) + expect(platform.createInlineReview).toBeCalled() + expect(platform.createInlineComment).not.toBeCalled() + }) + it("Invalid inline comment is ignored if ignoreOutOfDiffComments is true", async () => { const platform = new FakePlatform() const config = Object.assign({}, defaultConfig, { ignoreOutOfDiffComments: true }) @@ -355,7 +369,10 @@ describe("setup", () => { const dsl = await defaultDsl(platform) const previousComments = mockPayloadForResults(inlineMultipleWarnResults) - platform.getInlineComments = jest.fn().mockResolvedValueOnce(previousComments).mockResolvedValueOnce([]) + platform.getInlineComments = jest + .fn() + .mockResolvedValueOnce(previousComments) + .mockResolvedValueOnce([]) platform.updateInlineComment = jest.fn() platform.createInlineComment = jest.fn() platform.deleteInlineComment = jest.fn() From 68ab2e99c7742d098bd9082db515fabd6f15d0e9 Mon Sep 17 00:00:00 2001 From: Jonathan Burke Date: Fri, 7 Jan 2022 08:22:53 +0100 Subject: [PATCH 2/2] add changelog entry for pr 1176 --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 209ab0ac5..1d5238457 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,8 @@ +- Fix: [Github] Multiple Inline Comments on the same file/line should all be posted [#1176](https://github.com/danger/danger-js/pull/1176) [@Rouby] + # 10.7.1