From df785f620082c167900a589b0b66de394a8e4a80 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 22 Oct 2024 21:51:22 -0700 Subject: [PATCH 1/9] Fix edit team --- models/org_team.go | 9 +++-- modules/structs/org_team.go | 2 +- routers/api/v1/org/team.go | 46 ++++++++++++-------------- routers/web/org/teams.go | 51 ++++------------------------- services/doctor/fix8312.go | 2 +- services/forms/org.go | 11 ++----- services/org/team.go | 65 +++++++++++++++++++++++++++++++++++++ 7 files changed, 103 insertions(+), 83 deletions(-) create mode 100644 services/org/team.go diff --git a/models/org_team.go b/models/org_team.go index b6908478c7ea5..b4ea63d651b67 100644 --- a/models/org_team.go +++ b/models/org_team.go @@ -7,6 +7,7 @@ package models import ( "context" "fmt" + "slices" "strings" "code.gitea.io/gitea/models/db" @@ -218,11 +219,14 @@ func NewTeam(ctx context.Context, t *organization.Team) (err error) { } // UpdateTeam updates information of team. -func UpdateTeam(ctx context.Context, t *organization.Team, authChanged, includeAllChanged bool) (err error) { +func UpdateTeam(ctx context.Context, t *organization.Team, updateCols ...string) (err error) { if len(t.Name) == 0 { return util.NewInvalidArgumentErrorf("empty team name") } + authChanged := slices.Contains(updateCols, "authorize") + includeAllChanged := slices.Contains(updateCols, "includes_all_repositories") + if len(t.Description) > 255 { t.Description = t.Description[:255] } @@ -246,8 +250,7 @@ func UpdateTeam(ctx context.Context, t *organization.Team, authChanged, includeA } sess := db.GetEngine(ctx) - if _, err = sess.ID(t.ID).Cols("name", "lower_name", "description", - "can_create_org_repo", "authorize", "includes_all_repositories").Update(t); err != nil { + if _, err = sess.ID(t.ID).Cols(updateCols...).Update(t); err != nil { return fmt.Errorf("update: %w", err) } diff --git a/modules/structs/org_team.go b/modules/structs/org_team.go index f8899b236bf4f..0db38c87ca5f7 100644 --- a/modules/structs/org_team.go +++ b/modules/structs/org_team.go @@ -44,7 +44,7 @@ type EditTeamOption struct { Description *string `json:"description" binding:"MaxSize(255)"` IncludesAllRepositories *bool `json:"includes_all_repositories"` // enum: read,write,admin - Permission string `json:"permission"` + Permission string `json:"permission" binding:"` // example: ["repo.code","repo.issues","repo.ext_issues","repo.wiki","repo.pulls","repo.releases","repo.projects","repo.ext_wiki"] // Deprecated: This variable should be replaced by UnitsMap and will be dropped in later versions. Units []string `json:"units"` diff --git a/routers/api/v1/org/team.go b/routers/api/v1/org/team.go index c55837ff44fd4..c554888c3de49 100644 --- a/routers/api/v1/org/team.go +++ b/routers/api/v1/org/team.go @@ -293,45 +293,41 @@ func EditTeam(ctx *context.APIContext) { team.CanCreateOrgRepo = team.IsOwnerTeam() || *form.CanCreateOrgRepo } + teamName := team.Name if len(form.Name) > 0 { - team.Name = form.Name + teamName = form.Name } + description := team.Description if form.Description != nil { - team.Description = *form.Description + description = *form.Description } - isAuthChanged := false - isIncludeAllChanged := false - if !team.IsOwnerTeam() && len(form.Permission) != 0 { - // Validate permission level. - p := perm.ParseAccessMode(form.Permission) - if p < perm.AccessModeAdmin && len(form.UnitsMap) > 0 { - p = unit_model.MinUnitAccessMode(convertUnitsMap(form.UnitsMap)) - } - - if team.AccessMode != p { - isAuthChanged = true - team.AccessMode = p - } + includeAllRepos := team.IncludesAllRepositories + if form.IncludesAllRepositories != nil { + includeAllRepos = *form.IncludesAllRepositories + } - if form.IncludesAllRepositories != nil { - isIncludeAllChanged = true - team.IncludesAllRepositories = *form.IncludesAllRepositories - } + canCreateOrgRepo := team.CanCreateOrgRepo + if form.CanCreateOrgRepo != nil { + canCreateOrgRepo = *form.CanCreateOrgRepo } + unitPerms := make(map[unit_model.Type]perm.AccessMode) if team.AccessMode < perm.AccessModeAdmin { if len(form.UnitsMap) > 0 { - attachTeamUnitsMap(team, form.UnitsMap) - } else if len(form.Units) > 0 { - attachTeamUnits(team, form.Units) + unitPerms = convertUnitsMap(form.UnitsMap) } - } else { - attachAdminTeamUnits(team) } - if err := models.UpdateTeam(ctx, team, isAuthChanged, isIncludeAllChanged); err != nil { + if err := org_service.UpdateTeam(ctx, team, + teamName, + description, + form.Permission == "admin", + includeAllRepos, + canCreateOrgRepo, + unitPerms, + ); err != nil { ctx.Error(http.StatusInternalServerError, "EditTeam", err) return } diff --git a/routers/web/org/teams.go b/routers/web/org/teams.go index 31b9601ce79d7..30e38b93c462c 100644 --- a/routers/web/org/teams.go +++ b/routers/web/org/teams.go @@ -482,61 +482,24 @@ func EditTeam(ctx *context.Context) { func EditTeamPost(ctx *context.Context) { form := web.GetForm(ctx).(*forms.CreateTeamForm) t := ctx.Org.Team - newAccessMode := perm.ParseAccessMode(form.Permission) - unitPerms := getUnitPerms(ctx.Req.Form, newAccessMode) - if newAccessMode < perm.AccessModeAdmin { - // if newAccessMode is less than admin accessmode, then it should be general accessmode, - // so we should calculate the minial accessmode from units accessmodes. - newAccessMode = unit_model.MinUnitAccessMode(unitPerms) - } - isAuthChanged := false - isIncludeAllChanged := false - includesAllRepositories := form.RepoAccess == "all" + unitPerms := getUnitPerms(ctx.Req.Form, perm.ParseAccessMode(form.Permission)) ctx.Data["Title"] = ctx.Org.Organization.FullName ctx.Data["PageIsOrgTeams"] = true ctx.Data["Team"] = t ctx.Data["Units"] = unit_model.Units - if !t.IsOwnerTeam() { - t.Name = form.TeamName - if t.AccessMode != newAccessMode { - isAuthChanged = true - t.AccessMode = newAccessMode - } - - if t.IncludesAllRepositories != includesAllRepositories { - isIncludeAllChanged = true - t.IncludesAllRepositories = includesAllRepositories - } - t.CanCreateOrgRepo = form.CanCreateOrgRepo - } else { - t.CanCreateOrgRepo = true - } - - t.Description = form.Description - units := make([]*org_model.TeamUnit, 0, len(unitPerms)) - for tp, perm := range unitPerms { - units = append(units, &org_model.TeamUnit{ - OrgID: t.OrgID, - TeamID: t.ID, - Type: tp, - AccessMode: perm, - }) - } - t.Units = units - - if ctx.HasError() { - ctx.HTML(http.StatusOK, tplTeamNew) - return - } - if t.AccessMode < perm.AccessModeAdmin && len(unitPerms) == 0 { ctx.RenderWithErr(ctx.Tr("form.team_no_units_error"), tplTeamNew, &form) return } - if err := models.UpdateTeam(ctx, t, isAuthChanged, isIncludeAllChanged); err != nil { + if err := org_service.UpdateTeam(ctx, t, form.TeamName, form.Description, + form.Permission == "admin", + form.RepoAccess == "all", + form.CanCreateOrgRepo, + unitPerms, + ); err != nil { ctx.Data["Err_TeamName"] = true switch { case org_model.IsErrTeamAlreadyExist(err): diff --git a/services/doctor/fix8312.go b/services/doctor/fix8312.go index 4fc049873adb7..cfceb35effb1c 100644 --- a/services/doctor/fix8312.go +++ b/services/doctor/fix8312.go @@ -29,7 +29,7 @@ func fixOwnerTeamCreateOrgRepo(ctx context.Context, logger log.Logger, autofix b return nil } - return models.UpdateTeam(ctx, team, false, false) + return models.UpdateTeam(ctx, team, "can_create_org_repo") }, ) if err != nil { diff --git a/services/forms/org.go b/services/forms/org.go index db182f7e96b5b..304432ef2d52b 100644 --- a/services/forms/org.go +++ b/services/forms/org.go @@ -53,19 +53,12 @@ func (f *UpdateOrgSettingForm) Validate(req *http.Request, errs binding.Errors) return middleware.Validate(errs, ctx.Data, f, ctx.Locale) } -// ___________ -// \__ ___/___ _____ _____ -// | |_/ __ \\__ \ / \ -// | |\ ___/ / __ \| Y Y \ -// |____| \___ >____ /__|_| / -// \/ \/ \/ - // CreateTeamForm form for creating team type CreateTeamForm struct { TeamName string `binding:"Required;AlphaDashDot;MaxSize(255)"` Description string `binding:"MaxSize(255)"` - Permission string - RepoAccess string + Permission string `binding:"Required;In(admin, read)"` + RepoAccess string `binding:"Required;In(specified, all)"` CanCreateOrgRepo bool } diff --git a/services/org/team.go b/services/org/team.go new file mode 100644 index 0000000000000..a913715a9ed14 --- /dev/null +++ b/services/org/team.go @@ -0,0 +1,65 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package org + +import ( + "context" + + "code.gitea.io/gitea/models" + org_model "code.gitea.io/gitea/models/organization" + "code.gitea.io/gitea/models/perm" + unit_model "code.gitea.io/gitea/models/unit" +) + +func UpdateTeam(ctx context.Context, team *org_model.Team, teamName, description string, isAdmin, includesAllRepositories, canCreateOrgRepo bool, unitPerms map[unit_model.Type]perm.AccessMode) error { + var changedCols []string + + newAccessMode := perm.AccessModeRead + if isAdmin { + newAccessMode = perm.AccessModeAdmin + } else { + // if newAccessMode is less than admin accessmode, then it should be general accessmode, + // so we should calculate the minial accessmode from units accessmodes. + newAccessMode = unit_model.MinUnitAccessMode(unitPerms) + } + + if !team.IsOwnerTeam() { + team.Name = teamName + if team.AccessMode != newAccessMode { + team.AccessMode = newAccessMode + changedCols = append(changedCols, "authorize") + } + + if team.IncludesAllRepositories != includesAllRepositories { + team.IncludesAllRepositories = includesAllRepositories + changedCols = append(changedCols, "includes_all_repositories") + } + units := make([]*org_model.TeamUnit, 0, len(unitPerms)) + for tp, perm := range unitPerms { + units = append(units, &org_model.TeamUnit{ + OrgID: team.OrgID, + TeamID: team.ID, + Type: tp, + AccessMode: perm, + }) + } + team.Units = units + changedCols = append(changedCols, "units") + if team.CanCreateOrgRepo != canCreateOrgRepo { + team.CanCreateOrgRepo = canCreateOrgRepo + changedCols = append(changedCols, "can_create_org_repo") + } + } else { + team.CanCreateOrgRepo = true + team.IncludesAllRepositories = true + changedCols = append(changedCols, "can_create_org_repo", "includes_all_repositories") + } + + if team.Description != description { + changedCols = append(changedCols, "description") + team.Description = description + } + + return models.UpdateTeam(ctx, team, changedCols...) +} From 8e1cac1185d32a120b761a70d316a25ff1077020 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 23 Oct 2024 11:03:53 -0700 Subject: [PATCH 2/9] Use UpdateTeamOptions instead of multiple parameters of function --- routers/api/v1/org/team.go | 16 ++++++++-------- routers/web/org/teams.go | 14 ++++++++------ services/org/team.go | 33 +++++++++++++++++++++------------ 3 files changed, 37 insertions(+), 26 deletions(-) diff --git a/routers/api/v1/org/team.go b/routers/api/v1/org/team.go index c554888c3de49..858bd66b207de 100644 --- a/routers/api/v1/org/team.go +++ b/routers/api/v1/org/team.go @@ -320,14 +320,14 @@ func EditTeam(ctx *context.APIContext) { } } - if err := org_service.UpdateTeam(ctx, team, - teamName, - description, - form.Permission == "admin", - includeAllRepos, - canCreateOrgRepo, - unitPerms, - ); err != nil { + if err := org_service.UpdateTeam(ctx, team, org_service.UpdateTeamOptions{ + TeamName: teamName, + Description: description, + IsAdmin: form.Permission == "admin", + IncludesAllRepositories: includeAllRepos, + CanCreateOrgRepo: canCreateOrgRepo, + UnitPerms: unitPerms, + }); err != nil { ctx.Error(http.StatusInternalServerError, "EditTeam", err) return } diff --git a/routers/web/org/teams.go b/routers/web/org/teams.go index 30e38b93c462c..ee54c446535d7 100644 --- a/routers/web/org/teams.go +++ b/routers/web/org/teams.go @@ -494,12 +494,14 @@ func EditTeamPost(ctx *context.Context) { return } - if err := org_service.UpdateTeam(ctx, t, form.TeamName, form.Description, - form.Permission == "admin", - form.RepoAccess == "all", - form.CanCreateOrgRepo, - unitPerms, - ); err != nil { + if err := org_service.UpdateTeam(ctx, t, org_service.UpdateTeamOptions{ + TeamName: form.TeamName, + Description: form.Description, + IsAdmin: form.Permission == "admin", + IncludesAllRepositories: form.RepoAccess == "all", + CanCreateOrgRepo: form.CanCreateOrgRepo, + UnitPerms: unitPerms, + }); err != nil { ctx.Data["Err_TeamName"] = true switch { case org_model.IsErrTeamAlreadyExist(err): diff --git a/services/org/team.go b/services/org/team.go index a913715a9ed14..82951aa4e2b1d 100644 --- a/services/org/team.go +++ b/services/org/team.go @@ -12,31 +12,40 @@ import ( unit_model "code.gitea.io/gitea/models/unit" ) -func UpdateTeam(ctx context.Context, team *org_model.Team, teamName, description string, isAdmin, includesAllRepositories, canCreateOrgRepo bool, unitPerms map[unit_model.Type]perm.AccessMode) error { +type UpdateTeamOptions struct { + TeamName string + Description string + IsAdmin bool + IncludesAllRepositories bool + CanCreateOrgRepo bool + UnitPerms map[unit_model.Type]perm.AccessMode +} + +func UpdateTeam(ctx context.Context, team *org_model.Team, opts UpdateTeamOptions) error { var changedCols []string newAccessMode := perm.AccessModeRead - if isAdmin { + if opts.IsAdmin { newAccessMode = perm.AccessModeAdmin } else { // if newAccessMode is less than admin accessmode, then it should be general accessmode, // so we should calculate the minial accessmode from units accessmodes. - newAccessMode = unit_model.MinUnitAccessMode(unitPerms) + newAccessMode = unit_model.MinUnitAccessMode(opts.UnitPerms) } if !team.IsOwnerTeam() { - team.Name = teamName + team.Name = opts.TeamName if team.AccessMode != newAccessMode { team.AccessMode = newAccessMode changedCols = append(changedCols, "authorize") } - if team.IncludesAllRepositories != includesAllRepositories { - team.IncludesAllRepositories = includesAllRepositories + if team.IncludesAllRepositories != opts.IncludesAllRepositories { + team.IncludesAllRepositories = opts.IncludesAllRepositories changedCols = append(changedCols, "includes_all_repositories") } - units := make([]*org_model.TeamUnit, 0, len(unitPerms)) - for tp, perm := range unitPerms { + units := make([]*org_model.TeamUnit, 0, len(opts.UnitPerms)) + for tp, perm := range opts.UnitPerms { units = append(units, &org_model.TeamUnit{ OrgID: team.OrgID, TeamID: team.ID, @@ -46,8 +55,8 @@ func UpdateTeam(ctx context.Context, team *org_model.Team, teamName, description } team.Units = units changedCols = append(changedCols, "units") - if team.CanCreateOrgRepo != canCreateOrgRepo { - team.CanCreateOrgRepo = canCreateOrgRepo + if team.CanCreateOrgRepo != opts.CanCreateOrgRepo { + team.CanCreateOrgRepo = opts.CanCreateOrgRepo changedCols = append(changedCols, "can_create_org_repo") } } else { @@ -56,9 +65,9 @@ func UpdateTeam(ctx context.Context, team *org_model.Team, teamName, description changedCols = append(changedCols, "can_create_org_repo", "includes_all_repositories") } - if team.Description != description { + if team.Description != opts.Description { changedCols = append(changedCols, "description") - team.Description = description + team.Description = opts.Description } return models.UpdateTeam(ctx, team, changedCols...) From 253ec37632e2d74d9d69d8cc0c7fb50cfc451b2c Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 26 Oct 2024 00:27:14 -0700 Subject: [PATCH 3/9] Fix lint and test --- models/org_team_test.go | 4 ++-- modules/structs/org_team.go | 2 +- services/forms/org.go | 2 +- services/org/team.go | 2 +- services/repository/create_test.go | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/models/org_team_test.go b/models/org_team_test.go index cf2c8be536d8e..7881d8f4a247c 100644 --- a/models/org_team_test.go +++ b/models/org_team_test.go @@ -84,7 +84,7 @@ func TestUpdateTeam(t *testing.T) { team.Name = "newName" team.Description = strings.Repeat("A long description!", 100) team.AccessMode = perm.AccessModeAdmin - assert.NoError(t, UpdateTeam(db.DefaultContext, team, true, false)) + assert.NoError(t, UpdateTeam(db.DefaultContext, team, "authorize")) team = unittest.AssertExistsAndLoadBean(t, &organization.Team{Name: "newName"}) assert.True(t, strings.HasPrefix(team.Description, "A long description!")) @@ -103,7 +103,7 @@ func TestUpdateTeam2(t *testing.T) { team.LowerName = "owners" team.Name = "Owners" team.Description = strings.Repeat("A long description!", 100) - err := UpdateTeam(db.DefaultContext, team, true, false) + err := UpdateTeam(db.DefaultContext, team, "authorize") assert.True(t, organization.IsErrTeamAlreadyExist(err)) unittest.CheckConsistencyFor(t, &organization.Team{ID: team.ID}) diff --git a/modules/structs/org_team.go b/modules/structs/org_team.go index 0db38c87ca5f7..e906a51ba380e 100644 --- a/modules/structs/org_team.go +++ b/modules/structs/org_team.go @@ -44,7 +44,7 @@ type EditTeamOption struct { Description *string `json:"description" binding:"MaxSize(255)"` IncludesAllRepositories *bool `json:"includes_all_repositories"` // enum: read,write,admin - Permission string `json:"permission" binding:"` + Permission string `json:"permission" binding:"Required;In(read,write,admin)"` // example: ["repo.code","repo.issues","repo.ext_issues","repo.wiki","repo.pulls","repo.releases","repo.projects","repo.ext_wiki"] // Deprecated: This variable should be replaced by UnitsMap and will be dropped in later versions. Units []string `json:"units"` diff --git a/services/forms/org.go b/services/forms/org.go index 304432ef2d52b..a6df8aadeed72 100644 --- a/services/forms/org.go +++ b/services/forms/org.go @@ -57,7 +57,7 @@ func (f *UpdateOrgSettingForm) Validate(req *http.Request, errs binding.Errors) type CreateTeamForm struct { TeamName string `binding:"Required;AlphaDashDot;MaxSize(255)"` Description string `binding:"MaxSize(255)"` - Permission string `binding:"Required;In(admin, read)"` + Permission string `binding:"Required;In(read,write,admin)"` RepoAccess string `binding:"Required;In(specified, all)"` CanCreateOrgRepo bool } diff --git a/services/org/team.go b/services/org/team.go index 82951aa4e2b1d..f4b0d76324671 100644 --- a/services/org/team.go +++ b/services/org/team.go @@ -24,7 +24,7 @@ type UpdateTeamOptions struct { func UpdateTeam(ctx context.Context, team *org_model.Team, opts UpdateTeamOptions) error { var changedCols []string - newAccessMode := perm.AccessModeRead + var newAccessMode perm.AccessMode if opts.IsAdmin { newAccessMode = perm.AccessModeAdmin } else { diff --git a/services/repository/create_test.go b/services/repository/create_test.go index 41e6b615db7f3..bd8d9e0efaffc 100644 --- a/services/repository/create_test.go +++ b/services/repository/create_test.go @@ -111,7 +111,7 @@ func TestIncludesAllRepositoriesTeams(t *testing.T) { teams[4].IncludesAllRepositories = true teamRepos[4] = repoIDs for i, team := range teams { - assert.NoError(t, models.UpdateTeam(db.DefaultContext, team, false, true), "%s: UpdateTeam", team.Name) + assert.NoError(t, models.UpdateTeam(db.DefaultContext, team, "includes_all_repositories"), "%s: UpdateTeam", team.Name) testTeamRepositories(team.ID, teamRepos[i]) } From afade2f0c74b5fe77656640e361c573135561d96 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 30 Oct 2024 10:59:08 -0700 Subject: [PATCH 4/9] Fix test --- models/org_team_test.go | 4 ++-- modules/structs/org_team.go | 2 +- services/forms/org.go | 2 +- services/org/team.go | 32 ++++++++++++++++++++------------ 4 files changed, 24 insertions(+), 16 deletions(-) diff --git a/models/org_team_test.go b/models/org_team_test.go index 7881d8f4a247c..8e9d91dbfdabb 100644 --- a/models/org_team_test.go +++ b/models/org_team_test.go @@ -84,7 +84,7 @@ func TestUpdateTeam(t *testing.T) { team.Name = "newName" team.Description = strings.Repeat("A long description!", 100) team.AccessMode = perm.AccessModeAdmin - assert.NoError(t, UpdateTeam(db.DefaultContext, team, "authorize")) + assert.NoError(t, UpdateTeam(db.DefaultContext, team, "name", "lower_name", "description", "authorize")) team = unittest.AssertExistsAndLoadBean(t, &organization.Team{Name: "newName"}) assert.True(t, strings.HasPrefix(team.Description, "A long description!")) @@ -103,7 +103,7 @@ func TestUpdateTeam2(t *testing.T) { team.LowerName = "owners" team.Name = "Owners" team.Description = strings.Repeat("A long description!", 100) - err := UpdateTeam(db.DefaultContext, team, "authorize") + err := UpdateTeam(db.DefaultContext, team, "name", "lower_name", "description", "authorize") assert.True(t, organization.IsErrTeamAlreadyExist(err)) unittest.CheckConsistencyFor(t, &organization.Team{ID: team.ID}) diff --git a/modules/structs/org_team.go b/modules/structs/org_team.go index e906a51ba380e..f8899b236bf4f 100644 --- a/modules/structs/org_team.go +++ b/modules/structs/org_team.go @@ -44,7 +44,7 @@ type EditTeamOption struct { Description *string `json:"description" binding:"MaxSize(255)"` IncludesAllRepositories *bool `json:"includes_all_repositories"` // enum: read,write,admin - Permission string `json:"permission" binding:"Required;In(read,write,admin)"` + Permission string `json:"permission"` // example: ["repo.code","repo.issues","repo.ext_issues","repo.wiki","repo.pulls","repo.releases","repo.projects","repo.ext_wiki"] // Deprecated: This variable should be replaced by UnitsMap and will be dropped in later versions. Units []string `json:"units"` diff --git a/services/forms/org.go b/services/forms/org.go index a6df8aadeed72..4d85897e90ccb 100644 --- a/services/forms/org.go +++ b/services/forms/org.go @@ -57,7 +57,7 @@ func (f *UpdateOrgSettingForm) Validate(req *http.Request, errs binding.Errors) type CreateTeamForm struct { TeamName string `binding:"Required;AlphaDashDot;MaxSize(255)"` Description string `binding:"MaxSize(255)"` - Permission string `binding:"Required;In(read,write,admin)"` + Permission string `binding:"Required;In(,read,write,admin)"` RepoAccess string `binding:"Required;In(specified, all)"` CanCreateOrgRepo bool } diff --git a/services/org/team.go b/services/org/team.go index f4b0d76324671..26389bcf69bf9 100644 --- a/services/org/team.go +++ b/services/org/team.go @@ -5,6 +5,7 @@ package org import ( "context" + "strings" "code.gitea.io/gitea/models" org_model "code.gitea.io/gitea/models/organization" @@ -34,7 +35,12 @@ func UpdateTeam(ctx context.Context, team *org_model.Team, opts UpdateTeamOption } if !team.IsOwnerTeam() { - team.Name = opts.TeamName + if team.Name != opts.TeamName { + team.Name = opts.TeamName + team.LowerName = strings.ToLower(opts.TeamName) + changedCols = append(changedCols, "name", "lower_name") + } + if team.AccessMode != newAccessMode { team.AccessMode = newAccessMode changedCols = append(changedCols, "authorize") @@ -44,22 +50,24 @@ func UpdateTeam(ctx context.Context, team *org_model.Team, opts UpdateTeamOption team.IncludesAllRepositories = opts.IncludesAllRepositories changedCols = append(changedCols, "includes_all_repositories") } - units := make([]*org_model.TeamUnit, 0, len(opts.UnitPerms)) - for tp, perm := range opts.UnitPerms { - units = append(units, &org_model.TeamUnit{ - OrgID: team.OrgID, - TeamID: team.ID, - Type: tp, - AccessMode: perm, - }) + if len(opts.UnitPerms) > 0 { + units := make([]*org_model.TeamUnit, 0, len(opts.UnitPerms)) + for tp, perm := range opts.UnitPerms { + units = append(units, &org_model.TeamUnit{ + OrgID: team.OrgID, + TeamID: team.ID, + Type: tp, + AccessMode: perm, + }) + } + team.Units = units + changedCols = append(changedCols, "units") } - team.Units = units - changedCols = append(changedCols, "units") if team.CanCreateOrgRepo != opts.CanCreateOrgRepo { team.CanCreateOrgRepo = opts.CanCreateOrgRepo changedCols = append(changedCols, "can_create_org_repo") } - } else { + } else { // make the possible legacy data correct, we force to update these fields team.CanCreateOrgRepo = true team.IncludesAllRepositories = true changedCols = append(changedCols, "can_create_org_repo", "includes_all_repositories") From c0edbd4d676b71575f078ef8ab57561770b3651f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 30 Oct 2024 15:34:13 -0700 Subject: [PATCH 5/9] Fix test --- models/org_team.go | 2 +- routers/api/v1/org/team.go | 7 ++++++- services/org/team.go | 4 ++-- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/models/org_team.go b/models/org_team.go index b4ea63d651b67..443e78996d006 100644 --- a/models/org_team.go +++ b/models/org_team.go @@ -255,7 +255,7 @@ func UpdateTeam(ctx context.Context, t *organization.Team, updateCols ...string) } // update units for team - if len(t.Units) > 0 { + if slices.Contains(updateCols, "units") { for _, unit := range t.Units { unit.TeamID = t.ID } diff --git a/routers/api/v1/org/team.go b/routers/api/v1/org/team.go index 858bd66b207de..4ca8c8f85240c 100644 --- a/routers/api/v1/org/team.go +++ b/routers/api/v1/org/team.go @@ -320,10 +320,15 @@ func EditTeam(ctx *context.APIContext) { } } + isAdmin := team.AccessMode == perm.AccessModeAdmin + if form.Permission != "" && form.Permission == "admin" { + isAdmin = true + } + if err := org_service.UpdateTeam(ctx, team, org_service.UpdateTeamOptions{ TeamName: teamName, Description: description, - IsAdmin: form.Permission == "admin", + IsAdmin: isAdmin, IncludesAllRepositories: includeAllRepos, CanCreateOrgRepo: canCreateOrgRepo, UnitPerms: unitPerms, diff --git a/services/org/team.go b/services/org/team.go index 26389bcf69bf9..6dccc7a8f8e88 100644 --- a/services/org/team.go +++ b/services/org/team.go @@ -25,10 +25,10 @@ type UpdateTeamOptions struct { func UpdateTeam(ctx context.Context, team *org_model.Team, opts UpdateTeamOptions) error { var changedCols []string - var newAccessMode perm.AccessMode + newAccessMode := team.AccessMode if opts.IsAdmin { newAccessMode = perm.AccessModeAdmin - } else { + } else if len(opts.UnitPerms) > 0 { // if newAccessMode is less than admin accessmode, then it should be general accessmode, // so we should calculate the minial accessmode from units accessmodes. newAccessMode = unit_model.MinUnitAccessMode(opts.UnitPerms) From ec8fbde2c75f4f97836b7ef2160c1763461c31ac Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 30 Oct 2024 17:48:16 -0700 Subject: [PATCH 6/9] Add more check --- models/org_team.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/models/org_team.go b/models/org_team.go index 443e78996d006..ea03de71dd206 100644 --- a/models/org_team.go +++ b/models/org_team.go @@ -224,6 +224,11 @@ func UpdateTeam(ctx context.Context, t *organization.Team, updateCols ...string) return util.NewInvalidArgumentErrorf("empty team name") } + if slices.Contains(updateCols, "name") && !slices.Contains(updateCols, "lower_name") { + t.LowerName = strings.ToLower(t.Name) + updateCols = append(updateCols, "lower_name") + } + authChanged := slices.Contains(updateCols, "authorize") includeAllChanged := slices.Contains(updateCols, "includes_all_repositories") From c7da8860aaae745b443e465389ef850da8e25f85 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 1 Nov 2024 13:13:00 -0700 Subject: [PATCH 7/9] Fix test --- models/organization/team_fix.go | 72 ++++++++++++++++++++++++++++++ services/doctor/dbconsistency.go | 7 +++ tests/integration/api_team_test.go | 30 +++++++++++-- tests/integration/org_test.go | 40 +++++++++++++++++ 4 files changed, 145 insertions(+), 4 deletions(-) create mode 100644 models/organization/team_fix.go diff --git a/models/organization/team_fix.go b/models/organization/team_fix.go new file mode 100644 index 0000000000000..42f852167d37a --- /dev/null +++ b/models/organization/team_fix.go @@ -0,0 +1,72 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package organization + +import ( + "context" + "strings" + + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/perm" + "code.gitea.io/gitea/models/unit" + + "xorm.io/builder" +) + +// CountInconsistentOwnerTeams returns the amount of owner teams that any of +// their access modes not set to the right value. For external trackers and external wikis +// it should be Read and otherwise it should be Owner. +func CountInconsistentOwnerTeams(ctx context.Context) (int64, error) { + cnt1, err := db.GetEngine(ctx).Table("team"). + Join("INNER", "team_unit", "`team`.id = `team_unit`.team_id"). + Where("`team`.lower_name = ?", strings.ToLower(OwnerTeamName)). + And(builder.Or(builder.Eq{"`team_unit`.`type`": unit.TypeExternalTracker}, builder.Eq{"`team_unit`.`type`": unit.TypeExternalWiki})). + And("`team_unit`.`access_mode` <> ?", perm.AccessModeRead). + GroupBy("`team`.`id`"). + Count() + if err != nil { + return 0, err + } + + cnt2, err := db.GetEngine(ctx).Table("team"). + Join("INNER", "team_unit", "`team`.id = `team_unit`.team_id"). + Where("`team`.lower_name = ?", strings.ToLower(OwnerTeamName)). + And(builder.And(builder.Neq{"`team_unit`.`type`": unit.TypeExternalTracker}, builder.Neq{"`team_unit`.`type`": unit.TypeExternalWiki})). + And("`team_unit`.`access_mode` <> ?", perm.AccessModeOwner). + GroupBy("`team_unit`.team_id"). + Count() + if err != nil { + return 0, err + } + return cnt1 + cnt2, nil +} + +// FixInconsistentOwnerTeams fixes inconsistent owner teams that of +// their access modes not set to the right value. For external trackers and external wikis +// it should be Read and otherwise it should be Owner. +func FixInconsistentOwnerTeams(ctx context.Context) (int64, error) { + subQuery := builder.Select("id").From("team").Where(builder.Eq{"lower_name": strings.ToLower(OwnerTeamName)}) + updated1, err := db.GetEngine(ctx).Table("team_unit"). + Where(builder.Or(builder.Eq{"`team_unit`.`type`": unit.TypeExternalTracker}, builder.Eq{"`team_unit`.`type`": unit.TypeExternalWiki})). + In("team_id", subQuery). + Cols("access_mode"). + Update(&TeamUnit{ + AccessMode: perm.AccessModeRead, + }) + if err != nil { + return 0, err + } + + updated2, err := db.GetEngine(ctx).Table("team_unit"). + Where(builder.And(builder.Neq{"`team_unit`.`type`": unit.TypeExternalTracker}, builder.Neq{"`team_unit`.`type`": unit.TypeExternalWiki})). + In("team_id", subQuery). + Cols("access_mode"). + Update(&TeamUnit{ + AccessMode: perm.AccessModeOwner, + }) + if err != nil { + return 0, err + } + return updated1 + updated2, nil +} diff --git a/services/doctor/dbconsistency.go b/services/doctor/dbconsistency.go index 7cb7445148a0b..1527a46286673 100644 --- a/services/doctor/dbconsistency.go +++ b/services/doctor/dbconsistency.go @@ -11,6 +11,7 @@ import ( "code.gitea.io/gitea/models/db" issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/migrations" + org_model "code.gitea.io/gitea/models/organization" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" @@ -164,6 +165,12 @@ func prepareDBConsistencyChecks() []consistencyCheck { Fixer: repo_model.DeleteOrphanedTopics, FixedMessage: "Removed", }, + { + Name: "Owner teams with wrong admin access unit", + Counter: org_model.CountInconsistentOwnerTeams, + Fixer: org_model.FixInconsistentOwnerTeams, + FixedMessage: "Fixed", + }, } // TODO: function to recalc all counters diff --git a/tests/integration/api_team_test.go b/tests/integration/api_team_test.go index d14c66ff2ca64..f7bfd4e06a208 100644 --- a/tests/integration/api_team_test.go +++ b/tests/integration/api_team_test.go @@ -12,6 +12,7 @@ import ( auth_model "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/organization" + org_model "code.gitea.io/gitea/models/organization" "code.gitea.io/gitea/models/perm" "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unit" @@ -42,6 +43,7 @@ func TestAPITeam(t *testing.T) { DecodeJSON(t, resp, &apiTeam) assert.EqualValues(t, team.ID, apiTeam.ID) assert.Equal(t, team.Name, apiTeam.Name) + assert.Equal(t, team.Description, apiTeam.Description) assert.EqualValues(t, convert.ToOrganization(db.DefaultContext, org), apiTeam.Organization) // non team member user will not access the teams details @@ -59,10 +61,30 @@ func TestAPITeam(t *testing.T) { // Get an admin user able to create, update and delete teams. user = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + org = unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 6}) session = loginUser(t, user.Name) token = getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteOrganization) - org = unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 6}) + // edit owner team + editDescription := "this is owner team" + editFalse := false + teamToEdit := &api.EditTeamOption{ + Name: "not_owner_team", // should be ignored + Description: &editDescription, + Permission: "admin", // should be ignored + IncludesAllRepositories: &editFalse, // should be ignored + Units: []string{"repo.code", "repo.pulls", "repo.releases"}, // should be ignored + } + + req = NewRequestWithJSON(t, "PATCH", fmt.Sprintf("/api/v1/teams/%d", team.ID), teamToEdit). + AddTokenAuth(token) + resp = MakeRequest(t, req, http.StatusOK) + apiTeam = api.Team{} + DecodeJSON(t, resp, &apiTeam) + checkTeamResponse(t, "Edit Owner Team", &apiTeam, org_model.OwnerTeamName, *teamToEdit.Description, true, + "owner", unit.AllUnitKeyNames(), nil) + checkTeamBean(t, apiTeam.ID, org_model.OwnerTeamName, *teamToEdit.Description, true, + "owner", unit.AllUnitKeyNames(), nil) // Create team. teamToCreate := &api.CreateTeamOption{ @@ -84,9 +106,9 @@ func TestAPITeam(t *testing.T) { teamID := apiTeam.ID // Edit team. - editDescription := "team 1" - editFalse := false - teamToEdit := &api.EditTeamOption{ + editDescription = "team 1" + editFalse = false + teamToEdit = &api.EditTeamOption{ Name: "teamone", Description: &editDescription, Permission: "admin", diff --git a/tests/integration/org_test.go b/tests/integration/org_test.go index ef4ef2bb9b429..b5186cb70aacb 100644 --- a/tests/integration/org_test.go +++ b/tests/integration/org_test.go @@ -10,6 +10,10 @@ import ( "testing" auth_model "code.gitea.io/gitea/models/auth" + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/organization" + org_model "code.gitea.io/gitea/models/organization" + "code.gitea.io/gitea/models/perm" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" api "code.gitea.io/gitea/modules/structs" @@ -218,3 +222,39 @@ func TestTeamSearch(t *testing.T) { req = NewRequestf(t, "GET", "/org/%s/teams/-/search?q=%s", org.Name, "team") session.MakeRequest(t, req, http.StatusNotFound) } + +func TestOwnerTeamsEdit(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + org := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3, Type: user_model.UserTypeOrganization}) + session := loginUser(t, user1.Name) + + team := unittest.AssertExistsAndLoadBean(t, &organization.Team{OrgID: org.ID, Name: org_model.OwnerTeamName}) + assert.EqualValues(t, perm.AccessModeOwner, team.AccessMode) + assert.EqualValues(t, "", team.Description) + assert.NoError(t, team.LoadUnits(db.DefaultContext)) + assert.EqualValues(t, 7, len(team.Units)) + for _, unit := range team.Units { + assert.EqualValues(t, perm.AccessModeOwner, unit.AccessMode) + } + + req := NewRequestWithValues(t, "POST", fmt.Sprintf("/org/%s/teams/owners/edit", org.Name), map[string]string{ + "_csrf": GetUserCSRFToken(t, session), + "team_name": "Not Owners", // This change should be ignored + "description": "Just a description", + }) + session.MakeRequest(t, req, http.StatusSeeOther) + + // reload team + team = unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: team.ID}) + assert.EqualValues(t, perm.AccessModeOwner, team.AccessMode) + assert.EqualValues(t, org_model.OwnerTeamName, team.Name) + assert.EqualValues(t, "Just a description", team.Description) + + assert.NoError(t, team.LoadUnits(db.DefaultContext)) + assert.EqualValues(t, 7, len(team.Units)) + for _, unit := range team.Units { + assert.EqualValues(t, perm.AccessModeOwner, unit.AccessMode) + } +} From 9c0cc9b3941bda9a3db5b696bfb9f184f9a11085 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 1 Nov 2024 13:28:44 -0700 Subject: [PATCH 8/9] Fix lint --- tests/integration/api_team_test.go | 27 +++++++++++++-------------- tests/integration/org_test.go | 5 ++--- 2 files changed, 15 insertions(+), 17 deletions(-) diff --git a/tests/integration/api_team_test.go b/tests/integration/api_team_test.go index f7bfd4e06a208..28a400973a048 100644 --- a/tests/integration/api_team_test.go +++ b/tests/integration/api_team_test.go @@ -11,7 +11,6 @@ import ( auth_model "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/db" - "code.gitea.io/gitea/models/organization" org_model "code.gitea.io/gitea/models/organization" "code.gitea.io/gitea/models/perm" "code.gitea.io/gitea/models/repo" @@ -28,9 +27,9 @@ import ( func TestAPITeam(t *testing.T) { defer tests.PrepareTestEnv(t)() - teamUser := unittest.AssertExistsAndLoadBean(t, &organization.TeamUser{ID: 1}) - team := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: teamUser.TeamID}) - org := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: teamUser.OrgID}) + teamUser := unittest.AssertExistsAndLoadBean(t, &org_model.TeamUser{ID: 1}) + team := unittest.AssertExistsAndLoadBean(t, &org_model.Team{ID: teamUser.TeamID}) + org := unittest.AssertExistsAndLoadBean(t, &org_model.Organization{ID: teamUser.OrgID}) user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: teamUser.UID}) session := loginUser(t, user.Name) @@ -47,7 +46,7 @@ func TestAPITeam(t *testing.T) { assert.EqualValues(t, convert.ToOrganization(db.DefaultContext, org), apiTeam.Organization) // non team member user will not access the teams details - teamUser2 := unittest.AssertExistsAndLoadBean(t, &organization.TeamUser{ID: 3}) + teamUser2 := unittest.AssertExistsAndLoadBean(t, &org_model.TeamUser{ID: 3}) user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: teamUser2.UID}) session = loginUser(t, user2.Name) @@ -61,7 +60,7 @@ func TestAPITeam(t *testing.T) { // Get an admin user able to create, update and delete teams. user = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) - org = unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 6}) + org = unittest.AssertExistsAndLoadBean(t, &org_model.Organization{ID: 6}) session = loginUser(t, user.Name) token = getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteOrganization) @@ -140,7 +139,7 @@ func TestAPITeam(t *testing.T) { teamToEdit.Permission, unit.AllUnitKeyNames(), nil) // Read team. - teamRead := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: teamID}) + teamRead := unittest.AssertExistsAndLoadBean(t, &org_model.Team{ID: teamID}) assert.NoError(t, teamRead.LoadUnits(db.DefaultContext)) req = NewRequestf(t, "GET", "/api/v1/teams/%d", teamID). AddTokenAuth(token) @@ -154,7 +153,7 @@ func TestAPITeam(t *testing.T) { req = NewRequestf(t, "DELETE", "/api/v1/teams/%d", teamID). AddTokenAuth(token) MakeRequest(t, req, http.StatusNoContent) - unittest.AssertNotExistsBean(t, &organization.Team{ID: teamID}) + unittest.AssertNotExistsBean(t, &org_model.Team{ID: teamID}) // create team again via UnitsMap // Create team. @@ -211,7 +210,7 @@ func TestAPITeam(t *testing.T) { "read", nil, teamToEdit.UnitsMap) // Read team. - teamRead = unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: teamID}) + teamRead = unittest.AssertExistsAndLoadBean(t, &org_model.Team{ID: teamID}) req = NewRequestf(t, "GET", "/api/v1/teams/%d", teamID). AddTokenAuth(token) resp = MakeRequest(t, req, http.StatusOK) @@ -225,7 +224,7 @@ func TestAPITeam(t *testing.T) { req = NewRequestf(t, "DELETE", "/api/v1/teams/%d", teamID). AddTokenAuth(token) MakeRequest(t, req, http.StatusNoContent) - unittest.AssertNotExistsBean(t, &organization.Team{ID: teamID}) + unittest.AssertNotExistsBean(t, &org_model.Team{ID: teamID}) // Create admin team teamToCreate = &api.CreateTeamOption{ @@ -244,7 +243,7 @@ func TestAPITeam(t *testing.T) { if ut == unit.TypeExternalTracker || ut == unit.TypeExternalWiki { up = perm.AccessModeRead } - unittest.AssertExistsAndLoadBean(t, &organization.TeamUnit{ + unittest.AssertExistsAndLoadBean(t, &org_model.TeamUnit{ OrgID: org.ID, TeamID: apiTeam.ID, Type: ut, @@ -257,7 +256,7 @@ func TestAPITeam(t *testing.T) { req = NewRequestf(t, "DELETE", "/api/v1/teams/%d", teamID). AddTokenAuth(token) MakeRequest(t, req, http.StatusNoContent) - unittest.AssertNotExistsBean(t, &organization.Team{ID: teamID}) + unittest.AssertNotExistsBean(t, &org_model.Team{ID: teamID}) } func checkTeamResponse(t *testing.T, testName string, apiTeam *api.Team, name, description string, includesAllRepositories bool, permission string, units []string, unitsMap map[string]string) { @@ -278,7 +277,7 @@ func checkTeamResponse(t *testing.T, testName string, apiTeam *api.Team, name, d } func checkTeamBean(t *testing.T, id int64, name, description string, includesAllRepositories bool, permission string, units []string, unitsMap map[string]string) { - team := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: id}) + team := unittest.AssertExistsAndLoadBean(t, &org_model.Team{ID: id}) assert.NoError(t, team.LoadUnits(db.DefaultContext), "LoadUnits") apiTeam, err := convert.ToTeam(db.DefaultContext, team) assert.NoError(t, err) @@ -321,7 +320,7 @@ func TestAPIGetTeamRepo(t *testing.T) { user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 15}) teamRepo := unittest.AssertExistsAndLoadBean(t, &repo.Repository{ID: 24}) - team := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: 5}) + team := unittest.AssertExistsAndLoadBean(t, &org_model.Team{ID: 5}) var results api.Repository diff --git a/tests/integration/org_test.go b/tests/integration/org_test.go index b5186cb70aacb..1aa634a9d3c4b 100644 --- a/tests/integration/org_test.go +++ b/tests/integration/org_test.go @@ -11,7 +11,6 @@ import ( auth_model "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/db" - "code.gitea.io/gitea/models/organization" org_model "code.gitea.io/gitea/models/organization" "code.gitea.io/gitea/models/perm" "code.gitea.io/gitea/models/unittest" @@ -230,7 +229,7 @@ func TestOwnerTeamsEdit(t *testing.T) { org := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3, Type: user_model.UserTypeOrganization}) session := loginUser(t, user1.Name) - team := unittest.AssertExistsAndLoadBean(t, &organization.Team{OrgID: org.ID, Name: org_model.OwnerTeamName}) + team := unittest.AssertExistsAndLoadBean(t, &org_model.Team{OrgID: org.ID, Name: org_model.OwnerTeamName}) assert.EqualValues(t, perm.AccessModeOwner, team.AccessMode) assert.EqualValues(t, "", team.Description) assert.NoError(t, team.LoadUnits(db.DefaultContext)) @@ -247,7 +246,7 @@ func TestOwnerTeamsEdit(t *testing.T) { session.MakeRequest(t, req, http.StatusSeeOther) // reload team - team = unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: team.ID}) + team = unittest.AssertExistsAndLoadBean(t, &org_model.Team{ID: team.ID}) assert.EqualValues(t, perm.AccessModeOwner, team.AccessMode) assert.EqualValues(t, org_model.OwnerTeamName, team.Name) assert.EqualValues(t, "Just a description", team.Description) From 088cffb688fc4f183dc5ca7b99ab4a21f386bc97 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 6 Dec 2024 16:26:56 -0800 Subject: [PATCH 9/9] Fix update team --- services/org/team.go | 9 ++++----- services/org/team_test.go | 17 +++++++++++++---- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/services/org/team.go b/services/org/team.go index a8157f479864d..2e3c1bd125f2a 100644 --- a/services/org/team.go +++ b/services/org/team.go @@ -12,7 +12,6 @@ import ( git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/organization" - org_model "code.gitea.io/gitea/models/organization" "code.gitea.io/gitea/models/perm" access_model "code.gitea.io/gitea/models/perm/access" repo_model "code.gitea.io/gitea/models/repo" @@ -101,7 +100,7 @@ type UpdateTeamOptions struct { UnitPerms map[unit_model.Type]perm.AccessMode } -func UpdateTeam(ctx context.Context, team *org_model.Team, opts UpdateTeamOptions) error { +func UpdateTeam(ctx context.Context, team *organization.Team, opts UpdateTeamOptions) error { var changedCols []string newAccessMode := team.AccessMode @@ -130,9 +129,9 @@ func UpdateTeam(ctx context.Context, team *org_model.Team, opts UpdateTeamOption changedCols = append(changedCols, "includes_all_repositories") } if len(opts.UnitPerms) > 0 { - units := make([]*org_model.TeamUnit, 0, len(opts.UnitPerms)) + units := make([]*organization.TeamUnit, 0, len(opts.UnitPerms)) for tp, perm := range opts.UnitPerms { - units = append(units, &org_model.TeamUnit{ + units = append(units, &organization.TeamUnit{ OrgID: team.OrgID, TeamID: team.ID, Type: tp, @@ -157,7 +156,7 @@ func UpdateTeam(ctx context.Context, team *org_model.Team, opts UpdateTeamOption team.Description = opts.Description } - return org_model.UpdateTeam(ctx, team, changedCols...) + return organization.UpdateTeam(ctx, team, changedCols...) } // UpdateTeam updates information of team. diff --git a/services/org/team_test.go b/services/org/team_test.go index 9597a9a342a0b..2617c5a09069d 100644 --- a/services/org/team_test.go +++ b/services/org/team_test.go @@ -78,11 +78,14 @@ func TestUpdateTeam(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) team := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: 2}) - team.LowerName = "newname" team.Name = "newName" team.Description = strings.Repeat("A long description!", 100) team.AccessMode = perm.AccessModeAdmin - assert.NoError(t, UpdateTeam(db.DefaultContext, team, "name", "lower_name", "description", "authorize")) + assert.NoError(t, UpdateTeam(db.DefaultContext, team, UpdateTeamOptions{ + TeamName: "name", + Description: "description", + IsAdmin: true, + })) team = unittest.AssertExistsAndLoadBean(t, &organization.Team{Name: "newName"}) assert.True(t, strings.HasPrefix(team.Description, "A long description!")) @@ -101,7 +104,10 @@ func TestUpdateTeam2(t *testing.T) { team.LowerName = "owners" team.Name = "Owners" team.Description = strings.Repeat("A long description!", 100) - err := UpdateTeam(db.DefaultContext, team, "name", "lower_name", "description", "authorize") + err := UpdateTeam(db.DefaultContext, team, UpdateTeamOptions{ + TeamName: "name", + Description: "description", + }) assert.True(t, organization.IsErrTeamAlreadyExist(err)) unittest.CheckConsistencyFor(t, &organization.Team{ID: team.ID}) @@ -277,7 +283,10 @@ func TestIncludesAllRepositoriesTeams(t *testing.T) { teams[4].IncludesAllRepositories = true teamRepos[4] = repoIDs for i, team := range teams { - assert.NoError(t, UpdateTeam(db.DefaultContext, team, false, true), "%s: UpdateTeam", team.Name) + assert.NoError(t, UpdateTeam(db.DefaultContext, team, UpdateTeamOptions{ + IncludesAllRepositories: false, + CanCreateOrgRepo: true, + }), "%s: UpdateTeam", team.Name) testTeamRepositories(team.ID, teamRepos[i]) }