diff --git a/options/locale/locale_en-US.json b/options/locale/locale_en-US.json index acc88e830b9..9fcbd6eeb04 100644 --- a/options/locale/locale_en-US.json +++ b/options/locale/locale_en-US.json @@ -2487,7 +2487,6 @@ "repo.settings.choose_branch": "Choose a branch…", "repo.settings.no_protected_branch": "There are no protected branches.", "repo.settings.edit_protected_branch": "Edit", - "repo.settings.protected_branch_required_rule_name": "Required rule name", "repo.settings.protected_branch_duplicate_rule_name": "Duplicate rule name", "repo.settings.protected_branch_required_approvals_min": "Required approvals cannot be negative.", "repo.settings.tags": "Tags", diff --git a/routers/web/repo/setting/protected_branch.go b/routers/web/repo/setting/protected_branch.go index beb2cc58382..7eb16a47a7a 100644 --- a/routers/web/repo/setting/protected_branch.go +++ b/routers/web/repo/setting/protected_branch.go @@ -5,9 +5,7 @@ package setting import ( "errors" - "fmt" "net/http" - "net/url" "strconv" "strings" "time" @@ -124,14 +122,15 @@ func SettingsProtectedBranch(c *context.Context) { // SettingsProtectedBranchPost updates the protected branch settings func SettingsProtectedBranchPost(ctx *context.Context) { - f := web.GetForm[*forms.ProtectBranchForm](ctx) - var protectBranch *git_model.ProtectedBranch - if f.RuleName == "" { - ctx.Flash.Error(ctx.Tr("repo.settings.protected_branch_required_rule_name")) - ctx.Redirect(ctx.Repo.RepoLink + "/settings/branches/edit") + f := context.GetFetchActionForm[*forms.ProtectBranchForm](ctx) + if f == nil { return } - + if f.RequiredApprovals < 0 { + ctx.JSONErrorWithField(ctx.Tr("repo.settings.protected_branch_required_approvals_min"), "required_approvals") + return + } + var protectBranch *git_model.ProtectedBranch var err error if f.RuleID > 0 { // If the RuleID isn't 0, it must be an edit operation. So we get rule by id. @@ -149,8 +148,7 @@ func SettingsProtectedBranchPost(ctx *context.Context) { return } if sameNameProtectBranch != 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)) + ctx.JSONErrorWithField(ctx.Tr("repo.settings.protected_branch_duplicate_rule_name"), "rule_name") return } } @@ -176,11 +174,6 @@ func SettingsProtectedBranchPost(ctx *context.Context) { var whitelistUsers, whitelistTeams, forcePushAllowlistUsers, forcePushAllowlistTeams, deletionAllowlistUsers, deletionAllowlistTeams, mergeWhitelistUsers, mergeWhitelistTeams, approvalsWhitelistUsers, approvalsWhitelistTeams, bypassAllowlistUsers, bypassAllowlistTeams []int64 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/edit?rule_name=%s", ctx.Repo.RepoLink, f.RuleName)) - return - } switch f.EnablePush { case "all": @@ -271,16 +264,14 @@ func SettingsProtectedBranchPost(ctx *context.Context) { continue } if _, err := glob.Compile(trimmed); err != nil { - ctx.Flash.Error(ctx.Tr("repo.settings.protect_invalid_status_check_pattern", pattern)) - ctx.Redirect(fmt.Sprintf("%s/settings/branches/edit?rule_name=%s", ctx.Repo.RepoLink, url.QueryEscape(protectBranch.RuleName))) + ctx.JSONErrorWithField(ctx.Tr("repo.settings.protect_invalid_status_check_pattern", pattern), "status_check_contexts") return } validPatterns = append(validPatterns, trimmed) } if len(validPatterns) == 0 { // if status check is enabled, patterns slice is not allowed to be empty - ctx.Flash.Error(ctx.Tr("repo.settings.protect_no_valid_status_check_patterns")) - ctx.Redirect(fmt.Sprintf("%s/settings/branches/edit?rule_name=%s", ctx.Repo.RepoLink, url.QueryEscape(protectBranch.RuleName))) + ctx.JSONErrorWithField(ctx.Tr("repo.settings.protect_no_valid_status_check_patterns"), "status_check_contexts") return } protectBranch.StatusCheckContexts = validPatterns @@ -334,7 +325,7 @@ func SettingsProtectedBranchPost(ctx *context.Context) { } ctx.Flash.Success(ctx.Tr("repo.settings.update_protect_branch_success", protectBranch.RuleName)) - ctx.Redirect(fmt.Sprintf("%s/settings/branches?rule_name=%s", ctx.Repo.RepoLink, protectBranch.RuleName)) + ctx.JSONRedirect(ctx.Repo.RepoLink + "/settings/branches") } // DeleteProtectedBranchRulePost delete protected branch rule by id diff --git a/routers/web/web.go b/routers/web/web.go index 8d7b5510597..caedfc55658 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -1181,7 +1181,7 @@ func registerWebRoutes(m *web.Router, webAuth *AuthMiddleware) { m.Group("/branches", func() { m.Get("/", repo_setting.ProtectedBranchRules) m.Combo("/edit").Get(repo_setting.SettingsProtectedBranch). - Post(web.Bind[*forms.ProtectBranchForm](), context.RepoMustNotBeArchived(), repo_setting.SettingsProtectedBranchPost) + Post(context.RepoMustNotBeArchived(), repo_setting.SettingsProtectedBranchPost) m.Post("/{id}/delete", repo_setting.DeleteProtectedBranchRulePost) m.Post("/priority", context.RepoMustNotBeArchived(), repo_setting.UpdateBranchProtectionPriories) }) diff --git a/templates/repo/settings/protected_branch.tmpl b/templates/repo/settings/protected_branch.tmpl index 46a57e6f7bb..6fbd8f2f4e9 100644 --- a/templates/repo/settings/protected_branch.tmpl +++ b/templates/repo/settings/protected_branch.tmpl @@ -1,6 +1,6 @@ {{template "repo/settings/layout_head" (dict "pageClass" "repository settings branches")}}
-
+

{{if .Rule.RuleName}} {{ctx.Locale.Tr "repo.settings.branch_protection" .Rule.RuleName}} @@ -145,7 +145,7 @@

{{ctx.Locale.Tr "repo.settings.event_pull_request_approvals"}}
- +

{{ctx.Locale.Tr "repo.settings.protect_required_approvals_desc"}}

diff --git a/tests/integration/actions_scoped_workflow_test.go b/tests/integration/actions_scoped_workflow_test.go index aa34e728e44..e8663b4bda2 100644 --- a/tests/integration/actions_scoped_workflow_test.go +++ b/tests/integration/actions_scoped_workflow_test.go @@ -252,7 +252,7 @@ jobs: pbValues["enable_status_check"] = "true" pbValues["status_check_contexts"] = "ci/manual" } - user2Session.MakeRequest(t, NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/settings/branches/edit", consumer.OwnerName, consumer.Name), pbValues), http.StatusSeeOther) + user2Session.MakeRequest(t, NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/settings/branches/edit", consumer.OwnerName, consumer.Name), pbValues), http.StatusOK) prFile := &api.CreateFileOptions{ FileOptions: api.FileOptions{ diff --git a/tests/integration/editor_test.go b/tests/integration/editor_test.go index a6c4e4957f7..6f442adb0f4 100644 --- a/tests/integration/editor_test.go +++ b/tests/integration/editor_test.go @@ -91,12 +91,15 @@ func testCreateFile(t *testing.T, session *TestSession, user, repo, baseBranchNa func testEditorProtectedBranch(t *testing.T) { session := loginUser(t, "user2") + assert.JSONEq(t, `{"errorMessage":"Required approvals cannot be negative.","renderFormat":"html","errorFields":["required_approvals"]}`, + session.MakeRequest(t, NewRequestWithValues(t, "POST", "/user2/repo1/settings/branches/edit", map[string]string{"rule_name": "master", "required_approvals": "-1"}), http.StatusBadRequest).Body.String()) + // Change the "master" branch to "protected" req := NewRequestWithValues(t, "POST", "/user2/repo1/settings/branches/edit", map[string]string{ "rule_name": "master", "enable_push": "true", }) - session.MakeRequest(t, req, http.StatusSeeOther) + session.MakeRequest(t, req, http.StatusOK) flashMsg := session.GetCookieFlashMessage() assert.Equal(t, `Branch protection for rule "master" has been updated.`, flashMsg.SuccessMsg) @@ -112,7 +115,7 @@ func testEditorProtectedBranch(t *testing.T) { "unprotected_file_patterns": "docs/*.md", "enable_push": "true", }) - session.MakeRequest(t, req, http.StatusSeeOther) + session.MakeRequest(t, req, http.StatusOK) flashMsg = session.GetCookieFlashMessage() assert.Equal(t, `Branch protection for rule "master" has been updated.`, flashMsg.SuccessMsg) diff --git a/tests/integration/git_general_test.go b/tests/integration/git_general_test.go index 950c0204ac2..d8439c0a3fa 100644 --- a/tests/integration/git_general_test.go +++ b/tests/integration/git_general_test.go @@ -553,8 +553,8 @@ func doProtectBranchExt(ctx APITestContext, ruleName string, opts doProtectBranc } // Send the request to update branch protection settings - req := NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/settings/branches/edit", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame)), formData) - ctx.Session.MakeRequest(t, req, http.StatusSeeOther) + link := fmt.Sprintf("/%s/%s/settings/branches", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame)) + assert.Equal(t, link, test.RedirectURL(ctx.Session.MakeRequest(t, NewRequestWithValues(t, "POST", link+"/edit", formData), http.StatusOK))) // Check if the "master" branch has been locked successfully flashMsg := ctx.Session.GetCookieFlashMessage() diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 177abc046f8..12253eff299 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -829,7 +829,7 @@ func TestPullAutoMergeAfterCommitStatusSucceed(t *testing.T) { "enable_status_check": "true", "status_check_contexts": "gitea/actions", }) - session.MakeRequest(t, req, http.StatusSeeOther) + session.MakeRequest(t, req, http.StatusOK) addToQueuePullChan := make(chan automergequeue.AutoMergeItem, 1) resetAutoMergeQueueMock := test.MockVariableValue(&automergequeue.AddToQueue, func(item automergequeue.AutoMergeItem) { addToQueuePullChan <- item }) @@ -911,7 +911,7 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApproval(t *testing.T) { "status_check_contexts": "gitea/actions", "required_approvals": "1", }) - baseSession.MakeRequest(t, req, http.StatusSeeOther) + baseSession.MakeRequest(t, req, http.StatusOK) // first time insert automerge record, return true scheduled, err := automerge.ScheduleAutoMerge(t.Context(), baseUser, pr, repo_model.MergeStyleMerge, "auto merge test", false) @@ -1023,7 +1023,7 @@ func TestPullAutoMergeAfterCommitStatusSucceedAndApprovalForAgitFlow(t *testing. "status_check_contexts": "gitea/actions", "required_approvals": "1", }) - session.MakeRequest(t, req, http.StatusSeeOther) + session.MakeRequest(t, req, http.StatusOK) user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) // first time insert automerge record, return true @@ -1098,7 +1098,7 @@ func TestPullNonMergeForAdminWithBranchProtection(t *testing.T) { "status_check_contexts": "gitea/actions", "block_admin_merge_override": "true", }) - session.MakeRequest(t, pbCreateReq, http.StatusSeeOther) + session.MakeRequest(t, pbCreateReq, http.StatusOK) token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) @@ -1144,7 +1144,7 @@ func TestPullForceMergeForBypassAllowlistUser(t *testing.T) { "enable_bypass_allowlist": "on", "bypass_allowlist_users": strconv.FormatInt(bypassUser.ID, 10), }) - ownerSession.MakeRequest(t, pbCreateReq, http.StatusSeeOther) + ownerSession.MakeRequest(t, pbCreateReq, http.StatusOK) defer testAPIDeleteBranchProtection(t, "master", http.StatusNoContent) token := getTokenForLoggedInUser(t, bypassSession, auth_model.AccessTokenScopeWriteRepository)