From 0e384c6376ceb8fb7eb03636ba44c8aea338ad7a Mon Sep 17 00:00:00 2001 From: Ethan Koenig Date: Mon, 18 Dec 2017 06:06:51 -0800 Subject: [PATCH] Check ignored errors for issue and milestone count (#3213) --- models/issue_milestone.go | 48 ++++++++++++++++++++++++---------- models/issue_milestone_test.go | 24 ++++++++++++----- routers/repo/issue.go | 6 ++++- 3 files changed, 57 insertions(+), 21 deletions(-) diff --git a/models/issue_milestone.go b/models/issue_milestone.go index 6ec829123..761b598e9 100644 --- a/models/issue_milestone.go +++ b/models/issue_milestone.go @@ -165,31 +165,33 @@ func UpdateMilestone(m *Milestone) error { return updateMilestone(x, m) } -func countRepoMilestones(e Engine, repoID int64) int64 { - count, _ := e. +func countRepoMilestones(e Engine, repoID int64) (int64, error) { + return e. Where("repo_id=?", repoID). Count(new(Milestone)) - return count } -func countRepoClosedMilestones(e Engine, repoID int64) int64 { - closed, _ := e. +func countRepoClosedMilestones(e Engine, repoID int64) (int64, error) { + return e. Where("repo_id=? AND is_closed=?", repoID, true). Count(new(Milestone)) - return closed } // CountRepoClosedMilestones returns number of closed milestones in given repository. -func CountRepoClosedMilestones(repoID int64) int64 { +func CountRepoClosedMilestones(repoID int64) (int64, error) { return countRepoClosedMilestones(x, repoID) } // MilestoneStats returns number of open and closed milestones of given repository. -func MilestoneStats(repoID int64) (open int64, closed int64) { - open, _ = x. +func MilestoneStats(repoID int64) (open int64, closed int64, err error) { + open, err = x. Where("repo_id=? AND is_closed=?", repoID, false). Count(new(Milestone)) - return open, CountRepoClosedMilestones(repoID) + if err != nil { + return 0, 0, nil + } + closed, err = CountRepoClosedMilestones(repoID) + return open, closed, err } // ChangeMilestoneStatus changes the milestone open/closed status. @@ -210,8 +212,17 @@ func ChangeMilestoneStatus(m *Milestone, isClosed bool) (err error) { return err } - repo.NumMilestones = int(countRepoMilestones(sess, repo.ID)) - repo.NumClosedMilestones = int(countRepoClosedMilestones(sess, repo.ID)) + numMilestones, err := countRepoMilestones(sess, repo.ID) + if err != nil { + return err + } + numClosedMilestones, err := countRepoClosedMilestones(sess, repo.ID) + if err != nil { + return err + } + repo.NumMilestones = int(numMilestones) + repo.NumClosedMilestones = int(numClosedMilestones) + if _, err = sess.ID(repo.ID).Cols("num_milestones, num_closed_milestones").Update(repo); err != nil { return err } @@ -324,8 +335,17 @@ func DeleteMilestoneByRepoID(repoID, id int64) error { return err } - repo.NumMilestones = int(countRepoMilestones(sess, repo.ID)) - repo.NumClosedMilestones = int(countRepoClosedMilestones(sess, repo.ID)) + numMilestones, err := countRepoMilestones(sess, repo.ID) + if err != nil { + return err + } + numClosedMilestones, err := countRepoClosedMilestones(sess, repo.ID) + if err != nil { + return err + } + repo.NumMilestones = int(numMilestones) + repo.NumClosedMilestones = int(numClosedMilestones) + if _, err = sess.ID(repo.ID).Cols("num_milestones, num_closed_milestones").Update(repo); err != nil { return err } diff --git a/models/issue_milestone_test.go b/models/issue_milestone_test.go index e0bfe11df..f7987d45a 100644 --- a/models/issue_milestone_test.go +++ b/models/issue_milestone_test.go @@ -146,31 +146,42 @@ func TestCountRepoMilestones(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) test := func(repoID int64) { repo := AssertExistsAndLoadBean(t, &Repository{ID: repoID}).(*Repository) - assert.EqualValues(t, repo.NumMilestones, countRepoMilestones(x, repoID)) + count, err := countRepoMilestones(x, repoID) + assert.NoError(t, err) + assert.EqualValues(t, repo.NumMilestones, count) } test(1) test(2) test(3) - assert.EqualValues(t, 0, countRepoMilestones(x, NonexistentID)) + + count, err := countRepoMilestones(x, NonexistentID) + assert.NoError(t, err) + assert.EqualValues(t, 0, count) } func TestCountRepoClosedMilestones(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) test := func(repoID int64) { repo := AssertExistsAndLoadBean(t, &Repository{ID: repoID}).(*Repository) - assert.EqualValues(t, repo.NumClosedMilestones, CountRepoClosedMilestones(repoID)) + count, err := CountRepoClosedMilestones(repoID) + assert.NoError(t, err) + assert.EqualValues(t, repo.NumClosedMilestones, count) } test(1) test(2) test(3) - assert.EqualValues(t, 0, countRepoMilestones(x, NonexistentID)) + + count, err := CountRepoClosedMilestones(NonexistentID) + assert.NoError(t, err) + assert.EqualValues(t, 0, count) } func TestMilestoneStats(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) test := func(repoID int64) { repo := AssertExistsAndLoadBean(t, &Repository{ID: repoID}).(*Repository) - open, closed := MilestoneStats(repoID) + open, closed, err := MilestoneStats(repoID) + assert.NoError(t, err) assert.EqualValues(t, repo.NumMilestones-repo.NumClosedMilestones, open) assert.EqualValues(t, repo.NumClosedMilestones, closed) } @@ -178,7 +189,8 @@ func TestMilestoneStats(t *testing.T) { test(2) test(3) - open, closed := MilestoneStats(NonexistentID) + open, closed, err := MilestoneStats(NonexistentID) + assert.NoError(t, err) assert.EqualValues(t, 0, open) assert.EqualValues(t, 0, closed) } diff --git a/routers/repo/issue.go b/routers/repo/issue.go index ec8fda8f7..578ead134 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -1067,7 +1067,11 @@ func Milestones(ctx *context.Context) { ctx.Data["PageIsMilestones"] = true isShowClosed := ctx.Query("state") == "closed" - openCount, closedCount := models.MilestoneStats(ctx.Repo.Repository.ID) + openCount, closedCount, err := models.MilestoneStats(ctx.Repo.Repository.ID) + if err != nil { + ctx.Handle(500, "MilestoneStats", err) + return + } ctx.Data["OpenCount"] = openCount ctx.Data["ClosedCount"] = closedCount