Skip to content

Commit c1b0c9e

Browse files
lafrikslafriks
authored
Fix PR, milestone and label functionality if issue unit is disabled (#2710)
* Fix PR, milestone and label functionality if issue unit is disabled or not assigned to user * Fix multi-actions in PR page * Change error message * Fix comment update and delete functionality in PR
1 parent a75d5c7 commit c1b0c9e

File tree

7 files changed

+68
-66
lines changed

7 files changed

+68
-66
lines changed

‎routers/repo/issue.go

+28-15
Original file line numberDiff line numberDiff line change
@@ -720,11 +720,16 @@ func ViewIssue(ctx *context.Context) {
720720
func GetActionIssue(ctx *context.Context) *models.Issue {
721721
issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index"))
722722
if err != nil {
723-
if models.IsErrIssueNotExist(err) {
724-
ctx.Error(404, "GetIssueByIndex")
725-
} else {
726-
ctx.Handle(500, "GetIssueByIndex", err)
727-
}
723+
ctx.NotFoundOrServerError("GetIssueByIndex", models.IsErrIssueNotExist, err)
724+
return nil
725+
}
726+
if issue.IsPull && !ctx.Repo.Repository.UnitEnabled(models.UnitTypePullRequests) ||
727+
!issue.IsPull && !ctx.Repo.Repository.UnitEnabled(models.UnitTypeIssues) {
728+
ctx.Handle(404, "IssueOrPullRequestUnitNotAllowed", nil)
729+
return nil
730+
}
731+
if err = issue.LoadAttributes(); err != nil {
732+
ctx.Handle(500, "LoadAttributes", nil)
728733
return nil
729734
}
730735
return issue
@@ -749,6 +754,19 @@ func getActionIssues(ctx *context.Context) []*models.Issue {
749754
ctx.Handle(500, "GetIssuesByIDs", err)
750755
return nil
751756
}
757+
// Check access rights for all issues
758+
issueUnitEnabled := ctx.Repo.Repository.UnitEnabled(models.UnitTypeIssues)
759+
prUnitEnabled := ctx.Repo.Repository.UnitEnabled(models.UnitTypePullRequests)
760+
for _, issue := range issues {
761+
if issue.IsPull && !prUnitEnabled || !issue.IsPull && !issueUnitEnabled {
762+
ctx.Handle(404, "IssueOrPullRequestUnitNotAllowed", nil)
763+
return nil
764+
}
765+
if err = issue.LoadAttributes(); err != nil {
766+
ctx.Handle(500, "LoadAttributes", nil)
767+
return nil
768+
}
769+
}
752770
return issues
753771
}
754772

@@ -884,9 +902,8 @@ func UpdateIssueStatus(ctx *context.Context) {
884902

885903
// NewComment create a comment for issue
886904
func NewComment(ctx *context.Context, form auth.CreateCommentForm) {
887-
issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index"))
888-
if err != nil {
889-
ctx.NotFoundOrServerError("GetIssueByIndex", models.IsErrIssueNotExist, err)
905+
issue := GetActionIssue(ctx)
906+
if ctx.Written() {
890907
return
891908
}
892909

@@ -913,7 +930,7 @@ func NewComment(ctx *context.Context, form auth.CreateCommentForm) {
913930

914931
if form.Status == "reopen" && issue.IsPull {
915932
pull := issue.PullRequest
916-
pr, err = models.GetUnmergedPullRequest(pull.HeadRepoID, pull.BaseRepoID, pull.HeadBranch, pull.BaseBranch)
933+
pr, err := models.GetUnmergedPullRequest(pull.HeadRepoID, pull.BaseRepoID, pull.HeadBranch, pull.BaseBranch)
917934
if err != nil {
918935
if !models.IsErrPullRequestNotExist(err) {
919936
ctx.Handle(500, "GetUnmergedPullRequest", err)
@@ -935,7 +952,7 @@ func NewComment(ctx *context.Context, form auth.CreateCommentForm) {
935952
if pr != nil {
936953
ctx.Flash.Info(ctx.Tr("repo.pulls.open_unmerged_pull_exists", pr.Index))
937954
} else {
938-
if err = issue.ChangeStatus(ctx.User, ctx.Repo.Repository, form.Status == "close"); err != nil {
955+
if err := issue.ChangeStatus(ctx.User, ctx.Repo.Repository, form.Status == "close"); err != nil {
939956
log.Error(4, "ChangeStatus: %v", err)
940957
} else {
941958
log.Trace("Issue [%d] status changed to closed: %v", issue.ID, issue.IsClosed)
@@ -962,7 +979,7 @@ func NewComment(ctx *context.Context, form auth.CreateCommentForm) {
962979
return
963980
}
964981

965-
comment, err = models.CreateIssueComment(ctx.User, ctx.Repo.Repository, issue, form.Content, attachments)
982+
comment, err := models.CreateIssueComment(ctx.User, ctx.Repo.Repository, issue, form.Content, attachments)
966983
if err != nil {
967984
ctx.Handle(500, "CreateIssueComment", err)
968985
return
@@ -1032,10 +1049,6 @@ func DeleteComment(ctx *context.Context) {
10321049

10331050
// Milestones render milestones page
10341051
func Milestones(ctx *context.Context) {
1035-
MustEnableIssues(ctx)
1036-
if ctx.Written() {
1037-
return
1038-
}
10391052
ctx.Data["Title"] = ctx.Tr("repo.milestones")
10401053
ctx.Data["PageIsIssueList"] = true
10411054
ctx.Data["PageIsMilestones"] = true

‎routers/repo/issue_label.go

-4
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,6 @@ const (
1818

1919
// Labels render issue's labels page
2020
func Labels(ctx *context.Context) {
21-
MustEnableIssues(ctx)
22-
if ctx.Written() {
23-
return
24-
}
2521
ctx.Data["Title"] = ctx.Tr("repo.labels")
2622
ctx.Data["PageIsIssueList"] = true
2723
ctx.Data["PageIsLabels"] = true

‎routers/repo/issue_stopwatch.go

+12-10
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,12 @@ import (
1313

1414
// IssueStopwatch creates or stops a stopwatch for the given issue.
1515
func IssueStopwatch(c *context.Context) {
16-
issueIndex := c.ParamsInt64("index")
17-
issue, err := models.GetIssueByIndex(c.Repo.Repository.ID, issueIndex)
18-
19-
if err != nil {
20-
c.Handle(http.StatusInternalServerError, "GetIssueByIndex", err)
16+
issue := GetActionIssue(c)
17+
if c.Written() {
18+
return
19+
}
20+
if !c.Repo.CanUseTimetracker(issue, c.User) {
21+
c.Handle(http.StatusNotFound, "CanUseTimetracker", nil)
2122
return
2223
}
2324

@@ -32,11 +33,12 @@ func IssueStopwatch(c *context.Context) {
3233

3334
// CancelStopwatch cancel the stopwatch
3435
func CancelStopwatch(c *context.Context) {
35-
issueIndex := c.ParamsInt64("index")
36-
issue, err := models.GetIssueByIndex(c.Repo.Repository.ID, issueIndex)
37-
38-
if err != nil {
39-
c.Handle(http.StatusInternalServerError, "GetIssueByIndex", err)
36+
issue := GetActionIssue(c)
37+
if c.Written() {
38+
return
39+
}
40+
if !c.Repo.CanUseTimetracker(issue, c.User) {
41+
c.Handle(http.StatusNotFound, "CanUseTimetracker", nil)
4042
return
4143
}
4244

‎routers/repo/issue_timetrack.go

+6-8
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,12 @@ import (
1515

1616
// AddTimeManually tracks time manually
1717
func AddTimeManually(c *context.Context, form auth.AddTimeManuallyForm) {
18-
issueIndex := c.ParamsInt64("index")
19-
issue, err := models.GetIssueByIndex(c.Repo.Repository.ID, issueIndex)
20-
if err != nil {
21-
if models.IsErrIssueNotExist(err) {
22-
c.Handle(http.StatusNotFound, "GetIssueByIndex", err)
23-
return
24-
}
25-
c.Handle(http.StatusInternalServerError, "GetIssueByIndex", err)
18+
issue := GetActionIssue(c)
19+
if c.Written() {
20+
return
21+
}
22+
if !c.Repo.CanUseTimetracker(issue, c.User) {
23+
c.Handle(http.StatusNotFound, "CanUseTimetracker", nil)
2624
return
2725
}
2826
url := issue.HTMLURL()

‎routers/repo/issue_watch.go

+3-5
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,8 @@ func IssueWatch(c *context.Context) {
2121
return
2222
}
2323

24-
issueIndex := c.ParamsInt64("index")
25-
issue, err := models.GetIssueByIndex(c.Repo.Repository.ID, issueIndex)
26-
if err != nil {
27-
c.Handle(http.StatusInternalServerError, "GetIssueByIndex", err)
24+
issue := GetActionIssue(c)
25+
if c.Written() {
2826
return
2927
}
3028

@@ -33,6 +31,6 @@ func IssueWatch(c *context.Context) {
3331
return
3432
}
3533

36-
url := fmt.Sprintf("%s/issues/%d", c.Repo.RepoLink, issueIndex)
34+
url := fmt.Sprintf("%s/issues/%d", c.Repo.RepoLink, issue.Index)
3735
c.Redirect(url, http.StatusSeeOther)
3836
}

‎routers/routes/routes.go

+14-19
Original file line numberDiff line numberDiff line change
@@ -474,12 +474,13 @@ func RegisterRoutes(m *macaron.Macaron) {
474474
m.Get("/:username/:reponame/action/:action", reqSignIn, context.RepoAssignment(), repo.Action)
475475

476476
m.Group("/:username/:reponame", func() {
477-
// FIXME: should use different URLs but mostly same logic for comments of issue and pull reuqest.
478-
// So they can apply their own enable/disable logic on routers.
479477
m.Group("/issues", func() {
480478
m.Combo("/new").Get(context.RepoRef(), repo.NewIssue).
481479
Post(bindIgnErr(auth.CreateIssueForm{}), repo.NewIssuePost)
482-
480+
}, context.CheckUnit(models.UnitTypeIssues))
481+
// FIXME: should use different URLs but mostly same logic for comments of issue and pull reuqest.
482+
// So they can apply their own enable/disable logic on routers.
483+
m.Group("/issues", func() {
483484
m.Group("/:index", func() {
484485
m.Post("/title", repo.UpdateIssueTitle)
485486
m.Post("/content", repo.UpdateIssueContent)
@@ -491,38 +492,32 @@ func RegisterRoutes(m *macaron.Macaron) {
491492
m.Post("/toggle", repo.IssueStopwatch)
492493
m.Post("/cancel", repo.CancelStopwatch)
493494
})
494-
495-
}, func(ctx *context.Context) {
496-
if !ctx.Repo.CanUseTimetracker(repo.GetActionIssue(ctx), ctx.User) {
497-
ctx.Handle(404, ctx.Req.RequestURI, nil)
498-
return
499-
}
500495
})
501496
})
502497

503-
m.Post("/labels", repo.UpdateIssueLabel, reqRepoWriter)
504-
m.Post("/milestone", repo.UpdateIssueMilestone, reqRepoWriter)
505-
m.Post("/assignee", repo.UpdateIssueAssignee, reqRepoWriter)
506-
m.Post("/status", repo.UpdateIssueStatus, reqRepoWriter)
507-
}, context.CheckUnit(models.UnitTypeIssues))
498+
m.Post("/labels", reqRepoWriter, repo.UpdateIssueLabel)
499+
m.Post("/milestone", reqRepoWriter, repo.UpdateIssueMilestone)
500+
m.Post("/assignee", reqRepoWriter, repo.UpdateIssueAssignee)
501+
m.Post("/status", reqRepoWriter, repo.UpdateIssueStatus)
502+
})
508503
m.Group("/comments/:id", func() {
509504
m.Post("", repo.UpdateCommentContent)
510505
m.Post("/delete", repo.DeleteComment)
511-
}, context.CheckUnit(models.UnitTypeIssues))
506+
}, context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests))
512507
m.Group("/labels", func() {
513508
m.Post("/new", bindIgnErr(auth.CreateLabelForm{}), repo.NewLabel)
514509
m.Post("/edit", bindIgnErr(auth.CreateLabelForm{}), repo.UpdateLabel)
515510
m.Post("/delete", repo.DeleteLabel)
516511
m.Post("/initialize", bindIgnErr(auth.InitializeLabelsForm{}), repo.InitializeLabels)
517-
}, reqRepoWriter, context.RepoRef(), context.CheckUnit(models.UnitTypeIssues))
512+
}, reqRepoWriter, context.RepoRef(), context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests))
518513
m.Group("/milestones", func() {
519514
m.Combo("/new").Get(repo.NewMilestone).
520515
Post(bindIgnErr(auth.CreateMilestoneForm{}), repo.NewMilestonePost)
521516
m.Get("/:id/edit", repo.EditMilestone)
522517
m.Post("/:id/edit", bindIgnErr(auth.CreateMilestoneForm{}), repo.EditMilestonePost)
523518
m.Get("/:id/:action", repo.ChangeMilestonStatus)
524519
m.Post("/delete", repo.DeleteMilestone)
525-
}, reqRepoWriter, context.RepoRef(), context.CheckUnit(models.UnitTypeIssues))
520+
}, reqRepoWriter, context.RepoRef(), context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests))
526521

527522
m.Combo("/compare/*", repo.MustAllowPulls, repo.SetEditorconfigIfExists).
528523
Get(repo.CompareAndPullRequest).
@@ -593,8 +588,8 @@ func RegisterRoutes(m *macaron.Macaron) {
593588
m.Group("", func() {
594589
m.Get("/^:type(issues|pulls)$", repo.RetrieveLabels, repo.Issues)
595590
m.Get("/^:type(issues|pulls)$/:index", repo.ViewIssue)
596-
m.Get("/labels/", repo.RetrieveLabels, repo.Labels)
597-
m.Get("/milestones", repo.Milestones)
591+
m.Get("/labels/", context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests), repo.RetrieveLabels, repo.Labels)
592+
m.Get("/milestones", context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests), repo.Milestones)
598593
}, context.RepoRef())
599594

600595
m.Group("/wiki", func() {

‎templates/repo/issue/list.tmpl

+5-5
Original file line numberDiff line numberDiff line change
@@ -103,8 +103,8 @@
103103
</div>
104104
<div class="issue-actions">
105105
<div class="ui basic status buttons">
106-
<div class="ui green active basic button issue-action" data-action="open" data-url="{{$.Link}}/status">{{.i18n.Tr "repo.issues.action_open"}}</div>
107-
<div class="ui red active basic button issue-action" data-action="close" data-url="{{$.Link}}/status">{{.i18n.Tr "repo.issues.action_close"}}</div>
106+
<div class="ui green active basic button issue-action" data-action="open" data-url="{{$.RepoLink}}/issues/status">{{.i18n.Tr "repo.issues.action_open"}}</div>
107+
<div class="ui red active basic button issue-action" data-action="close" data-url="{{$.RepoLink}}/issues/status">{{.i18n.Tr "repo.issues.action_close"}}</div>
108108
</div>
109109

110110
<div class="ui secondary filter menu floated right">
@@ -116,7 +116,7 @@
116116
</span>
117117
<div class="menu">
118118
{{range .Labels}}
119-
<div class="item issue-action" data-action="toggle" data-element-id="{{.ID}}" data-url="{{$.Link}}/labels">
119+
<div class="item issue-action" data-action="toggle" data-element-id="{{.ID}}" data-url="{{$.RepoLink}}/issues/labels">
120120
<span class="octicon {{if eq $.SelectLabels .ID}}octicon-check{{end}}"></span><span class="label color" style="background-color: {{.Color}}"></span> {{.Name | Sanitize}}
121121
</div>
122122
{{end}}
@@ -134,7 +134,7 @@
134134
{{.i18n.Tr "repo.issues.action_milestone_no_select"}}
135135
</div>
136136
{{range .Milestones}}
137-
<div class="item issue-action" data-element-id="{{.ID}}" data-url="{{$.Link}}/milestone">
137+
<div class="item issue-action" data-element-id="{{.ID}}" data-url="{{$.RepoLink}}/issues/milestone">
138138
{{.Name | Sanitize}}
139139
</div>
140140
{{end}}
@@ -152,7 +152,7 @@
152152
{{.i18n.Tr "repo.issues.action_assignee_no_select"}}
153153
</div>
154154
{{range .Assignees}}
155-
<div class="item issue-action" data-element-id="{{.ID}}" data-url="{{$.Link}}/assignee">
155+
<div class="item issue-action" data-element-id="{{.ID}}" data-url="{{$.RepoLink}}/issues/assignee">
156156
<img src="{{.RelAvatarLink}}"> {{.Name}}
157157
</div>
158158
{{end}}

0 commit comments

Comments
 (0)