[GitLab] Do not delete system resolved danger inline comments

Removing system resolved danger discussions on old versions of the diff can result in a state where the system note becomes the first note on the discussion resulting in poor change history on the MR.

To remedy this we look for system notes on resolved danger discussions and abort deletion.

This could be considered a brittle check, as there is no data to check to see if the system note did resolve it. However currently I am unaware of any other events triggering system notes on a discussion itself.
As long as that assumption holds true the check should work as intended and not delete system resolved discussions.

CHANGELOG.md

@@ -20,6 +20,7 @@
 - GitLab: [#1386] Move from `@gitbeaker/node` to `@gitbeaker/rest` [@heltoft]
 - GitLab: [#1412] Danger fails to create inline comments on Gitlab [@heltoft]
 - GitLab: [#1405] Can't post multiple inline comments [@heltoft]
+- GitLab: Do not delete system resolved danger inline comments [@heltoft]
 <!-- Your comment above this -->
 
 ## 11.3.1

source/platforms/GitLab.ts

@@ -84,6 +84,12 @@ class GitLab implements Platform {
     const dangerUserID = (await this.api.getUser()).id
 
     let dangerDiscussions = await this.getDangerDiscussions(dangerID)
+    // Remove system resolved danger discussions to not end up deleting danger inline comments
+    // on old versions of the diff. This is preferred as otherwise the discussion ends up in a state where
+    // the auto resolve system note can become the first note on the discussion resulting in poor change history on the MR.
+    dangerDiscussions = dangerDiscussions.filter((discussion) => !this.isDangerDiscussionSystemResolved(discussion))
+    d("getInlineComments found danger discussions", dangerDiscussions)
+
     const diffNotes = this.getDiffNotesFromDiscussions(dangerDiscussions)
     return diffNotes.map((note) => {
       return {
@@ -259,6 +265,23 @@ class GitLab implements Platform {
     return discussions.filter(({ notes }) => notes && notes.length && noteFilter(notes[0]))
   }
 
+  isDangerDiscussionSystemResolved = (discussion: Types.DiscussionSchema): boolean => {
+    const notes = discussion.notes
+    if (!notes) {
+      return false
+    }
+
+    const dangerNote = notes[0]
+    if (!dangerNote || !dangerNote.resolved) {
+      return false
+    }
+
+    // Check for a system note that resolved it
+    return notes.some((note) => {
+      return note.system === true
+    })
+  }
+
   getDiffNotesFromDiscussions = (discussions: Types.DiscussionSchema[]): Types.DiscussionNoteSchema[] => {
     const diffNotes = discussions.map((discussion) => {
       const note = discussion.notes?.[0]

source/platforms/_tests/_gitlab.test.ts

@@ -18,25 +18,47 @@ function mockNote(
   id: number,
   authorId: number,
   body = "",
+  resolved = false,
   type: "DiffNote" | "DiscussionNote" | null = "DiffNote"
 ): Types.MergeRequestNoteSchema {
   const author: Partial<Types.UserSchema> = { id: authorId }
-  const note: Partial<Types.MergeRequestNoteSchema> = { author: author as Types.UserSchema, body, id, type: type }
+  const note: Partial<Types.MergeRequestNoteSchema> = {
+    author: author as Types.UserSchema,
+    body,
+    id,
+    type: type,
+    resolved: resolved,
+  }
   return note as Types.MergeRequestNoteSchema
 }
 
+function mockSystemAutoresolveNote(id: number, authorId: number): Types.MergeRequestNoteSchema {
+  let autoresolvedSystemNote = mockNote(
+    id,
+    authorId,
+    "changed this line in [version 2 of the diff](/some/url/merge_requests/2322/diffs?diff_id=12345&start_sha=31c4ab)"
+  )
+  autoresolvedSystemNote.system = true
+  return autoresolvedSystemNote
+}
+
+function mockDangerNote(id: number, resolved = false): Types.MergeRequestNoteSchema {
+  return mockNote(id, dangerUserId, `some body ${dangerIdString} asdf`, resolved)
+}
+
 function mockDiscussion(id: string, notes: Types.DiscussionNoteSchema[]): Types.DiscussionSchema {
   const discussion: Partial<Types.DiscussionSchema> = { id, notes }
   return discussion as Types.DiscussionSchema
 }
 
 function mockApi(withExisting = false): GitLabAPI {
-  const note1 = mockNote(1001, dangerUserId, `some body ${dangerIdString} asdf`)
+  const note1 = mockDangerNote(1001)
   const note2 = mockNote(1002, 125235)
   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`,
+    false,
     null
   )
   const note4 = mockNote(1004, 745774)
@@ -190,3 +212,33 @@ describe("deleteMainComment", () => {
     expect(api.updateMergeRequestNote).toHaveBeenCalledTimes(0)
   })
 })
+
+describe("getInlineComments", () => {
+  it("getInlineComments returns not autoresolved danger discussions", async () => {
+    const autoresolvedDangerNote = mockDangerNote(2000, true)
+    const autoresolveSystemNote1 = mockSystemAutoresolveNote(2001, 12345)
+    const autoresolvedDiscussion = mockDiscussion("autoresolvedDiscussion", [
+      autoresolvedDangerNote,
+      autoresolveSystemNote1,
+    ])
+
+    const notAutoresolvedDangerNote = mockDangerNote(2001)
+    const autoresolveSystemNote2 = mockSystemAutoresolveNote(2002, 12345)
+    const notAutoresolvedDiscussion = mockDiscussion("notAutoresolvedDiscussion", [
+      notAutoresolvedDangerNote,
+      autoresolveSystemNote2,
+    ])
+
+    const notDangerNote = mockNote(2002, 12345)
+    const notDangerDiscussionNote = mockNote(2003, 23456, "", false, "DiscussionNote")
+    const notDangerDiscussion = mockDiscussion("notDangerDiscussion", [notDangerNote, notDangerDiscussionNote])
+
+    const api = mockApi()
+    api.getMergeRequestDiscussions = jest.fn(() =>
+      Promise.resolve([autoresolvedDiscussion, notAutoresolvedDiscussion, notDangerDiscussion])
+    )
+    const result = await new GitLab(api as GitLabAPI).getInlineComments(dangerID)
+    expect(result).toMatchObject([{ id: "2001" }])
+    expect(api.getMergeRequestDiscussions).toHaveBeenCalledTimes(1)
+  })
+})