fix(repo): prevent double-write redirect collisions on dependency errors, fix ui (#38627)

1. fix `AddDependency` and ` RemoveDependency` to respond correctly
2. refactor the form to use "form-fetch-action"
3. fix the issue dependency icon layout regression

---------

Signed-off-by: Sudhanshu Singh <sudhanshuwriterblc@gmail.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

routers/web/repo/issue_dependency.go

@@ -4,8 +4,6 @@
 package repo
 
 import (
-	"net/http"
-
 	issues_model "gitea.dev/models/issues"
 	access_model "gitea.dev/models/perm/access"
 	"gitea.dev/modules/setting"
@@ -23,7 +21,7 @@ func AddDependency(ctx *context.Context) {
 
 	// Check if the Repo is allowed to have dependencies
 	if !ctx.Repo.CanCreateIssueDependencies(ctx, ctx.Doer, issue.IsPull) {
-		ctx.HTTPError(http.StatusForbidden, "CanCreateIssueDependencies")
+		ctx.JSONError(ctx.Locale.TrString("error.permission_denied"))
 		return
 	}
 
@@ -34,20 +32,17 @@ func AddDependency(ctx *context.Context) {
 		return
 	}
 
-	// Redirect
-	defer ctx.Redirect(issue.Link())
-
 	// Dependency
 	dep, err := issues_model.GetIssueByID(ctx, depID)
 	if err != nil {
-		ctx.Flash.Error(ctx.Tr("repo.issues.dependency.add_error_dep_issue_not_exist"))
+		ctx.JSONError(ctx.Tr("repo.issues.dependency.add_error_dep_issue_not_exist"))
 		return
 	}
 
 	// Check if both issues are in the same repo if cross repository dependencies is not enabled
 	if issue.RepoID != dep.RepoID {
 		if !setting.Service.AllowCrossRepositoryDependencies {
-			ctx.Flash.Error(ctx.Tr("repo.issues.dependency.add_error_dep_not_same_repo"))
+			ctx.JSONError(ctx.Tr("repo.issues.dependency.add_error_dep_not_same_repo"))
 			return
 		}
 		if err := dep.LoadRepo(ctx); err != nil {
@@ -61,29 +56,29 @@ func AddDependency(ctx *context.Context) {
 			return
 		}
 		if !depRepoPerm.CanReadIssuesOrPulls(dep.IsPull) {
-			// you can't see this dependency
+			ctx.JSONError(ctx.Locale.TrString("error.permission_denied"))
 			return
 		}
 	}
 
 	// Check if issue and dependency is the same
 	if dep.ID == issue.ID {
-		ctx.Flash.Error(ctx.Tr("repo.issues.dependency.add_error_same_issue"))
+		ctx.JSONError(ctx.Tr("repo.issues.dependency.add_error_same_issue"))
 		return
 	}
 
 	err = issues_model.CreateIssueDependency(ctx, ctx.Doer, issue, dep)
-	if err != nil {
-		if issues_model.IsErrDependencyExists(err) {
-			ctx.Flash.Error(ctx.Tr("repo.issues.dependency.add_error_dep_exists"))
-			return
-		} else if issues_model.IsErrCircularDependency(err) {
-			ctx.Flash.Error(ctx.Tr("repo.issues.dependency.add_error_cannot_create_circular"))
-			return
-		}
+	if issues_model.IsErrDependencyExists(err) {
+		ctx.JSONError(ctx.Tr("repo.issues.dependency.add_error_dep_exists"))
+		return
+	} else if issues_model.IsErrCircularDependency(err) {
+		ctx.JSONError(ctx.Tr("repo.issues.dependency.add_error_cannot_create_circular"))
+		return
+	} else if err != nil {
 		ctx.ServerError("CreateOrUpdateIssueDependency", err)
 		return
 	}
+	ctx.JSONOK()
 }
 
 // RemoveDependency removes the dependency
@@ -97,7 +92,7 @@ func RemoveDependency(ctx *context.Context) {
 
 	// Check if the Repo is allowed to have dependencies
 	if !ctx.Repo.CanCreateIssueDependencies(ctx, ctx.Doer, issue.IsPull) {
-		ctx.HTTPError(http.StatusForbidden, "CanCreateIssueDependencies")
+		ctx.JSONError(ctx.Locale.TrString("error.permission_denied"))
 		return
 	}
 
@@ -119,7 +114,7 @@ func RemoveDependency(ctx *context.Context) {
 	case "blocking":
 		depType = issues_model.DependencyTypeBlocking
 	default:
-		ctx.HTTPError(http.StatusBadRequest, "GetDependencyType")
+		ctx.JSONError("invalid dependency type")
 		return
 	}
 
@@ -144,20 +139,18 @@ func RemoveDependency(ctx *context.Context) {
 			return
 		}
 		if !depRepoPerm.CanReadIssuesOrPulls(dep.IsPull) {
-			ctx.Redirect(issue.Link())
+			ctx.JSONError(ctx.Locale.TrString("error.permission_denied"))
 			return
 		}
 	}
 
-	if err = issues_model.RemoveIssueDependency(ctx, ctx.Doer, issue, dep, depType); err != nil {
-		if issues_model.IsErrDependencyNotExists(err) {
-			ctx.Flash.Error(ctx.Tr("repo.issues.dependency.add_error_dep_not_exist"))
-			return
-		}
+	err = issues_model.RemoveIssueDependency(ctx, ctx.Doer, issue, dep, depType)
+	if issues_model.IsErrDependencyNotExists(err) {
+		ctx.JSONError(ctx.Tr("repo.issues.dependency.add_error_dep_not_exist"))
+		return
+	} else if err != nil {
 		ctx.ServerError("RemoveIssueDependency", err)
 		return
 	}
-
-	// Redirect
-	ctx.Redirect(issue.Link())
+	ctx.JSONOK()
 }

templates/repo/issue/sidebar/issue_dependencies.tmpl

@@ -18,10 +18,10 @@
 			<span class="text" data-tooltip-content="{{if .Issue.IsPull}}{{ctx.Locale.Tr "repo.issues.dependency.pr_close_blocks"}}{{else}}{{ctx.Locale.Tr "repo.issues.dependency.issue_close_blocks"}}{{end}}">
 				<strong>{{ctx.Locale.Tr "repo.issues.dependency.blocks_short"}}</strong>
 			</span>
-			<div class="ui divided list">
+			<div class="flex-divided-list">
 				{{range .BlockingDependencies}}
-					<div class="item dependency{{if .Issue.IsClosed}} is-closed{{end}} flex-left-right">
-						<div class="item-left tw-flex tw-justify-center tw-flex-col tw-flex-1 gt-ellipsis">
+					<div class="item {{if .Issue.IsClosed}}is-closed{{end}} flex-left-right">
+						<div class="item-left">
 							<a class="muted issue-dependency-title gt-ellipsis" href="{{.Issue.Link}}" data-tooltip-content="#{{.Issue.Index}} {{.Issue.Title | ctx.RenderUtils.RenderEmoji}}">
 								#{{.Issue.Index}} {{.Issue.Title | ctx.RenderUtils.RenderEmoji}}
 							</a>
@@ -29,7 +29,7 @@
 								{{.Repository.OwnerName}}/{{.Repository.Name}}
 							</div>
 						</div>
-						<div class="item-right tw-flex tw-items-center tw-m-1">
+						<div class="item-right">
 							{{if and $.CanCreateIssueDependencies (not $.Repository.IsArchived)}}
 								<a class="muted show-modal" data-modal="#issue-remove-dependency-confirm"
 									data-modal-remove-dependency-id="{{.Issue.ID}}" data-modal-dependency-type="blocking"
@@ -52,10 +52,10 @@
 			<span class="text" data-tooltip-content="{{if .Issue.IsPull}}{{ctx.Locale.Tr "repo.issues.dependency.pr_closing_blockedby"}}{{else}}{{ctx.Locale.Tr "repo.issues.dependency.issue_closing_blockedby"}}{{end}}">
 				<strong>{{ctx.Locale.Tr "repo.issues.dependency.blocked_by_short"}}</strong>
 			</span>
-			<div class="ui divided list">
+			<div class="flex-divided-list">
 				{{range .BlockedByDependencies}}
-					<div class="item dependency{{if .Issue.IsClosed}} is-closed{{end}} flex-left-right">
-						<div class="item-left tw-flex tw-justify-center tw-flex-col tw-flex-1 gt-ellipsis">
+					<div class="item {{if .Issue.IsClosed}}is-closed{{end}} flex-left-right">
+						<div class="item-left">
 							<a class="muted issue-dependency-title gt-ellipsis" href="{{.Issue.Link}}" data-tooltip-content="#{{.Issue.Index}} {{.Issue.Title | ctx.RenderUtils.RenderEmoji}}">
 								#{{.Issue.Index}} {{.Issue.Title | ctx.RenderUtils.RenderEmoji}}
 							</a>
@@ -63,7 +63,7 @@
 								{{.Repository.OwnerName}}/{{.Repository.Name}}
 							</div>
 						</div>
-						<div class="item-right tw-flex tw-items-center tw-m-1">
+						<div class="item-right">
 							{{if and $.CanCreateIssueDependencies (not $.Repository.IsArchived)}}
 								<a class="muted show-modal" data-modal="#issue-remove-dependency-confirm"
 									data-modal-remove-dependency-id="{{.Issue.ID}}" data-modal-dependency-type="blockedBy"
@@ -76,8 +76,8 @@
 				{{end}}
 				{{if $.CanCreateIssueDependencies}}
 					{{range .BlockedByDependenciesNotPermitted}}
-						<div class="item dependency{{if .Issue.IsClosed}} is-closed{{end}} flex-left-right">
-							<div class="item-left tw-flex tw-justify-center tw-flex-col tw-flex-1 gt-ellipsis">
+						<div class="item {{if .Issue.IsClosed}}is-closed{{end}} flex-left-right">
+							<div class="item-left">
 								<div class="gt-ellipsis">
 									<span data-tooltip-content="{{ctx.Locale.Tr "repo.issues.dependency.no_permission.can_remove"}}">{{svg "octicon-lock" 16}}</span>
 									<span class="gt-ellipsis issue-dependency-title" data-tooltip-content="#{{.Issue.Index}} {{.Issue.Title | ctx.RenderUtils.RenderEmoji}}">
@@ -88,7 +88,7 @@
 									{{.Repository.OwnerName}}/{{.Repository.Name}}
 								</div>
 							</div>
-							<div class="item-right tw-flex tw-items-center tw-m-1">
+							<div class="item-right">
 								{{if and $.CanCreateIssueDependencies (not $.Repository.IsArchived)}}
 									<a class="muted show-modal" data-modal="#issue-remove-dependency-confirm"
 										data-modal-remove-dependency-id="{{.Issue.ID}}" data-modal-dependency-type="blocking"
@@ -109,7 +109,7 @@
 
 		{{if and .CanCreateIssueDependencies (not .Repository.IsArchived)}}
 			<div>
-				<form method="post" action="{{.Issue.Link}}/dependency/add" id="addDependencyForm">
+				<form method="post" action="{{.Issue.Link}}/dependency/add" id="addDependencyForm" class="form-fetch-action">
 					<div class="ui fluid action input">
 						<div class="ui search selection dropdown" id="new-dependency-drop-list" data-issue-id="{{.Issue.ID}}" data-issue-cross-repo-search="{{.AllowCrossRepositoryDependencies}}">
 							<input name="newDependency" type="hidden">
@@ -127,7 +127,7 @@
 	</div>
 
 	{{if and .CanCreateIssueDependencies (not .Repository.IsArchived)}}
-		<form id="issue-remove-dependency-confirm" class="ui g-modal-confirm modal" method="post" action="{{.Issue.Link}}/dependency/delete">
+		<form id="issue-remove-dependency-confirm" class="ui g-modal-confirm modal form-fetch-action" method="post" action="{{.Issue.Link}}/dependency/delete">
 			<div class="header">{{svg "octicon-trash"}} {{ctx.Locale.Tr "repo.issues.dependency.remove_header"}}</div>
 			<div class="content">
 				<input type="hidden" value="" name="removeDependencyID" class="remove-dependency-id">

tests/integration/api_issue_dependency_test.go

@@ -19,6 +19,7 @@ import (
 	"gitea.dev/models/unittest"
 	user_model "gitea.dev/models/user"
 	api "gitea.dev/modules/structs"
+	"gitea.dev/modules/test"
 	repo_service "gitea.dev/services/repository"
 	"gitea.dev/tests"
 
@@ -175,12 +176,9 @@ func TestWebDeleteIssueDependencyCrossRepoPermission(t *testing.T) {
 		"removeDependencyID": strconv.FormatInt(dependencyIssue.ID, 10),
 		"dependencyType":     "blockedBy",
 	})
-	session.MakeRequest(t, req, http.StatusSeeOther)
-
-	unittest.AssertExistsAndLoadBean(t, &issues_model.IssueDependency{
-		IssueID:      targetIssue.ID,
-		DependencyID: dependencyIssue.ID,
-	})
+	resp := session.MakeRequest(t, req, http.StatusBadRequest)
+	assert.Equal(t, "Permission denied.", test.ParseJSONError(resp.Body.Bytes()).ErrorMessage)
+	unittest.AssertExistsAndLoadBean(t, &issues_model.IssueDependency{IssueID: targetIssue.ID, DependencyID: dependencyIssue.ID})
 
 	assert.NoError(t, repo_service.AddOrUpdateCollaborator(t.Context(), dependencyRepo, user40, perm.AccessModeRead))
 
@@ -188,10 +186,6 @@ func TestWebDeleteIssueDependencyCrossRepoPermission(t *testing.T) {
 		"removeDependencyID": strconv.FormatInt(dependencyIssue.ID, 10),
 		"dependencyType":     "blockedBy",
 	})
-	session.MakeRequest(t, req, http.StatusSeeOther)
-
-	unittest.AssertNotExistsBean(t, &issues_model.IssueDependency{
-		IssueID:      targetIssue.ID,
-		DependencyID: dependencyIssue.ID,
-	})
+	session.MakeRequest(t, req, http.StatusOK)
+	unittest.AssertNotExistsBean(t, &issues_model.IssueDependency{IssueID: targetIssue.ID, DependencyID: dependencyIssue.ID})
 }

web_src/css/repo.css

@@ -649,6 +649,13 @@ td .commit-summary {
   text-decoration: line-through;
 }
 
+.repository.view.issue .ui.depending .item .item-left {
+  display: flex;
+  flex: 1;
+  flex-direction: column;
+  min-width: 0;
+}
+
 .repository .comment.form .content .field:first-child {
   clear: none;
 }