Merge pull request #1331 from hcomde/issue_1138_gitlab_threads

GitLab: Add support to use threads instead of comments

CHANGELOG.md

@@ -22,7 +22,7 @@
 - GitLab: Added provider tests [#1319](https://github.com/danger/danger-js/pull/1319) [@ivankatliarchuk]
 
 - GitHub: Added `danger.github.pr.draft` field to DSL
-
+- GitLab: Add support for using threads instead of comments [#1331](https://github.com/danger/danger-js/pull/1331) [@uncaught]
 <!-- Your comment above this -->
 
 ## 11.1.2

docs/usage/gitlab.html.md

@@ -41,3 +41,8 @@ danger.gitlab.
   /** The commits associated with the merge request */
   commits: GitLabMRCommit[]
 ```
+
+---
+
+If you want danger to open threads (discussions) instead of just commenting in merge requests, set an environment
+variable `DANGER_GITLAB_USE_THREADS` with value `1` or `true`.

source/danger.d.ts

@@ -1662,6 +1662,12 @@ interface GitLabNote {
   noteable_iid: number
 }
 
+interface GitLabDiscussion {
+  id: string //40 character hex
+  individual_note: boolean
+  notes: GitLabNote[]
+}
+
 interface GitLabDiscussionTextPosition {
   position_type: "text"
   base_sha: string
@@ -1673,6 +1679,10 @@ interface GitLabDiscussionTextPosition {
   old_line: number | null
 }
 
+interface GitLabDiscussionCreationOptions {
+  position?: GitLabDiscussionTextPosition
+}
+
 interface GitLabInlineNote extends GitLabNote {
   position: {
     base_sha: string

source/dsl/GitLabDSL.ts

@@ -204,6 +204,12 @@ export interface GitLabNote {
   noteable_iid: number
 }
 
+export interface GitLabDiscussion {
+  id: string; //40 character hex
+  individual_note: boolean;
+  notes: GitLabNote[];
+}
+
 export interface GitLabDiscussionTextPosition {
   position_type: "text"
   base_sha: string
@@ -215,6 +221,10 @@ export interface GitLabDiscussionTextPosition {
   old_line: number | null
 }
 
+export interface GitLabDiscussionCreationOptions {
+  position?: GitLabDiscussionTextPosition
+}
+
 export interface GitLabInlineNote extends GitLabNote {
   position: {
     base_sha: string

source/platforms/GitLab.ts

@@ -1,13 +1,16 @@
 import GitLabAPI from "./gitlab/GitLabAPI"
-import { Platform, Comment } from "./platform"
+import { Comment, Platform } from "./platform"
 import { GitDSL, GitJSONDSL } from "../dsl/GitDSL"
 import { GitCommit } from "../dsl/Commit"
-import { GitLabDSL, GitLabJSONDSL, GitLabNote } from "../dsl/GitLabDSL"
-
+import { GitLabDiscussion, GitLabDSL, GitLabJSONDSL, GitLabNote } from "../dsl/GitLabDSL"
 import { debug } from "../debug"
 import { dangerIDToString } from "../runner/templates/githubIssueTemplate"
+
 const d = debug("GitLab")
 
+const useThreads = () => process.env.DANGER_GITLAB_USE_THREADS === "1" ||
+  process.env.DANGER_GITLAB_USE_THREADS === "true"
+
 class GitLab implements Platform {
   public readonly name: string
 
@@ -78,7 +81,8 @@ class GitLab implements Platform {
       return {
         id: `${note.id}`,
         body: note.body,
-        // XXX: we should re-use the logic in getDangerNotes, need to check what inline comment template we're using if any
+        // XXX: we should re-use the logic in getDangerNotes, need to check what inline comment template we're using if
+        // any
         ownedByDanger: note.author.id === dangerUserID && note.body.includes(dangerID),
       }
     })
@@ -95,51 +99,54 @@ class GitLab implements Platform {
   updateOrCreateComment = async (dangerID: string, newComment: string): Promise<string> => {
     d("updateOrCreateComment", { dangerID, newComment })
 
-    const notes: GitLabNote[] = await this.getDangerNotes(dangerID)
-
-    let note: GitLabNote
-
-    if (notes.length) {
-      // update the first
-      note = await this.api.updateMergeRequestNote(notes[0].id, newComment)
+    //Even when we don't set danger to create threads, we still need to delete them if someone answered to a single
+    // comment created by danger, resulting in a discussion/thread. Otherwise we are left with dangling comments
+    // that will most likely have no meaning out of context.
+    const discussions = await this.getDangerDiscussions(dangerID)
+    const firstDiscussion = discussions.shift()
+    const existingNote = firstDiscussion?.notes[0]
+    // Delete all notes from all other danger discussions (discussions cannot be deleted as a whole):
+    await this.deleteNotes(this.reduceNotesFromDiscussions(discussions)) //delete the rest
 
-      // delete the rest
-      for (let deleteme of notes) {
-        if (deleteme === notes[0]) {
-          continue
-        }
+    let newOrUpdatedNote: GitLabNote
 
-        await this.api.deleteMergeRequestNote(deleteme.id)
-      }
+    if (existingNote) {
+      // update the existing comment
+      newOrUpdatedNote = await this.api.updateMergeRequestNote(existingNote.id, newComment)
     } else {
-      // create a new note
-      note = await this.api.createMergeRequestNote(newComment)
+      // create a new comment
+      newOrUpdatedNote = await this.createComment(newComment)
     }
 
     // create URL from note
     // "https://gitlab.com/group/project/merge_requests/154#note_132143425"
-    return `${this.api.mergeRequestURL}#note_${note.id}`
+    return `${this.api.mergeRequestURL}#note_${newOrUpdatedNote.id}`
   }
 
-  createComment = async (comment: string): Promise<any> => {
+  createComment = async (comment: string): Promise<GitLabNote> => {
     d("createComment", { comment })
+    if (useThreads()) {
+      return (await this.api.createMergeRequestDiscussion(comment)).notes[0]
+    }
     return this.api.createMergeRequestNote(comment)
   }
 
-  createInlineComment = async (git: GitDSL, comment: string, path: string, line: number): Promise<string> => {
+  createInlineComment = async (git: GitDSL, comment: string, path: string, line: number): Promise<GitLabDiscussion> => {
     d("createInlineComment", { git, comment, path, line })
 
     const mr = await this.api.getMergeRequestInfo()
 
     return this.api.createMergeRequestDiscussion(comment, {
-      position_type: "text",
-      base_sha: mr.diff_refs.base_sha,
-      start_sha: mr.diff_refs.start_sha,
-      head_sha: mr.diff_refs.head_sha,
-      old_path: path,
-      old_line: null,
-      new_path: path,
-      new_line: line,
+      position: {
+        position_type: "text",
+        base_sha: mr.diff_refs.base_sha,
+        start_sha: mr.diff_refs.start_sha,
+        head_sha: mr.diff_refs.head_sha,
+        old_path: path,
+        old_line: null,
+        new_path: path,
+        new_line: line,
+      },
     })
   }
 
@@ -156,26 +163,54 @@ class GitLab implements Platform {
   }
 
   deleteMainComment = async (dangerID: string): Promise<boolean> => {
-    const notes = await this.getDangerNotes(dangerID)
-    for (let note of notes) {
-      d("deleteMainComment", { id: note.id })
+    //We fetch the discussions even if we are not set to use threads because users could still have replied to a
+    // comment by danger and thus created a discussion/thread. To not leave dangling notes, we delete the entire thread.
+    //There is no API to delete entire discussion. They can only be deleted fully by deleting every note:
+    const discussions = await this.getDangerDiscussions(dangerID)
+    return await this.deleteNotes(this.reduceNotesFromDiscussions(discussions))
+  }
+
+  deleteNotes = async (notes: GitLabNote[]): Promise<boolean> => {
+    for (const note of notes) {
+      d("deleteNotes", { id: note.id })
       await this.api.deleteMergeRequestNote(note.id)
     }
 
     return notes.length > 0
   }
 
+  /**
+   * Only fetches the discussions where danger owns the top note
+   */
+  getDangerDiscussions = async (dangerID: string): Promise<GitLabDiscussion[]> => {
+    const noteFilter = await this.getDangerNoteFilter(dangerID)
+    const discussions: GitLabDiscussion[] = await this.api.getMergeRequestDiscussions()
+    return discussions.filter(({ notes }) => notes.length && noteFilter(notes[0]))
+  }
+
+  reduceNotesFromDiscussions = (discussions: GitLabDiscussion[]): GitLabNote[] => {
+    return discussions.reduce<GitLabNote[]>((acc, { notes }) => [...acc, ...notes], [])
+  }
+
   getDangerNotes = async (dangerID: string): Promise<GitLabNote[]> => {
-    const { id: dangerUserId } = await this.api.getUser()
+    const noteFilter = await this.getDangerNoteFilter(dangerID)
     const notes: GitLabNote[] = await this.api.getMergeRequestNotes()
+    return notes.filter(noteFilter)
+  }
 
-    return notes.filter(
-      ({ author: { id }, body, system, type }: GitLabNote) =>
-        !system && // system notes are generated when the user interacts with the UI e.g. changing a PR title
-        type == null && // we only want "normal" comments on the main body of the MR;
+  getDangerNoteFilter = async (
+    dangerID: string,
+  ): Promise<(note: GitLabNote) => boolean> => {
+    const { id: dangerUserId } = await this.api.getUser()
+    return ({ author: { id }, body, system }: GitLabNote): boolean => {
+      return !system && // system notes are generated when the user interacts with the UI e.g. changing a PR title
         id === dangerUserId &&
-        body!.includes(dangerIDToString(dangerID)) // danger-id-(dangerID) is included in a hidden comment in the githubIssueTemplate
-    )
+        //we do not check the `type` - it's `null` most of the time,
+        // only in discussions/threads this is `DiscussionNote` for all notes. But even if danger only creates a
+        // normal `null`-comment, any user replying to that comment will turn it into a `DiscussionNote`-typed one.
+        // So we cannot assume anything here about danger's note type.
+        body.includes(dangerIDToString(dangerID))
+    }
   }
 
   updateStatus = async (): Promise<boolean> => {

source/platforms/_tests/_gitlab.test.ts

@@ -0,0 +1,132 @@
+import GitLabAPI from "../gitlab/GitLabAPI"
+import GitLab from "../GitLab"
+import { GitLabDiscussion, GitLabNote, GitLabUser, GitLabUserProfile } from "../../dsl/GitLabDSL"
+
+const baseUri = "https://my-gitlab.org/my-project"
+
+const dangerUserId = 8797215
+
+const getUser = async (): Promise<GitLabUserProfile> => {
+  const user: Partial<GitLabUserProfile> = { id: dangerUserId }
+  return user as GitLabUserProfile
+}
+
+const dangerID = "dddanger1234"
+const dangerIdString = `DangerID: danger-id-${dangerID};`
+
+function mockNote(id: number, authorId: number, body = ""): GitLabNote {
+  const author: Partial<GitLabUser> = { id: authorId }
+  const note: Partial<GitLabNote> = { author: author as GitLabUser, body, id }
+  return note as GitLabNote
+}
+
+function mockDiscussion(id: string, notes: GitLabNote[]): GitLabDiscussion {
+  const discussion: Partial<GitLabDiscussion> = { id, notes }
+  return discussion as GitLabDiscussion
+}
+
+function mockApi(withExisting = false): GitLabAPI {
+  const note1 = mockNote(1001, dangerUserId, `some body ${dangerIdString} asdf`)
+  const note2 = mockNote(1002, 125235)
+  const note3 = mockNote(1003, dangerUserId, `another danger ${dangerIdString} body`)
+  const note4 = mockNote(1004, 745774)
+  const note5 = mockNote(1005, 745774)
+  const discussion1 = mockDiscussion("aaaaffff1111", [note1, note2])
+  const discussion2 = mockDiscussion("aaaaffff2222", [note3, note4])
+  const discussion3 = mockDiscussion("aaaaffff3333", [note5])
+
+  const newNote = mockNote(4711, dangerUserId)
+  const newDiscussion = mockDiscussion("aaaaffff0000", [newNote])
+
+  const api: Partial<GitLabAPI> = {
+    getUser,
+    createMergeRequestDiscussion: jest.fn(() => Promise.resolve(newDiscussion)),
+    createMergeRequestNote: jest.fn(() => Promise.resolve(newNote)),
+    deleteMergeRequestNote: jest.fn(() => Promise.resolve(true)),
+    getMergeRequestDiscussions: jest.fn(() => Promise.resolve(withExisting
+      ? [discussion1, discussion2, discussion3]
+      : [])),
+    mergeRequestURL: baseUri,
+    updateMergeRequestNote: jest.fn(() => Promise.resolve(newNote)),
+  }
+  return api as GitLabAPI
+}
+
+describe("updateOrCreateComment", () => {
+  const comment = "my new comment"
+
+  it("create a new single comment", async () => {
+    delete process.env.DANGER_GITLAB_USE_THREADS
+
+    const api = mockApi()
+    const url = await (new GitLab(api as GitLabAPI)).updateOrCreateComment(dangerID, comment)
+    expect(url).toEqual(`${baseUri}#note_4711`)
+
+    expect(api.getMergeRequestDiscussions).toHaveBeenCalledTimes(1)
+    expect(api.deleteMergeRequestNote).toHaveBeenCalledTimes(0)
+    expect(api.createMergeRequestDiscussion).toHaveBeenCalledTimes(0)
+    expect(api.createMergeRequestNote).toHaveBeenCalledTimes(1)
+    expect(api.createMergeRequestNote).toHaveBeenCalledWith(comment)
+    expect(api.updateMergeRequestNote).toHaveBeenCalledTimes(0)
+  })
+
+  it("create a new thread", async () => {
+    process.env.DANGER_GITLAB_USE_THREADS = "true"
+
+    const api = mockApi()
+    const url = await (new GitLab(api as GitLabAPI)).updateOrCreateComment(dangerID, comment)
+    expect(url).toEqual(`${baseUri}#note_4711`)
+
+    expect(api.getMergeRequestDiscussions).toHaveBeenCalledTimes(1)
+    expect(api.deleteMergeRequestNote).toHaveBeenCalledTimes(0)
+    expect(api.createMergeRequestDiscussion).toHaveBeenCalledTimes(1)
+    expect(api.createMergeRequestDiscussion).toHaveBeenCalledWith(comment)
+    expect(api.createMergeRequestNote).toHaveBeenCalledTimes(0)
+    expect(api.updateMergeRequestNote).toHaveBeenCalledTimes(0)
+  })
+
+  it("update an existing thread", async () => {
+    const api = mockApi(true)
+    const url = await (new GitLab(api as GitLabAPI)).updateOrCreateComment(dangerID, comment)
+    expect(url).toEqual(`${baseUri}#note_4711`)
+
+    expect(api.getMergeRequestDiscussions).toHaveBeenCalledTimes(1)
+    expect(api.deleteMergeRequestNote).toHaveBeenCalledTimes(2)
+    expect(api.deleteMergeRequestNote).toHaveBeenNthCalledWith(1, 1003)
+    expect(api.deleteMergeRequestNote).toHaveBeenNthCalledWith(2, 1004)
+    expect(api.createMergeRequestDiscussion).toHaveBeenCalledTimes(0)
+    expect(api.createMergeRequestNote).toHaveBeenCalledTimes(0)
+    expect(api.updateMergeRequestNote).toHaveBeenCalledTimes(1)
+    expect(api.updateMergeRequestNote).toHaveBeenCalledWith(1001, comment)
+  })
+})
+
+describe("deleteMainComment", () => {
+  it("delete nothing", async () => {
+    const api = mockApi()
+    const result = await (new GitLab(api as GitLabAPI)).deleteMainComment(dangerID)
+    expect(result).toEqual(false)
+
+    expect(api.getMergeRequestDiscussions).toHaveBeenCalledTimes(1)
+    expect(api.deleteMergeRequestNote).toHaveBeenCalledTimes(0)
+    expect(api.createMergeRequestDiscussion).toHaveBeenCalledTimes(0)
+    expect(api.createMergeRequestNote).toHaveBeenCalledTimes(0)
+    expect(api.updateMergeRequestNote).toHaveBeenCalledTimes(0)
+  })
+
+  it("delete all danger attached notes", async () => {
+    const api = mockApi(true)
+    const result = await (new GitLab(api as GitLabAPI)).deleteMainComment(dangerID)
+    expect(result).toEqual(true)
+
+    expect(api.getMergeRequestDiscussions).toHaveBeenCalledTimes(1)
+    expect(api.deleteMergeRequestNote).toHaveBeenCalledTimes(4)
+    expect(api.deleteMergeRequestNote).toHaveBeenNthCalledWith(1, 1001)
+    expect(api.deleteMergeRequestNote).toHaveBeenNthCalledWith(2, 1002)
+    expect(api.deleteMergeRequestNote).toHaveBeenNthCalledWith(3, 1003)
+    expect(api.deleteMergeRequestNote).toHaveBeenNthCalledWith(4, 1004)
+    expect(api.createMergeRequestDiscussion).toHaveBeenCalledTimes(0)
+    expect(api.createMergeRequestNote).toHaveBeenCalledTimes(0)
+    expect(api.updateMergeRequestNote).toHaveBeenCalledTimes(0)
+  })
+})