Update milestone counters on new issue (#16183) (#16224)

Co-authored-by: 6543 <6543@obermui.de>
Co-authored-by: zeripath <art27@cantab.net>

diff --git a/models/consistency.go b/models/consistency.go
index 77a8018266e5121698590bc32e3069cc7c3f5f39..db5811ccd95a3f19956b5b3c826c14a40b01de89 100644 (file)
--- a/models/consistency.go
+++ b/models/consistency.go
@@ -141,6 +141,12 @@ func (milestone *Milestone) checkForConsistency(t *testing.T) {
        actual := getCount(t, x.Where("is_closed=?", true), &Issue{MilestoneID: milestone.ID})
        assert.EqualValues(t, milestone.NumClosedIssues, actual,
                "Unexpected number of closed issues for milestone %+v", milestone)
+
+       completeness := 0
+       if milestone.NumIssues > 0 {
+               completeness = milestone.NumClosedIssues * 100 / milestone.NumIssues
+       }
+       assert.Equal(t, completeness, milestone.Completeness)
 }
 
 func (label *Label) checkForConsistency(t *testing.T) {

diff --git a/models/issue.go b/models/issue.go
index 760aaaab09694cfe7b27b73ff683bfac38bae02d..331ea01fdc1986a6fa63bf992eebc89465f40a64 100644 (file)
--- a/models/issue.go
+++ b/models/issue.go
@@ -648,8 +648,10 @@ func (issue *Issue) doChangeStatus(e *xorm.Session, doer *User, isMergePull bool
        }
 
        // Update issue count of milestone
-       if err := updateMilestoneClosedNum(e, issue.MilestoneID); err != nil {
-               return nil, err
+       if issue.MilestoneID > 0 {
+               if err := updateMilestoneCounters(e, issue.MilestoneID); err != nil {
+                       return nil, err
+               }
        }
 
        if err := issue.updateClosedNum(e); err != nil {
@@ -912,7 +914,7 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) {
        opts.Issue.Index = inserted.Index
 
        if opts.Issue.MilestoneID > 0 {
-               if _, err = e.Exec("UPDATE `milestone` SET num_issues=num_issues+1 WHERE id=?", opts.Issue.MilestoneID); err != nil {
+               if err := updateMilestoneCounters(e, opts.Issue.MilestoneID); err != nil {
                        return err
                }
 

diff --git a/models/issue_milestone.go b/models/issue_milestone.go
index ec3cbb91dbc15701e9ab37d51c063167f99d3f20..e639b503527a713388aeb2b972525a3f00da9b09 100644 (file)
--- a/models/issue_milestone.go
+++ b/models/issue_milestone.go
@@ -129,8 +129,12 @@ func GetMilestoneByRepoIDANDName(repoID int64, name string) (*Milestone, error)
 
 // GetMilestoneByID returns the milestone via id .
 func GetMilestoneByID(id int64) (*Milestone, error) {
+       return getMilestoneByID(x, id)
+}
+
+func getMilestoneByID(e Engine, id int64) (*Milestone, error) {
        var m Milestone
-       has, err := x.ID(id).Get(&m)
+       has, err := e.ID(id).Get(&m)
        if err != nil {
                return nil, err
        } else if !has {
@@ -155,10 +159,6 @@ func UpdateMilestone(m *Milestone, oldIsClosed bool) error {
                return err
        }
 
-       if err := updateMilestoneCompleteness(sess, m.ID); err != nil {
-               return err
-       }
-
        // if IsClosed changed, update milestone numbers of repository
        if oldIsClosed != m.IsClosed {
                if err := updateRepoMilestoneNum(sess, m.RepoID); err != nil {
@@ -171,23 +171,31 @@ func UpdateMilestone(m *Milestone, oldIsClosed bool) error {
 
 func updateMilestone(e Engine, m *Milestone) error {
        m.Name = strings.TrimSpace(m.Name)
-       _, err := e.ID(m.ID).AllCols().
+       _, err := e.ID(m.ID).AllCols().Update(m)
+       if err != nil {
+               return err
+       }
+       return updateMilestoneCounters(e, m.ID)
+}
+
+// updateMilestoneCounters calculates NumIssues, NumClosesIssues and Completeness
+func updateMilestoneCounters(e Engine, id int64) error {
+       _, err := e.ID(id).
                SetExpr("num_issues", builder.Select("count(*)").From("issue").Where(
-                       builder.Eq{"milestone_id": m.ID},
+                       builder.Eq{"milestone_id": id},
                )).
                SetExpr("num_closed_issues", builder.Select("count(*)").From("issue").Where(
                        builder.Eq{
-                               "milestone_id": m.ID,
+                               "milestone_id": id,
                                "is_closed":    true,
                        },
                )).
-               Update(m)
-       return err
-}
-
-func updateMilestoneCompleteness(e Engine, milestoneID int64) error {
-       _, 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,
+               Update(&Milestone{})
+       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=?",
+               id,
        )
        return err
 }
@@ -256,25 +264,15 @@ func changeMilestoneAssign(e *xorm.Session, doer *User, issue *Issue, oldMilesto
        }
 
        if oldMilestoneID > 0 {
-               if err := updateMilestoneTotalNum(e, oldMilestoneID); err != nil {
+               if err := updateMilestoneCounters(e, oldMilestoneID); err != nil {
                        return err
                }
-               if issue.IsClosed {
-                       if err := updateMilestoneClosedNum(e, oldMilestoneID); err != nil {
-                               return err
-                       }
-               }
        }
 
        if issue.MilestoneID > 0 {
-               if err := updateMilestoneTotalNum(e, issue.MilestoneID); err != nil {
+               if err := updateMilestoneCounters(e, issue.MilestoneID); err != nil {
                        return err
                }
-               if issue.IsClosed {
-                       if err := updateMilestoneClosedNum(e, issue.MilestoneID); err != nil {
-                               return err
-                       }
-               }
        }
 
        if oldMilestoneID > 0 || issue.MilestoneID > 0 {
@@ -558,29 +556,6 @@ func updateRepoMilestoneNum(e Engine, repoID int64) error {
        return err
 }
 
-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
-       }
-
-       return updateMilestoneCompleteness(e, milestoneID)
-}
-
-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 updateMilestoneCompleteness(e, milestoneID)
-}
-
 //  _____               _            _ _____ _
 // |_   _| __ __ _  ___| | _____  __| |_   _(_)_ __ ___   ___  ___
 //   | || '__/ _` |/ __| |/ / _ \/ _` | | | | | '_ ` _ \ / _ \/ __|

diff --git a/models/issue_milestone_test.go b/models/issue_milestone_test.go
index af264aa274576280d0e4316536178e7ecaf24c9a..5406129884fb0a2e336daa95b413e0ad9b411bda 100644 (file)
--- a/models/issue_milestone_test.go
+++ b/models/issue_milestone_test.go
@@ -215,7 +215,7 @@ func TestChangeMilestoneStatus(t *testing.T) {
        CheckConsistencyFor(t, &Repository{ID: milestone.RepoID}, &Milestone{})
 }
 
-func TestUpdateMilestoneClosedNum(t *testing.T) {
+func TestUpdateMilestoneCounters(t *testing.T) {
        assert.NoError(t, PrepareTestDatabase())
        issue := AssertExistsAndLoadBean(t, &Issue{MilestoneID: 1},
                "is_closed=0").(*Issue)
@@ -224,14 +224,14 @@ func TestUpdateMilestoneClosedNum(t *testing.T) {
        issue.ClosedUnix = timeutil.TimeStampNow()
        _, err := x.ID(issue.ID).Cols("is_closed", "closed_unix").Update(issue)
        assert.NoError(t, err)
-       assert.NoError(t, updateMilestoneClosedNum(x, issue.MilestoneID))
+       assert.NoError(t, updateMilestoneCounters(x, issue.MilestoneID))
        CheckConsistencyFor(t, &Milestone{})
 
        issue.IsClosed = false
        issue.ClosedUnix = 0
        _, err = x.ID(issue.ID).Cols("is_closed", "closed_unix").Update(issue)
        assert.NoError(t, err)
-       assert.NoError(t, updateMilestoneClosedNum(x, issue.MilestoneID))
+       assert.NoError(t, updateMilestoneCounters(x, issue.MilestoneID))
        CheckConsistencyFor(t, &Milestone{})
 }