diff --git a/models/consistency.go b/models/consistency.go index 77a8018266e5..db5811ccd95a 100644 --- 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 760aaaab0969..331ea01fdc19 100644 --- 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 ec3cbb91dbc1..e639b503527a 100644 --- 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 af264aa27457..5406129884fb 100644 --- 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{}) }