From 98f0ab2a1f474960d7e8b457468fc2e420215922 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 20 Jul 2025 12:32:07 -0700 Subject: [PATCH 1/4] Fix the bug when the last code comment deleted, a review comment type comment is still in the activity list of the pull request --- models/fixtures/comment.yml | 9 +++++ models/issues/comment.go | 53 ++++++++++++++++++++++++---- routers/api/v1/repo/issue_comment.go | 2 +- routers/web/repo/issue_comment.go | 10 ++++-- services/issue/comments.go | 8 ++--- services/issue/comments_test.go | 40 +++++++++++++++++++++ services/user/delete.go | 2 +- web_src/js/features/repo-issue.ts | 7 ++++ 8 files changed, 116 insertions(+), 15 deletions(-) create mode 100644 services/issue/comments_test.go diff --git a/models/fixtures/comment.yml b/models/fixtures/comment.yml index 8fde386e226d4..7d472cdea409a 100644 --- a/models/fixtures/comment.yml +++ b/models/fixtures/comment.yml @@ -102,3 +102,12 @@ review_id: 22 assignee_id: 5 created_unix: 946684817 + +- + id: 12 + type: 22 # review + poster_id: 100 + issue_id: 3 + content: "" + review_id: 10 + created_unix: 946684812 diff --git a/models/issues/comment.go b/models/issues/comment.go index 4fdb0c1808fb4..df80b75d32bf1 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -1122,21 +1122,21 @@ func UpdateComment(ctx context.Context, c *Comment, contentVersion int, doer *us } // DeleteComment deletes the comment -func DeleteComment(ctx context.Context, comment *Comment) error { +func DeleteComment(ctx context.Context, comment *Comment) (*Comment, error) { e := db.GetEngine(ctx) if _, err := e.ID(comment.ID).NoAutoCondition().Delete(comment); err != nil { - return err + return nil, err } if _, err := db.DeleteByBean(ctx, &ContentHistory{ CommentID: comment.ID, }); err != nil { - return err + return nil, err } if comment.Type.CountedAsConversation() { if err := UpdateIssueNumComments(ctx, comment.IssueID); err != nil { - return err + return nil, err } } if _, err := e.Table("action"). @@ -1144,14 +1144,53 @@ func DeleteComment(ctx context.Context, comment *Comment) error { Update(map[string]any{ "is_deleted": true, }); err != nil { - return err + return nil, err + } + + var deletedReviewComment *Comment + // delete review & review comment if the code comment is the last comment of the review + if comment.Type == CommentTypeCode && comment.ReviewID > 0 { + if err := comment.LoadReview(ctx); err != nil { + return nil, err + } + if comment.Review != nil && comment.Review.Type == ReviewTypeComment { + res, err := db.GetEngine(ctx).ID(comment.ReviewID). + Where("NOT EXISTS (SELECT 1 FROM comment WHERE review_id = ? AND `type` = ?)", comment.ReviewID, CommentTypeCode). + Delete(new(Review)) + if err != nil { + return nil, err + } + if res > 0 { + var reviewComment Comment + has, err := db.GetEngine(ctx).Where("review_id = ?", comment.ReviewID). + And("`type` = ?", CommentTypeReview).Get(&reviewComment) + if err != nil { + return nil, err + } + if has && reviewComment.Content == "" { + if err := reviewComment.LoadAttachments(ctx); err != nil { + return nil, err + } + if len(reviewComment.Attachments) == 0 { + if _, err := db.GetEngine(ctx).ID(reviewComment.ID).Delete(new(Comment)); err != nil { + return nil, err + } + deletedReviewComment = &reviewComment + } + } + comment.ReviewID = 0 // reset review ID to 0 for the notification + } + } } if err := comment.neuterCrossReferences(ctx); err != nil { - return err + return nil, err } - return DeleteReaction(ctx, &ReactionOptions{CommentID: comment.ID}) + if err := DeleteReaction(ctx, &ReactionOptions{CommentID: comment.ID}); err != nil { + return nil, err + } + return deletedReviewComment, nil } // UpdateCommentsMigrationsByType updates comments' migrations information via given git service type and original id and poster id diff --git a/routers/api/v1/repo/issue_comment.go b/routers/api/v1/repo/issue_comment.go index cc342a9313c71..061e212a46dfa 100644 --- a/routers/api/v1/repo/issue_comment.go +++ b/routers/api/v1/repo/issue_comment.go @@ -726,7 +726,7 @@ func deleteIssueComment(ctx *context.APIContext) { return } - if err = issue_service.DeleteComment(ctx, ctx.Doer, comment); err != nil { + if _, err = issue_service.DeleteComment(ctx, ctx.Doer, comment); err != nil { ctx.APIErrorInternal(err) return } diff --git a/routers/web/repo/issue_comment.go b/routers/web/repo/issue_comment.go index cb5b2d801952d..1ad6c588a7798 100644 --- a/routers/web/repo/issue_comment.go +++ b/routers/web/repo/issue_comment.go @@ -325,12 +325,18 @@ func DeleteComment(ctx *context.Context) { return } - if err = issue_service.DeleteComment(ctx, ctx.Doer, comment); err != nil { + deletedReviewComment, err := issue_service.DeleteComment(ctx, ctx.Doer, comment) + if err != nil { ctx.ServerError("DeleteComment", err) return } - ctx.Status(http.StatusOK) + res := map[string]any{} + if deletedReviewComment != nil { + res["deletedReviewCommentHashTag"] = deletedReviewComment.HashTag() + } + + ctx.JSON(http.StatusOK, res) } // ChangeCommentReaction create a reaction for comment diff --git a/services/issue/comments.go b/services/issue/comments.go index 10c81198d57e2..74159992f4da9 100644 --- a/services/issue/comments.go +++ b/services/issue/comments.go @@ -131,17 +131,17 @@ func UpdateComment(ctx context.Context, c *issues_model.Comment, contentVersion } // DeleteComment deletes the comment -func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_model.Comment) error { - err := db.WithTx(ctx, func(ctx context.Context) error { +func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_model.Comment) (*issues_model.Comment, error) { + deletedReviewComment, err := db.WithTx2(ctx, func(ctx context.Context) (*issues_model.Comment, error) { return issues_model.DeleteComment(ctx, comment) }) if err != nil { - return err + return nil, err } notify_service.DeleteComment(ctx, doer, comment) - return nil + return deletedReviewComment, nil } // LoadCommentPushCommits Load push commits diff --git a/services/issue/comments_test.go b/services/issue/comments_test.go new file mode 100644 index 0000000000000..4088bd1020d25 --- /dev/null +++ b/services/issue/comments_test.go @@ -0,0 +1,40 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package issue + +import ( + "testing" + + "code.gitea.io/gitea/models/db" + issues_model "code.gitea.io/gitea/models/issues" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + + "github.com/stretchr/testify/assert" +) + +func Test_DeleteCommentWithReview(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 7}) + assert.NoError(t, comment.LoadAttachments(t.Context())) + assert.Len(t, comment.Attachments, 1) + assert.Equal(t, int64(13), comment.Attachments[0].ID) + assert.Equal(t, int64(10), comment.ReviewID) + review := unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: comment.ReviewID}) + user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + + // since this is the last comment of the review, it should be deleted when the comment is deleted + deletedReviewComment, err := DeleteComment(db.DefaultContext, user1, comment) + assert.NoError(t, err) + assert.NotNil(t, deletedReviewComment) + + // the review should be deleted as well + unittest.AssertNotExistsBean(t, &issues_model.Review{ID: review.ID}) + // the attachment should be deleted as well + newAttachment, err := repo_model.GetAttachmentByID(t.Context(), comment.Attachments[0].ID) + assert.Error(t, err) + assert.Nil(t, newAttachment) +} diff --git a/services/user/delete.go b/services/user/delete.go index 39c6ef052dca7..f05eb6464b43d 100644 --- a/services/user/delete.go +++ b/services/user/delete.go @@ -117,7 +117,7 @@ func deleteUser(ctx context.Context, u *user_model.User, purge bool) (err error) } for _, comment := range comments { - if err = issues_model.DeleteComment(ctx, comment); err != nil { + if _, err = issues_model.DeleteComment(ctx, comment); err != nil { return err } } diff --git a/web_src/js/features/repo-issue.ts b/web_src/js/features/repo-issue.ts index 49e8fc40a23a2..342c455bae9ad 100644 --- a/web_src/js/features/repo-issue.ts +++ b/web_src/js/features/repo-issue.ts @@ -150,6 +150,13 @@ export function initRepoIssueCommentDelete() { counter.textContent = String(num); } + const json: Record = await response.json(); + if (json.errorMessage) throw new Error(json.errorMessage); + + if (json.deletedReviewCommentHashTag) { + document.querySelector(`#${json.deletedReviewCommentHashTag}`)?.remove(); + } + document.querySelector(`#${deleteButton.getAttribute('data-comment-id')}`)?.remove(); if (conversationHolder && !conversationHolder.querySelector('.comment')) { From 98290c69e9f2a1a696ec45cdba352b57bf27c5c9 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 20 Jul 2025 12:43:05 -0700 Subject: [PATCH 2/4] Allow code comment deleted --- routers/api/v1/repo/issue_comment.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/api/v1/repo/issue_comment.go b/routers/api/v1/repo/issue_comment.go index 061e212a46dfa..feb9f1da64cfe 100644 --- a/routers/api/v1/repo/issue_comment.go +++ b/routers/api/v1/repo/issue_comment.go @@ -721,7 +721,7 @@ func deleteIssueComment(ctx *context.APIContext) { if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) { ctx.Status(http.StatusForbidden) return - } else if comment.Type != issues_model.CommentTypeComment { + } else if comment.Type != issues_model.CommentTypeComment && comment.Type != issues_model.CommentTypeCode { ctx.Status(http.StatusNoContent) return } From 840a5cadfc607f3701f895125c875da5128aa264 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 20 Jul 2025 13:15:26 -0700 Subject: [PATCH 3/4] Fix test --- models/issues/pull_list_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/models/issues/pull_list_test.go b/models/issues/pull_list_test.go index eb2de006d60a4..feb59df216045 100644 --- a/models/issues/pull_list_test.go +++ b/models/issues/pull_list_test.go @@ -39,9 +39,8 @@ func TestPullRequestList_LoadReviewCommentsCounts(t *testing.T) { reviewComments, err := prs.LoadReviewCommentsCounts(db.DefaultContext) assert.NoError(t, err) assert.Len(t, reviewComments, 2) - for _, pr := range prs { - assert.Equal(t, 1, reviewComments[pr.IssueID]) - } + assert.Equal(t, 1, reviewComments[prs[0].IssueID]) + assert.Equal(t, 2, reviewComments[prs[1].IssueID]) } func TestPullRequestList_LoadReviews(t *testing.T) { From 880192ea5d6130c0cafb2b69a8350463e8a6c010 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 1 Aug 2025 10:58:34 -0700 Subject: [PATCH 4/4] Fix test --- models/fixtures/comment.yml | 17 +++++++++++++---- models/issues/comment.go | 9 ++------- services/issue/comments_test.go | 13 +++---------- 3 files changed, 18 insertions(+), 21 deletions(-) diff --git a/models/fixtures/comment.yml b/models/fixtures/comment.yml index 7d472cdea409a..00f7f6e536653 100644 --- a/models/fixtures/comment.yml +++ b/models/fixtures/comment.yml @@ -105,9 +105,18 @@ - id: 12 - type: 22 # review - poster_id: 100 + type: 22 # review comment + poster_id: 1 issue_id: 3 content: "" - review_id: 10 - created_unix: 946684812 + review_id: 5 + created_unix: 946684810 + +- + id: 13 + type: 21 # code comment + poster_id: 1 + issue_id: 3 + content: "Some codes need to be changed" + review_id: 5 + created_unix: 946684810 diff --git a/models/issues/comment.go b/models/issues/comment.go index df80b75d32bf1..7ff4db6aaadab 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -1168,15 +1168,10 @@ func DeleteComment(ctx context.Context, comment *Comment) (*Comment, error) { return nil, err } if has && reviewComment.Content == "" { - if err := reviewComment.LoadAttachments(ctx); err != nil { + if _, err := db.GetEngine(ctx).ID(reviewComment.ID).Delete(new(Comment)); err != nil { return nil, err } - if len(reviewComment.Attachments) == 0 { - if _, err := db.GetEngine(ctx).ID(reviewComment.ID).Delete(new(Comment)); err != nil { - return nil, err - } - deletedReviewComment = &reviewComment - } + deletedReviewComment = &reviewComment } comment.ReviewID = 0 // reset review ID to 0 for the notification } diff --git a/services/issue/comments_test.go b/services/issue/comments_test.go index 4088bd1020d25..771a070330d8d 100644 --- a/services/issue/comments_test.go +++ b/services/issue/comments_test.go @@ -8,7 +8,6 @@ import ( "code.gitea.io/gitea/models/db" issues_model "code.gitea.io/gitea/models/issues" - repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" @@ -18,11 +17,8 @@ import ( func Test_DeleteCommentWithReview(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) - comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 7}) - assert.NoError(t, comment.LoadAttachments(t.Context())) - assert.Len(t, comment.Attachments, 1) - assert.Equal(t, int64(13), comment.Attachments[0].ID) - assert.Equal(t, int64(10), comment.ReviewID) + comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 13}) + assert.Equal(t, int64(5), comment.ReviewID) review := unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: comment.ReviewID}) user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) @@ -33,8 +29,5 @@ func Test_DeleteCommentWithReview(t *testing.T) { // the review should be deleted as well unittest.AssertNotExistsBean(t, &issues_model.Review{ID: review.ID}) - // the attachment should be deleted as well - newAttachment, err := repo_model.GetAttachmentByID(t.Context(), comment.Attachments[0].ID) - assert.Error(t, err) - assert.Nil(t, newAttachment) + unittest.AssertNotExistsBean(t, &issues_model.Comment{ID: deletedReviewComment.ID}) }