Merge pull request #375 from danger/ux_fixes

Improve the UX around duped issues, and the commit status

changelog.md

@@ -2,6 +2,12 @@
 
 ### Master
 
+Some UX fixes:
+
+- Don't show warnings about not setting a commit status (unless in verbose) - orta
+- Delete duplicate Danger message, due to fast Peril edits - orta
+- Show Peril in the commit status if inside Peril, not just Danger - orta
+
 ### 2.0.0-alpha.15
 
 - Updates `diffForFile`, `JSONPatchForFile`, and `JSONDiffForFile` to include created and removed files - #368 - bdotdub

source/api/fetch.ts

@@ -10,7 +10,11 @@ declare const global: any
  * @param {fetch.RequestInit} [init] the usual options
  * @returns {Promise<fetch.Response>} network-y promise
  */
-export function api(url: string | node_fetch.Request, init: node_fetch.RequestInit): Promise<node_fetch.Response> {
+export function api(
+  url: string | node_fetch.Request,
+  init: node_fetch.RequestInit,
+  suppressErrorReporting?: boolean
+): Promise<node_fetch.Response> {
   if (global.verbose && global.verbose === true) {
     const output = ["curl", "-i"]
 
@@ -48,7 +52,7 @@ export function api(url: string | node_fetch.Request, init: node_fetch.RequestIn
   const originalFetch: any = node_fetch
   return originalFetch(url, init).then(async (response: node_fetch.Response) => {
     // Handle failing errors
-    if (!response.ok) {
+    if (!suppressErrorReporting && !response.ok) {
       // we should not modify the response when an error occur to allow body stream to be read again if needed
       let clonedResponse = response.clone()
       console.warn(`Request failed [${clonedResponse.status}]: ${clonedResponse.url}`)

source/platforms/FakePlatform.ts

@@ -38,10 +38,6 @@ export class FakePlatform implements Platform {
     return true
   }
 
-  async editMainComment(_comment: string): Promise<boolean> {
-    return true
-  }
-
   async updateStatus(_success: boolean, _message: string): Promise<boolean> {
     return true
   }

source/platforms/GitHub.ts

@@ -104,12 +104,12 @@ export class GitHub {
    * @returns {Promise<boolean>} did it work?
    */
   async deleteMainComment(): Promise<boolean> {
-    const commentID = await this.api.getDangerCommentID()
-    if (commentID) {
-      return await this.api.deleteCommentWithID(commentID)
+    const commentIDs = await this.api.getDangerCommentIDs()
+    for (let commentID of commentIDs) {
+      await this.api.deleteCommentWithID(commentID)
     }
 
-    return commentID !== null
+    return commentIDs.length > 0
   }
 
   /**
@@ -119,29 +119,23 @@ export class GitHub {
    * @returns {Promise<boolean>} success of posting comment
    */
   async updateOrCreateComment(newComment: string): Promise<boolean> {
-    const commentID = await this.api.getDangerCommentID()
-    if (commentID) {
-      await this.api.updateCommentWithID(commentID, newComment)
+    const commentIDs = await this.api.getDangerCommentIDs()
+
+    if (commentIDs.length) {
+      // Edit the first comment
+      await this.api.updateCommentWithID(commentIDs[0], newComment)
+
+      // Delete any dupes
+      for (let commentID of commentIDs) {
+        if (commentID !== commentIDs[0]) {
+          await this.api.deleteCommentWithID(commentID)
+        }
+      }
     } else {
       await this.createComment(newComment)
     }
-    return true
-  }
 
-  /**
-   * Updates the main Danger comment, when Danger has run
-   * more than once
-   *
-   * @param {string} comment updated text
-   *
-   * @returns {Promise<boolean>} did it work?
-   */
-  async editMainComment(comment: string): Promise<boolean> {
-    const commentID = await this.api.getDangerCommentID()
-    if (commentID) {
-      await this.api.updateCommentWithID(commentID, comment)
-    }
-    return commentID !== null
+    return true
   }
 
   /**

source/platforms/github/GitHubAPI.ts

@@ -1,6 +1,5 @@
 import * as GitHubNodeAPI from "github"
 import * as debug from "debug"
-import * as find from "lodash.find"
 import * as node_fetch from "node-fetch"
 import * as parse from "parse-link-header"
 import * as v from "voca"
@@ -76,14 +75,13 @@ export class GitHubAPI {
 
   // The above is the API for Platform
 
-  async getDangerCommentID(): Promise<number | null> {
+  async getDangerCommentIDs(): Promise<number[]> {
     const userID = await this.getUserID()
     const allComments: any[] = await this.getPullRequestComments()
-    const dangerComment = find(
-      allComments,
-      (comment: any) => (!userID || comment.user.id === userID) && v.includes(comment.body, dangerSignaturePostfix)
-    )
-    return dangerComment ? dangerComment.id : null
+    return allComments
+      .filter(comment => userID || comment.user.id === userID)
+      .filter(comment => v.includes(comment.body, dangerSignaturePostfix))
+      .map(comment => comment.id)
   }
 
   async updateCommentWithID(id: number, comment: string): Promise<any> {
@@ -266,7 +264,7 @@ export class GitHubAPI {
       {},
       {
         state: passed ? "success" : "failure",
-        context: "Danger",
+        context: process.env["PERIL_INTEGRATION_ID"] ? "Peril" : "Danger",
         target_url: "http://danger.systems/js",
         description: message,
       }
@@ -277,7 +275,7 @@ export class GitHubAPI {
 
   // API Helpers
 
-  private api(path: string, headers: any = {}, body: any = {}, method: string) {
+  private api(path: string, headers: any = {}, body: any = {}, method: string, suppressErrors?: boolean) {
     if (this.token) {
       headers["Authorization"] = `token ${this.token}`
     }
@@ -292,24 +290,28 @@ export class GitHubAPI {
       // e.g. https://gist.github.com/LTe/5270348
       customAccept = { Accept: `${this.additionalHeaders.Accept}, ${headers.Accept}` }
     }
-    return this.fetch(url, {
-      method: method,
-      body: body,
-      headers: {
-        "Content-Type": "application/json",
-        ...headers,
-        ...this.additionalHeaders,
-        ...customAccept,
+    return this.fetch(
+      url,
+      {
+        method: method,
+        body: body,
+        headers: {
+          "Content-Type": "application/json",
+          ...headers,
+          ...this.additionalHeaders,
+          ...customAccept,
+        },
       },
-    })
+      suppressErrors
+    )
   }
 
   get(path: string, headers: any = {}, body: any = {}): Promise<node_fetch.Response> {
     return this.api(path, headers, body, "GET")
   }
 
-  post(path: string, headers: any = {}, body: any = {}): Promise<node_fetch.Response> {
-    return this.api(path, headers, JSON.stringify(body), "POST")
+  post(path: string, headers: any = {}, body: any = {}, suppressErrors?: boolean): Promise<node_fetch.Response> {
+    return this.api(path, headers, JSON.stringify(body), "POST", suppressErrors)
   }
 
   patch(path: string, headers: any = {}, body: any = {}): Promise<node_fetch.Response> {

source/platforms/github/_tests/_github_api.test.ts

@@ -99,15 +99,19 @@ describe("Peril", () => {
 
   it("Allows setting additional headers", async () => {
     const request = await api.get("user")
-    expect(api.fetch).toHaveBeenCalledWith("https://api.github.com/user", {
-      body: {},
-      headers: {
-        Authorization: "token ABCDE",
-        CUSTOM: "HEADER",
-        "Content-Type": "application/json",
+    expect(api.fetch).toHaveBeenCalledWith(
+      "https://api.github.com/user",
+      {
+        body: {},
+        headers: {
+          Authorization: "token ABCDE",
+          CUSTOM: "HEADER",
+          "Content-Type": "application/json",
+        },
+        method: "GET",
       },
-      method: "GET",
-    })
+      undefined
+    )
   })
 
   it("Merges two Accept headers", async () => {
@@ -117,15 +121,19 @@ describe("Peril", () => {
       Accept: "application/vnd.github.v3.diff",
     })
 
-    expect(api.fetch).toHaveBeenCalledWith("https://api.github.com/user", {
-      body: {},
-      headers: {
-        Accept: "application/vnd.github.machine-man-preview+json, application/vnd.github.v3.diff",
-        Authorization: "token ABCDE",
-        "Content-Type": "application/json",
+    expect(api.fetch).toHaveBeenCalledWith(
+      "https://api.github.com/user",
+      {
+        body: {},
+        headers: {
+          Accept: "application/vnd.github.machine-man-preview+json, application/vnd.github.v3.diff",
+          Authorization: "token ABCDE",
+          "Content-Type": "application/json",
+        },
+        method: "GET",
       },
-      method: "GET",
-    })
+      undefined
+    )
   })
 
   describe("Allows setting DANGER_GITHUB_APP env variable", () => {

source/platforms/platform.ts

@@ -40,8 +40,6 @@ export interface Platform {
   /** Delete the main Danger comment */
   deleteMainComment: () => Promise<boolean>
   /** Replace the main Danger comment */
-  editMainComment: (newComment: string) => Promise<any>
-  /** Replace the main Danger comment */
   updateOrCreateComment: (newComment: string) => Promise<any>
   /** Sets the current PR's status */
   updateStatus: (passed: boolean, message: string) => Promise<boolean>

source/runner/Executor.ts

@@ -147,6 +147,10 @@ export class Executor {
 
     const failed = fails.length > 0
     const successPosting = await this.platform.updateStatus(!failed, messageForResults(results))
+    if (this.options.verbose) {
+      console.log("Could not add a commit status, the GitHub token for Danger does not have access rights.")
+      console.log("If the build fails, then danger will use a failing exit code.")
+    }
 
     if (failureCount + messageCount === 0) {
       console.log("No issues or messages were sent. Removing any existing messages.")