show "Add File", check branch name, move prompt, handle json error

options/locale/locale_en-US.ini

@@ -1404,6 +1404,7 @@ editor.fork_create_description = You can not edit this repository directly. Inst
 editor.fork_edit_description = You can not edit this repository directly. The changes will be written to your fork <b>%s</b>, so you can create a pull request.
 editor.fork_not_editable = You have forked this repository but your fork is not editable.
 editor.fork_failed_to_push_branch = Failed to push branch %s to your repository.
+editor.fork_branch_exists = Branch "%s" already exists in your fork, please choose a new branch name.
 
 commits.desc = Browse source code change history.
 commits.commits = Commits

routers/web/repo/editor.go

@@ -146,7 +146,17 @@ func prepareEditorCommitSubmittedForm[T forms.CommitCommonFormInterface](ctx *co
 	oldBranchName := ctx.Repo.BranchName
 	fromBaseBranch := ctx.FormString("from_base_branch")
 	if fromBaseBranch != "" {
-		err = editorPushBranchToForkedRepository(ctx, ctx.Doer, ctx.Repo.Repository.BaseRepo, fromBaseBranch, ctx.Repo.Repository, targetBranchName)
+		// if target branch exists, we should warn users
+		targetBranchExists, err := git_model.IsBranchExist(ctx, commitFormOptions.TargetRepo.ID, targetBranchName)
+		if err != nil {
+			ctx.ServerError("IsBranchExist", err)
+			return nil
+		}
+		if targetBranchExists {
+			ctx.JSONError(ctx.Tr("repo.editor.fork_branch_exists", targetBranchName))
+			return nil
+		}
+		err = editorPushBranchToForkedRepository(ctx, ctx.Doer, ctx.Repo.Repository.BaseRepo, fromBaseBranch, commitFormOptions.TargetRepo, targetBranchName)
 		if err != nil {
 			log.Error("Unable to editorPushBranchToForkedRepository: %v", err)
 			ctx.JSONError(ctx.Tr("repo.editor.fork_failed_to_push_branch", targetBranchName))

templates/repo/editor/cherry_pick.tmpl

@@ -5,6 +5,7 @@
 		{{template "base/alert" .}}
 		<form class="ui edit form form-fetch-action" method="post" action="{{.CommitFormOptions.TargetFormAction}}">
 			{{.CsrfTokenHtml}}
+			{{template "repo/editor/common_top" .}}
 			<input type="hidden" name="revert" value="{{if eq .CherryPickType "revert"}}true{{else}}false{{end}}">
 			<div class="repo-editor-header">
 				<div class="breadcrumb">

templates/repo/editor/commit_form.tmpl

@@ -78,12 +78,6 @@
 		{{end}}
 	</div>
 	<input type="hidden" name="last_commit" value="{{.last_commit}}">
-
-	{{if .CommitFormOptions.WillSubmitToFork}}
-	<div class="ui blue message">
-		<p>{{ctx.Locale.Tr "repo.editor.fork_edit_description" .CommitFormOptions.TargetRepo.FullName}}</p>
-	</div>
-	{{end}}
 	<button id="commit-button" type="submit" class="ui primary button">
 		{{if eq .commit_choice "commit-to-new-branch"}}{{ctx.Locale.Tr "repo.editor.propose_file_change"}}{{else}}{{ctx.Locale.Tr "repo.editor.commit_changes"}}{{end}}
 	</button>

templates/repo/editor/common_top.tmpl

@@ -0,0 +1,5 @@
+{{if .CommitFormOptions.WillSubmitToFork}}
+<div class="ui blue message">
+	{{ctx.Locale.Tr "repo.editor.fork_edit_description" .CommitFormOptions.TargetRepo.FullName}}
+</div>
+{{end}}

templates/repo/editor/delete.tmpl

@@ -5,6 +5,7 @@
 		{{template "base/alert" .}}
 		<form class="ui form form-fetch-action" method="post" action="{{.CommitFormOptions.TargetFormAction}}">
 			{{.CsrfTokenHtml}}
+			{{template "repo/editor/common_top" .}}
 			{{template "repo/editor/commit_form" .}}
 		</form>
 	</div>

templates/repo/editor/edit.tmpl

@@ -12,6 +12,7 @@
 				{{template "repo/editor/common_breadcrumb" .}}
 			</div>
 			{{if not .NotEditableReason}}
+				{{template "repo/editor/common_top" .}}
 				<div class="field">
 					<div class="ui top attached header">
 						<div class="ui compact small menu small-menu-items repo-editor-menu">

templates/repo/editor/patch.tmpl

@@ -19,6 +19,7 @@
 					<input id="file-name" type="hidden" value="diff.patch">
 				</div>
 			</div>
+			{{template "repo/editor/common_top" .}}
 			<div class="field">
 				<div class="ui compact small menu small-menu-items repo-editor-menu">
 					<a class="active item" data-tab="write">{{svg "octicon-code" 16 "tw-mr-1"}}{{ctx.Locale.Tr "repo.editor.new_patch"}}</a>

templates/repo/editor/upload.tmpl

@@ -8,6 +8,7 @@
 			<div class="repo-editor-header">
 				{{template "repo/editor/common_breadcrumb" .}}
 			</div>
+			{{template "repo/editor/common_top" .}}
 			<div class="field">
 				{{template "repo/upload" .}}
 			</div>

templates/repo/view_content.tmpl

@@ -41,8 +41,8 @@
 		<a href="{{.Repository.Link}}/find/{{.RefTypeNameSubURL}}" class="ui compact basic button">{{ctx.Locale.Tr "repo.find_file.go_to_file"}}</a>
 	{{end}}
 
-	{{if and .CanWriteCode .RefFullName.IsBranch (not .Repository.IsMirror) (not .Repository.IsArchived) (not .IsViewFile)}}
+	{{if and .RefFullName.IsBranch (not .IsViewFile)}}
-		<button class="ui dropdown basic compact jump button"{{if not .Repository.CanEnableEditor}} disabled{{end}}>
+		<button class="ui dropdown basic compact jump button repo-add-file" {{if not .Repository.CanEnableEditor}}disabled{{end}}>
 			{{ctx.Locale.Tr "repo.editor.add_file"}}
 			{{svg "octicon-triangle-down" 14 "dropdown icon"}}
 			<div class="menu">

tests/integration/editor_test.go

@@ -342,9 +342,15 @@ index 0000000000..bbbbbbbbbb
 }
 
 func forkToEdit(t *testing.T, session *TestSession, owner, repo, operation, branch, filePath string) {
-	// attempt to edit a file, see the guideline page
+	// visit the base repo, see the "Add File" button
-	req := NewRequest(t, "GET", path.Join(owner, repo, operation, branch, filePath))
+	req := NewRequest(t, "GET", path.Join(owner, repo))
 	resp := session.MakeRequest(t, req, http.StatusOK)
+	htmlDoc := NewHTMLParser(t, resp.Body)
+	AssertHTMLElement(t, htmlDoc, ".repo-add-file", 1)
+
+	// attempt to edit a file, see the guideline page
+	req = NewRequest(t, "GET", path.Join(owner, repo, operation, branch, filePath))
+	resp = session.MakeRequest(t, req, http.StatusOK)
 	assert.Contains(t, resp.Body.String(), "Fork Repository to Propose Changes")
 
 	// fork the repository
@@ -406,20 +412,27 @@ func testForkToEditFile(t *testing.T, session *TestSession, user, owner, repo, b
 	lastCommit := form.Find("input[name=last_commit]").AttrOr("value", "")
 	assert.NotEmpty(t, lastCommit)
 
-	// change a file in the forked repo
+	editRequestForm := map[string]string{
-	req = NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s-1/_edit/%s/%s?from_base_branch=%s", user, repo, branch, filePath, branch),
+		"_csrf":           GetUserCSRFToken(t, session),
-		map[string]string{
+		"last_commit":     lastCommit,
-			"_csrf":           GetUserCSRFToken(t, session),
+		"tree_path":       filePath,
-			"last_commit":     lastCommit,
+		"content":         "new content in fork",
-			"tree_path":       filePath,
+		"commit_choice":   commitChoice,
-			"content":         "new content in fork",
+		"new_branch_name": "master",
-			"commit_choice":   commitChoice,
+	}
-			"new_branch_name": newBranchName,
+	// change a file in the forked repo with existing branch name (should fail)
-		},
+	req = NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s-1/_edit/%s/%s?from_base_branch=%s", user, repo, branch, filePath, branch), editRequestForm)
-	)
+	resp = session.MakeRequest(t, req, http.StatusBadRequest)
+	respJSON := test.ParseJSONError(resp.Body.Bytes())
+	assert.Equal(t, `Branch "master" already exists in your fork, please choose a new branch name.`, respJSON.ErrorMessage)
+
+	// change a file in the forked repo (should succeed)
+	editRequestForm["new_branch_name"] = newBranchName
+	req = NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s-1/_edit/%s/%s?from_base_branch=%s", user, repo, branch, filePath, branch), editRequestForm)
 	resp = session.MakeRequest(t, req, http.StatusOK)
 	assert.Equal(t, fmt.Sprintf("/%s/%s/compare/%s...%s/%s-1:%s", owner, repo, branch, user, repo, newBranchName), test.RedirectURL(resp))
 
+	// check the file in the fork's branch is changed
 	req = NewRequest(t, "GET", fmt.Sprintf("/%s/%s-1/src/branch/%s/%s", user, repo, newBranchName, filePath))
 	resp = session.MakeRequest(t, req, http.StatusOK)
 	assert.Contains(t, resp.Body.String(), "new content in fork")

tests/integration/html_helper.go

@@ -42,7 +42,7 @@ func (doc *HTMLDoc) GetCSRF() string {
 	return doc.GetInputValueByName("_csrf")
 }
 
-// AssertHTMLElement check if element by selector exists or does not exist depending on checkExists
+// AssertHTMLElement check if the element by selector exists or does not exist depending on checkExists
 func AssertHTMLElement[T int | bool](t testing.TB, doc *HTMLDoc, selector string, checkExists T) {
 	sel := doc.doc.Find(selector)
 	switch v := any(checkExists).(type) {

web_src/js/features/common-fetch-action.ts

@@ -5,7 +5,7 @@ import {confirmModal} from './comp/ConfirmModal.ts';
 import type {RequestOpts} from '../types.ts';
 import {ignoreAreYouSure} from '../vendor/jquery.are-you-sure.ts';
 
-const {appSubUrl, i18n} = window.config;
+const {appSubUrl} = window.config;
 
 // fetchActionDoRedirect does real redirection to bypass the browser's limitations of "location"
 // more details are in the backend's fetch-redirect handler
@@ -23,11 +23,20 @@ function fetchActionDoRedirect(redirect: string) {
 }
 
 async function fetchActionDoRequest(actionElem: HTMLElement, url: string, opt: RequestOpts) {
+  const showErrorForResponse = (code: number, message: string) => {
+    showErrorToast(`Error ${code || 'request'}: ${message}`);
+  };
+
+  let respStatus = 0;
+  let respText = '';
   try {
     hideToastsAll();
     const resp = await request(url, opt);
-    if (resp.status === 200) {
+    respStatus = resp.status;
-      let {redirect} = await resp.json();
+    respText = await resp.text();
+    const respJson = JSON.parse(respText);
+    if (respStatus === 200) {
+      let {redirect} = respJson;
       redirect = redirect || actionElem.getAttribute('data-redirect');
       ignoreAreYouSure(actionElem); // ignore the areYouSure check before reloading
       if (redirect) {
@@ -38,22 +47,19 @@ async function fetchActionDoRequest(actionElem: HTMLElement, url: string, opt: R
       return;
     }
 
-    if (resp.status >= 400 && resp.status < 500) {
+    if (respStatus >= 400 && respStatus < 500 && respJson?.errorMessage) {
-      const data = await resp.json();
       // the code was quite messy, sometimes the backend uses "err", sometimes it uses "error", and even "user_error"
       // but at the moment, as a new approach, we only use "errorMessage" here, backend can use JSONError() to respond.
-      if (data.errorMessage) {
+      showErrorToast(respJson.errorMessage, {useHtmlBody: respJson.renderFormat === 'html'});
-        showErrorToast(data.errorMessage, {useHtmlBody: data.renderFormat === 'html'});
-      } else {
-        showErrorToast(`server error: ${resp.status}`);
-      }
     } else {
-      showErrorToast(`server error: ${resp.status}`);
+      showErrorForResponse(respStatus, respText);
     }
   } catch (e) {
-    if (e.name !== 'AbortError') {
+    if (e.name === 'SyntaxError') {
-      console.error('error when doRequest', e);
+      showErrorForResponse(respStatus, (respText || '').substring(0, 100));
-      showErrorToast(`${i18n.network_error} ${e}`);
+    } else if (e.name !== 'AbortError') {
+      console.error('fetchActionDoRequest error', e);
+      showErrorForResponse(respStatus, `${e}`);
     }
   }
   actionElem.classList.remove('is-loading', 'loading-icon-2px');