Fix: [Github] Multiple Inline Comments on the same file/line should all be posted

CHANGELOG.md

@@ -15,6 +15,8 @@
 
 <!-- Your comment below this -->
 
+- Fix: [Github] Multiple Inline Comments on the same file/line should all be posted [#1176](https://github.com/danger/danger-js/pull/1176) [@Rouby]
+
 <!-- Your comment above this -->
 
 # 10.8.0

source/platforms/FakePlatform.ts

@@ -85,4 +85,9 @@ export class FakePlatform implements Platform {
   }
 
   getFileContents = (path: string) => new Promise<string>(res => res(readFileSync(path, "utf8")))
+
+  createInlineReview?: (
+    git: GitDSL,
+    comments: { comment: string; path: string; line: number }[]
+  ) => Promise<any> = undefined
 }

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(

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

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

source/platforms/platform.ts

@@ -81,6 +81,8 @@ export interface PlatformCommunicator {
   updateInlineComment: (comment: string, commentId: string) => Promise<any>
   /** Delete an inline comment */
   deleteInlineComment: (commentId: string) => Promise<boolean>
+  /** Create a review with inline comments */
+  createInlineReview?: (git: GitDSL, comments: { comment: string; path: string; line: number }[]) => Promise<any>
   /** Delete the main Danger comment */
   deleteMainComment: (dangerID: string) => Promise<boolean>
   /** Replace the main Danger comment, returning the URL to the issue */

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<any>[] = []
+    const inlineResultsForReview: DangerInlineResults[] = []
     for (let inlineResult of sortedInlineResults) {
       const index = deleteComments.findIndex(p =>
         p.body.includes(fileLineToString(inlineResult.file, inlineResult.line))
       )
-      let promise: Promise<any>
+      let promise: Promise<any> | 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<DangerResults>(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<any> {
+    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<any> {
     const comment = this.inlineCommentTemplate(inlineResults)
     return await this.platform.createInlineComment(git, comment, inlineResults.file, inlineResults.line)

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