fix(web): populate the reason for "cannot commit to branch" in web editor commit form (#39155)

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

models/repo/repo.go

@@ -575,14 +575,13 @@ func (repo *Repository) IsOwnedBy(userID int64) bool {
 	return repo.OwnerID == userID
 }
 
-// CanCreateBranch returns true if repository meets the requirements for creating new branches.
-func (repo *Repository) CanCreateBranch() bool {
-	return !repo.IsMirror
-}
-
 // CanEnablePulls returns true if repository meets the requirements of accepting pulls.
 func (repo *Repository) CanEnablePulls() bool {
-	return !repo.IsMirror && !repo.IsEmpty
+	return repo.CanContentChange() && !repo.IsEmpty
+}
+
+func (repo *Repository) CanContentChange() bool {
+	return !repo.IsMirror && !repo.IsArchived
 }
 
 // AllowsPulls returns true if repository meets the requirements of accepting pulls and has them enabled.
@@ -590,17 +589,6 @@ func (repo *Repository) AllowsPulls(ctx context.Context) bool {
 	return repo.CanEnablePulls() && repo.UnitEnabled(ctx, unit.TypePullRequests)
 }
 
-// CanEnableEditor returns true if repository meets the requirements of web editor.
-// FIXME: most CanEnableEditor calls should be replaced with CanContentChange
-// And all other like CanCreateBranch / CanEnablePulls should also be updated
-func (repo *Repository) CanEnableEditor() bool {
-	return repo.CanContentChange()
-}
-
-func (repo *Repository) CanContentChange() bool {
-	return !repo.IsMirror && !repo.IsArchived
-}
-
 // DescriptionHTML does special handles to description and return HTML string.
 func (repo *Repository) DescriptionHTML(ctx context.Context) template.HTML {
 	return markup.PostProcessDescriptionHTML(markup.NewRenderContext(ctx), htmlutil.EscapeString(repo.Description))

options/locale/locale_en-US.json

@@ -1324,7 +1324,9 @@
   "repo.editor.upload_files_to_dir": "Upload files to \"%s\"",
   "repo.editor.cannot_commit_to_protected_branch": "Cannot commit to protected branch \"%s\".",
   "repo.editor.no_commit_to_branch": "Not allowed to commit directly to branch because:",
-  "repo.editor.user_no_push_to_branch": "User cannot push to branch",
+  "repo.editor.no_write_permission": "No write permission.",
+  "repo.editor.repo_not_editable": "Repository is not editable.",
+  "repo.editor.branch_is_protected": "Branch is protected",
   "repo.editor.require_signed_commit": "Branch requires a signed commit",
   "repo.editor.cherry_pick": "Cherry-pick %s onto:",
   "repo.editor.revert": "Revert %s onto:",

routers/api/v1/api.go

@@ -874,7 +874,7 @@ func mustNotBeArchived(ctx *context.APIContext) {
 }
 
 func mustEnableEditor(ctx *context.APIContext) {
-	if !ctx.Repo.Repository.CanEnableEditor() {
+	if !ctx.Repo.Repository.CanContentChange() {
 		ctx.APIError(http.StatusLocked, "repo is not allowed to edit")
 		return
 	}

routers/web/repo/editor.go

@@ -67,7 +67,7 @@ func prepareEditorPageFormOptions(ctx *context.Context, editorAction string) *co
 		return nil
 	}
 
-	if commitFormOptions.WillSubmitToFork && !commitFormOptions.TargetRepo.CanEnableEditor() {
+	if commitFormOptions.WillSubmitToFork && !commitFormOptions.TargetRepo.CanContentChange() {
 		ctx.Data["NotFoundPrompt"] = ctx.Locale.Tr("repo.editor.fork_not_editable")
 		ctx.NotFound(nil)
 	}

routers/web/repo/repo.go

@@ -49,7 +49,7 @@ func MustBeNotEmpty(ctx *context.Context) {
 
 // MustBeEditable check that repo can be edited
 func MustBeEditable(ctx *context.Context) {
-	if !ctx.Repo.Repository.CanEnableEditor() {
+	if !ctx.Repo.Repository.CanContentChange() {
 		ctx.NotFound(nil)
 		return
 	}

routers/web/repo/view_file.go

@@ -255,7 +255,7 @@ func prepareFileView(ctx *context.Context, entry *git.TreeEntry) {
 
 func prepareFileViewEditorButtons(ctx *context.Context) bool {
 	// archived or mirror repository, the buttons should not be shown
-	if !ctx.Repo.Repository.CanEnableEditor() {
+	if !ctx.Repo.Repository.CanContentChange() {
 		return true
 	}
 

routers/web/repo/view_readme.go

@@ -220,7 +220,7 @@ func prepareToRenderReadmeFile(ctx *context.Context, subfolder string, readmeFil
 		ctx.Data["EscapeStatus"], ctx.Data["FileContent"] = charset.EscapeControlHTML(template.HTML(contentEscaped), ctx.Locale)
 	}
 
-	if !fInfo.isLFSFile() && ctx.Repo.Repository.CanEnableEditor() {
+	if !fInfo.isLFSFile() && ctx.Repo.Repository.CanContentChange() {
 		ctx.Data["CanEditReadmeFile"] = true
 	}
 }

services/context/repo.go

@@ -9,6 +9,7 @@ import (
 	"errors"
 	"fmt"
 	"html"
+	"html/template"
 	"net/http"
 	"net/url"
 	"path"
@@ -124,7 +125,7 @@ func (r *Repository) CanWriteToBranch(ctx context.Context, user *user_model.User
 
 // CanCreateBranch returns true if repository is editable and user has proper access level.
 func (r *Repository) CanCreateBranch() bool {
-	return r.Permission.CanWrite(unit_model.TypeCode) && r.Repository.CanCreateBranch()
+	return r.Permission.CanWrite(unit_model.TypeCode) && r.Repository.CanContentChange()
 }
 
 func (r *Repository) GetObjectFormat() git.ObjectFormat {
@@ -143,15 +144,18 @@ func RepoMustNotBeArchived() func(ctx *Context) {
 type CommitFormOptions struct {
 	NeedFork bool
 
-	TargetRepo               *repo_model.Repository
+	TargetRepo       *repo_model.Repository
-	TargetFormAction         string
+	TargetFormAction string
-	WillSubmitToFork         bool
+
+	WillSubmitToFork bool
+
 	CanCommitToBranch        bool
-	UserCanPush              bool
+	DenyCommitToBranchReason template.HTML
-	RequireSigned            bool
+
-	WillSign                 bool
+	WillSign              bool
-	SigningKeyFormDisplay    string
+	SigningKeyFormDisplay string
-	WontSignReason           string
+	WontSignReason        string
+
 	CanCreatePullRequest     bool
 	CanCreateBasePullRequest bool
 }
@@ -172,7 +176,7 @@ func PrepareCommitFormOptions(ctx *Context, doer *user_model.User, targetRepo *r
 		}
 		// now, we get our own forked repo; it must be writable by us.
 	}
-	submitToForkedRepo := targetRepo.ID != originRepo.ID
+
 	err := targetRepo.GetBaseRepo(ctx)
 	if err != nil {
 		return nil, err
@@ -214,20 +218,14 @@ func PrepareCommitFormOptions(ctx *Context, doer *user_model.User, targetRepo *r
 		return nil, err
 	}
 
-	canCommitToBranch := !submitToForkedRepo /* same repo */ && targetRepo.CanEnableEditor() && canPushWithProtection
-	if protectionRequireSigned {
-		canCommitToBranch = canCommitToBranch && willSign
-	}
-
 	canCreateBasePullRequest := targetRepo.BaseRepo != nil && targetRepo.BaseRepo.UnitEnabled(ctx, unit_model.TypePullRequests)
 	canCreatePullRequest := targetRepo.UnitEnabled(ctx, unit_model.TypePullRequests) || canCreateBasePullRequest
 
 	opts := &CommitFormOptions{
-		TargetRepo:            targetRepo,
+		TargetRepo: targetRepo,
-		WillSubmitToFork:      submitToForkedRepo,
+
-		CanCommitToBranch:     canCommitToBranch,
+		WillSubmitToFork: targetRepo.ID != originRepo.ID,
-		UserCanPush:           canPushWithProtection,
+
-		RequireSigned:         protectionRequireSigned,
 		WillSign:              willSign,
 		SigningKeyFormDisplay: asymkey_model.GetDisplaySigningKey(signKey),
 		WontSignReason:        wontSignReason,
@@ -235,12 +233,28 @@ func PrepareCommitFormOptions(ctx *Context, doer *user_model.User, targetRepo *r
 		CanCreatePullRequest:     canCreatePullRequest,
 		CanCreateBasePullRequest: canCreateBasePullRequest,
 	}
+
 	editorAction := ctx.PathParam("editor_action")
 	editorPathParamRemaining := util.PathEscapeSegments(branchName) + "/" + util.PathEscapeSegments(ctx.Repo.TreePath)
-	if submitToForkedRepo {
+
+	opts.CanCommitToBranch = false
+	if opts.WillSubmitToFork {
+		opts.DenyCommitToBranchReason = ctx.Locale.Tr("repo.editor.no_write_permission")
 		// there is only "default branch" in forked repo, we will use "from_base_branch" to get a new branch from base repo
 		editorPathParamRemaining = util.PathEscapeSegments(targetRepo.DefaultBranch) + "/" + util.PathEscapeSegments(ctx.Repo.TreePath) + "?from_base_branch=" + url.QueryEscape(branchName)
+	} else {
+		// if the user is committing to the same repo, we need to check if the branch is protected and if the user can push to it
+		if !targetRepo.CanContentChange() {
+			opts.DenyCommitToBranchReason = ctx.Locale.Tr("repo.editor.repo_not_editable")
+		} else if !canPushWithProtection {
+			opts.DenyCommitToBranchReason = ctx.Locale.Tr("repo.editor.branch_is_protected")
+		} else if protectionRequireSigned && !willSign {
+			opts.DenyCommitToBranchReason = ctx.Locale.Tr("repo.editor.require_signed_commit")
+		} else {
+			opts.CanCommitToBranch = true
+		}
 	}
+
 	if editorAction == "_cherrypick" {
 		opts.TargetFormAction = targetRepo.Link() + "/" + editorAction + "/" + ctx.PathParam("sha") + "/" + editorPathParamRemaining
 	} else {

templates/repo/diff/box.tmpl

@@ -160,7 +160,7 @@
 												<a class="item" rel="nofollow" href="{{$.BeforeSourcePath}}/{{PathEscapeSegments .Name}}">{{ctx.Locale.Tr "repo.diff.view_file"}}</a>
 											{{else}}
 												<a class="item" rel="nofollow" href="{{$.SourcePath}}/{{PathEscapeSegments .Name}}">{{ctx.Locale.Tr "repo.diff.view_file"}}</a>
-												{{if and $.Repository.CanEnableEditor $.CanEditFile}}
+												{{if and $.Repository.CanContentChange $.CanEditFile}}
 													<a class="item" rel="nofollow" href="{{$.HeadRepoLink}}/_edit/{{PathEscapeSegments $.HeadBranchName}}/{{PathEscapeSegments $file.Name}}?return_uri={{print $.BackToLink "#diff-" $file.NameHash | QueryEscape}}">{{ctx.Locale.Tr "repo.editor.edit_this_file"}}</a>
 												{{end}}
 											{{end}}

templates/repo/editor/commit_form.tmpl

@@ -32,10 +32,9 @@
 						{{if not .CommitFormOptions.CanCommitToBranch}}
 						<div class="tw-mt-2">
 							{{ctx.Locale.Tr "repo.editor.no_commit_to_branch"}}
-							<ul class="tw-mb-0">
+							{{if .CommitFormOptions.DenyCommitToBranchReason}}
-								{{if not .CommitFormOptions.UserCanPush}}<li>{{ctx.Locale.Tr "repo.editor.user_no_push_to_branch"}}</li>{{end}}
+								<ul class="tw-mb-0"><li>{{.CommitFormOptions.DenyCommitToBranchReason}}</li></ul>
-								{{if and .CommitFormOptions.RequireSigned (not .CommitFormOptions.WillSign)}}<li>{{ctx.Locale.Tr "repo.editor.require_signed_commit"}}</li>{{end}}
+							{{end}}
-							</ul>
 						</div>
 						{{end}}
 					</label>

templates/repo/view_content.tmpl

@@ -70,7 +70,7 @@
 					{{$addFilePath = ""}}
 				{{end}}
 			{{end}}
-			<button class="ui dropdown basic compact jump button repo-add-file" {{if not .Repository.CanEnableEditor}}disabled{{end}}>
+			<button class="ui dropdown basic compact jump button repo-add-file" {{if not .Repository.CanContentChange}}disabled{{end}}>
 				{{ctx.Locale.Tr "repo.editor.add_file"}}
 				{{svg "octicon-triangle-down" 14 "dropdown icon"}}
 				<div class="menu">

templates/repo/view_file.tmpl

@@ -69,7 +69,7 @@
 					{{svg "octicon-rss"}}
 				</a>
 				{{end}}
-				{{if .Repository.CanEnableEditor}}
+				{{if .Repository.CanContentChange}}
 					{{if .CanEditFile}}
 						<a class="btn-octicon" data-tooltip-content="{{.EditFileTooltip}}" href="{{.RepoLink}}/_edit/{{PathEscapeSegments .BranchName}}/{{PathEscapeSegments .TreePath}}">{{svg "octicon-pencil"}}</a>
 					{{else}}