From 3d0644b127b045ded6370fc63a26baa405e113c0 Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Tue, 13 May 2025 16:28:15 +0200 Subject: [PATCH 1/2] Fix remove org user failure on mssql * mssql does not support fetching 0 repositories * extend admin api test to purge user 2 --- services/org/user.go | 11 ++++++-- tests/integration/admin_user_test.go | 42 ++++++++++++++++++++++------ 2 files changed, 42 insertions(+), 11 deletions(-) diff --git a/services/org/user.go b/services/org/user.go index 3565ecc2fcd23..6b9b138f5f647 100644 --- a/services/org/user.go +++ b/services/org/user.go @@ -64,10 +64,15 @@ func RemoveOrgUser(ctx context.Context, org *organization.Organization, user *us if err != nil { return fmt.Errorf("AccessibleReposEnv: %w", err) } - repoIDs, err := env.RepoIDs(ctx, 1, org.NumRepos) - if err != nil { - return fmt.Errorf("GetUserRepositories [%d]: %w", user.ID, err) + var repoIDs []int64 + // mssql does not support fetching 0 repositories + if org.NumRepos > 0 { + repoIDs, err = env.RepoIDs(ctx, 1, org.NumRepos) + if err != nil { + return fmt.Errorf("GetUserRepositories [%d]: %w", user.ID, err) + } } + for _, repoID := range repoIDs { repo, err := repo_model.GetRepositoryByID(ctx, repoID) if err != nil { diff --git a/tests/integration/admin_user_test.go b/tests/integration/admin_user_test.go index d5d7e70bc7ef2..95e03ab75023e 100644 --- a/tests/integration/admin_user_test.go +++ b/tests/integration/admin_user_test.go @@ -4,6 +4,7 @@ package integration import ( + "fmt" "net/http" "strconv" "testing" @@ -72,12 +73,37 @@ func TestAdminDeleteUser(t *testing.T) { session := loginUser(t, "user1") - csrf := GetUserCSRFToken(t, session) - req := NewRequestWithValues(t, "POST", "/-/admin/users/8/delete", map[string]string{ - "_csrf": csrf, - }) - session.MakeRequest(t, req, http.StatusSeeOther) - - assertUserDeleted(t, 8) - unittest.CheckConsistencyFor(t, &user_model.User{}) + usersToDelete := []struct { + userID int64 + purge bool + }{ + { + userID: 2, + purge: true, + }, + { + userID: 8, + }, + } + + for _, entry := range usersToDelete { + t.Run(fmt.Sprintf("DeleteUser%d", entry.userID), func(t *testing.T) { + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: entry.userID}) + assert.NotNil(t, user) + + var query string + if entry.purge { + query = "?purge=true" + } + + csrf := GetUserCSRFToken(t, session) + req := NewRequestWithValues(t, "POST", fmt.Sprintf("/-/admin/users/%d/delete%s", entry.userID, query), map[string]string{ + "_csrf": csrf, + }) + session.MakeRequest(t, req, http.StatusSeeOther) + + assertUserDeleted(t, entry.userID) + unittest.CheckConsistencyFor(t, &user_model.User{}) + }) + } } From 1c32f8cffe582933aa0db3edee94aebb7f898b9a Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 14 May 2025 01:03:00 +0800 Subject: [PATCH 2/2] fix --- models/activities/action.go | 2 +- models/organization/org_test.go | 21 +-------------------- models/repo/org_repo.go | 31 ++++--------------------------- services/org/user.go | 10 +++------- 4 files changed, 9 insertions(+), 55 deletions(-) diff --git a/models/activities/action.go b/models/activities/action.go index c89ba3e14e099..6f1837d9f6b4e 100644 --- a/models/activities/action.go +++ b/models/activities/action.go @@ -530,7 +530,7 @@ func ActivityQueryCondition(ctx context.Context, opts GetFeedsOptions) (builder. if opts.RequestedTeam != nil { env := repo_model.AccessibleTeamReposEnv(organization.OrgFromUser(opts.RequestedUser), opts.RequestedTeam) - teamRepoIDs, err := env.RepoIDs(ctx, 1, opts.RequestedUser.NumRepos) + teamRepoIDs, err := env.RepoIDs(ctx) if err != nil { return nil, fmt.Errorf("GetTeamRepositories: %w", err) } diff --git a/models/organization/org_test.go b/models/organization/org_test.go index 666a6c44d4ee7..234325a8cd87e 100644 --- a/models/organization/org_test.go +++ b/models/organization/org_test.go @@ -334,7 +334,7 @@ func TestAccessibleReposEnv_RepoIDs(t *testing.T) { testSuccess := func(userID int64, expectedRepoIDs []int64) { env, err := repo_model.AccessibleReposEnv(db.DefaultContext, org, userID) assert.NoError(t, err) - repoIDs, err := env.RepoIDs(db.DefaultContext, 1, 100) + repoIDs, err := env.RepoIDs(db.DefaultContext) assert.NoError(t, err) assert.Equal(t, expectedRepoIDs, repoIDs) } @@ -342,25 +342,6 @@ func TestAccessibleReposEnv_RepoIDs(t *testing.T) { testSuccess(4, []int64{3, 32}) } -func TestAccessibleReposEnv_Repos(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) - org := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3}) - testSuccess := func(userID int64, expectedRepoIDs []int64) { - env, err := repo_model.AccessibleReposEnv(db.DefaultContext, org, userID) - assert.NoError(t, err) - repos, err := env.Repos(db.DefaultContext, 1, 100) - assert.NoError(t, err) - expectedRepos := make(repo_model.RepositoryList, len(expectedRepoIDs)) - for i, repoID := range expectedRepoIDs { - expectedRepos[i] = unittest.AssertExistsAndLoadBean(t, - &repo_model.Repository{ID: repoID}) - } - assert.Equal(t, expectedRepos, repos) - } - testSuccess(2, []int64{3, 5, 32}) - testSuccess(4, []int64{3, 32}) -} - func TestAccessibleReposEnv_MirrorRepos(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) org := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3}) diff --git a/models/repo/org_repo.go b/models/repo/org_repo.go index fa519d25b1980..96f21ba2aca7a 100644 --- a/models/repo/org_repo.go +++ b/models/repo/org_repo.go @@ -48,8 +48,7 @@ func GetTeamRepositories(ctx context.Context, opts *SearchTeamRepoOptions) (Repo // accessible to a particular user type AccessibleReposEnvironment interface { CountRepos(ctx context.Context) (int64, error) - RepoIDs(ctx context.Context, page, pageSize int) ([]int64, error) - Repos(ctx context.Context, page, pageSize int) (RepositoryList, error) + RepoIDs(ctx context.Context) ([]int64, error) MirrorRepos(ctx context.Context) (RepositoryList, error) AddKeyword(keyword string) SetSort(db.SearchOrderBy) @@ -132,40 +131,18 @@ func (env *accessibleReposEnv) CountRepos(ctx context.Context) (int64, error) { return repoCount, nil } -func (env *accessibleReposEnv) RepoIDs(ctx context.Context, page, pageSize int) ([]int64, error) { - if page <= 0 { - page = 1 - } - - repoIDs := make([]int64, 0, pageSize) +func (env *accessibleReposEnv) RepoIDs(ctx context.Context) ([]int64, error) { + var repoIDs []int64 return repoIDs, db.GetEngine(ctx). Table("repository"). Join("INNER", "team_repo", "`team_repo`.repo_id=`repository`.id"). Where(env.cond()). - GroupBy("`repository`.id,`repository`."+strings.Fields(string(env.orderBy))[0]). + GroupBy("`repository`.id,`repository`." + strings.Fields(string(env.orderBy))[0]). OrderBy(string(env.orderBy)). - Limit(pageSize, (page-1)*pageSize). Cols("`repository`.id"). Find(&repoIDs) } -func (env *accessibleReposEnv) Repos(ctx context.Context, page, pageSize int) (RepositoryList, error) { - repoIDs, err := env.RepoIDs(ctx, page, pageSize) - if err != nil { - return nil, fmt.Errorf("GetUserRepositoryIDs: %w", err) - } - - repos := make([]*Repository, 0, len(repoIDs)) - if len(repoIDs) == 0 { - return repos, nil - } - - return repos, db.GetEngine(ctx). - In("`repository`.id", repoIDs). - OrderBy(string(env.orderBy)). - Find(&repos) -} - func (env *accessibleReposEnv) MirrorRepoIDs(ctx context.Context) ([]int64, error) { repoIDs := make([]int64, 0, 10) return repoIDs, db.GetEngine(ctx). diff --git a/services/org/user.go b/services/org/user.go index 6b9b138f5f647..26927253d25a2 100644 --- a/services/org/user.go +++ b/services/org/user.go @@ -64,13 +64,9 @@ func RemoveOrgUser(ctx context.Context, org *organization.Organization, user *us if err != nil { return fmt.Errorf("AccessibleReposEnv: %w", err) } - var repoIDs []int64 - // mssql does not support fetching 0 repositories - if org.NumRepos > 0 { - repoIDs, err = env.RepoIDs(ctx, 1, org.NumRepos) - if err != nil { - return fmt.Errorf("GetUserRepositories [%d]: %w", user.ID, err) - } + repoIDs, err := env.RepoIDs(ctx) + if err != nil { + return fmt.Errorf("GetUserRepositories [%d]: %w", user.ID, err) } for _, repoID := range repoIDs {