diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 0931e8694..c47a7ba66 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -2424,7 +2424,7 @@ settings.choose_branch = Choose a branch… settings.no_protected_branch = There are no protected branches. settings.edit_protected_branch = Edit settings.protected_branch_required_rule_name = Required rule name -settings.protected_branch_duplicate_rule_name = Duplicate rule name +settings.protected_branch_duplicate_rule_name = There is already a rule for this set of branches settings.protected_branch_required_approvals_min = Required approvals cannot be negative. settings.tags = Tags settings.tags.protection = Tag Protection diff --git a/routers/web/repo/setting/protected_branch.go b/routers/web/repo/setting/protected_branch.go index 85068f0ab..827ebea7f 100644 --- a/routers/web/repo/setting/protected_branch.go +++ b/routers/web/repo/setting/protected_branch.go @@ -132,12 +132,15 @@ func SettingsProtectedBranchPost(ctx *context.Context) { } } } else { - // FIXME: If a new ProtectBranch has a duplicate RuleName, an error should be returned. - // Currently, if a new ProtectBranch with a duplicate RuleName is created, the existing ProtectBranch will be updated. - // But we cannot modify this logic now because many unit tests rely on it. + // Check if a rule already exists with this rulename, if so redirect to it. protectBranch, err = git_model.GetProtectedBranchRuleByName(ctx, ctx.Repo.Repository.ID, f.RuleName) if err != nil { - ctx.ServerError("GetProtectBranchOfRepoByName", err) + ctx.ServerError("GetProtectedBranchRuleByName", err) + return + } + if protectBranch != nil { + ctx.Flash.Error(ctx.Tr("repo.settings.protected_branch_duplicate_rule_name")) + ctx.Redirect(fmt.Sprintf("%s/settings/branches/edit?rule_name=%s", ctx.Repo.RepoLink, protectBranch.RuleName)) return } } diff --git a/tests/integration/git_test.go b/tests/integration/git_test.go index d02427ffc..2163c67cb 100644 --- a/tests/integration/git_test.go +++ b/tests/integration/git_test.go @@ -18,6 +18,7 @@ import ( auth_model "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/db" + git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/perm" repo_model "code.gitea.io/gitea/models/repo" @@ -413,12 +414,17 @@ func doBranchProtectPRMerge(baseCtx *APITestContext, dstPath string) func(t *tes func doProtectBranch(ctx APITestContext, branch, userToWhitelist, unprotectedFilePatterns string) func(t *testing.T) { // We are going to just use the owner to set the protection. return func(t *testing.T) { + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{Name: ctx.Reponame, OwnerName: ctx.Username}) + rule := &git_model.ProtectedBranch{RuleName: branch, RepoID: repo.ID} + unittest.LoadBeanIfExists(rule) + csrf := GetCSRF(t, ctx.Session, fmt.Sprintf("/%s/%s/settings/branches", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame))) if userToWhitelist == "" { // Change branch to protected req := NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/settings/branches/edit", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame)), map[string]string{ "_csrf": csrf, + "rule_id": strconv.FormatInt(rule.ID, 10), "rule_name": branch, "unprotected_file_patterns": unprotectedFilePatterns, }) @@ -430,6 +436,7 @@ func doProtectBranch(ctx APITestContext, branch, userToWhitelist, unprotectedFil req := NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/settings/branches/edit", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame)), map[string]string{ "_csrf": csrf, "rule_name": branch, + "rule_id": strconv.FormatInt(rule.ID, 10), "enable_push": "whitelist", "enable_whitelist": "on", "whitelist_users": strconv.FormatInt(user.ID, 10), diff --git a/tests/integration/repo_settings_test.go b/tests/integration/repo_settings_test.go index 16e0bb3d7..307614815 100644 --- a/tests/integration/repo_settings_test.go +++ b/tests/integration/repo_settings_test.go @@ -9,10 +9,12 @@ import ( "testing" "code.gitea.io/gitea/models/db" + git_model "code.gitea.io/gitea/models/git" repo_model "code.gitea.io/gitea/models/repo" unit_model "code.gitea.io/gitea/models/unit" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" + gitea_context "code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/setting" repo_service "code.gitea.io/gitea/services/repository" "code.gitea.io/gitea/tests" @@ -128,3 +130,43 @@ func TestRepoAddMoreUnits(t *testing.T) { assertAddMore(t, false) }) } + +func TestProtectedBranch(t *testing.T) { + defer tests.PrepareTestEnv(t)() + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1, OwnerID: user.ID}) + session := loginUser(t, user.Name) + + t.Run("Add", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + link := fmt.Sprintf("/%s/settings/branches/edit", repo.FullName()) + + req := NewRequestWithValues(t, "POST", link, map[string]string{ + "_csrf": GetCSRF(t, session, link), + "rule_name": "master", + "enable_push": "true", + }) + session.MakeRequest(t, req, http.StatusSeeOther) + + // Verify it was added. + unittest.AssertExistsIf(t, true, &git_model.ProtectedBranch{RuleName: "master", RepoID: repo.ID}) + }) + + t.Run("Add duplicate", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + link := fmt.Sprintf("/%s/settings/branches/edit", repo.FullName()) + + req := NewRequestWithValues(t, "POST", link, map[string]string{ + "_csrf": GetCSRF(t, session, link), + "rule_name": "master", + "require_signed_": "true", + }) + session.MakeRequest(t, req, http.StatusSeeOther) + flashCookie := session.GetCookie(gitea_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.EqualValues(t, "error%3DThere%2Bis%2Balready%2Ba%2Brule%2Bfor%2Bthis%2Bset%2Bof%2Bbranches", flashCookie.Value) + + // Verify it wasn't added. + unittest.AssertCount(t, &git_model.ProtectedBranch{RuleName: "master", RepoID: repo.ID}, 1) + }) +}