From 82ecd3b19eb1382521b7999bd0e2dd977c3b994d Mon Sep 17 00:00:00 2001 From: Ashley Nelson Date: Sat, 22 Oct 2022 10:08:10 -0500 Subject: [PATCH] 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 --- .../expected_milestone.yml | 19 ++++++++ .../Test_updateOpenMilestoneCounts/issue.yml | 25 ++++++++++ .../milestone.yml | 19 ++++++++ models/migrations/migrations.go | 2 + models/migrations/v229.go | 47 +++++++++++++++++++ models/migrations/v229_test.go | 46 ++++++++++++++++++ services/issue/issue.go | 5 ++ 7 files changed, 163 insertions(+) create mode 100644 models/migrations/fixtures/Test_updateOpenMilestoneCounts/expected_milestone.yml create mode 100644 models/migrations/fixtures/Test_updateOpenMilestoneCounts/issue.yml create mode 100644 models/migrations/fixtures/Test_updateOpenMilestoneCounts/milestone.yml create mode 100644 models/migrations/v229.go create mode 100644 models/migrations/v229_test.go diff --git a/models/migrations/fixtures/Test_updateOpenMilestoneCounts/expected_milestone.yml b/models/migrations/fixtures/Test_updateOpenMilestoneCounts/expected_milestone.yml new file mode 100644 index 000000000..9326fa550 --- /dev/null +++ b/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 diff --git a/models/migrations/fixtures/Test_updateOpenMilestoneCounts/issue.yml b/models/migrations/fixtures/Test_updateOpenMilestoneCounts/issue.yml new file mode 100644 index 000000000..fdaacd9f6 --- /dev/null +++ b/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 diff --git a/models/migrations/fixtures/Test_updateOpenMilestoneCounts/milestone.yml b/models/migrations/fixtures/Test_updateOpenMilestoneCounts/milestone.yml new file mode 100644 index 000000000..0bcf4cffd --- /dev/null +++ b/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 diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 46ef05282..cca6c52d4 100644 --- a/models/migrations/migrations.go +++ b/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 diff --git a/models/migrations/v229.go b/models/migrations/v229.go new file mode 100644 index 000000000..42ec2033f --- /dev/null +++ b/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 +} diff --git a/models/migrations/v229_test.go b/models/migrations/v229_test.go new file mode 100644 index 000000000..f8a147c9b --- /dev/null +++ b/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) + } +} diff --git a/services/issue/issue.go b/services/issue/issue.go index 69b87686c..47782e50d 100644 --- a/services/issue/issue.go +++ b/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 }