Fix milestone num_issues (#8221)

* fix milestone num_issues

* update missing completeness

* only update milestone closed number when closed issue is assigned a new milestone or clear milestone

* fix tests

* fix update milestone num

* fix completeness calculate

* make completeness calucation more clear

3 files changed, 44 insertions, 41 deletions

diff --git a/models/issue.go b/models/issue.go
index 9590bc04ff..e4cc1291c2 100644
--- a/models/issue.go
+++ b/models/issue.go
@@ -766,7 +766,7 @@ func (issue *Issue) changeStatus(e *xorm.Session, doer *User, isClosed bool) (er
 	}
 
 	// Update issue count of milestone
-	if err = changeMilestoneIssueStats(e, issue); err != nil {
+	if err := updateMilestoneClosedNum(e, issue.MilestoneID); err != nil {
 		return err
 	}
 
@@ -1119,7 +1119,7 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) {
 	opts.Issue.Index = inserted.Index
 
 	if opts.Issue.MilestoneID > 0 {
-		if err = changeMilestoneAssign(e, doer, opts.Issue, -1); err != nil {
+		if _, err = e.Exec("UPDATE `milestone` SET num_issues=num_issues+1 WHERE id=?", opts.Issue.MilestoneID); err != nil {
 			return err
 		}
 	}
diff --git a/models/issue_milestone.go b/models/issue_milestone.go
index f8f414e716..29e13689bf 100644
--- a/models/issue_milestone.go
+++ b/models/issue_milestone.go
@@ -311,71 +311,74 @@ func ChangeMilestoneStatus(m *Milestone, isClosed bool) (err error) {
 	return sess.Commit()
 }
 
-func changeMilestoneIssueStats(e *xorm.Session, issue *Issue) error {
-	if issue.MilestoneID == 0 {
-		return nil
+func updateMilestoneTotalNum(e Engine, milestoneID int64) (err error) {
+	if _, err = e.Exec("UPDATE `milestone` SET num_issues=(SELECT count(*) FROM issue WHERE milestone_id=?) WHERE id=?",
+		milestoneID,
+		milestoneID,
+	); err != nil {
+		return
 	}
 
-	m, err := getMilestoneByRepoID(e, issue.RepoID, issue.MilestoneID)
-	if err != nil {
-		return err
-	}
+	_, err = e.Exec("UPDATE `milestone` SET completeness=100*num_closed_issues/(CASE WHEN num_issues > 0 THEN num_issues ELSE 1 END) WHERE id=?",
+		milestoneID,
+	)
 
-	if issue.IsClosed {
-		m.NumOpenIssues--
-		m.NumClosedIssues++
-	} else {
-		m.NumOpenIssues++
-		m.NumClosedIssues--
+	return
+}
+
+func updateMilestoneClosedNum(e Engine, milestoneID int64) (err error) {
+	if _, err = e.Exec("UPDATE `milestone` SET num_closed_issues=(SELECT count(*) FROM issue WHERE milestone_id=? AND is_closed=?) WHERE id=?",
+		milestoneID,
+		true,
+		milestoneID,
+	); err != nil {
+		return
 	}
 
-	return updateMilestone(e, m)
+	_, err = e.Exec("UPDATE `milestone` SET completeness=100*num_closed_issues/(CASE WHEN num_issues > 0 THEN num_issues ELSE 1 END) WHERE id=?",
+		milestoneID,
+	)
+	return
 }
 
 func changeMilestoneAssign(e *xorm.Session, doer *User, issue *Issue, oldMilestoneID int64) error {
+	if err := updateIssueCols(e, issue, "milestone_id"); err != nil {
+		return err
+	}
+
 	if oldMilestoneID > 0 {
-		m, err := getMilestoneByRepoID(e, issue.RepoID, oldMilestoneID)
-		if err != nil {
+		if err := updateMilestoneTotalNum(e, oldMilestoneID); err != nil {
 			return err
 		}
-
-		m.NumIssues--
 		if issue.IsClosed {
-			m.NumClosedIssues--
-		}
-
-		if err = updateMilestone(e, m); err != nil {
-			return err
+			if err := updateMilestoneClosedNum(e, oldMilestoneID); err != nil {
+				return err
+			}
 		}
 	}
 
 	if issue.MilestoneID > 0 {
-		m, err := getMilestoneByRepoID(e, issue.RepoID, issue.MilestoneID)
-		if err != nil {
+		if err := updateMilestoneTotalNum(e, issue.MilestoneID); err != nil {
 			return err
 		}
-
-		m.NumIssues++
 		if issue.IsClosed {
-			m.NumClosedIssues++
+			if err := updateMilestoneClosedNum(e, issue.MilestoneID); err != nil {
+				return err
+			}
 		}
+	}
 
-		if err = updateMilestone(e, m); err != nil {
+	if oldMilestoneID > 0 || issue.MilestoneID > 0 {
+		if err := issue.loadRepo(e); err != nil {
 			return err
 		}
-	}
-
-	if err := issue.loadRepo(e); err != nil {
-		return err
-	}
 
-	if oldMilestoneID > 0 || issue.MilestoneID > 0 {
 		if _, err := createMilestoneComment(e, doer, issue.Repo, issue, oldMilestoneID, issue.MilestoneID); err != nil {
 			return err
 		}
 	}
 
-	return updateIssueCols(e, issue, "milestone_id")
+	return nil
 }
 
 // ChangeMilestoneAssign changes assignment of milestone for issue.
diff --git a/models/issue_milestone_test.go b/models/issue_milestone_test.go
index 09c6ff7595..6f8548ec67 100644
--- a/models/issue_milestone_test.go
+++ b/models/issue_milestone_test.go
@@ -231,7 +231,7 @@ func TestChangeMilestoneStatus(t *testing.T) {
 	CheckConsistencyFor(t, &Repository{ID: milestone.RepoID}, &Milestone{})
 }
 
-func TestChangeMilestoneIssueStats(t *testing.T) {
+func TestUpdateMilestoneClosedNum(t *testing.T) {
 	assert.NoError(t, PrepareTestDatabase())
 	issue := AssertExistsAndLoadBean(t, &Issue{MilestoneID: 1},
 		"is_closed=0").(*Issue)
@@ -240,14 +240,14 @@ func TestChangeMilestoneIssueStats(t *testing.T) {
 	issue.ClosedUnix = timeutil.TimeStampNow()
 	_, err := x.Cols("is_closed", "closed_unix").Update(issue)
 	assert.NoError(t, err)
-	assert.NoError(t, changeMilestoneIssueStats(x.NewSession(), issue))
+	assert.NoError(t, updateMilestoneClosedNum(x, issue.MilestoneID))
 	CheckConsistencyFor(t, &Milestone{})
 
 	issue.IsClosed = false
 	issue.ClosedUnix = 0
 	_, err = x.Cols("is_closed", "closed_unix").Update(issue)
 	assert.NoError(t, err)
-	assert.NoError(t, changeMilestoneIssueStats(x.NewSession(), issue))
+	assert.NoError(t, updateMilestoneClosedNum(x, issue.MilestoneID))
 	CheckConsistencyFor(t, &Milestone{})
 }