diff --git a/source/platforms/GitLab.ts b/source/platforms/GitLab.ts index 6af3d6135..60c7e526e 100644 --- a/source/platforms/GitLab.ts +++ b/source/platforms/GitLab.ts @@ -8,6 +8,9 @@ import { dangerIDToString } from "../runner/templates/githubIssueTemplate" const d = debug("GitLab") +/** + * Determines whether Danger should use threads for the "main" Danger comment. + */ const useThreads = () => process.env.DANGER_GITLAB_USE_THREADS === "1" || process.env.DANGER_GITLAB_USE_THREADS === "true" @@ -81,8 +84,6 @@ 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 ownedByDanger: note.author.id === dangerUserID && note.body.includes(dangerID), } }) @@ -99,28 +100,49 @@ class GitLab implements Platform { updateOrCreateComment = async (dangerID: string, newComment: string): Promise => { d("updateOrCreateComment", { dangerID, 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 - - let newOrUpdatedNote: GitLabNote + if (useThreads()) { + 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 + + let newOrUpdatedNote: GitLabNote + + if (existingNote) { + // update the existing comment + newOrUpdatedNote = await this.api.updateMergeRequestNote(existingNote.id, newComment) + } else { + // create a new comment + newOrUpdatedNote = await this.createComment(newComment) + } - if (existingNote) { - // update the existing comment - newOrUpdatedNote = await this.api.updateMergeRequestNote(existingNote.id, newComment) + // create URL from note + // "https://gitlab.com/group/project/merge_requests/154#note_132143425" + return `${this.api.mergeRequestURL}#note_${newOrUpdatedNote.id}` } else { - // create a new comment - newOrUpdatedNote = await this.createComment(newComment) + const notes: GitLabNote[] = await this.getMainDangerNotes(dangerID) + + let note: GitLabNote + + if (notes.length) { + // update the first + note = await this.api.updateMergeRequestNote(notes[0].id, newComment) + // delete the rest + for (let deleteme of notes) { + if (deleteme === notes[0]) { + continue + } + await this.api.deleteMergeRequestNote(deleteme.id) + } + } else { + // create a new note + note = await this.api.createMergeRequestNote(newComment) + } + // create URL from note + // "https://gitlab.com/group/project/merge_requests/154#note_132143425" + return `${this.api.mergeRequestURL}#note_${note.id}` } - - // create URL from note - // "https://gitlab.com/group/project/merge_requests/154#note_132143425" - return `${this.api.mergeRequestURL}#note_${newOrUpdatedNote.id}` } createComment = async (comment: string): Promise => { @@ -162,12 +184,24 @@ class GitLab implements Platform { return await this.api.deleteMergeRequestNote(nid) } + /** + * Attempts to delete the "main" Danger comment. If the "main" Danger + * comment has any comments on it then that comment will not be deleted. + */ deleteMainComment = async (dangerID: string): Promise => { - //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)) + if (useThreads()) { + // 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)) + } else { + const notes = await this.getMainDangerNotes(dangerID) + for (let note of notes) { + d("deleteMainComment", { id: note.id }) + await this.api.deleteMergeRequestNote(note.id) + } + + return notes.length > 0 + } } deleteNotes = async (notes: GitLabNote[]): Promise => { @@ -183,7 +217,7 @@ class GitLab implements Platform { * Only fetches the discussions where danger owns the top note */ getDangerDiscussions = async (dangerID: string): Promise => { - const noteFilter = await this.getDangerNoteFilter(dangerID) + const noteFilter = await this.getDangerDiscussionNoteFilter(dangerID) const discussions: GitLabDiscussion[] = await this.api.getMergeRequestDiscussions() return discussions.filter(({ notes }) => notes.length && noteFilter(notes[0])) } @@ -192,22 +226,49 @@ class GitLab implements Platform { return discussions.reduce((acc, { notes }) => [...acc, ...notes], []) } - getDangerNotes = async (dangerID: string): Promise => { - const noteFilter = await this.getDangerNoteFilter(dangerID) + /** + * Attempts to find the "main" Danger note and should return at most + * one item. If the "main" Danger note has any comments on it then that + * note will not be returned. + */ + getMainDangerNotes = async (dangerID: string): Promise => { + const noteFilter = await this.getDangerMainNoteFilter(dangerID) const notes: GitLabNote[] = await this.api.getMergeRequestNotes() return notes.filter(noteFilter) } - getDangerNoteFilter = async (dangerID: string): Promise<(note: GitLabNote) => boolean> => { + /** + * Filters a note to determine if it was created by Danger. + */ + getDangerDiscussionNoteFilter = 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 && - //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)) + ) + } + } + + /** + * Filters a note to the "main" Danger note. If that note has any + * comments on it then it will not be found. + */ + getDangerMainNoteFilter = async (dangerID: string): Promise<(note: GitLabNote) => boolean> => { + const { id: dangerUserId } = await this.api.getUser() + return ({ author: { id }, body, system, type }: GitLabNote): boolean => { + return ( + !system && // system notes are generated when the user interacts with the UI e.g. changing a PR title + id === dangerUserId && + // This check for the type being `null` seems to be the best option + // we have to determine whether this note is the "main" Danger note. + // This assumption does not hold if there are any comments on the + // "main" Danger note and in that case a new "main" Danger note will + // be created instead of updating the existing note. This behavior is better + // than the current alternative which is the bug described here: + // https://github.com/danger/danger-js/issues/1351 + type == null && body.includes(dangerIDToString(dangerID)) ) } diff --git a/source/platforms/_tests/_gitlab.test.ts b/source/platforms/_tests/_gitlab.test.ts index 8295f5b15..7bcc477c6 100644 --- a/source/platforms/_tests/_gitlab.test.ts +++ b/source/platforms/_tests/_gitlab.test.ts @@ -14,9 +14,14 @@ const getUser = async (): Promise => { const dangerID = "dddanger1234" const dangerIdString = `DangerID: danger-id-${dangerID};` -function mockNote(id: number, authorId: number, body = ""): GitLabNote { +function mockNote( + id: number, + authorId: number, + body = "", + type: "DiffNote" | "DiscussionNote" | null = "DiffNote" +): GitLabNote { const author: Partial = { id: authorId } - const note: Partial = { author: author as GitLabUser, body, id } + const note: Partial = { author: author as GitLabUser, body, id, type: type } return note as GitLabNote } @@ -28,7 +33,7 @@ function mockDiscussion(id: string, notes: GitLabNote[]): 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 note3 = mockNote(1003, dangerUserId, `Main Danger comment ${dangerIdString} More text to ensure the Danger ID string can be found in the middle of a comment`, null) const note4 = mockNote(1004, 745774) const note5 = mockNote(1005, 745774) const discussion1 = mockDiscussion("aaaaffff1111", [note1, note2]) @@ -46,6 +51,9 @@ function mockApi(withExisting = false): GitLabAPI { getMergeRequestDiscussions: jest.fn(() => Promise.resolve(withExisting ? [discussion1, discussion2, discussion3] : []) ), + getMergeRequestNotes: jest.fn(() => + Promise.resolve(withExisting ? [note1, note2, note3, note4, note5] : []) + ), mergeRequestURL: baseUri, updateMergeRequestNote: jest.fn(() => Promise.resolve(newNote)), } @@ -55,14 +63,14 @@ function mockApi(withExisting = false): GitLabAPI { describe("updateOrCreateComment", () => { const comment = "my new comment" - it("create a new single comment", async () => { + it("create a new single comment not using threads", 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.getMergeRequestDiscussions).toHaveBeenCalledTimes(0) expect(api.deleteMergeRequestNote).toHaveBeenCalledTimes(0) expect(api.createMergeRequestDiscussion).toHaveBeenCalledTimes(0) expect(api.createMergeRequestNote).toHaveBeenCalledTimes(1) @@ -70,6 +78,21 @@ describe("updateOrCreateComment", () => { expect(api.updateMergeRequestNote).toHaveBeenCalledTimes(0) }) + it("create a new single comment using threads", 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("create a new thread", async () => { process.env.DANGER_GITLAB_USE_THREADS = "true" @@ -85,7 +108,24 @@ describe("updateOrCreateComment", () => { expect(api.updateMergeRequestNote).toHaveBeenCalledTimes(0) }) - it("update an existing thread", async () => { + it("update an existing main note", async () => { + delete process.env.DANGER_GITLAB_USE_THREADS + + const api = mockApi(true) + const url = await new GitLab(api as GitLabAPI).updateOrCreateComment(dangerID, comment) + expect(url).toEqual(`${baseUri}#note_4711`) + + expect(api.getMergeRequestNotes).toHaveBeenCalledTimes(1) + expect(api.deleteMergeRequestNote).toHaveBeenCalledTimes(0) + expect(api.createMergeRequestDiscussion).toHaveBeenCalledTimes(0) + expect(api.createMergeRequestNote).toHaveBeenCalledTimes(0) + expect(api.updateMergeRequestNote).toHaveBeenCalledTimes(1) + expect(api.updateMergeRequestNote).toHaveBeenCalledWith(1003, comment) + }) + + it("update an existing main thread", async () => { + process.env.DANGER_GITLAB_USE_THREADS = "true" + const api = mockApi(true) const url = await new GitLab(api as GitLabAPI).updateOrCreateComment(dangerID, comment) expect(url).toEqual(`${baseUri}#note_4711`) @@ -114,7 +154,24 @@ describe("deleteMainComment", () => { expect(api.updateMergeRequestNote).toHaveBeenCalledTimes(0) }) - it("delete all danger attached notes", async () => { + it("delete all danger attached notes not using threads", async () => { + delete process.env.DANGER_GITLAB_USE_THREADS + + const api = mockApi(true) + const result = await new GitLab(api as GitLabAPI).deleteMainComment(dangerID) + expect(result).toEqual(true) + + expect(api.getMergeRequestNotes).toHaveBeenCalledTimes(1) + expect(api.deleteMergeRequestNote).toHaveBeenCalledTimes(1) + expect(api.deleteMergeRequestNote).toHaveBeenNthCalledWith(1, 1003) + expect(api.createMergeRequestDiscussion).toHaveBeenCalledTimes(0) + expect(api.createMergeRequestNote).toHaveBeenCalledTimes(0) + expect(api.updateMergeRequestNote).toHaveBeenCalledTimes(0) + }) + + it("delete all danger attached notes using threads", async () => { + process.env.DANGER_GITLAB_USE_THREADS = "true" + const api = mockApi(true) const result = await new GitLab(api as GitLabAPI).deleteMainComment(dangerID) expect(result).toEqual(true)