Update milestone counters when issue is deleted (#21459)

When actions besides "delete" are performed on issues, the milestone
counter is updated. However, since deleting issues goes through a
different code path, the associated milestone's count wasn't being
updated, resulting in inaccurate counts until another issue in the same
milestone had a non-delete action performed on it.

I verified this change fixes the inaccurate counts using a local docker
build.

Fixes #21254

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

models/migrations/fixtures/Test_updateOpenMilestoneCounts/expected_milestone.yml

@@ -0,0 +1,19 @@
+# type Milestone struct {
+#   ID              int64                  `xorm:"pk autoincr"`
+#   IsClosed        bool
+#   NumIssues       int
+#   NumClosedIssues int
+#   Completeness    int  // Percentage(1-100).
+# }
+-
+  id: 1
+  is_closed: false
+  num_issues: 3
+  num_closed_issues: 1
+  completeness: 33
+-
+  id: 2
+  is_closed: true
+  num_issues: 5
+  num_closed_issues: 5
+  completeness: 100

models/migrations/fixtures/Test_updateOpenMilestoneCounts/issue.yml

@@ -0,0 +1,25 @@
+# type Issue struct {
+#   ID               int64 `xorm:"pk autoincr"`
+#   RepoID           int64 `xorm:"INDEX UNIQUE(repo_index)"`
+#   Index            int64 `xorm:"UNIQUE(repo_index)"` // Index in one repository.
+#   MilestoneID      int64 `xorm:"INDEX"`
+#   IsClosed         bool  `xorm:"INDEX"`
+# }
+-
+  id: 1
+  repo_id: 1
+  index: 1
+  milestone_id: 1
+  is_closed: false
+-
+  id: 2
+  repo_id: 1
+  index: 2
+  milestone_id: 1
+  is_closed: true
+-
+  id: 4
+  repo_id: 1
+  index: 3
+  milestone_id: 1
+  is_closed: false

models/migrations/fixtures/Test_updateOpenMilestoneCounts/milestone.yml

@@ -0,0 +1,19 @@
+# type Milestone struct {
+#   ID              int64                  `xorm:"pk autoincr"`
+#   IsClosed        bool
+#   NumIssues       int
+#   NumClosedIssues int
+#   Completeness    int  // Percentage(1-100).
+# }
+-
+  id: 1
+  is_closed: false
+  num_issues: 4
+  num_closed_issues: 2
+  completeness: 50
+-
+  id: 2
+  is_closed: true
+  num_issues: 5
+  num_closed_issues: 5
+  completeness: 100

models/migrations/migrations.go

@@ -419,6 +419,8 @@ var migrations = []Migration{
 	NewMigration("Create key/value table for system settings", createSystemSettingsTable),
 	// v228 -> v229
 	NewMigration("Add TeamInvite table", addTeamInviteTable),
+	// v229 -> v230
+	NewMigration("Update counts of all open milestones", updateOpenMilestoneCounts),
 }
 
 // GetCurrentDBVersion returns the current db version

models/migrations/v229.go

@@ -0,0 +1,47 @@
+// Copyright 2022 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package migrations
+
+import (
+	"fmt"
+
+	"code.gitea.io/gitea/models/issues"
+
+	"xorm.io/builder"
+	"xorm.io/xorm"
+)
+
+func updateOpenMilestoneCounts(x *xorm.Engine) error {
+	var openMilestoneIDs []int64
+	err := x.Table("milestone").Select("id").Where(builder.Neq{"is_closed": 1}).Find(&openMilestoneIDs)
+	if err != nil {
+		return fmt.Errorf("error selecting open milestone IDs: %w", err)
+	}
+
+	for _, id := range openMilestoneIDs {
+		_, err := x.ID(id).
+			SetExpr("num_issues", builder.Select("count(*)").From("issue").Where(
+				builder.Eq{"milestone_id": id},
+			)).
+			SetExpr("num_closed_issues", builder.Select("count(*)").From("issue").Where(
+				builder.Eq{
+					"milestone_id": id,
+					"is_closed":    true,
+				},
+			)).
+			Update(&issues.Milestone{})
+		if err != nil {
+			return fmt.Errorf("error updating issue counts in milestone %d: %w", id, err)
+		}
+		_, err = x.Exec("UPDATE `milestone` SET completeness=100*num_closed_issues/(CASE WHEN num_issues > 0 THEN num_issues ELSE 1 END) WHERE id=?",
+			id,
+		)
+		if err != nil {
+			return fmt.Errorf("error setting completeness on milestone %d: %w", id, err)
+		}
+	}
+
+	return nil
+}

models/migrations/v229_test.go

@@ -0,0 +1,46 @@
+// Copyright 2022 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package migrations
+
+import (
+	"testing"
+
+	"code.gitea.io/gitea/models/issues"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_updateOpenMilestoneCounts(t *testing.T) {
+	type ExpectedMilestone issues.Milestone
+
+	// Prepare and load the testing database
+	x, deferable := prepareTestEnv(t, 0, new(issues.Milestone), new(ExpectedMilestone), new(issues.Issue))
+	defer deferable()
+	if x == nil || t.Failed() {
+		return
+	}
+
+	if err := updateOpenMilestoneCounts(x); err != nil {
+		assert.NoError(t, err)
+		return
+	}
+
+	expected := []ExpectedMilestone{}
+	if err := x.Table("expected_milestone").Asc("id").Find(&expected); !assert.NoError(t, err) {
+		return
+	}
+
+	got := []issues.Milestone{}
+	if err := x.Table("milestone").Asc("id").Find(&got); !assert.NoError(t, err) {
+		return
+	}
+
+	for i, e := range expected {
+		got := got[i]
+		assert.Equal(t, e.ID, got.ID)
+		assert.Equal(t, e.NumIssues, got.NumIssues)
+		assert.Equal(t, e.NumClosedIssues, got.NumClosedIssues)
+	}
+}

services/issue/issue.go

@@ -224,6 +224,11 @@ func deleteIssue(issue *issues_model.Issue) error {
 		return err
 	}
 
+	if err := issues_model.UpdateMilestoneCounters(ctx, issue.MilestoneID); err != nil {
+		return fmt.Errorf("error updating counters for milestone id %d: %w",
+			issue.MilestoneID, err)
+	}
+
 	if err := activities_model.DeleteIssueActions(ctx, issue.RepoID, issue.ID); err != nil {
 		return err
 	}