fix: GitLab Inline comments deleted with main comment updates #1382
Conversation
DavidBrunow
force-pushed
the
bugfix/gitLabInlineComments
branch
2 times, most recently
from
0f72699
to
328ab2d
Compare
| await this.deleteNotes(this.reduceNotesFromDiscussions(discussions)) //delete the rest | ||
|
|
||
| let newOrUpdatedNote: GitLabNote | ||
| if (useThreads()) { |
There was a problem hiding this comment.
The main difference in the approach in this pull request is to treat the new "create a thread for the main comment" behavior as completely separate from the previous behavior to "create a note for the main comment".
Mixing the logic of these two behaviors caused issues where certain scenarios were not handled, like trying to update the "main" Danger comment and instead updating an inline comment because it was a discussion and the "main" Danger comment was not.
| } else { | ||
| // create a new comment | ||
| newOrUpdatedNote = await this.createComment(newComment) | ||
| const notes: GitLabNote[] = await this.getMainDangerNotes(dangerID) |
There was a problem hiding this comment.
This is copied from the code that existed before the change in #1331
| const discussions = await this.getDangerDiscussions(dangerID) | ||
| return await this.deleteNotes(this.reduceNotesFromDiscussions(discussions)) | ||
| } else { | ||
| const notes = await this.getMainDangerNotes(dangerID) |
There was a problem hiding this comment.
This is copied from the code that existed before the change in #1331
DavidBrunow
force-pushed
the
bugfix/gitLabInlineComments
branch
from
328ab2d
to
5c5846a
Compare
|
@squarefrog No problem if you don't have the bandwidth, but if you do I'd appreciate you giving this branch a test. |
|
I’m off work until next week but I can give it a try then, thanks for the tag! |
|
I'm really sorry for the delay in testing this. I've been on leave from work, then we've had issues with our CI setup. I'll need to see if I can figure out how to build the binary of this branch then try it. |
Not a problem, life is busy and I keep getting distracted from this as well :) |
|
The good news is I figured out how to create a build of this and put it on our CI and it seems to work really well. Great job with this @DavidBrunow 🙌 |
|
@orta Do you want to review or do you recommend any other reviewers before merging this? |
|
We are facing the same problem here. @orta it would be great if you could approve this merge. |
|
Yeah, is @squarefrog is happy, I am happy |
|
Shipped |
Fixes #1351 and possibly #1380.
I verified this fix with an inline comment and a main Danger note with
DANGER_GITLAB_USE_THREADSboth set and unset. With it unset all behavior looked good to me.With that flag set, I am seeing an issue where the main Danger note was replacing the inline comment. Since that is not the intended behavior I tested the latest release with that flag set and also saw the same bad behavior there. Since that behavior is consistent I'd like to move forward with this pull request without trying to fix that behavior.