Skip to content

Fix the bug when the last code comment deleted, a review comment type comment is still in the activity list of the pull request #35133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions models/fixtures/comment.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
53 changes: 46 additions & 7 deletions models/issues/comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -1122,36 +1122,75 @@ 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").
Where("comment_id = ?", comment.ID).
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
Expand Down
5 changes: 2 additions & 3 deletions models/issues/pull_list_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions routers/api/v1/repo/issue_comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
10 changes: 8 additions & 2 deletions routers/web/repo/issue_comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions services/issue/comments.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
40 changes: 40 additions & 0 deletions services/issue/comments_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
2 changes: 1 addition & 1 deletion services/user/delete.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down
7 changes: 7 additions & 0 deletions web_src/js/features/repo-issue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,13 @@ export function initRepoIssueCommentDelete() {
counter.textContent = String(num);
}

const json: Record<string, any> = 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')) {
Expand Down
Loading