Delete inline comments handling

source/platforms/BitBucketServer.ts

@@ -120,6 +120,14 @@ export class BitBucketServer implements Platform {
   updateInlineComment = (_comment: string, _commentId: string): Promise<any> =>
     new Promise((_resolve, reject) => reject())
 
+  /**
+   * Deletes an inline comment, used when you have
+   * fixed all your failures.
+   *
+   * @returns {Promise<boolean>} did it work?
+   */
+  deleteInlineComment = async (_id: string): Promise<boolean> => new Promise<boolean>((_resolve, reject) => reject())
+
   /**
    * Deletes the main Danger comment, used when you have
    * fixed all your failures.

source/platforms/FakePlatform.ts

@@ -64,6 +64,10 @@ export class FakePlatform implements Platform {
     return true
   }
 
+  async deleteInlineComment(_id: string): Promise<boolean> {
+    return true
+  }
+
   async deleteMainComment(): Promise<boolean> {
     return true
   }

source/platforms/GitHub.ts

@@ -204,7 +204,7 @@ export class GitHub implements Platform {
    *
    * @returns {Promise<boolean>} did it work?
    */
-  deleteInlineComment = async (id: number): Promise<boolean> => this.api.deleteInlineCommentWithID(id)
+  deleteInlineComment = async (id: string): Promise<boolean> => this.api.deleteInlineCommentWithID(id)
 
   /**
    * Either updates an existing comment, or makes a new one

source/platforms/LocalGit.ts

@@ -87,6 +87,10 @@ export class LocalGit implements Platform {
     return true
   }
 
+  async deleteInlineComment(_id: string): Promise<boolean> {
+    return true
+  }
+
   async deleteMainComment(): Promise<boolean> {
     return true
   }

source/platforms/github/GitHubAPI.ts

@@ -114,7 +114,7 @@ export class GitHubAPI {
     return Promise.resolve(res.status === 204)
   }
 
-  deleteInlineCommentWithID = async (id: number): Promise<boolean> => {
+  deleteInlineCommentWithID = async (id: string): Promise<boolean> => {
     const repo = this.repoMetadata.repoSlug
     const res = await this.api(`repos/${repo}/pulls/comments/${id}`, {}, {}, "DELETE")
 

source/platforms/platform.ts

@@ -49,8 +49,10 @@ export interface Platform {
   // Here we pass GitDSL because platforms have different endpoints for inline comments
   // Wasn't sure if passing the dsl is the best way of achieving this, though
   createInlineComment: (git: GitDSL, comment: string, path: string, line: number) => Promise<any>
-  /** Updates an inline comment with given id */
+  /** Updates an inline comment */
   updateInlineComment: (comment: string, commentId: string) => Promise<any>
+  /** Delete an inline comment */
+  deleteInlineComment: (commentId: string) => Promise<boolean>
   /** Delete the main Danger comment */
   deleteMainComment: (dangerID: string) => Promise<boolean>
   /** Replace the main Danger comment */

source/runner/Executor.ts

@@ -202,6 +202,10 @@ export class Executor {
     if (failureCount + messageCount === 0) {
       console.log("No issues or messages were sent. Removing any existing messages.")
       await this.platform.deleteMainComment(dangerID)
+      const previousComments = await this.platform.getInlineComments()
+      for (const comment of previousComments) {
+        await this.deleteInlineComment(comment)
+      }
     } else {
       if (fails.length > 0) {
         const s = fails.length === 1 ? "" : "s"
@@ -217,7 +221,6 @@ export class Executor {
         console.log("Found only messages, passing those to review.")
       }
       const previousComments = await this.platform.getInlineComments()
-      console.log("Previous comments:\n" + JSON.stringify(previousComments))
       const inline = inlineResults(results)
       const inlineLeftovers = await this.sendInlineComments(inline, git, previousComments)
       const regular = regularResults(results)
@@ -269,7 +272,10 @@ export class Executor {
       }
       commentPromises.push(promise.then(_r => emptyDangerResults).catch(_e => inlineResultsIntoResults(inlineResult)))
     }
-    // TODO: Remove leftovers from deleteComments array
+    deleteComments.forEach(comment => {
+      let promise = this.deleteInlineComment(comment)
+      commentPromises.push(promise.then(_r => emptyDangerResults).catch(_e => emptyDangerResults))
+    })
 
     return Promise.all(commentPromises).then(dangerResults => {
       return new Promise<DangerResults>(resolve => {
@@ -293,6 +299,10 @@ export class Executor {
     return await this.platform.updateInlineComment(body, previousComment.id)
   }
 
+  async deleteInlineComment(comment: Comment): Promise<any> {
+    return await this.platform.deleteInlineComment(comment.id)
+  }
+
   inlineCommentTemplate(inlineResults: DangerInlineResults): string {
     const results = inlineResultsIntoResults(inlineResults)
     const comment = process.env["DANGER_BITBUCKETSERVER_HOST"]

source/runner/_tests/_executor.test.ts

@@ -17,7 +17,7 @@ import { DangerDSLType } from "../../dsl/DangerDSL"
 import { singleViolationSingleFileResults } from "../../dsl/_tests/fixtures/ExampleDangerResults"
 import { Comment } from "../../platforms/platform"
 import { inlineTemplate } from "../templates/githubIssueTemplate"
-import { resultsIntoInlineResults } from "../../dsl/DangerResults"
+import { resultsIntoInlineResults, DangerResults, inlineResultsIntoResults } from "../../dsl/DangerResults"
 
 const defaultConfig = {
   stdoutOnly: false,
@@ -26,7 +26,7 @@ const defaultConfig = {
   dangerID: "123",
 }
 
-let defaultDsl = (platform): Promise<DangerDSLType> => {
+const defaultDsl = (platform): Promise<DangerDSLType> => {
   return jsonDSLGenerator(platform).then(jsonDSL => {
     jsonDSL.github = {
       pr: { number: 1, base: { sha: "321" }, head: { sha: "123", repo: { full_name: "123" } } },
@@ -35,6 +35,18 @@ let defaultDsl = (platform): Promise<DangerDSLType> => {
   })
 }
 
+const mockPayloadForResults = (results: DangerResults): any => {
+  return resultsIntoInlineResults(results).map(inlineResult => {
+    const comment = inlineTemplate(
+      defaultConfig.dangerID,
+      inlineResultsIntoResults(inlineResult),
+      inlineResult.file,
+      inlineResult.line
+    )
+    return { id: 1234, body: comment, ownedByDanger: true }
+  })
+}
+
 describe("setup", () => {
   it("gets diff / pr info in setup", async () => {
     const platform = new FakePlatform()
@@ -135,7 +147,7 @@ describe("setup", () => {
     const previousResults = inlineWarnResults
     const newResults = inlineFailResults
     const inlineResults = resultsIntoInlineResults(previousResults)[0]
-    const comment = inlineTemplate(defaultConfig.dangerID, inlineWarnResults, inlineResults.file, inlineResults.line)
+    const comment = inlineTemplate(defaultConfig.dangerID, previousResults, inlineResults.file, inlineResults.line)
     const previousComments = [{ id: 1234, body: comment, ownedByDanger: true }]
     platform.getInlineComments = jest.fn().mockReturnValue(new Promise(r => r(previousComments)))
     platform.updateInlineComment = jest.fn()
@@ -153,7 +165,7 @@ describe("setup", () => {
     const previousResults = inlineWarnResults
     const newResults = previousResults
     const inlineResults = resultsIntoInlineResults(previousResults)[0]
-    const comment = inlineTemplate(defaultConfig.dangerID, inlineWarnResults, inlineResults.file, inlineResults.line)
+    const comment = inlineTemplate(defaultConfig.dangerID, previousResults, inlineResults.file, inlineResults.line)
     const previousComments = [{ id: 1234, body: comment, ownedByDanger: true }]
     platform.getInlineComments = jest.fn().mockReturnValue(new Promise(r => r(previousComments)))
     platform.updateInlineComment = jest.fn()
@@ -171,7 +183,7 @@ describe("setup", () => {
     const previousResults = inlineWarnResults
     const newResults = inlineMessageResults
     const inlineResults = resultsIntoInlineResults(previousResults)[0]
-    const comment = inlineTemplate(defaultConfig.dangerID, inlineWarnResults, inlineResults.file, inlineResults.line)
+    const comment = inlineTemplate(defaultConfig.dangerID, previousResults, inlineResults.file, inlineResults.line)
     const previousComments = [{ id: 1234, body: comment, ownedByDanger: true }]
     platform.getInlineComments = jest.fn().mockReturnValue(new Promise(r => r(previousComments)))
     platform.updateInlineComment = jest.fn()
@@ -182,17 +194,58 @@ describe("setup", () => {
     expect(platform.createInlineComment).toBeCalled()
   })
 
-  // it("Deletes all old inline comments because new results are all clear", async () => {
-  //   // old results: [warnings: [{"1", file: "1.swift", line: 1}, {"2", file: "2.swift", line: 2}], fails: [], messages: [], markdowns: []]
-  //   // new results: [warnings: [], fails: [], messages: [], markdowns: []]
-  //   // check for 2 deletions
-  // })
+  it("Deletes all old inline comments because new results are all clear", async () => {
+    const platform = new FakePlatform()
+    const exec = new Executor(new FakeCI({}), platform, inlineRunner, defaultConfig)
+    const dsl = await defaultDsl(platform)
+    const previousResults = {
+      fails: [],
+      warnings: [{ message: "1", file: "1.swift", line: 1 }, { message: "2", file: "2.swift", line: 2 }],
+      messages: [],
+      markdowns: [],
+    }
+    const previousComments = mockPayloadForResults(previousResults)
+    const newResults = emptyResults
+
+    platform.getInlineComments = jest.fn().mockReturnValue(new Promise(r => r(previousComments)))
+    platform.updateInlineComment = jest.fn()
+    platform.createInlineComment = jest.fn()
+    platform.deleteInlineComment = jest.fn()
+
+    await exec.handleResults(newResults, dsl.git)
+    expect(platform.updateInlineComment).not.toBeCalled()
+    expect(platform.createInlineComment).not.toBeCalled()
+    expect(platform.deleteInlineComment).toHaveBeenCalledTimes(2)
+  })
+
+  it("Deletes old inline comment when not applicable in new results", async () => {
+    const platform = new FakePlatform()
+    const exec = new Executor(new FakeCI({}), platform, inlineRunner, defaultConfig)
+    const dsl = await defaultDsl(platform)
+    const previousResults = {
+      fails: [],
+      warnings: [{ message: "1", file: "1.swift", line: 1 }, { message: "2", file: "2.swift", line: 2 }],
+      messages: [],
+      markdowns: [],
+    }
+    const newResults = {
+      fails: [],
+      warnings: [{ message: "1", file: "1.swift", line: 2 }, { message: "2", file: "2.swift", line: 3 }],
+      messages: [],
+      markdowns: [],
+    }
+    const previousComments = mockPayloadForResults(previousResults)
 
-  // it("Deletes old inline comment when not applicable in new results", async () => {
-  //   // old results: [warnings: [{"1", file: "1.swift", line: 1}, {"2", file: "2.swift", line: 2}], fails: [], messages: [], markdowns: []]
-  //   // new results: [warnings: [{"1", file: "1.swift", line: 2}, {"2", file: "2.swift", line: 3}], fails: [], messages: [], markdowns: []]
-  //   // check for 2 deletions and 2 creations
-  // })
+    platform.getInlineComments = jest.fn().mockReturnValue(new Promise(r => r(previousComments)))
+    platform.updateInlineComment = jest.fn()
+    platform.createInlineComment = jest.fn()
+    platform.deleteInlineComment = jest.fn()
+
+    await exec.handleResults(newResults, dsl.git)
+    expect(platform.updateInlineComment).not.toBeCalled()
+    expect(platform.createInlineComment).toHaveBeenCalledTimes(2)
+    expect(platform.deleteInlineComment).toHaveBeenCalledTimes(2)
+  })
 
   it("Updates the status with success for a passed results", async () => {
     const platform = new FakePlatform()