From 000d87efe1d99de1447631a94138995067480465 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 16 Jul 2025 13:30:55 -0700 Subject: [PATCH] Fix deleting code comment bug --- models/issues/comment.go | 44 +++++++++++++++++++++++----- models/issues/comment_test.go | 15 ++++++++++ routers/api/v1/repo/issue_comment.go | 4 +-- routers/web/repo/issue_comment.go | 10 +++++-- services/issue/comments.go | 35 ++++++++++++++++++---- services/user/delete.go | 2 +- web_src/js/features/repo-issue.ts | 6 ++++ 7 files changed, 99 insertions(+), 17 deletions(-) diff --git a/models/issues/comment.go b/models/issues/comment.go index 4fdb0c1808f..5cd05f86d12 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,44 @@ 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 { + 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 := 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/models/issues/comment_test.go b/models/issues/comment_test.go index c08e3b970d3..c014cc8a612 100644 --- a/models/issues/comment_test.go +++ b/models/issues/comment_test.go @@ -124,3 +124,18 @@ func Test_UpdateIssueNumComments(t *testing.T) { issue2 = unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2}) assert.Equal(t, 1, issue2.NumComments) } + +func Test_DeleteCommentWithReview(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 7}) + assert.Equal(t, int64(10), comment.ReviewID) + review := unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: comment.ReviewID}) + + // FIXME: the test fixtures needs a review type comment to be created + + // since this is the last comment of the review, it should be deleted when the comment is deleted + assert.NoError(t, issues_model.DeleteComment(db.DefaultContext, comment)) + + unittest.AssertNotExistsBean(t, &issues_model.Review{ID: review.ID}) +} diff --git a/routers/api/v1/repo/issue_comment.go b/routers/api/v1/repo/issue_comment.go index cc342a9313c..ab5647fe7cb 100644 --- a/routers/api/v1/repo/issue_comment.go +++ b/routers/api/v1/repo/issue_comment.go @@ -721,12 +721,12 @@ 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 } - 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 cb5b2d80195..1ad6c588a77 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 10c81198d57..8ac405cb243 100644 --- a/services/issue/comments.go +++ b/services/issue/comments.go @@ -15,6 +15,8 @@ import ( user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/json" + "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/storage" "code.gitea.io/gitea/modules/timeutil" git_service "code.gitea.io/gitea/services/git" notify_service "code.gitea.io/gitea/services/notify" @@ -131,17 +133,40 @@ 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 { - return issues_model.DeleteComment(ctx, comment) +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) { + if err := comment.LoadAttachments(ctx); err != nil { + return nil, err + } + + deletedReviewComment, err := issues_model.DeleteComment(ctx, comment) + if err != nil { + return nil, err + } + + // delete comment attachments + if _, err := repo_model.DeleteAttachments(ctx, comment.Attachments, true); err != nil { + return nil, fmt.Errorf("delete attachments: %w", err) + } + + for _, attachment := range comment.Attachments { + if err := storage.Attachments.Delete(repo_model.AttachmentRelativePath(attachment.UUID)); err != nil { + // Even delete files failed, but the attachments has been removed from database, so we + // should not return error but only record the error on logs. + // users have to delete this attachments manually or we should have a + // synchronize between database attachment table and attachment storage + log.Error("delete attachment[uuid: %s] failed: %v", attachment.UUID, err) + } + } + return deletedReviewComment, nil }) 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/user/delete.go b/services/user/delete.go index 39c6ef052dc..f05eb6464b4 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 49e8fc40a23..693ec4f6876 100644 --- a/web_src/js/features/repo-issue.ts +++ b/web_src/js/features/repo-issue.ts @@ -150,6 +150,12 @@ 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')) {