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 <[email protected]>
Co-authored-by: delvh <[email protected]>

Author: 3 people

custom/conf/app.example.ini

@@ -1044,7 +1044,7 @@ LEVEL = Info
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ;; List of reasons why a Pull Request or Issue can be locked
 ;LOCK_REASONS = Too heated,Off-topic,Resolved,Spam
-;; Maximum number of pinned Issues
+;; Maximum number of pinned Issues per repo
 ;; Set to 0 to disable pinning Issues
 ;MAX_PINNED = 3
 

docs/content/doc/administration/config-cheat-sheet.en-us.md

@@ -141,7 +141,7 @@ In addition there is _`StaticRootPath`_ which can be set as a built-in at build
 ### Repository - Issue (`repository.issue`)
 
 - `LOCK_REASONS`: **Too heated,Off-topic,Resolved,Spam**: A list of reasons why a Pull Request or Issue can be locked
-- `MAX_PINNED`: **3**: Maximum number of pinned Issues. Set to 0 to disable pinning Issues.
+- `MAX_PINNED`: **3**: Maximum number of pinned Issues per Repo. Set to 0 to disable pinning Issues.
 
 ### Repository - Upload (`repository.upload`)
 

models/issues/issue.go

@@ -687,6 +687,8 @@ func (issue *Issue) HasOriginalAuthor() bool {
 	return issue.OriginalAuthor != "" && issue.OriginalAuthorID != 0
 }
 
+var ErrIssueMaxPinReached = util.NewInvalidArgumentErrorf("the max number of pinned issues has been readched")
+
 // IsPinned returns if a Issue is pinned
 func (issue *Issue) IsPinned() bool {
 	return issue.PinOrder != 0
@@ -707,7 +709,7 @@ func (issue *Issue) Pin(ctx context.Context, user *user_model.User) error {
 
 	// Check if the maximum allowed Pins reached
 	if maxPin >= setting.Repository.Issue.MaxPinned {
-		return fmt.Errorf("You have reached the max number of pinned Issues")
+		return ErrIssueMaxPinReached
 	}
 
 	_, err = db.GetEngine(ctx).Table("issue").
@@ -856,10 +858,15 @@ func GetPinnedIssues(ctx context.Context, repoID int64, isPull bool) ([]*Issue,
 // IsNewPinnedAllowed returns if a new Issue or Pull request can be pinned
 func IsNewPinAllowed(ctx context.Context, repoID int64, isPull bool) (bool, error) {
 	var maxPin int
-	_, err := db.GetEngine(ctx).SQL("SELECT MAX(pin_order) FROM issue WHERE repo_id = ? AND is_pull = ?", repoID, isPull).Get(&maxPin)
+	_, err := db.GetEngine(ctx).SQL("SELECT COUNT(pin_order) FROM issue WHERE repo_id = ? AND is_pull = ? AND pin_order > 0", repoID, isPull).Get(&maxPin)
 	if err != nil {
 		return false, err
 	}
 
 	return maxPin < setting.Repository.Issue.MaxPinned, nil
 }
+
+// IsErrIssueMaxPinReached returns if the error is, that the User can't pin more Issues
+func IsErrIssueMaxPinReached(err error) bool {
+	return err == ErrIssueMaxPinReached
+}

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))

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,22 +36,26 @@ 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
 	}
 
 	// If we don't do this, it will crash when trying to add the pin event to the comment history
 	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
 	}
 

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)

templates/repo/issue/list.tmpl

@@ -6,7 +6,7 @@
 	{{if .PinnedIssues}}
 		<div id="issue-pins" {{if .IsRepoAdmin}}data-is-repo-admin{{end}}>
 			{{range .PinnedIssues}}
-				<div class="pinned-issue-card gt-word-break" data-move-url="{{$.Link}}/pin_move" data-issue-id="{{.ID}}">
+				<div class="pinned-issue-card gt-word-break" data-move-url="{{$.Link}}/move_pin" data-issue-id="{{.ID}}">
 					{{if eq $.Project.CardType 1}}
 						<div class="card-attachment-images">
 							{{range (index $.issuesAttachmentMap .ID)}}
@@ -21,7 +21,7 @@
 							</div>
 							<a class="pinned-issue-title muted issue-title" href="{{.Link}}">{{.Title | RenderEmoji $.Context | RenderCodeBlock}}</a>
 							{{if $.IsRepoAdmin}}
-								<a role="button" class="pinned-issue-unpin muted gt-df gt-ac" data-tooltip-content={{$.locale.Tr "repo.issues.unpin_issue"}} data-issue-id="{{.ID}}" data-unpin-url="{{$.Link}}/unpin/{{.ID}}">
+								<a role="button" class="pinned-issue-unpin muted gt-df gt-ac" data-tooltip-content={{$.locale.Tr "repo.issues.unpin_issue"}} data-issue-id="{{.ID}}" data-unpin-url="{{$.Link}}/unpin/{{.Index}}">
 									{{svg "octicon-x" 16}}
 								</a>
 							{{end}}

web_src/css/repo.css

@@ -3419,14 +3419,6 @@ tbody.commit-list {
   background: var(--color-card);
 }
 
-.pinned-issue-card .meta a {
-  color: inherit;
-}
-
-.pinned-issue-card .meta a:hover {
-  color: var(--color-primary);
-}
-
 .pinned-issue-icon,
 .pinned-issue-unpin {
   margin-top: 1px;