From ad304ec43544bf01711ad527d2f4a6718dc8441c Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 22 Oct 2022 17:59:21 +0800 Subject: [PATCH] Fix tests --- models/git/branches.go | 2 +- models/git/protected_branch.go | 68 ++++++++++++++++++-- models/org_team.go | 29 ++------- models/user.go | 24 +------ modules/convert/convert.go | 4 +- modules/structs/repo_branch.go | 2 +- options/locale/locale_en-US.ini | 1 + routers/api/v1/repo/branch.go | 4 +- routers/web/repo/branch.go | 15 ++--- routers/web/repo/setting_protected_branch.go | 32 ++++++--- templates/repo/settings/branches.tmpl | 2 +- tests/integration/api_branch_test.go | 8 +-- tests/integration/editor_test.go | 12 +++- 13 files changed, 117 insertions(+), 86 deletions(-) diff --git a/models/git/branches.go b/models/git/branches.go index af239dc316..54814da507 100644 --- a/models/git/branches.go +++ b/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 diff --git a/models/git/protected_branch.go b/models/git/protected_branch.go index d1037ab8b3..f9356f414f 100644 --- a/models/git/protected_branch.go +++ b/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.isPlainName = protectBranch.globRule.Match(protectBranch.BranchName) + protectBranch.globRule = glob.MustCompile(protectBranch.RuleName, '/') + 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 +} diff --git a/models/org_team.go b/models/org_team.go index 6066e7f5c9..e1919b8f64 100644 --- a/models/org_team.go +++ b/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 len(p.WhitelistTeamIDs) != 0 { - 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) - } + if err := git_model.RemoveTeamIDFromProtectedBranch(ctx, p, t.ID); err != nil { + return 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 } diff --git a/models/user.go b/models/user.go index 68be0d8555..2050fa73a8 100644 --- a/models/user.go +++ b/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 len(p.WhitelistUserIDs) != 0 { - 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) - } + if err := git_model.RemoveUserIDFromProtectedBranch(ctx, p, u.ID); err != nil { + return err } } } diff --git a/modules/convert/convert.go b/modules/convert/convert.go index 187c67fa76..c895ae9e8e 100644 --- a/modules/convert/convert.go +++ b/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, diff --git a/modules/structs/repo_branch.go b/modules/structs/repo_branch.go index 02a3915f16..b3dcde7060 100644 --- a/modules/structs/repo_branch.go +++ b/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"` diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index fb8ec46641..999ae0360a 100644 --- a/options/locale/locale_en-US.ini +++ b/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 diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index 79ccfbbd19..496daf1421 100644 --- a/routers/api/v1/repo/branch.go +++ b/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 diff --git a/routers/web/repo/branch.go b/routers/web/repo/branch.go index 6af1185f42..77d3dd66c6 100644 --- a/routers/web/repo/branch.go +++ b/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 - for _, b := range protectedBranches { - if b.BranchName == branchName { - isProtected = true - break - } - } + p := protectedBranches.GetFirstMatched(branchName) + isProtected := p != nil divergence := &git.DivergeObject{ Ahead: -1, diff --git a/routers/web/repo/setting_protected_branch.go b/routers/web/repo/setting_protected_branch.go index 6d299e9d24..89fe3ab5c6 100644 --- a/routers/web/repo/setting_protected_branch.go +++ b/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 { @@ -171,13 +171,13 @@ func SettingsProtectedBranchPost(ctx *context.Context) { } else { // No options found, create defaults. protectBranch = &git_model.ProtectedBranch{ - RepoID: ctx.Repo.Repository.ID, - BranchName: f.RuleName, + RepoID: ctx.Repo.Repository.ID, + 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), }) diff --git a/templates/repo/settings/branches.tmpl b/templates/repo/settings/branches.tmpl index 181bb976dc..86896dda18 100644 --- a/templates/repo/settings/branches.tmpl +++ b/templates/repo/settings/branches.tmpl @@ -58,7 +58,7 @@ {{range .ProtectedBranches}} -
{{.BranchName}}
+
{{.RuleName}}
{{$.locale.Tr "repo.settings.edit_protected_branch"}}