Fix tests

models/git/branches.go

@@ -158,7 +158,7 @@ func RenameBranch(repo *repo_model.Repository, from, to string, gitAction func(i
 	}
 
 	if protectedBranch != nil {
-		protectedBranch.BranchName = to
+		protectedBranch.RuleName = to
 		_, err = sess.ID(protectedBranch.ID).Cols("branch_name").Update(protectedBranch)
 		if err != nil {
 			return err

models/git/protected_branch.go

@@ -28,7 +28,7 @@ import (
 type ProtectedBranch struct {
 	ID                            int64     `xorm:"pk autoincr"`
 	RepoID                        int64     `xorm:"UNIQUE(s)"`
-	BranchName                    string    `xorm:"UNIQUE(s)"` // a branch name or a glob match to branch name
+	RuleName                      string    `xorm:"'branch_name' UNIQUE(s)"` // a branch name or a glob match to branch name
 	globRule                      glob.Glob `xorm:"-"`
 	isPlainName                   bool      `xorm:"-"`
 	CanPush                       bool      `xorm:"NOT NULL DEFAULT false"`
@@ -63,17 +63,17 @@ func init() {
 
 func (protectBranch *ProtectedBranch) loadGlob() {
 	if protectBranch.globRule == nil {
-		protectBranch.globRule = glob.MustCompile(protectBranch.BranchName, '/')
+		protectBranch.globRule = glob.MustCompile(protectBranch.RuleName, '/')
-		protectBranch.isPlainName = protectBranch.globRule.Match(protectBranch.BranchName)
+		protectBranch.isPlainName = protectBranch.globRule.Match(protectBranch.RuleName)
 	}
 }
 
 // Match tests if branchName matches the rule
 func (protectBranch *ProtectedBranch) Match(branchName string) bool {
-	if strings.EqualFold(protectBranch.BranchName, branchName) {
-		return true
-	}
 	protectBranch.loadGlob()
+	if protectBranch.isPlainName {
+		return strings.EqualFold(protectBranch.RuleName, branchName)
+	}
 
 	return protectBranch.globRule.Match(branchName)
 }
@@ -247,7 +247,7 @@ func (protectBranch *ProtectedBranch) IsUnprotectedFile(patterns []glob.Glob, pa
 
 // GetProtectedBranchRuleByName getting protected branch rule by name
 func GetProtectedBranchRuleByName(ctx context.Context, repoID int64, ruleName string) (*ProtectedBranch, error) {
-	rel := &ProtectedBranch{RepoID: repoID, BranchName: ruleName}
+	rel := &ProtectedBranch{RepoID: repoID, RuleName: ruleName}
 	has, err := db.GetByBean(ctx, rel)
 	if err != nil {
 		return nil, err
@@ -432,3 +432,57 @@ func DeleteProtectedBranch(repoID, id int64) (err error) {
 
 	return nil
 }
+
+// RemoveUserIDFromProtectedBranch remove all user ids from protected branch options
+func RemoveUserIDFromProtectedBranch(ctx context.Context, p *ProtectedBranch, userID int64) error {
+	var matched1, matched2, matched3 bool
+	if len(p.WhitelistUserIDs) != 0 {
+		p.WhitelistUserIDs, matched1 = util.RemoveIDFromList(
+			p.WhitelistUserIDs, userID)
+	}
+	if len(p.ApprovalsWhitelistUserIDs) != 0 {
+		p.ApprovalsWhitelistUserIDs, matched2 = util.RemoveIDFromList(
+			p.ApprovalsWhitelistUserIDs, userID)
+	}
+	if len(p.MergeWhitelistUserIDs) != 0 {
+		p.MergeWhitelistUserIDs, matched3 = util.RemoveIDFromList(
+			p.MergeWhitelistUserIDs, userID)
+	}
+	if matched1 || matched2 || matched3 {
+		if _, err := db.GetEngine(ctx).ID(p.ID).Cols(
+			"whitelist_user_i_ds",
+			"merge_whitelist_user_i_ds",
+			"approvals_whitelist_user_i_ds",
+		).Update(p); err != nil {
+			return fmt.Errorf("updateProtectedBranches: %v", err)
+		}
+	}
+	return nil
+}
+
+// RemoveTeamIDFromProtectedBranch remove all team ids from protected branch options
+func RemoveTeamIDFromProtectedBranch(ctx context.Context, p *ProtectedBranch, teamID int64) error {
+	var matched1, matched2, matched3 bool
+	if len(p.WhitelistTeamIDs) != 0 {
+		p.WhitelistTeamIDs, matched1 = util.RemoveIDFromList(
+			p.WhitelistTeamIDs, teamID)
+	}
+	if len(p.ApprovalsWhitelistTeamIDs) != 0 {
+		p.ApprovalsWhitelistTeamIDs, matched2 = util.RemoveIDFromList(
+			p.ApprovalsWhitelistTeamIDs, teamID)
+	}
+	if len(p.MergeWhitelistTeamIDs) != 0 {
+		p.MergeWhitelistTeamIDs, matched3 = util.RemoveIDFromList(
+			p.MergeWhitelistTeamIDs, teamID)
+	}
+	if matched1 || matched2 || matched3 {
+		if _, err := db.GetEngine(ctx).ID(p.ID).Cols(
+			"whitelist_team_i_ds",
+			"merge_whitelist_team_i_ds",
+			"approvals_whitelist_team_i_ds",
+		).Update(p); err != nil {
+			return fmt.Errorf("updateProtectedBranches: %v", err)
+		}
+	}
+	return nil
+}

models/org_team.go

@@ -20,7 +20,6 @@ import (
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
-	"code.gitea.io/gitea/modules/util"
 
 	"xorm.io/builder"
 )
@@ -380,7 +379,6 @@ func DeleteTeam(t *organization.Team) error {
 		return err
 	}
 	defer committer.Close()
-	sess := db.GetEngine(ctx)
 
 	if err := t.GetRepositoriesCtx(ctx); err != nil {
 		return err
@@ -393,34 +391,15 @@ func DeleteTeam(t *organization.Team) error {
 	// update branch protections
 	{
 		protections := make([]*git_model.ProtectedBranch, 0, 10)
-		err := sess.In("repo_id",
+		err := db.GetEngine(ctx).In("repo_id",
 			builder.Select("id").From("repository").Where(builder.Eq{"owner_id": t.OrgID})).
 			Find(&protections)
 		if err != nil {
 			return fmt.Errorf("findProtectedBranches: %v", err)
 		}
 		for _, p := range protections {
-			var matched1, matched2, matched3 bool
+			if err := git_model.RemoveTeamIDFromProtectedBranch(ctx, p, t.ID); err != nil {
-			if len(p.WhitelistTeamIDs) != 0 {
+				return err
-				p.WhitelistTeamIDs, matched1 = util.RemoveIDFromList(
-					p.WhitelistTeamIDs, t.ID)
-			}
-			if len(p.ApprovalsWhitelistTeamIDs) != 0 {
-				p.ApprovalsWhitelistTeamIDs, matched2 = util.RemoveIDFromList(
-					p.ApprovalsWhitelistTeamIDs, t.ID)
-			}
-			if len(p.MergeWhitelistTeamIDs) != 0 {
-				p.MergeWhitelistTeamIDs, matched3 = util.RemoveIDFromList(
-					p.MergeWhitelistTeamIDs, t.ID)
-			}
-			if matched1 || matched2 || matched3 {
-				if _, err = sess.ID(p.ID).Cols(
-					"whitelist_team_i_ds",
-					"merge_whitelist_team_i_ds",
-					"approvals_whitelist_team_i_ds",
-				).Update(p); err != nil {
-					return fmt.Errorf("updateProtectedBranches: %v", err)
-				}
 			}
 		}
 	}
@@ -441,7 +420,7 @@ func DeleteTeam(t *organization.Team) error {
 	}
 
 	// Update organization number of teams.
-	if _, err := sess.Exec("UPDATE `user` SET num_teams=num_teams-1 WHERE id=?", t.OrgID); err != nil {
+	if _, err := db.Exec(ctx, "UPDATE `user` SET num_teams=num_teams-1 WHERE id=?", t.OrgID); err != nil {
 		return err
 	}
 

models/user.go

@@ -24,7 +24,6 @@ import (
 	repo_model "code.gitea.io/gitea/models/repo"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/setting"
-	"code.gitea.io/gitea/modules/util"
 )
 
 // DeleteUser deletes models associated to an user.
@@ -141,27 +140,8 @@ func DeleteUser(ctx context.Context, u *user_model.User, purge bool) (err error)
 				break
 			}
 			for _, p := range protections {
-				var matched1, matched2, matched3 bool
+				if err := git_model.RemoveUserIDFromProtectedBranch(ctx, p, u.ID); err != nil {
-				if len(p.WhitelistUserIDs) != 0 {
+					return err
-					p.WhitelistUserIDs, matched1 = util.RemoveIDFromList(
-						p.WhitelistUserIDs, u.ID)
-				}
-				if len(p.ApprovalsWhitelistUserIDs) != 0 {
-					p.ApprovalsWhitelistUserIDs, matched2 = util.RemoveIDFromList(
-						p.ApprovalsWhitelistUserIDs, u.ID)
-				}
-				if len(p.MergeWhitelistUserIDs) != 0 {
-					p.MergeWhitelistUserIDs, matched3 = util.RemoveIDFromList(
-						p.MergeWhitelistUserIDs, u.ID)
-				}
-				if matched1 || matched2 || matched3 {
-					if _, err = e.ID(p.ID).Cols(
-						"whitelist_user_i_ds",
-						"merge_whitelist_user_i_ds",
-						"approvals_whitelist_user_i_ds",
-					).Update(p); err != nil {
-						return fmt.Errorf("updateProtectedBranches: %v", err)
-					}
 				}
 			}
 		}

modules/convert/convert.go

@@ -81,7 +81,7 @@ func ToBranch(repo *repo_model.Repository, b *git.Branch, c *git.Commit, bp *git
 	}
 
 	if isRepoAdmin {
-		branch.EffectiveBranchProtectionName = bp.BranchName
+		branch.EffectiveBranchProtectionName = bp.RuleName
 	}
 
 	if user != nil {
@@ -124,7 +124,7 @@ func ToBranchProtection(bp *git_model.ProtectedBranch) *api.BranchProtection {
 	}
 
 	return &api.BranchProtection{
-		BranchName:                    bp.BranchName,
+		RuleName:                      bp.RuleName,
 		EnablePush:                    bp.CanPush,
 		EnablePushWhitelist:           bp.EnableWhitelist,
 		PushWhitelistUsernames:        pushWhitelistUsernames,

modules/structs/repo_branch.go

@@ -23,7 +23,7 @@ type Branch struct {
 
 // BranchProtection represents a branch protection for a repository
 type BranchProtection struct {
-	BranchName                    string   `json:"branch_name"`
+	RuleName                      string   `json:"branch_name"`
 	EnablePush                    bool     `json:"enable_push"`
 	EnablePushWhitelist           bool     `json:"enable_push_whitelist"`
 	PushWhitelistUsernames        []string `json:"push_whitelist_usernames"`

options/locale/locale_en-US.ini

@@ -2098,6 +2098,7 @@ settings.add_protected_branch = Enable protection
 settings.delete_protected_branch = Disable protection
 settings.update_protect_branch_success = Branch protection for rule '%s' has been updated.
 settings.remove_protected_branch_success = Branch protection for rule '%s' has been removed.
+settings.remove_protected_branch_failed = Removing branch protection rule '%s' failed.
 settings.protected_branch_deletion = Delete Branch Protection
 settings.protected_branch_deletion_desc = Disabling branch protection allows users with write permission to push to the branch. Continue?
 settings.block_rejected_reviews = Block merge on rejected reviews

routers/api/v1/repo/branch.go

@@ -496,7 +496,7 @@ func CreateBranchProtection(ctx *context.APIContext) {
 
 	protectBranch = &git_model.ProtectedBranch{
 		RepoID:                        ctx.Repo.Repository.ID,
-		BranchName:                    form.RuleName,
+		RuleName:                      form.RuleName,
 		CanPush:                       form.EnablePush,
 		EnableWhitelist:               form.EnablePush && form.EnablePushWhitelist,
 		EnableMergeWhitelist:          form.EnableMergeWhitelist,
@@ -822,7 +822,7 @@ func EditBranchProtection(ctx *context.APIContext) {
 			}
 
 			// FIXME: since we only need to recheck files protected rules, we could improve this
-			matchedBranches, err := git_model.FindAllMatchedBranches(ctx, ctx.Repo.GitRepo, protectBranch.BranchName)
+			matchedBranches, err := git_model.FindAllMatchedBranches(ctx, ctx.Repo.GitRepo, protectBranch.RuleName)
 			if err != nil {
 				ctx.Error(http.StatusInternalServerError, "FindAllMatchedBranches", err)
 				return

routers/web/repo/branch.go

@@ -205,7 +205,7 @@ func loadBranches(ctx *context.Context, skip, limit int) (*Branch, []*Branch, in
 			continue
 		}
 
-		branch := loadOneBranch(ctx, rawBranches[i], defaultBranch, rules, repoIDToRepo, repoIDToGitRepo)
+		branch := loadOneBranch(ctx, rawBranches[i], defaultBranch, &rules, repoIDToRepo, repoIDToGitRepo)
 		if branch == nil {
 			return nil, nil, 0
 		}
@@ -217,7 +217,7 @@ func loadBranches(ctx *context.Context, skip, limit int) (*Branch, []*Branch, in
 	if defaultBranch != nil {
 		// Always add the default branch
 		log.Debug("loadOneBranch: load default: '%s'", defaultBranch.Name)
-		defaultBranchBranch = loadOneBranch(ctx, defaultBranch, defaultBranch, rules, repoIDToRepo, repoIDToGitRepo)
+		defaultBranchBranch = loadOneBranch(ctx, defaultBranch, defaultBranch, &rules, repoIDToRepo, repoIDToGitRepo)
 		branches = append(branches, defaultBranchBranch)
 	}
 
@@ -233,7 +233,7 @@ func loadBranches(ctx *context.Context, skip, limit int) (*Branch, []*Branch, in
 	return defaultBranchBranch, branches, totalNumOfBranches
 }
 
-func loadOneBranch(ctx *context.Context, rawBranch, defaultBranch *git.Branch, protectedBranches []*git_model.ProtectedBranch,
+func loadOneBranch(ctx *context.Context, rawBranch, defaultBranch *git.Branch, protectedBranches *git_model.ProtectedBranchRules,
 	repoIDToRepo map[int64]*repo_model.Repository,
 	repoIDToGitRepo map[int64]*git.Repository,
 ) *Branch {
@@ -246,13 +246,8 @@ func loadOneBranch(ctx *context.Context, rawBranch, defaultBranch *git.Branch, p
 	}
 
 	branchName := rawBranch.Name
-	var isProtected bool
+	p := protectedBranches.GetFirstMatched(branchName)
-	for _, b := range protectedBranches {
+	isProtected := p != nil
-		if b.BranchName == branchName {
-			isProtected = true
-			break
-		}
-	}
 
 	divergence := &git.DivergeObject{
 		Ahead:  -1,

routers/web/repo/setting_protected_branch.go

@@ -105,7 +105,7 @@ func SettingsProtectedBranch(c *context.Context) {
 	}
 
 	c.Data["PageIsSettingsBranches"] = true
-	c.Data["Title"] = c.Tr("repo.settings.protected_branch") + " - " + rule.BranchName
+	c.Data["Title"] = c.Tr("repo.settings.protected_branch") + " - " + rule.RuleName
 
 	users, err := access_model.GetRepoReaders(c.Repo.Repository)
 	if err != nil {
@@ -172,12 +172,12 @@ func SettingsProtectedBranchPost(ctx *context.Context) {
 		// No options found, create defaults.
 		protectBranch = &git_model.ProtectedBranch{
 			RepoID:   ctx.Repo.Repository.ID,
-			BranchName: f.RuleName,
+			RuleName: f.RuleName,
 		}
 	}
 
 	var whitelistUsers, whitelistTeams, mergeWhitelistUsers, mergeWhitelistTeams, approvalsWhitelistUsers, approvalsWhitelistTeams []int64
-	protectBranch.BranchName = f.RuleName
+	protectBranch.RuleName = f.RuleName
 	if f.RequiredApprovals < 0 {
 		ctx.Flash.Error(ctx.Tr("repo.settings.protected_branch_required_approvals_min"))
 		ctx.Redirect(fmt.Sprintf("%s/settings/branches/%d", ctx.Repo.RepoLink, ruleID))
@@ -254,7 +254,7 @@ func SettingsProtectedBranchPost(ctx *context.Context) {
 	}
 
 	// FIXME: since we only need to recheck files protected rules, we could improve this
-	matchedBranches, err := git_model.FindAllMatchedBranches(ctx, ctx.Repo.GitRepo, protectBranch.BranchName)
+	matchedBranches, err := git_model.FindAllMatchedBranches(ctx, ctx.Repo.GitRepo, protectBranch.RuleName)
 	if err != nil {
 		ctx.ServerError("FindAllMatchedBranches", err)
 		return
@@ -266,7 +266,7 @@ func SettingsProtectedBranchPost(ctx *context.Context) {
 		}
 	}
 
-	ctx.Flash.Success(ctx.Tr("repo.settings.update_protect_branch_success", protectBranch.BranchName))
+	ctx.Flash.Success(ctx.Tr("repo.settings.update_protect_branch_success", protectBranch.RuleName))
 	ctx.Redirect(fmt.Sprintf("%s/settings/branches/%d", ctx.Repo.RepoLink, protectBranch.ID))
 }
 
@@ -274,23 +274,39 @@ func SettingsProtectedBranchPost(ctx *context.Context) {
 func DeleteProtectedBranchRulePost(ctx *context.Context) {
 	ruleID := ctx.ParamsInt64("id")
 	if ruleID <= 0 {
+		ctx.Flash.Error(ctx.Tr("repo.settings.remove_protected_branch_failed", fmt.Sprintf("%d", ruleID)))
+		ctx.JSON(http.StatusOK, map[string]interface{}{
+			"redirect": fmt.Sprintf("%s/settings/branches", ctx.Repo.RepoLink),
+		})
 		return
 	}
+
 	rule, err := git_model.GetProtectedBranchRuleByID(ctx, ctx.Repo.Repository.ID, ruleID)
 	if err != nil {
+		ctx.Flash.Error(ctx.Tr("repo.settings.remove_protected_branch_failed", fmt.Sprintf("%d", ruleID)))
+		ctx.JSON(http.StatusOK, map[string]interface{}{
+			"redirect": fmt.Sprintf("%s/settings/branches", ctx.Repo.RepoLink),
+		})
 		return
 	}
 
 	if rule == nil {
+		ctx.Flash.Error(ctx.Tr("repo.settings.remove_protected_branch_failed", fmt.Sprintf("%d", ruleID)))
+		ctx.JSON(http.StatusOK, map[string]interface{}{
+			"redirect": fmt.Sprintf("%s/settings/branches", ctx.Repo.RepoLink),
+		})
 		return
 	}
 
 	if err := git_model.DeleteProtectedBranch(ctx.Repo.Repository.ID, ruleID); err != nil {
-		ctx.ServerError("DeleteProtectedBranch", err)
+		ctx.Flash.Error(ctx.Tr("repo.settings.remove_protected_branch_failed", rule.RuleName))
+		ctx.JSON(http.StatusOK, map[string]interface{}{
+			"redirect": fmt.Sprintf("%s/settings/branches", ctx.Repo.RepoLink),
+		})
 		return
 	}
 
-	ctx.Flash.Success(ctx.Tr("repo.settings.remove_protected_branch_success", rule.BranchName))
+	ctx.Flash.Success(ctx.Tr("repo.settings.remove_protected_branch_success", rule.RuleName))
 	ctx.JSON(http.StatusOK, map[string]interface{}{
 		"redirect": fmt.Sprintf("%s/settings/branches", ctx.Repo.RepoLink),
 	})

templates/repo/settings/branches.tmpl

@@ -58,7 +58,7 @@
 							<tbody>
 								{{range .ProtectedBranches}}
 									<tr>
-										<td><div class="ui basic primary label">{{.BranchName}}</div></td>
+										<td><div class="ui basic primary label">{{.RuleName}}</div></td>
 										<td class="right aligned">
 										<a class="rm ui button" href="{{$.Repository.Link}}/settings/branches/{{.ID}}">{{$.locale.Tr "repo.settings.edit_protected_branch"}}</a>
 										<button class="ui red tiny button delete-button" data-url="{{$.Repository.Link}}/settings/branches/{{.ID}}/delete" data-id="{{.ID}}">

tests/integration/api_branch_test.go

@@ -41,7 +41,7 @@ func testAPIGetBranchProtection(t *testing.T, branchName string, expectedHTTPSta
 	if resp.Code == http.StatusOK {
 		var branchProtection api.BranchProtection
 		DecodeJSON(t, resp, &branchProtection)
-		assert.EqualValues(t, branchName, branchProtection.BranchName)
+		assert.EqualValues(t, branchName, branchProtection.RuleName)
 	}
 }
 
@@ -49,14 +49,14 @@ func testAPICreateBranchProtection(t *testing.T, branchName string, expectedHTTP
 	session := loginUser(t, "user2")
 	token := getTokenForLoggedInUser(t, session)
 	req := NewRequestWithJSON(t, "POST", "/api/v1/repos/user2/repo1/branch_protections?token="+token, &api.BranchProtection{
-		BranchName: branchName,
+		RuleName: branchName,
 	})
 	resp := session.MakeRequest(t, req, expectedHTTPStatus)
 
 	if resp.Code == http.StatusCreated {
 		var branchProtection api.BranchProtection
 		DecodeJSON(t, resp, &branchProtection)
-		assert.EqualValues(t, branchName, branchProtection.BranchName)
+		assert.EqualValues(t, branchName, branchProtection.RuleName)
 	}
 }
 
@@ -69,7 +69,7 @@ func testAPIEditBranchProtection(t *testing.T, branchName string, body *api.Bran
 	if resp.Code == http.StatusOK {
 		var branchProtection api.BranchProtection
 		DecodeJSON(t, resp, &branchProtection)
-		assert.EqualValues(t, branchName, branchProtection.BranchName)
+		assert.EqualValues(t, branchName, branchProtection.RuleName)
 	}
 }
 

tests/integration/editor_test.go

@@ -5,6 +5,7 @@
 package integration
 
 import (
+	"encoding/json"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -53,7 +54,7 @@ func TestCreateFileOnProtectedBranch(t *testing.T) {
 		// Check if master branch has been locked successfully
 		flashCookie := session.GetCookie("macaron_flash")
 		assert.NotNil(t, flashCookie)
-		assert.EqualValues(t, "success%3DBranch%2Bprotection%2Bfor%2Bbranch%2B%2527master%2527%2Bhas%2Bbeen%2Bupdated.", flashCookie.Value)
+		assert.EqualValues(t, "success%3DBranch%2Bprotection%2Bfor%2Brule%2B%2527master%2527%2Bhas%2Bbeen%2Bupdated.", flashCookie.Value)
 
 		// Request editor page
 		req = NewRequest(t, "GET", "/user2/repo1/_new/master/")
@@ -84,11 +85,16 @@ func TestCreateFileOnProtectedBranch(t *testing.T) {
 			"_csrf": csrf,
 		})
 
-		session.MakeRequest(t, req, http.StatusSeeOther)
+		resp = session.MakeRequest(t, req, http.StatusOK)
+
+		res := make(map[string]string)
+		assert.NoError(t, json.NewDecoder(resp.Body).Decode(&res))
+		assert.EqualValues(t, "/user2/repo1/settings/branches", res["redirect"])
+
 		// Check if master branch has been locked successfully
 		flashCookie = session.GetCookie("macaron_flash")
 		assert.NotNil(t, flashCookie)
-		assert.EqualValues(t, "success%3DBranch%2Bprotection%2Bfor%2Bbranch%2B%2527master%2527%2Bhas%2Bbeen%2Bdisabled.", flashCookie.Value)
+		assert.EqualValues(t, "error%3DRemoving%2Bbranch%2Bprotection%2Brule%2B%25271%2527%2Bfailed.", flashCookie.Value)
 	})
 }