From 1b115296d3f2f396eebcb40d79aea814d282edaf Mon Sep 17 00:00:00 2001 From: JakobDev Date: Tue, 30 May 2023 17:26:51 +0200 Subject: [PATCH] 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 Co-authored-by: delvh --- custom/conf/app.example.ini | 2 +- .../administration/config-cheat-sheet.en-us.md | 2 +- models/issues/issue.go | 11 +++++++++-- routers/api/v1/repo/issue_pin.go | 16 ++++++++++++---- routers/web/repo/issue_pin.go | 14 ++++++++++++-- routers/web/web.go | 4 ++-- templates/repo/issue/list.tmpl | 4 ++-- web_src/css/repo.css | 8 -------- 8 files changed, 39 insertions(+), 22 deletions(-) diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index 6b8eed1efc..01abb15c1d 100644 --- a/custom/conf/app.example.ini +++ b/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 diff --git a/docs/content/doc/administration/config-cheat-sheet.en-us.md b/docs/content/doc/administration/config-cheat-sheet.en-us.md index 4f58dc1773..514719a99a 100644 --- a/docs/content/doc/administration/config-cheat-sheet.en-us.md +++ b/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`) diff --git a/models/issues/issue.go b/models/issues/issue.go index 5015824e9b..eab18f4892 100644 --- a/models/issues/issue.go +++ b/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 +} 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) diff --git a/templates/repo/issue/list.tmpl b/templates/repo/issue/list.tmpl index dab6652d21..05aa86fd79 100644 --- a/templates/repo/issue/list.tmpl +++ b/templates/repo/issue/list.tmpl @@ -6,7 +6,7 @@ {{if .PinnedIssues}}
{{range .PinnedIssues}} -
+
{{if eq $.Project.CardType 1}}
{{range (index $.issuesAttachmentMap .ID)}} @@ -21,7 +21,7 @@
{{.Title | RenderEmoji $.Context | RenderCodeBlock}} {{if $.IsRepoAdmin}} - + {{svg "octicon-x" 16}} {{end}} diff --git a/web_src/css/repo.css b/web_src/css/repo.css index 05f0019020..1f870658d5 100644 --- a/web_src/css/repo.css +++ b/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;