Skip to content

Commit 13013e9

Browse files
lafrikslafriks
authored
Fix PR, milestone and label functionality if issue unit is disabled (#2710) (#2714)
* 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 785ba17 commit 13013e9

File tree

7 files changed

+74
-42
lines changed

7 files changed

+74
-42
lines changed

‎models/repo.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -409,6 +409,21 @@ func (repo *Repository) UnitEnabled(tp UnitType) bool {
409409
return false
410410
}
411411

412+
// AnyUnitEnabled if this repository has the any of the given units enabled
413+
func (repo *Repository) AnyUnitEnabled(tps ...UnitType) bool {
414+
if err := repo.getUnits(x); err != nil {
415+
log.Warn("Error loading repository (ID: %d) units: %s", repo.ID, err.Error())
416+
}
417+
for _, unit := range repo.Units {
418+
for _, tp := range tps {
419+
if unit.Type == tp {
420+
return true
421+
}
422+
}
423+
}
424+
return false
425+
}
426+
412427
var (
413428
// ErrUnitNotExist organization does not exist
414429
ErrUnitNotExist = errors.New("Unit does not exist")

‎modules/context/repo.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -614,6 +614,15 @@ func CheckUnit(unitType models.UnitType) macaron.Handler {
614614
}
615615
}
616616

617+
// CheckAnyUnit will check whether any of the unit types are enabled
618+
func CheckAnyUnit(unitTypes ...models.UnitType) macaron.Handler {
619+
return func(ctx *Context) {
620+
if !ctx.Repo.Repository.AnyUnitEnabled(unitTypes...) {
621+
ctx.Handle(404, "CheckAnyUnit", fmt.Errorf("%s: %v", ctx.Tr("units.error.unit_not_allowed"), unitTypes))
622+
}
623+
}
624+
}
625+
617626
// GitHookService checks if repository Git hooks service has been enabled.
618627
func GitHookService() macaron.Handler {
619628
return func(ctx *Context) {

‎routers/repo/issue.go

Lines changed: 28 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -676,11 +676,16 @@ func ViewIssue(ctx *context.Context) {
676676
func getActionIssue(ctx *context.Context) *models.Issue {
677677
issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index"))
678678
if err != nil {
679-
if models.IsErrIssueNotExist(err) {
680-
ctx.Error(404, "GetIssueByIndex")
681-
} else {
682-
ctx.Handle(500, "GetIssueByIndex", err)
683-
}
679+
ctx.NotFoundOrServerError("GetIssueByIndex", models.IsErrIssueNotExist, err)
680+
return nil
681+
}
682+
if issue.IsPull && !ctx.Repo.Repository.UnitEnabled(models.UnitTypePullRequests) ||
683+
!issue.IsPull && !ctx.Repo.Repository.UnitEnabled(models.UnitTypeIssues) {
684+
ctx.Handle(404, "IssueOrPullRequestUnitNotAllowed", nil)
685+
return nil
686+
}
687+
if err = issue.LoadAttributes(); err != nil {
688+
ctx.Handle(500, "LoadAttributes", nil)
684689
return nil
685690
}
686691
return issue
@@ -705,6 +710,19 @@ func getActionIssues(ctx *context.Context) []*models.Issue {
705710
ctx.Handle(500, "GetIssuesByIDs", err)
706711
return nil
707712
}
713+
// Check access rights for all issues
714+
issueUnitEnabled := ctx.Repo.Repository.UnitEnabled(models.UnitTypeIssues)
715+
prUnitEnabled := ctx.Repo.Repository.UnitEnabled(models.UnitTypePullRequests)
716+
for _, issue := range issues {
717+
if issue.IsPull && !prUnitEnabled || !issue.IsPull && !issueUnitEnabled {
718+
ctx.Handle(404, "IssueOrPullRequestUnitNotAllowed", nil)
719+
return nil
720+
}
721+
if err = issue.LoadAttributes(); err != nil {
722+
ctx.Handle(500, "LoadAttributes", nil)
723+
return nil
724+
}
725+
}
708726
return issues
709727
}
710728

@@ -840,9 +858,8 @@ func UpdateIssueStatus(ctx *context.Context) {
840858

841859
// NewComment create a comment for issue
842860
func NewComment(ctx *context.Context, form auth.CreateCommentForm) {
843-
issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index"))
844-
if err != nil {
845-
ctx.NotFoundOrServerError("GetIssueByIndex", models.IsErrIssueNotExist, err)
861+
issue := getActionIssue(ctx)
862+
if ctx.Written() {
846863
return
847864
}
848865

@@ -869,7 +886,7 @@ func NewComment(ctx *context.Context, form auth.CreateCommentForm) {
869886

870887
if form.Status == "reopen" && issue.IsPull {
871888
pull := issue.PullRequest
872-
pr, err = models.GetUnmergedPullRequest(pull.HeadRepoID, pull.BaseRepoID, pull.HeadBranch, pull.BaseBranch)
889+
pr, err := models.GetUnmergedPullRequest(pull.HeadRepoID, pull.BaseRepoID, pull.HeadBranch, pull.BaseBranch)
873890
if err != nil {
874891
if !models.IsErrPullRequestNotExist(err) {
875892
ctx.Handle(500, "GetUnmergedPullRequest", err)
@@ -891,7 +908,7 @@ func NewComment(ctx *context.Context, form auth.CreateCommentForm) {
891908
if pr != nil {
892909
ctx.Flash.Info(ctx.Tr("repo.pulls.open_unmerged_pull_exists", pr.Index))
893910
} else {
894-
if err = issue.ChangeStatus(ctx.User, ctx.Repo.Repository, form.Status == "close"); err != nil {
911+
if err := issue.ChangeStatus(ctx.User, ctx.Repo.Repository, form.Status == "close"); err != nil {
895912
log.Error(4, "ChangeStatus: %v", err)
896913
} else {
897914
log.Trace("Issue [%d] status changed to closed: %v", issue.ID, issue.IsClosed)
@@ -918,7 +935,7 @@ func NewComment(ctx *context.Context, form auth.CreateCommentForm) {
918935
return
919936
}
920937

921-
comment, err = models.CreateIssueComment(ctx.User, ctx.Repo.Repository, issue, form.Content, attachments)
938+
comment, err := models.CreateIssueComment(ctx.User, ctx.Repo.Repository, issue, form.Content, attachments)
922939
if err != nil {
923940
ctx.Handle(500, "CreateIssueComment", err)
924941
return
@@ -988,10 +1005,6 @@ func DeleteComment(ctx *context.Context) {
9881005

9891006
// Milestones render milestones page
9901007
func Milestones(ctx *context.Context) {
991-
MustEnableIssues(ctx)
992-
if ctx.Written() {
993-
return
994-
}
9951008
ctx.Data["Title"] = ctx.Tr("repo.milestones")
9961009
ctx.Data["PageIsIssueList"] = true
9971010
ctx.Data["PageIsMilestones"] = true

‎routers/repo/issue_label.go

Lines changed: 0 additions & 4 deletions
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_watch.go

Lines changed: 3 additions & 5 deletions
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

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -471,42 +471,43 @@ func RegisterRoutes(m *macaron.Macaron) {
471471
m.Get("/:username/:reponame/action/:action", reqSignIn, context.RepoAssignment(), repo.Action)
472472

473473
m.Group("/:username/:reponame", func() {
474-
// FIXME: should use different URLs but mostly same logic for comments of issue and pull reuqest.
475-
// So they can apply their own enable/disable logic on routers.
476474
m.Group("/issues", func() {
477475
m.Combo("/new", repo.MustEnableIssues).Get(context.RepoRef(), repo.NewIssue).
478476
Post(bindIgnErr(auth.CreateIssueForm{}), repo.NewIssuePost)
479-
477+
}, context.CheckUnit(models.UnitTypeIssues))
478+
// FIXME: should use different URLs but mostly same logic for comments of issue and pull reuqest.
479+
// So they can apply their own enable/disable logic on routers.
480+
m.Group("/issues", func() {
480481
m.Group("/:index", func() {
481482
m.Post("/title", repo.UpdateIssueTitle)
482483
m.Post("/content", repo.UpdateIssueContent)
483484
m.Post("/watch", repo.IssueWatch)
484485
m.Combo("/comments").Post(bindIgnErr(auth.CreateCommentForm{}), repo.NewComment)
485486
})
486487

487-
m.Post("/labels", repo.UpdateIssueLabel, reqRepoWriter)
488-
m.Post("/milestone", repo.UpdateIssueMilestone, reqRepoWriter)
489-
m.Post("/assignee", repo.UpdateIssueAssignee, reqRepoWriter)
490-
m.Post("/status", repo.UpdateIssueStatus, reqRepoWriter)
491-
}, context.CheckUnit(models.UnitTypeIssues))
488+
m.Post("/labels", reqRepoWriter, repo.UpdateIssueLabel)
489+
m.Post("/milestone", reqRepoWriter, repo.UpdateIssueMilestone)
490+
m.Post("/assignee", reqRepoWriter, repo.UpdateIssueAssignee)
491+
m.Post("/status", reqRepoWriter, repo.UpdateIssueStatus)
492+
})
492493
m.Group("/comments/:id", func() {
493494
m.Post("", repo.UpdateCommentContent)
494495
m.Post("/delete", repo.DeleteComment)
495-
}, context.CheckUnit(models.UnitTypeIssues))
496+
}, context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests))
496497
m.Group("/labels", func() {
497498
m.Post("/new", bindIgnErr(auth.CreateLabelForm{}), repo.NewLabel)
498499
m.Post("/edit", bindIgnErr(auth.CreateLabelForm{}), repo.UpdateLabel)
499500
m.Post("/delete", repo.DeleteLabel)
500501
m.Post("/initialize", bindIgnErr(auth.InitializeLabelsForm{}), repo.InitializeLabels)
501-
}, reqRepoWriter, context.RepoRef(), context.CheckUnit(models.UnitTypeIssues))
502+
}, reqRepoWriter, context.RepoRef(), context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests))
502503
m.Group("/milestones", func() {
503504
m.Combo("/new").Get(repo.NewMilestone).
504505
Post(bindIgnErr(auth.CreateMilestoneForm{}), repo.NewMilestonePost)
505506
m.Get("/:id/edit", repo.EditMilestone)
506507
m.Post("/:id/edit", bindIgnErr(auth.CreateMilestoneForm{}), repo.EditMilestonePost)
507508
m.Get("/:id/:action", repo.ChangeMilestonStatus)
508509
m.Post("/delete", repo.DeleteMilestone)
509-
}, reqRepoWriter, context.RepoRef(), context.CheckUnit(models.UnitTypeIssues))
510+
}, reqRepoWriter, context.RepoRef(), context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests))
510511

511512
m.Combo("/compare/*", repo.MustAllowPulls, repo.SetEditorconfigIfExists).
512513
Get(repo.CompareAndPullRequest).
@@ -573,8 +574,8 @@ func RegisterRoutes(m *macaron.Macaron) {
573574
m.Group("", func() {
574575
m.Get("/^:type(issues|pulls)$", repo.RetrieveLabels, repo.Issues)
575576
m.Get("/^:type(issues|pulls)$/:index", repo.ViewIssue)
576-
m.Get("/labels/", repo.RetrieveLabels, repo.Labels)
577-
m.Get("/milestones", repo.Milestones)
577+
m.Get("/labels/", context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests), repo.RetrieveLabels, repo.Labels)
578+
m.Get("/milestones", context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests), repo.Milestones)
578579
}, context.RepoRef())
579580

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

‎templates/repo/issue/list.tmpl

Lines changed: 5 additions & 5 deletions
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)