Skip to content

Commit

Permalink
Fix issue comment number (#30556) (#33055)
Browse files Browse the repository at this point in the history 
Backport #30556 by @lunny

Fix #22419

Only comments with types `CommentTypeComment` and `CommentTypeReview`
will be counted as conversations of the pull request.
`CommentTypeReview` was missed in the previous implementation.

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
  • Loading branch information
@GiteaBot @lunny
GiteaBot and lunny authored Dec 30, 2024
1 parent 27de603 commit 968c04c
Show file tree
Hide file tree
Showing 5 changed files with 89 additions and 19 deletions.
38 changes: 33 additions & 5 deletions models/issues/comment.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,20 @@ func (t CommentType) HasMailReplySupport() bool {
return false
}

func (t CommentType) CountedAsConversation() bool {
for _, ct := range ConversationCountedCommentType() {
if t == ct {
return true
}
}
return false
}

// ConversationCountedCommentType returns the comment types that are counted as a conversation
func ConversationCountedCommentType() []CommentType {
return []CommentType{CommentTypeComment, CommentTypeReview}
}

// RoleInRepo presents the user's participation in the repo
type RoleInRepo string

Expand Down Expand Up @@ -893,7 +907,7 @@ func updateCommentInfos(ctx context.Context, opts *CreateCommentOptions, comment
}
fallthrough
case CommentTypeComment:
if _, err = db.Exec(ctx, "UPDATE `issue` SET num_comments=num_comments+1 WHERE id=?", opts.Issue.ID); err != nil {
if err := UpdateIssueNumComments(ctx, opts.Issue.ID); err != nil {
return err
}
fallthrough
Expand Down Expand Up @@ -1182,8 +1196,8 @@ func DeleteComment(ctx context.Context, comment *Comment) error {
return err
}

if comment.Type == CommentTypeComment {
if _, err := e.ID(comment.IssueID).Decr("num_comments").Update(new(Issue)); err != nil {
if comment.Type.CountedAsConversation() {
if err := UpdateIssueNumComments(ctx, comment.IssueID); err != nil {
return err
}
}
Expand Down Expand Up @@ -1300,6 +1314,21 @@ func (c *Comment) HasOriginalAuthor() bool {
return c.OriginalAuthor != "" && c.OriginalAuthorID != 0
}

func UpdateIssueNumCommentsBuilder(issueID int64) *builder.Builder {
subQuery := builder.Select("COUNT(*)").From("`comment`").Where(
builder.Eq{"issue_id": issueID}.And(
builder.In("`type`", ConversationCountedCommentType()),
))

return builder.Update(builder.Eq{"num_comments": subQuery}).
From("`issue`").Where(builder.Eq{"id": issueID})
}

func UpdateIssueNumComments(ctx context.Context, issueID int64) error {
_, err := db.GetEngine(ctx).Exec(UpdateIssueNumCommentsBuilder(issueID))
return err
}

// InsertIssueComments inserts many comments of issues.
func InsertIssueComments(ctx context.Context, comments []*Comment) error {
if len(comments) == 0 {
Expand Down Expand Up @@ -1332,8 +1361,7 @@ func InsertIssueComments(ctx context.Context, comments []*Comment) error {
}

for _, issueID := range issueIDs {
if _, err := db.Exec(ctx, "UPDATE issue set num_comments = (SELECT count(*) FROM comment WHERE issue_id = ? AND `type`=?) WHERE id = ?",
issueID, CommentTypeComment, issueID); err != nil {
if err := UpdateIssueNumComments(ctx, issueID); err != nil {
return err
}
}
Expand Down
9 changes: 9 additions & 0 deletions models/issues/comment_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,3 +97,12 @@ func TestMigrate_InsertIssueComments(t *testing.T) {

unittest.CheckConsistencyFor(t, &issues_model.Issue{})
}

func Test_UpdateIssueNumComments(t *testing.T) {
assert.NoError(t, unittest.PrepareTestDatabase())
issue2 := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2})

assert.NoError(t, issues_model.UpdateIssueNumComments(db.DefaultContext, issue2.ID))
issue2 = unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2})
assert.EqualValues(t, 1, issue2.NumComments)
}
4 changes: 4 additions & 0 deletions models/issues/review.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -639,6 +639,10 @@ func InsertReviews(ctx context.Context, reviews []*Review) error {
return err
}
}

if err := UpdateIssueNumComments(ctx, review.IssueID); err != nil {
return err
}
}

return committer.Commit()
Expand Down
43 changes: 29 additions & 14 deletions models/repo.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import (
"code.gitea.io/gitea/models/unit"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/log"

"xorm.io/builder"
)

// Init initialize model
Expand All @@ -27,7 +29,7 @@ func Init(ctx context.Context) error {
}

type repoChecker struct {
querySQL func(ctx context.Context) ([]map[string][]byte, error)
querySQL func(ctx context.Context) ([]int64, error)
correctSQL func(ctx context.Context, id int64) error
desc string
}
Expand All @@ -38,8 +40,7 @@ func repoStatsCheck(ctx context.Context, checker *repoChecker) {
log.Error("Select %s: %v", checker.desc, err)
return
}
for _, result := range results {
id, _ := strconv.ParseInt(string(result["id"]), 10, 64)
for _, id := range results {
select {
case <-ctx.Done():
log.Warn("CheckRepoStats: Cancelled before checking %s for with id=%d", checker.desc, id)
Expand All @@ -54,21 +55,23 @@ func repoStatsCheck(ctx context.Context, checker *repoChecker) {
}
}

func StatsCorrectSQL(ctx context.Context, sql string, id int64) error {
_, err := db.GetEngine(ctx).Exec(sql, id, id)
func StatsCorrectSQL(ctx context.Context, sql any, ids ...any) error {
args := []any{sql}
args = append(args, ids...)
_, err := db.GetEngine(ctx).Exec(args...)
return err
}

func repoStatsCorrectNumWatches(ctx context.Context, id int64) error {
return StatsCorrectSQL(ctx, "UPDATE `repository` SET num_watches=(SELECT COUNT(*) FROM `watch` WHERE repo_id=? AND mode<>2) WHERE id=?", id)
return StatsCorrectSQL(ctx, "UPDATE `repository` SET num_watches=(SELECT COUNT(*) FROM `watch` WHERE repo_id=? AND mode<>2) WHERE id=?", id, id)
}

func repoStatsCorrectNumStars(ctx context.Context, id int64) error {
return StatsCorrectSQL(ctx, "UPDATE `repository` SET num_stars=(SELECT COUNT(*) FROM `star` WHERE repo_id=?) WHERE id=?", id)
return StatsCorrectSQL(ctx, "UPDATE `repository` SET num_stars=(SELECT COUNT(*) FROM `star` WHERE repo_id=?) WHERE id=?", id, id)
}

func labelStatsCorrectNumIssues(ctx context.Context, id int64) error {
return StatsCorrectSQL(ctx, "UPDATE `label` SET num_issues=(SELECT COUNT(*) FROM `issue_label` WHERE label_id=?) WHERE id=?", id)
return StatsCorrectSQL(ctx, "UPDATE `label` SET num_issues=(SELECT COUNT(*) FROM `issue_label` WHERE label_id=?) WHERE id=?", id, id)
}

func labelStatsCorrectNumIssuesRepo(ctx context.Context, id int64) error {
Expand Down Expand Up @@ -105,11 +108,11 @@ func milestoneStatsCorrectNumIssuesRepo(ctx context.Context, id int64) error {
}

func userStatsCorrectNumRepos(ctx context.Context, id int64) error {
return StatsCorrectSQL(ctx, "UPDATE `user` SET num_repos=(SELECT COUNT(*) FROM `repository` WHERE owner_id=?) WHERE id=?", id)
return StatsCorrectSQL(ctx, "UPDATE `user` SET num_repos=(SELECT COUNT(*) FROM `repository` WHERE owner_id=?) WHERE id=?", id, id)
}

func repoStatsCorrectIssueNumComments(ctx context.Context, id int64) error {
return StatsCorrectSQL(ctx, "UPDATE `issue` SET num_comments=(SELECT COUNT(*) FROM `comment` WHERE issue_id=? AND type=0) WHERE id=?", id)
return StatsCorrectSQL(ctx, issues_model.UpdateIssueNumCommentsBuilder(id))
}

func repoStatsCorrectNumIssues(ctx context.Context, id int64) error {
Expand All @@ -128,9 +131,12 @@ func repoStatsCorrectNumClosedPulls(ctx context.Context, id int64) error {
return repo_model.UpdateRepoIssueNumbers(ctx, id, true, true)
}

func statsQuery(args ...any) func(context.Context) ([]map[string][]byte, error) {
return func(ctx context.Context) ([]map[string][]byte, error) {
return db.GetEngine(ctx).Query(args...)
// statsQuery returns a function that queries the database for a list of IDs
// sql could be a string or a *builder.Builder
func statsQuery(sql any, args ...any) func(context.Context) ([]int64, error) {
return func(ctx context.Context) ([]int64, error) {
var ids []int64
return ids, db.GetEngine(ctx).SQL(sql, args...).Find(&ids)
}
}

Expand Down Expand Up @@ -201,7 +207,16 @@ func CheckRepoStats(ctx context.Context) error {
},
// Issue.NumComments
{
statsQuery("SELECT `issue`.id FROM `issue` WHERE `issue`.num_comments!=(SELECT COUNT(*) FROM `comment` WHERE issue_id=`issue`.id AND type=0)"),
statsQuery(builder.Select("`issue`.id").From("`issue`").Where(
builder.Neq{
"`issue`.num_comments": builder.Select("COUNT(*)").From("`comment`").Where(
builder.Expr("issue_id = `issue`.id").And(
builder.In("type", issues_model.ConversationCountedCommentType()),
),
),
},
),
),
repoStatsCorrectIssueNumComments,
"issue count 'num_comments'",
},
Expand Down
14 changes: 14 additions & 0 deletions models/repo_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"testing"

"code.gitea.io/gitea/models/db"
issues_model "code.gitea.io/gitea/models/issues"
"code.gitea.io/gitea/models/unittest"

"github.com/stretchr/testify/assert"
Expand All @@ -22,3 +23,16 @@ func TestDoctorUserStarNum(t *testing.T) {

assert.NoError(t, DoctorUserStarNum(db.DefaultContext))
}

func Test_repoStatsCorrectIssueNumComments(t *testing.T) {
assert.NoError(t, unittest.PrepareTestDatabase())

issue2 := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2})
assert.NotNil(t, issue2)
assert.EqualValues(t, 0, issue2.NumComments) // the fixture data is wrong, but we don't fix it here

assert.NoError(t, repoStatsCorrectIssueNumComments(db.DefaultContext, 2))
// reload the issue
issue2 = unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2})
assert.EqualValues(t, 1, issue2.NumComments)
}

0 comments on commit 968c04c

Please sign in to comment.