Followup to pinned Issues (#24945)

This addressees some things from #24406 that came up after the PR was
merged. Mostly from @delvh.

---------

Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: delvh <dev.lh@web.de>

3 files changed, 26 insertions, 8 deletions

diff --git a/routers/api/v1/repo/issue_pin.go b/routers/api/v1/repo/issue_pin.go
index c96ede45f5..1e774d4afa 100644
--- a/routers/api/v1/repo/issue_pin.go
+++ b/routers/api/v1/repo/issue_pin.go
@@ -45,6 +45,8 @@ func PinIssue(ctx *context.APIContext) {
 	if err != nil {
 		if issues_model.IsErrIssueNotExist(err) {
 			ctx.NotFound()
+		} else if issues_model.IsErrIssueMaxPinReached(err) {
+			ctx.Error(http.StatusBadRequest, "MaxPinReached", err)
 		} else {
 			ctx.Error(http.StatusInternalServerError, "GetIssueByIndex", err)
 		}
@@ -55,11 +57,13 @@ func PinIssue(ctx *context.APIContext) {
 	err = issue.LoadRepo(ctx)
 	if err != nil {
 		ctx.Error(http.StatusInternalServerError, "LoadRepo", err)
+		return
 	}
 
 	err = issue.Pin(ctx, ctx.Doer)
 	if err != nil {
 		ctx.Error(http.StatusInternalServerError, "PinIssue", err)
+		return
 	}
 
 	ctx.Status(http.StatusNoContent)
@@ -108,11 +112,13 @@ func UnpinIssue(ctx *context.APIContext) {
 	err = issue.LoadRepo(ctx)
 	if err != nil {
 		ctx.Error(http.StatusInternalServerError, "LoadRepo", err)
+		return
 	}
 
 	err = issue.Unpin(ctx, ctx.Doer)
 	if err != nil {
 		ctx.Error(http.StatusInternalServerError, "UnpinIssue", err)
+		return
 	}
 
 	ctx.Status(http.StatusNoContent)
@@ -166,6 +172,7 @@ func MoveIssuePin(ctx *context.APIContext) {
 	err = issue.MovePin(ctx, int(ctx.ParamsInt64(":position")))
 	if err != nil {
 		ctx.Error(http.StatusInternalServerError, "MovePin", err)
+		return
 	}
 
 	ctx.Status(http.StatusNoContent)
@@ -193,12 +200,12 @@ func ListPinnedIssues(ctx *context.APIContext) {
 	//   "200":
 	//     "$ref": "#/responses/IssueList"
 	issues, err := issues_model.GetPinnedIssues(ctx, ctx.Repo.Repository.ID, false)
-
-	if err == nil {
-		ctx.JSON(http.StatusOK, convert.ToAPIIssueList(ctx, issues))
-	} else {
+	if err != nil {
 		ctx.Error(http.StatusInternalServerError, "LoadPinnedIssues", err)
+		return
 	}
+
+	ctx.JSON(http.StatusOK, convert.ToAPIIssueList(ctx, issues))
 }
 
 // ListPinnedPullRequests returns a list of all pinned PRs
@@ -225,6 +232,7 @@ func ListPinnedPullRequests(ctx *context.APIContext) {
 	issues, err := issues_model.GetPinnedIssues(ctx, ctx.Repo.Repository.ID, true)
 	if err != nil {
 		ctx.Error(http.StatusInternalServerError, "LoadPinnedPullRequests", err)
+		return
 	}
 
 	apiPrs := make([]*api.PullRequest, len(issues))
diff --git a/routers/web/repo/issue_pin.go b/routers/web/repo/issue_pin.go
index 13f2d02fe8..6586372fc5 100644
--- a/routers/web/repo/issue_pin.go
+++ b/routers/web/repo/issue_pin.go
@@ -9,6 +9,7 @@ import (
 	issues_model "code.gitea.io/gitea/models/issues"
 	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/json"
+	"code.gitea.io/gitea/modules/log"
 )
 
 // IssuePinOrUnpin pin or unpin a Issue
@@ -19,12 +20,14 @@ func IssuePinOrUnpin(ctx *context.Context) {
 	err := issue.LoadRepo(ctx)
 	if err != nil {
 		ctx.Status(http.StatusInternalServerError)
+		log.Error(err.Error())
 		return
 	}
 
 	err = issue.PinOrUnpin(ctx, ctx.Doer)
 	if err != nil {
 		ctx.Status(http.StatusInternalServerError)
+		log.Error(err.Error())
 		return
 	}
 
@@ -33,9 +36,10 @@ func IssuePinOrUnpin(ctx *context.Context) {
 
 // IssueUnpin unpins a Issue
 func IssueUnpin(ctx *context.Context) {
-	issue, err := issues_model.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":id"))
+	issue, err := issues_model.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index"))
 	if err != nil {
-		ctx.Status(http.StatusNoContent)
+		ctx.Status(http.StatusInternalServerError)
+		log.Error(err.Error())
 		return
 	}
 
@@ -43,12 +47,15 @@ func IssueUnpin(ctx *context.Context) {
 	err = issue.LoadRepo(ctx)
 	if err != nil {
 		ctx.Status(http.StatusInternalServerError)
+		log.Error(err.Error())
 		return
 	}
 
 	err = issue.Unpin(ctx, ctx.Doer)
 	if err != nil {
 		ctx.Status(http.StatusInternalServerError)
+		log.Error(err.Error())
+		return
 	}
 
 	ctx.Status(http.StatusNoContent)
@@ -69,18 +76,21 @@ func IssuePinMove(ctx *context.Context) {
 	form := &movePinIssueForm{}
 	if err := json.NewDecoder(ctx.Req.Body).Decode(&form); err != nil {
 		ctx.Status(http.StatusInternalServerError)
+		log.Error(err.Error())
 		return
 	}
 
 	issue, err := issues_model.GetIssueByID(ctx, form.ID)
 	if err != nil {
 		ctx.Status(http.StatusInternalServerError)
+		log.Error(err.Error())
 		return
 	}
 
 	err = issue.MovePin(ctx, form.Position)
 	if err != nil {
 		ctx.Status(http.StatusInternalServerError)
+		log.Error(err.Error())
 		return
 	}
 
diff --git a/routers/web/web.go b/routers/web/web.go
index a38638c483..a70e9cc3d3 100644
--- a/routers/web/web.go
+++ b/routers/web/web.go
@@ -1025,8 +1025,8 @@ func registerRoutes(m *web.Route) {
 			m.Post("/resolve_conversation", reqRepoIssuesOrPullsReader, repo.UpdateResolveConversation)
 			m.Post("/attachments", repo.UploadIssueAttachment)
 			m.Post("/attachments/remove", repo.DeleteAttachment)
-			m.Delete("/unpin/{id}", reqRepoAdmin, repo.IssueUnpin)
-			m.Post("/pin_move", reqRepoAdmin, repo.IssuePinMove)
+			m.Delete("/unpin/{index}", reqRepoAdmin, repo.IssueUnpin)
+			m.Post("/move_pin", reqRepoAdmin, repo.IssuePinMove)
 		}, context.RepoMustNotBeArchived())
 		m.Group("/comments/{id}", func() {
 			m.Post("", repo.UpdateCommentContent)