From 994bd93e69056943571384a2c47ca51bdefc702f Mon Sep 17 00:00:00 2001 From: Robert Wolff Date: Wed, 31 Jul 2024 12:40:24 +0200 Subject: [PATCH] feat(UI): add package counter to repo/user/org overview pages - add package counter to repo/user/org overview pages - add go unit tests for repo/user has/count packages - add many more unit tests for packages model - fix error for non-existing packages in DeletePackageByID and SetRepositoryLink --- models/packages/package.go | 54 +++++- models/packages/package_test.go | 269 ++++++++++++++++++++++++++- routers/web/shared/user/header.go | 14 +- services/context/repo.go | 8 +- templates/org/menu.tmpl | 4 + templates/repo/header.tmpl | 3 + templates/user/overview/header.tmpl | 4 + tests/integration/user_count_test.go | 10 +- 8 files changed, 341 insertions(+), 25 deletions(-) diff --git a/models/packages/package.go b/models/packages/package.go index 50717c695..84e2fa7ee 100644 --- a/models/packages/package.go +++ b/models/packages/package.go @@ -1,4 +1,5 @@ // Copyright 2021 The Gitea Authors. All rights reserved. +// Copyright 2024 The Forgejo Authors. All rights reserved. // SPDX-License-Identifier: MIT package packages @@ -12,6 +13,7 @@ import ( "code.gitea.io/gitea/modules/util" "xorm.io/builder" + "xorm.io/xorm" ) func init() { @@ -212,13 +214,19 @@ func TryInsertPackage(ctx context.Context, p *Package) (*Package, error) { // DeletePackageByID deletes a package by id func DeletePackageByID(ctx context.Context, packageID int64) error { - _, err := db.GetEngine(ctx).ID(packageID).Delete(&Package{}) + n, err := db.GetEngine(ctx).ID(packageID).Delete(&Package{}) + if n == 0 && err == nil { + return ErrPackageNotExist + } return err } // SetRepositoryLink sets the linked repository func SetRepositoryLink(ctx context.Context, packageID, repoID int64) error { - _, err := db.GetEngine(ctx).ID(packageID).Cols("repo_id").Update(&Package{RepoID: repoID}) + n, err := db.GetEngine(ctx).ID(packageID).Cols("repo_id").Update(&Package{RepoID: repoID}) + if n == 0 && err == nil { + return ErrPackageNotExist + } return err } @@ -293,19 +301,45 @@ func FindUnreferencedPackages(ctx context.Context) ([]int64, error) { return pIDs, nil } -// HasOwnerPackages tests if a user/org has accessible packages -func HasOwnerPackages(ctx context.Context, ownerID int64) (bool, error) { +func getPackages(ctx context.Context) *xorm.Session { return db.GetEngine(ctx). Table("package_version"). Join("INNER", "package", "package.id = package_version.package_id"). - Where(builder.Eq{ - "package_version.is_internal": false, - "package.owner_id": ownerID, - }). - Exist(&PackageVersion{}) + Where("package_version.is_internal = ?", false) +} + +func getOwnerPackages(ctx context.Context, ownerID int64) *xorm.Session { + return getPackages(ctx). + Where("package.owner_id = ?", ownerID) +} + +// HasOwnerPackages tests if a user/org has accessible packages +func HasOwnerPackages(ctx context.Context, ownerID int64) (bool, error) { + return getOwnerPackages(ctx, ownerID). + Exist(&Package{}) +} + +// CountOwnerPackages counts user/org accessible packages +func CountOwnerPackages(ctx context.Context, ownerID int64) (int64, error) { + return getOwnerPackages(ctx, ownerID). + Distinct("package.id"). + Count(&Package{}) +} + +func getRepositoryPackages(ctx context.Context, repositoryID int64) *xorm.Session { + return getPackages(ctx). + Where("package.repo_id = ?", repositoryID) } // HasRepositoryPackages tests if a repository has packages func HasRepositoryPackages(ctx context.Context, repositoryID int64) (bool, error) { - return db.GetEngine(ctx).Where("repo_id = ?", repositoryID).Exist(&Package{}) + return getRepositoryPackages(ctx, repositoryID). + Exist(&PackageVersion{}) +} + +// CountRepositoryPackages counts packages of a repository +func CountRepositoryPackages(ctx context.Context, repositoryID int64) (int64, error) { + return getRepositoryPackages(ctx, repositoryID). + Distinct("package.id"). + Count(&Package{}) } diff --git a/models/packages/package_test.go b/models/packages/package_test.go index 8ab7d31e0..1c96e08f0 100644 --- a/models/packages/package_test.go +++ b/models/packages/package_test.go @@ -1,4 +1,5 @@ // Copyright 2022 The Gitea Authors. All rights reserved. +// Copyright 2024 The Forgejo Authors. All rights reserved. // SPDX-License-Identifier: MIT package packages_test @@ -8,6 +9,7 @@ import ( "code.gitea.io/gitea/models/db" packages_model "code.gitea.io/gitea/models/packages" + repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" @@ -15,7 +17,6 @@ import ( _ "code.gitea.io/gitea/models/actions" _ "code.gitea.io/gitea/models/activities" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -23,21 +24,185 @@ func TestMain(m *testing.M) { unittest.MainTest(m) } -func TestHasOwnerPackages(t *testing.T) { +func prepareExamplePackage(t *testing.T) *packages_model.Package { + require.NoError(t, unittest.PrepareTestDatabase()) + + owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) + + p0 := &packages_model.Package{ + OwnerID: owner.ID, + RepoID: repo.ID, + LowerName: "package", + Type: packages_model.TypeGeneric, + } + + p, err := packages_model.TryInsertPackage(db.DefaultContext, p0) + require.NotNil(t, p) + require.NoError(t, err) + require.Equal(t, *p0, *p) + return p +} + +func deletePackage(t *testing.T, p *packages_model.Package) { + err := packages_model.DeletePackageByID(db.DefaultContext, p.ID) + require.NoError(t, err) +} + +func TestTryInsertPackage(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + p0 := &packages_model.Package{ + OwnerID: owner.ID, + LowerName: "package", + } + + // Insert package should return the package and yield no error + p, err := packages_model.TryInsertPackage(db.DefaultContext, p0) + require.NotNil(t, p) + require.NoError(t, err) + require.Equal(t, *p0, *p) + + // Insert same package again should return the same package and yield ErrDuplicatePackage + p, err = packages_model.TryInsertPackage(db.DefaultContext, p0) + require.NotNil(t, p) + require.IsType(t, packages_model.ErrDuplicatePackage, err) + require.Equal(t, *p0, *p) + + err = packages_model.DeletePackageByID(db.DefaultContext, p0.ID) + require.NoError(t, err) +} + +func TestGetPackageByID(t *testing.T) { + p0 := prepareExamplePackage(t) + + // Get package should return package and yield no error + p, err := packages_model.GetPackageByID(db.DefaultContext, p0.ID) + require.NotNil(t, p) + require.Equal(t, *p0, *p) + require.NoError(t, err) + + // Get package with non-existng ID should yield ErrPackageNotExist + p, err = packages_model.GetPackageByID(db.DefaultContext, 999) + require.Nil(t, p) + require.Error(t, err) + require.IsType(t, packages_model.ErrPackageNotExist, err) + + deletePackage(t, p0) +} + +func TestDeletePackageByID(t *testing.T) { + p0 := prepareExamplePackage(t) + + // Delete existing package should yield no error + err := packages_model.DeletePackageByID(db.DefaultContext, p0.ID) + require.NoError(t, err) + + // Delete (now) non-existing package should yield ErrPackageNotExist + err = packages_model.DeletePackageByID(db.DefaultContext, p0.ID) + require.Error(t, err) + require.IsType(t, packages_model.ErrPackageNotExist, err) +} + +func TestSetRepositoryLink(t *testing.T) { + p0 := prepareExamplePackage(t) + + // Set repository link to package should yield no error and package RepoID should be updated + err := packages_model.SetRepositoryLink(db.DefaultContext, p0.ID, 5) + require.NoError(t, err) + + p, err := packages_model.GetPackageByID(db.DefaultContext, p0.ID) + require.NoError(t, err) + require.EqualValues(t, 5, p.RepoID) + + // Set repository link to non-existing package should yied ErrPackageNotExist + err = packages_model.SetRepositoryLink(db.DefaultContext, 999, 5) + require.Error(t, err) + require.IsType(t, packages_model.ErrPackageNotExist, err) + + deletePackage(t, p0) +} + +func TestUnlinkRepositoryFromAllPackages(t *testing.T) { + p0 := prepareExamplePackage(t) + + // Unlink repository from all packages should yield no error and package with p0.ID should have RepoID 0 + err := packages_model.UnlinkRepositoryFromAllPackages(db.DefaultContext, p0.RepoID) + require.NoError(t, err) + + p, err := packages_model.GetPackageByID(db.DefaultContext, p0.ID) + require.NoError(t, err) + require.EqualValues(t, 0, p.RepoID) + + // Unlink repository again from all packages should also yield no error + err = packages_model.UnlinkRepositoryFromAllPackages(db.DefaultContext, p0.RepoID) + require.NoError(t, err) + + deletePackage(t, p0) +} + +func TestGetPackageByName(t *testing.T) { + p0 := prepareExamplePackage(t) + + // Get package should return package and yield no error + p, err := packages_model.GetPackageByName(db.DefaultContext, p0.OwnerID, p0.Type, p0.LowerName) + require.NotNil(t, p) + require.Equal(t, *p0, *p) + require.NoError(t, err) + + // Get package with uppercase name should return package and yield no error + p, err = packages_model.GetPackageByName(db.DefaultContext, p0.OwnerID, p0.Type, "Package") + require.NotNil(t, p) + require.Equal(t, *p0, *p) + require.NoError(t, err) + + // Get package with wrong owner ID, type or name should return no package and yield ErrPackageNotExist + p, err = packages_model.GetPackageByName(db.DefaultContext, 999, p0.Type, p0.LowerName) + require.Nil(t, p) + require.Error(t, err) + require.IsType(t, packages_model.ErrPackageNotExist, err) + p, err = packages_model.GetPackageByName(db.DefaultContext, p0.OwnerID, packages_model.TypeDebian, p0.LowerName) + require.Nil(t, p) + require.Error(t, err) + require.IsType(t, packages_model.ErrPackageNotExist, err) + p, err = packages_model.GetPackageByName(db.DefaultContext, p0.OwnerID, p0.Type, "package1") + require.Nil(t, p) + require.Error(t, err) + require.IsType(t, packages_model.ErrPackageNotExist, err) + + deletePackage(t, p0) +} + +func TestHasCountPackages(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) p, err := packages_model.TryInsertPackage(db.DefaultContext, &packages_model.Package{ OwnerID: owner.ID, + RepoID: repo.ID, LowerName: "package", }) - assert.NotNil(t, p) + require.NotNil(t, p) require.NoError(t, err) - // A package without package versions gets automatically cleaned up and should return false + // A package without package versions gets automatically cleaned up and should return false for owner has, err := packages_model.HasOwnerPackages(db.DefaultContext, owner.ID) - assert.False(t, has) + require.False(t, has) + require.NoError(t, err) + count, err := packages_model.CountOwnerPackages(db.DefaultContext, owner.ID) + require.EqualValues(t, 0, count) + require.NoError(t, err) + + // A package without package versions gets automatically cleaned up and should return false for repository + has, err = packages_model.HasRepositoryPackages(db.DefaultContext, repo.ID) + require.False(t, has) + require.NoError(t, err) + count, err = packages_model.CountRepositoryPackages(db.DefaultContext, repo.ID) + require.EqualValues(t, 0, count) require.NoError(t, err) pv, err := packages_model.GetOrInsertVersion(db.DefaultContext, &packages_model.PackageVersion{ @@ -45,12 +210,21 @@ func TestHasOwnerPackages(t *testing.T) { LowerVersion: "internal", IsInternal: true, }) - assert.NotNil(t, pv) + require.NotNil(t, pv) require.NoError(t, err) // A package with an internal package version gets automatically cleaned up and should return false has, err = packages_model.HasOwnerPackages(db.DefaultContext, owner.ID) - assert.False(t, has) + require.False(t, has) + require.NoError(t, err) + count, err = packages_model.CountOwnerPackages(db.DefaultContext, owner.ID) + require.EqualValues(t, 0, count) + require.NoError(t, err) + has, err = packages_model.HasRepositoryPackages(db.DefaultContext, repo.ID) + require.False(t, has) + require.NoError(t, err) + count, err = packages_model.CountRepositoryPackages(db.DefaultContext, repo.ID) + require.EqualValues(t, 0, count) require.NoError(t, err) pv, err = packages_model.GetOrInsertVersion(db.DefaultContext, &packages_model.PackageVersion{ @@ -58,11 +232,88 @@ func TestHasOwnerPackages(t *testing.T) { LowerVersion: "normal", IsInternal: false, }) - assert.NotNil(t, pv) + require.NotNil(t, pv) require.NoError(t, err) // A package with a normal package version should return true has, err = packages_model.HasOwnerPackages(db.DefaultContext, owner.ID) - assert.True(t, has) + require.True(t, has) + require.NoError(t, err) + count, err = packages_model.CountOwnerPackages(db.DefaultContext, owner.ID) + require.EqualValues(t, 1, count) + require.NoError(t, err) + has, err = packages_model.HasRepositoryPackages(db.DefaultContext, repo.ID) + require.True(t, has) + require.NoError(t, err) + count, err = packages_model.CountRepositoryPackages(db.DefaultContext, repo.ID) + require.EqualValues(t, 1, count) + require.NoError(t, err) + + pv2, err := packages_model.GetOrInsertVersion(db.DefaultContext, &packages_model.PackageVersion{ + PackageID: p.ID, + LowerVersion: "normal2", + IsInternal: false, + }) + require.NotNil(t, pv2) + require.NoError(t, err) + + // A package withmultiple package versions should be counted only once + has, err = packages_model.HasOwnerPackages(db.DefaultContext, owner.ID) + require.True(t, has) + require.NoError(t, err) + count, err = packages_model.CountOwnerPackages(db.DefaultContext, owner.ID) + require.EqualValues(t, 1, count) + require.NoError(t, err) + has, err = packages_model.HasRepositoryPackages(db.DefaultContext, repo.ID) + require.True(t, has) + require.NoError(t, err) + count, err = packages_model.CountRepositoryPackages(db.DefaultContext, repo.ID) + require.EqualValues(t, 1, count) + require.NoError(t, err) + + // For owner ID 0 there should be no packages + has, err = packages_model.HasOwnerPackages(db.DefaultContext, 0) + require.False(t, has) + require.NoError(t, err) + count, err = packages_model.CountOwnerPackages(db.DefaultContext, 0) + require.EqualValues(t, 0, count) + require.NoError(t, err) + + // For repo ID 0 there should be no packages + has, err = packages_model.HasRepositoryPackages(db.DefaultContext, 0) + require.False(t, has) + require.NoError(t, err) + count, err = packages_model.CountRepositoryPackages(db.DefaultContext, 0) + require.EqualValues(t, 0, count) + require.NoError(t, err) + + p1, err := packages_model.TryInsertPackage(db.DefaultContext, &packages_model.Package{ + OwnerID: owner.ID, + LowerName: "package0", + }) + require.NotNil(t, p1) + require.NoError(t, err) + p1v, err := packages_model.GetOrInsertVersion(db.DefaultContext, &packages_model.PackageVersion{ + PackageID: p1.ID, + LowerVersion: "normal", + IsInternal: false, + }) + require.NotNil(t, p1v) + require.NoError(t, err) + + // Owner owner.ID should have two packages now + has, err = packages_model.HasOwnerPackages(db.DefaultContext, owner.ID) + require.True(t, has) + require.NoError(t, err) + count, err = packages_model.CountOwnerPackages(db.DefaultContext, owner.ID) + require.EqualValues(t, 2, count) + require.NoError(t, err) + + // For repo ID 0 there should be now one package, because p1 is not assigned to a repo + has, err = packages_model.HasRepositoryPackages(db.DefaultContext, 0) + require.True(t, has) + require.NoError(t, err) + count, err = packages_model.CountRepositoryPackages(db.DefaultContext, 0) + require.EqualValues(t, 1, count) require.NoError(t, err) } diff --git a/routers/web/shared/user/header.go b/routers/web/shared/user/header.go index 7d0b34cb7..fd7605c33 100644 --- a/routers/web/shared/user/header.go +++ b/routers/web/shared/user/header.go @@ -1,4 +1,5 @@ // Copyright 2022 The Gitea Authors. All rights reserved. +// Copyright 2024 The Forgejo Authors. All rights reserved. // SPDX-License-Identifier: MIT package user @@ -8,6 +9,7 @@ import ( "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/organization" + packages_model "code.gitea.io/gitea/models/packages" access_model "code.gitea.io/gitea/models/perm/access" project_model "code.gitea.io/gitea/models/project" repo_model "code.gitea.io/gitea/models/repo" @@ -125,7 +127,9 @@ func RenderUserHeader(ctx *context.Context) { func LoadHeaderCount(ctx *context.Context) error { prepareContextForCommonProfile(ctx) - repoCount, err := repo_model.CountRepository(ctx, &repo_model.SearchRepoOptions{ + var err error + + ctx.Data["RepoCount"], err = repo_model.CountRepository(ctx, &repo_model.SearchRepoOptions{ Actor: ctx.Doer, OwnerID: ctx.ContextUser.ID, Private: ctx.IsSigned, @@ -135,7 +139,6 @@ func LoadHeaderCount(ctx *context.Context) error { if err != nil { return err } - ctx.Data["RepoCount"] = repoCount var projectType project_model.Type if ctx.ContextUser.IsOrganization() { @@ -143,7 +146,7 @@ func LoadHeaderCount(ctx *context.Context) error { } else { projectType = project_model.TypeIndividual } - projectCount, err := db.Count[project_model.Project](ctx, project_model.SearchOptions{ + ctx.Data["ProjectCount"], err = db.Count[project_model.Project](ctx, project_model.SearchOptions{ OwnerID: ctx.ContextUser.ID, IsClosed: optional.Some(false), Type: projectType, @@ -151,7 +154,10 @@ func LoadHeaderCount(ctx *context.Context) error { if err != nil { return err } - ctx.Data["ProjectCount"] = projectCount + ctx.Data["PackageCount"], err = packages_model.CountOwnerPackages(ctx, ctx.ContextUser.ID) + if err != nil { + return err + } return nil } diff --git a/services/context/repo.go b/services/context/repo.go index e4cacbc53..74616ec24 100644 --- a/services/context/repo.go +++ b/services/context/repo.go @@ -1,6 +1,6 @@ -// Copyright 2024 The Forgejo Authors. All rights reserved. // Copyright 2014 The Gogs Authors. All rights reserved. // Copyright 2017 The Gitea Authors. All rights reserved. +// Copyright 2024 The Forgejo Authors. All rights reserved. // SPDX-License-Identifier: MIT package context @@ -19,6 +19,7 @@ import ( "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" + packages_model "code.gitea.io/gitea/models/packages" access_model "code.gitea.io/gitea/models/perm/access" repo_model "code.gitea.io/gitea/models/repo" unit_model "code.gitea.io/gitea/models/unit" @@ -579,6 +580,11 @@ func RepoAssignment(ctx *Context) context.CancelFunc { ctx.ServerError("GetReleaseCountByRepoID", err) return nil } + ctx.Data["NumPackages"], err = packages_model.CountRepositoryPackages(ctx, ctx.Repo.Repository.ID) + if err != nil { + ctx.ServerError("GetPackageCountByRepoID", err) + return nil + } ctx.Data["Title"] = owner.Name + "/" + repo.Name ctx.Data["Repository"] = repo diff --git a/templates/org/menu.tmpl b/templates/org/menu.tmpl index 212154995..6258f1737 100644 --- a/templates/org/menu.tmpl +++ b/templates/org/menu.tmpl @@ -20,6 +20,10 @@ {{if and .IsPackageEnabled .CanReadPackages}} {{svg "octicon-package"}} {{ctx.Locale.Tr "packages.title"}} + {{if .PackageCount}} +
{{.PackageCount}}
+ {{end}} +
{{end}} {{if and .IsRepoIndexerEnabled .CanReadCode}} diff --git a/templates/repo/header.tmpl b/templates/repo/header.tmpl index e81e65bc7..777453e4b 100644 --- a/templates/repo/header.tmpl +++ b/templates/repo/header.tmpl @@ -135,6 +135,9 @@ {{if .Permission.CanRead $.UnitTypePackages}} {{svg "octicon-package"}} {{ctx.Locale.Tr "packages.title"}} + {{if .NumPackages}} + {{CountFmt .NumPackages}} + {{end}} {{end}} diff --git a/templates/user/overview/header.tmpl b/templates/user/overview/header.tmpl index 27568c311..ea5d8052f 100644 --- a/templates/user/overview/header.tmpl +++ b/templates/user/overview/header.tmpl @@ -24,6 +24,10 @@ {{if and .IsPackageEnabled (or .ContextUser.IsIndividual .CanReadPackages)}} {{svg "octicon-package"}} {{ctx.Locale.Tr "packages.title"}} + {{if .PackageCount}} +
{{.PackageCount}}
+ {{end}} +
{{end}} {{if and .IsRepoIndexerEnabled (or .ContextUser.IsIndividual .CanReadCode)}} diff --git a/tests/integration/user_count_test.go b/tests/integration/user_count_test.go index c0837d57f..e76c30c1d 100644 --- a/tests/integration/user_count_test.go +++ b/tests/integration/user_count_test.go @@ -11,6 +11,7 @@ import ( "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/organization" + packages_model "code.gitea.io/gitea/models/packages" project_model "code.gitea.io/gitea/models/project" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" @@ -29,6 +30,7 @@ type userCountTest struct { session *TestSession repoCount int64 projectCount int64 + packageCount int64 memberCount int64 teamCount int64 } @@ -54,12 +56,14 @@ func (countTest *userCountTest) Init(t *testing.T, doerID, userID int64) { } else { projectType = project_model.TypeIndividual } - countTest.projectCount, err = db.Count[project_model.Project](db.DefaultContext, project_model.SearchOptions{ + countTest.projectCount, err = db.Count[project_model.Project](db.DefaultContext, &project_model.SearchOptions{ OwnerID: countTest.user.ID, IsClosed: optional.Some(false), Type: projectType, }) require.NoError(t, err) + countTest.packageCount, err = packages_model.CountOwnerPackages(db.DefaultContext, countTest.user.ID) + require.NoError(t, err) if !countTest.user.IsOrganization() { return @@ -114,6 +118,10 @@ func (countTest *userCountTest) TestPage(t *testing.T, page string, orgLink bool require.NoError(t, err) assert.Equal(t, countTest.projectCount, projectCount) + packageCount, err := countTest.getCount(htmlDoc.doc, "package-count") + require.NoError(t, err) + assert.Equal(t, countTest.packageCount, packageCount) + if !countTest.user.IsOrganization() { return }