From b9c743440a7d80def574931013d13354d974e26c Mon Sep 17 00:00:00 2001 From: Ashley Nelson Date: Sat, 22 Oct 2022 10:08:10 -0500 Subject: [PATCH 1/2] Backport: 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. --- .../expected_milestone.yml | 19 ++++++++ .../Test_updateOpenMilestoneCounts/issue.yml | 25 ++++++++++ .../milestone.yml | 19 ++++++++ models/migrations/migrations.go | 2 + models/migrations/v224.go | 47 +++++++++++++++++++ models/migrations/v224_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/v224.go create mode 100644 models/migrations/v224_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 0000000000000..9326fa550b148 --- /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 0000000000000..fdaacd9f68270 --- /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 0000000000000..0bcf4cffd3aae --- /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 2719f45efbbbc..185a7dc8cc8f2 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -406,6 +406,8 @@ var migrations = []Migration{ NewMigration("Drop old CredentialID column", dropOldCredentialIDColumn), // v223 -> v224 NewMigration("Rename CredentialIDBytes column to CredentialID", renameCredentialIDBytes), + // v224 -> v225 + NewMigration("Update counts of all open milestones", updateOpenMilestoneCounts), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v224.go b/models/migrations/v224.go new file mode 100644 index 0000000000000..42ec2033fee2f --- /dev/null +++ b/models/migrations/v224.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/v224_test.go b/models/migrations/v224_test.go new file mode 100644 index 0000000000000..f8a147c9bd69a --- /dev/null +++ b/models/migrations/v224_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 467bc14b84594..7c94582b82ae4 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 := models.DeleteIssueActions(ctx, issue.RepoID, issue.ID); err != nil { return err } From c5c79c7c4f16f058eb4917edd1c1acb5aed36175 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Wed, 26 Oct 2022 00:19:46 +0200 Subject: [PATCH 2/2] rm migration --- .../expected_milestone.yml | 19 -------- .../Test_updateOpenMilestoneCounts/issue.yml | 25 ---------- .../milestone.yml | 19 -------- models/migrations/migrations.go | 2 - models/migrations/v224.go | 47 ------------------- models/migrations/v224_test.go | 46 ------------------ 6 files changed, 158 deletions(-) delete mode 100644 models/migrations/fixtures/Test_updateOpenMilestoneCounts/expected_milestone.yml delete mode 100644 models/migrations/fixtures/Test_updateOpenMilestoneCounts/issue.yml delete mode 100644 models/migrations/fixtures/Test_updateOpenMilestoneCounts/milestone.yml delete mode 100644 models/migrations/v224.go delete mode 100644 models/migrations/v224_test.go diff --git a/models/migrations/fixtures/Test_updateOpenMilestoneCounts/expected_milestone.yml b/models/migrations/fixtures/Test_updateOpenMilestoneCounts/expected_milestone.yml deleted file mode 100644 index 9326fa550b148..0000000000000 --- a/models/migrations/fixtures/Test_updateOpenMilestoneCounts/expected_milestone.yml +++ /dev/null @@ -1,19 +0,0 @@ -# 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 deleted file mode 100644 index fdaacd9f68270..0000000000000 --- a/models/migrations/fixtures/Test_updateOpenMilestoneCounts/issue.yml +++ /dev/null @@ -1,25 +0,0 @@ -# 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 deleted file mode 100644 index 0bcf4cffd3aae..0000000000000 --- a/models/migrations/fixtures/Test_updateOpenMilestoneCounts/milestone.yml +++ /dev/null @@ -1,19 +0,0 @@ -# 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 185a7dc8cc8f2..2719f45efbbbc 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -406,8 +406,6 @@ var migrations = []Migration{ NewMigration("Drop old CredentialID column", dropOldCredentialIDColumn), // v223 -> v224 NewMigration("Rename CredentialIDBytes column to CredentialID", renameCredentialIDBytes), - // v224 -> v225 - NewMigration("Update counts of all open milestones", updateOpenMilestoneCounts), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v224.go b/models/migrations/v224.go deleted file mode 100644 index 42ec2033fee2f..0000000000000 --- a/models/migrations/v224.go +++ /dev/null @@ -1,47 +0,0 @@ -// 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/v224_test.go b/models/migrations/v224_test.go deleted file mode 100644 index f8a147c9bd69a..0000000000000 --- a/models/migrations/v224_test.go +++ /dev/null @@ -1,46 +0,0 @@ -// 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) - } -}