diff --git a/CHANGELOG.md b/CHANGELOG.md index 98ae97f01..e1119ce7e 100644 --- a/CHANGELOG.md +++ b/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] ## 11.1.2 diff --git a/docs/usage/gitlab.html.md b/docs/usage/gitlab.html.md index cfb95fb96..4409ae5ba 100644 --- a/docs/usage/gitlab.html.md +++ b/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`. diff --git a/source/danger.d.ts b/source/danger.d.ts index 31a5b8fa5..22891238a 100644 --- a/source/danger.d.ts +++ b/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 diff --git a/source/dsl/GitLabDSL.ts b/source/dsl/GitLabDSL.ts index 6f056e05c..cb264c1b8 100644 --- a/source/dsl/GitLabDSL.ts +++ b/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 diff --git a/source/platforms/GitLab.ts b/source/platforms/GitLab.ts index 90d776228..21316a582 100644 --- a/source/platforms/GitLab.ts +++ b/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 => { 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 => { + createComment = async (comment: string): Promise => { 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 => { + createInlineComment = async (git: GitDSL, comment: string, path: string, line: number): Promise => { 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 => { - 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 => { + 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 => { + 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((acc, { notes }) => [...acc, ...notes], []) + } + getDangerNotes = async (dangerID: string): Promise => { - 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 => { diff --git a/source/platforms/_tests/_gitlab.test.ts b/source/platforms/_tests/_gitlab.test.ts new file mode 100644 index 000000000..e5892ee2f --- /dev/null +++ b/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 => { + const user: Partial = { 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 = { id: authorId } + const note: Partial = { author: author as GitLabUser, body, id } + return note as GitLabNote +} + +function mockDiscussion(id: string, notes: GitLabNote[]): GitLabDiscussion { + const discussion: Partial = { 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 = { + 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) + }) +}) diff --git a/source/platforms/gitlab/GitLabAPI.ts b/source/platforms/gitlab/GitLabAPI.ts index acf8c4bd2..f070028c4 100644 --- a/source/platforms/gitlab/GitLabAPI.ts +++ b/source/platforms/gitlab/GitLabAPI.ts @@ -1,17 +1,17 @@ import { RepoMetaData } from "../../dsl/BitBucketServerDSL" import { - GitLabDiscussionTextPosition, + GitLabApproval, + GitLabDiscussion, + GitLabDiscussionCreationOptions, GitLabInlineNote, GitLabMR, GitLabMRChange, GitLabMRChanges, GitLabMRCommit, GitLabNote, - GitLabUserProfile, GitLabRepositoryCompare, - GitLabApproval, + GitLabUserProfile, } from "../../dsl/GitLabDSL" - import { Gitlab } from "@gitbeaker/node" import { RepositoryFileSchema } from "@gitbeaker/core/dist/types/services/RepositoryFiles" import { Env } from "../../ci_source/ci_source" @@ -19,6 +19,7 @@ import { debug } from "../../debug" export type GitLabAPIToken = string export type GitLabOAuthToken = string + export interface GitLabAPICredentials { host: string token?: GitLabAPIToken @@ -36,8 +37,8 @@ export function getGitLabAPICredentialsFromEnv(env: Env): GitLabAPICredentials { const protocolRegex = /^https?:\/\//i host = protocolRegex.test(envHost) ? envHost : `https://${envHost}` } else if (envCIAPI) { - // GitLab >= v11.7 supplies the API Endpoint in an environment variable and we can work out our host value from that. - // See https://docs.gitlab.com/ce/ci/variables/predefined_variables.html + // GitLab >= v11.7 supplies the API Endpoint in an environment variable and we can work out our host value from + // that. See https://docs.gitlab.com/ce/ci/variables/predefined_variables.html const hostRegex = /^(https?):\/\/([^\/]+)\//i if (hostRegex.test(envCIAPI)) { const matches = hostRegex.exec(envCIAPI)! @@ -123,6 +124,14 @@ class GitLabAPI { return commits } + getMergeRequestDiscussions = async (): Promise => { + this.d("getMergeRequestDiscussions", this.repoSlug, this.prId) + const api = this.api.MergeRequestDiscussions + const discussions = (await api.all(this.repoSlug, this.prId, {})) as GitLabDiscussion[] + this.d("getMergeRequestDiscussions", discussions) + return discussions + } + getMergeRequestNotes = async (): Promise => { this.d("getMergeRequestNotes", this.repoSlug, this.prId) const api = this.api.MergeRequestNotes @@ -139,15 +148,16 @@ class GitLabAPI { return inlineNotes } - createMergeRequestDiscussion = async (content: string, position: GitLabDiscussionTextPosition): Promise => { - this.d("createMergeRequestDiscussion", this.repoSlug, this.prId, content, position) + createMergeRequestDiscussion = async ( + content: string, + options?: GitLabDiscussionCreationOptions, + ): Promise => { + this.d("createMergeRequestDiscussion", this.repoSlug, this.prId, content, options) const api = this.api.MergeRequestDiscussions try { - const result = await api.create(this.repoSlug, this.prId, content, { - position: position, - }) + const result = await api.create(this.repoSlug, this.prId, content, options) as GitLabDiscussion this.d("createMergeRequestDiscussion", result) - return result as unknown as string // not sure why? + return result } catch (e) { this.d("createMergeRequestDiscussion", e) throw e diff --git a/source/platforms/gitlab/_tests/_gitlab_api.test.ts b/source/platforms/gitlab/_tests/_gitlab_api.test.ts index 467b8415a..8ab6d4cd5 100644 --- a/source/platforms/gitlab/_tests/_gitlab_api.test.ts +++ b/source/platforms/gitlab/_tests/_gitlab_api.test.ts @@ -132,6 +132,14 @@ describe("GitLab API", () => { expect(result).toEqual(response) }) + it("getMergeRequestDiscussions", async () => { + const { nockDone } = await nockBack("getMergeRequestDiscussions.json") + const result = await api.getMergeRequestDiscussions() + nockDone() + const { response } = loadFixture("getMergeRequestDiscussions") + expect(result).toEqual(response) + }) + it("getMergeRequestNotes", async () => { const { nockDone } = await nockBack("getMergeRequestNotes.json") const result = await api.getMergeRequestNotes() diff --git a/source/platforms/gitlab/_tests/fixtures/getMergeRequestDiscussions.json b/source/platforms/gitlab/_tests/fixtures/getMergeRequestDiscussions.json new file mode 100644 index 000000000..b5ffdc6d7 --- /dev/null +++ b/source/platforms/gitlab/_tests/fixtures/getMergeRequestDiscussions.json @@ -0,0 +1,92 @@ +[ + { + "scope": "https://gitlab.com:443", + "method": "GET", + "path": "/api/v4/projects/gitlab-org%2Fgitlab-foss/merge_requests/27117/discussions", + "body": "", + "status": 200, + "response": [ + { + "id": "6fe51a9c5758556dfef42725a66dee2729a6fb82", + "individual_note": false, + "notes": [ + { + "id": 166211215, + "type": null, + "body": "mentioned in issue gitlab-org/release/tasks#778", + "attachment": null, + "author": { + "id": 1786152, + "name": "🤖 GitLab Bot 🤖", + "username": "gitlab-bot", + "state": "active", + "avatar_url": "https://gl-canary.freetls.fastly.net/uploads/-/system/user/avatar/1786152/avatar.png", + "web_url": "https://gitlab.com/gitlab-bot" + }, + "created_at": "2019-05-02T14:34:54.054Z", + "updated_at": "2019-05-02T14:34:54.054Z", + "system": true, + "noteable_id": 27253868, + "noteable_type": "MergeRequest", + "resolvable": false, + "noteable_iid": 27117 + } + ] + } + ], + "rawHeaders": [ + "Server", + "nginx", + "Date", + "Mon, 20 May 2019 11:19:03 GMT", + "Content-Type", + "application/json", + "Content-Length", + "9682", + "Connection", + "close", + "Cache-Control", + "max-age=0, private, must-revalidate", + "Etag", + "W/\"edab8aad8eea37dd376785c34c7c250f\"", + "Link", + "; rel=\"first\", ; rel=\"last\"", + "Vary", + "Origin", + "X-Content-Type-Options", + "nosniff", + "X-Frame-Options", + "SAMEORIGIN", + "X-Next-Page", + "", + "X-Page", + "1", + "X-Per-Page", + "20", + "X-Prev-Page", + "", + "X-Request-Id", + "yd9SuXYdPCa", + "X-Runtime", + "1.305536", + "X-Total", + "15", + "X-Total-Pages", + "1", + "Strict-Transport-Security", + "max-age=31536000", + "Referrer-Policy", + "strict-origin-when-cross-origin", + "RateLimit-Limit", + "600", + "RateLimit-Observed", + "1", + "RateLimit-Remaining", + "599", + "RateLimit-Reset", + "1558351203", + "RateLimit-ResetTime", + "Mon, 20 May 2019 11:20:03 GMT" + ] + } +]