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

models/repo.go

@@ -409,6 +409,21 @@ func (repo *Repository) UnitEnabled(tp UnitType) bool {
 	return false
 }
 
+// AnyUnitEnabled if this repository has the any of the given units enabled
+func (repo *Repository) AnyUnitEnabled(tps ...UnitType) bool {
+	if err := repo.getUnits(x); err != nil {
+		log.Warn("Error loading repository (ID: %d) units: %s", repo.ID, err.Error())
+	}
+	for _, unit := range repo.Units {
+		for _, tp := range tps {
+			if unit.Type == tp {
+				return true
+			}
+		}
+	}
+	return false
+}
+
 var (
 	// ErrUnitNotExist organization does not exist
 	ErrUnitNotExist = errors.New("Unit does not exist")

modules/context/repo.go

@@ -614,6 +614,15 @@ func CheckUnit(unitType models.UnitType) macaron.Handler {
 	}
 }
 
+// CheckAnyUnit will check whether any of the unit types are enabled
+func CheckAnyUnit(unitTypes ...models.UnitType) macaron.Handler {
+	return func(ctx *Context) {
+		if !ctx.Repo.Repository.AnyUnitEnabled(unitTypes...) {
+			ctx.Handle(404, "CheckAnyUnit", fmt.Errorf("%s: %v", ctx.Tr("units.error.unit_not_allowed"), unitTypes))
+		}
+	}
+}
+
 // GitHookService checks if repository Git hooks service has been enabled.
 func GitHookService() macaron.Handler {
 	return func(ctx *Context) {

routers/repo/issue.go

@@ -676,11 +676,16 @@ func ViewIssue(ctx *context.Context) {
 func getActionIssue(ctx *context.Context) *models.Issue {
 	issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index"))
 	if err != nil {
-		if models.IsErrIssueNotExist(err) {
+		ctx.NotFoundOrServerError("GetIssueByIndex", models.IsErrIssueNotExist, err)
-			ctx.Error(404, "GetIssueByIndex")
+		return nil
-		} else {
-			ctx.Handle(500, "GetIssueByIndex", err)
 	}
+	if issue.IsPull && !ctx.Repo.Repository.UnitEnabled(models.UnitTypePullRequests) ||
+		!issue.IsPull && !ctx.Repo.Repository.UnitEnabled(models.UnitTypeIssues) {
+		ctx.Handle(404, "IssueOrPullRequestUnitNotAllowed", nil)
+		return nil
+	}
+	if err = issue.LoadAttributes(); err != nil {
+		ctx.Handle(500, "LoadAttributes", nil)
 		return nil
 	}
 	return issue
@@ -705,6 +710,19 @@ func getActionIssues(ctx *context.Context) []*models.Issue {
 		ctx.Handle(500, "GetIssuesByIDs", err)
 		return nil
 	}
+	// Check access rights for all issues
+	issueUnitEnabled := ctx.Repo.Repository.UnitEnabled(models.UnitTypeIssues)
+	prUnitEnabled := ctx.Repo.Repository.UnitEnabled(models.UnitTypePullRequests)
+	for _, issue := range issues {
+		if issue.IsPull && !prUnitEnabled || !issue.IsPull && !issueUnitEnabled {
+			ctx.Handle(404, "IssueOrPullRequestUnitNotAllowed", nil)
+			return nil
+		}
+		if err = issue.LoadAttributes(); err != nil {
+			ctx.Handle(500, "LoadAttributes", nil)
+			return nil
+		}
+	}
 	return issues
 }
 
@@ -840,9 +858,8 @@ func UpdateIssueStatus(ctx *context.Context) {
 
 // NewComment create a comment for issue
 func NewComment(ctx *context.Context, form auth.CreateCommentForm) {
-	issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index"))
+	issue := getActionIssue(ctx)
-	if err != nil {
+	if ctx.Written() {
-		ctx.NotFoundOrServerError("GetIssueByIndex", models.IsErrIssueNotExist, err)
 		return
 	}
 
@@ -869,7 +886,7 @@ func NewComment(ctx *context.Context, form auth.CreateCommentForm) {
 
 			if form.Status == "reopen" && issue.IsPull {
 				pull := issue.PullRequest
-				pr, err = models.GetUnmergedPullRequest(pull.HeadRepoID, pull.BaseRepoID, pull.HeadBranch, pull.BaseBranch)
+				pr, err := models.GetUnmergedPullRequest(pull.HeadRepoID, pull.BaseRepoID, pull.HeadBranch, pull.BaseBranch)
 				if err != nil {
 					if !models.IsErrPullRequestNotExist(err) {
 						ctx.Handle(500, "GetUnmergedPullRequest", err)
@@ -891,7 +908,7 @@ func NewComment(ctx *context.Context, form auth.CreateCommentForm) {
 			if pr != nil {
 				ctx.Flash.Info(ctx.Tr("repo.pulls.open_unmerged_pull_exists", pr.Index))
 			} else {
-				if err = issue.ChangeStatus(ctx.User, ctx.Repo.Repository, form.Status == "close"); err != nil {
+				if err := issue.ChangeStatus(ctx.User, ctx.Repo.Repository, form.Status == "close"); err != nil {
 					log.Error(4, "ChangeStatus: %v", err)
 				} else {
 					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) {
 		return
 	}
 
-	comment, err = models.CreateIssueComment(ctx.User, ctx.Repo.Repository, issue, form.Content, attachments)
+	comment, err := models.CreateIssueComment(ctx.User, ctx.Repo.Repository, issue, form.Content, attachments)
 	if err != nil {
 		ctx.Handle(500, "CreateIssueComment", err)
 		return
@@ -988,10 +1005,6 @@ func DeleteComment(ctx *context.Context) {
 
 // Milestones render milestones page
 func Milestones(ctx *context.Context) {
-	MustEnableIssues(ctx)
-	if ctx.Written() {
-		return
-	}
 	ctx.Data["Title"] = ctx.Tr("repo.milestones")
 	ctx.Data["PageIsIssueList"] = true
 	ctx.Data["PageIsMilestones"] = true

routers/repo/issue_label.go

@@ -18,10 +18,6 @@ const (
 
 // Labels render issue's labels page
 func Labels(ctx *context.Context) {
-	MustEnableIssues(ctx)
-	if ctx.Written() {
-		return
-	}
 	ctx.Data["Title"] = ctx.Tr("repo.labels")
 	ctx.Data["PageIsIssueList"] = true
 	ctx.Data["PageIsLabels"] = true

routers/repo/issue_watch.go

@@ -21,10 +21,8 @@ func IssueWatch(c *context.Context) {
 		return
 	}
 
-	issueIndex := c.ParamsInt64("index")
+	issue := getActionIssue(c)
-	issue, err := models.GetIssueByIndex(c.Repo.Repository.ID, issueIndex)
+	if c.Written() {
-	if err != nil {
-		c.Handle(http.StatusInternalServerError, "GetIssueByIndex", err)
 		return
 	}
 
@@ -33,6 +31,6 @@ func IssueWatch(c *context.Context) {
 		return
 	}
 
-	url := fmt.Sprintf("%s/issues/%d", c.Repo.RepoLink, issueIndex)
+	url := fmt.Sprintf("%s/issues/%d", c.Repo.RepoLink, issue.Index)
 	c.Redirect(url, http.StatusSeeOther)
 }

routers/routes/routes.go

@@ -471,12 +471,13 @@ func RegisterRoutes(m *macaron.Macaron) {
 	m.Get("/:username/:reponame/action/:action", reqSignIn, context.RepoAssignment(), repo.Action)
 
 	m.Group("/:username/:reponame", func() {
-		// FIXME: should use different URLs but mostly same logic for comments of issue and pull reuqest.
-		// So they can apply their own enable/disable logic on routers.
 		m.Group("/issues", func() {
 			m.Combo("/new", repo.MustEnableIssues).Get(context.RepoRef(), repo.NewIssue).
 				Post(bindIgnErr(auth.CreateIssueForm{}), repo.NewIssuePost)
-
+		}, context.CheckUnit(models.UnitTypeIssues))
+		// FIXME: should use different URLs but mostly same logic for comments of issue and pull reuqest.
+		// So they can apply their own enable/disable logic on routers.
+		m.Group("/issues", func() {
 			m.Group("/:index", func() {
 				m.Post("/title", repo.UpdateIssueTitle)
 				m.Post("/content", repo.UpdateIssueContent)
@@ -484,21 +485,21 @@ func RegisterRoutes(m *macaron.Macaron) {
 				m.Combo("/comments").Post(bindIgnErr(auth.CreateCommentForm{}), repo.NewComment)
 			})
 
-			m.Post("/labels", repo.UpdateIssueLabel, reqRepoWriter)
+			m.Post("/labels", reqRepoWriter, repo.UpdateIssueLabel)
-			m.Post("/milestone", repo.UpdateIssueMilestone, reqRepoWriter)
+			m.Post("/milestone", reqRepoWriter, repo.UpdateIssueMilestone)
-			m.Post("/assignee", repo.UpdateIssueAssignee, reqRepoWriter)
+			m.Post("/assignee", reqRepoWriter, repo.UpdateIssueAssignee)
-			m.Post("/status", repo.UpdateIssueStatus, reqRepoWriter)
+			m.Post("/status", reqRepoWriter, repo.UpdateIssueStatus)
-		}, context.CheckUnit(models.UnitTypeIssues))
+		})
 		m.Group("/comments/:id", func() {
 			m.Post("", repo.UpdateCommentContent)
 			m.Post("/delete", repo.DeleteComment)
-		}, context.CheckUnit(models.UnitTypeIssues))
+		}, context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests))
 		m.Group("/labels", func() {
 			m.Post("/new", bindIgnErr(auth.CreateLabelForm{}), repo.NewLabel)
 			m.Post("/edit", bindIgnErr(auth.CreateLabelForm{}), repo.UpdateLabel)
 			m.Post("/delete", repo.DeleteLabel)
 			m.Post("/initialize", bindIgnErr(auth.InitializeLabelsForm{}), repo.InitializeLabels)
-		}, reqRepoWriter, context.RepoRef(), context.CheckUnit(models.UnitTypeIssues))
+		}, reqRepoWriter, context.RepoRef(), context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests))
 		m.Group("/milestones", func() {
 			m.Combo("/new").Get(repo.NewMilestone).
 				Post(bindIgnErr(auth.CreateMilestoneForm{}), repo.NewMilestonePost)
@@ -506,7 +507,7 @@ func RegisterRoutes(m *macaron.Macaron) {
 			m.Post("/:id/edit", bindIgnErr(auth.CreateMilestoneForm{}), repo.EditMilestonePost)
 			m.Get("/:id/:action", repo.ChangeMilestonStatus)
 			m.Post("/delete", repo.DeleteMilestone)
-		}, reqRepoWriter, context.RepoRef(), context.CheckUnit(models.UnitTypeIssues))
+		}, reqRepoWriter, context.RepoRef(), context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests))
 
 		m.Combo("/compare/*", repo.MustAllowPulls, repo.SetEditorconfigIfExists).
 			Get(repo.CompareAndPullRequest).
@@ -573,8 +574,8 @@ func RegisterRoutes(m *macaron.Macaron) {
 		m.Group("", func() {
 			m.Get("/^:type(issues|pulls)$", repo.RetrieveLabels, repo.Issues)
 			m.Get("/^:type(issues|pulls)$/:index", repo.ViewIssue)
-			m.Get("/labels/", repo.RetrieveLabels, repo.Labels)
+			m.Get("/labels/", context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests), repo.RetrieveLabels, repo.Labels)
-			m.Get("/milestones", repo.Milestones)
+			m.Get("/milestones", context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests), repo.Milestones)
 		}, context.RepoRef())
 
 		m.Group("/wiki", func() {

templates/repo/issue/list.tmpl

@@ -103,8 +103,8 @@
 		</div>
 		<div class="issue-actions">
 			<div class="ui basic status buttons">
-				<div class="ui green active basic button issue-action" data-action="open" data-url="{{$.Link}}/status">{{.i18n.Tr "repo.issues.action_open"}}</div>
+				<div class="ui green active basic button issue-action" data-action="open" data-url="{{$.RepoLink}}/issues/status">{{.i18n.Tr "repo.issues.action_open"}}</div>
-				<div class="ui red active basic button issue-action" data-action="close" data-url="{{$.Link}}/status">{{.i18n.Tr "repo.issues.action_close"}}</div>
+				<div class="ui red active basic button issue-action" data-action="close" data-url="{{$.RepoLink}}/issues/status">{{.i18n.Tr "repo.issues.action_close"}}</div>
 			</div>
 
 			<div class="ui secondary filter menu floated right">
@@ -116,7 +116,7 @@
 					</span>
 					<div class="menu">
 						{{range .Labels}}
-							<div class="item issue-action" data-action="toggle" data-element-id="{{.ID}}" data-url="{{$.Link}}/labels">
+							<div class="item issue-action" data-action="toggle" data-element-id="{{.ID}}" data-url="{{$.RepoLink}}/issues/labels">
 								<span class="octicon {{if eq $.SelectLabels .ID}}octicon-check{{end}}"></span><span class="label color" style="background-color: {{.Color}}"></span> {{.Name | Sanitize}}
 							</div>
 						{{end}}
@@ -134,7 +134,7 @@
 						  {{.i18n.Tr "repo.issues.action_milestone_no_select"}}
 						</div>
 						{{range .Milestones}}
-							<div class="item issue-action" data-element-id="{{.ID}}" data-url="{{$.Link}}/milestone">
+							<div class="item issue-action" data-element-id="{{.ID}}" data-url="{{$.RepoLink}}/issues/milestone">
 								{{.Name | Sanitize}}
 							</div>
 						{{end}}
@@ -152,7 +152,7 @@
 							{{.i18n.Tr "repo.issues.action_assignee_no_select"}}
 						</div>
 						{{range .Assignees}}
-							<div class="item issue-action" data-element-id="{{.ID}}" data-url="{{$.Link}}/assignee">
+							<div class="item issue-action" data-element-id="{{.ID}}" data-url="{{$.RepoLink}}/issues/assignee">
 								<img src="{{.RelAvatarLink}}"> {{.Name}}
 							</div>
 						{{end}}