Fix edit team

Author: lunny

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)
 	}
 

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"`

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
 	}

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):

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 {

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
 }
 

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...)
+}