Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix PR, milestone and label functionality if issue unit is disabled #2710

Merged
merged 5 commits into from
Oct 16, 2017
Merged

Fix PR, milestone and label functionality if issue unit is disabled #2710

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ef3792f
Fix PR, milestone and label functionality if issue unit is disabled o…
lafriks Oct 15, 2017
d2237b9
Fix multi-actions in PR page
lafriks Oct 15, 2017
956aa30
Change error message
lafriks Oct 16, 2017
5e68565
Fix comment update and delete functionality in PR
lafriks Oct 16, 2017
f1db8e5
Merge branch 'master' into fix/pr_with_ext_issues
lafriks Oct 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 28 additions & 15 deletions routers/repo/issue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -720,11 +720,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.Error(404, "GetIssueByIndex")
} else {
ctx.Handle(500, "GetIssueByIndex", err)
}
ctx.NotFoundOrServerError("GetIssueByIndex", models.IsErrIssueNotExist, err)
return nil
}
if issue.IsPull && !ctx.Repo.Repository.UnitEnabled(models.UnitTypePullRequests) ||
!issue.IsPull && !ctx.Repo.Repository.UnitEnabled(models.UnitTypeIssues) {
ctx.Handle(404, "UnitEnabled", nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error message should be UnitDisabled

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daviian no, there should be function name that returned error so that from log entries it would be possible to understand what happened

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From that point of view yes, but it is confusing at the first moment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is how it is used in rest of the code :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lafriks However in this case the UnitEnabled doesn't even return an error and 404 happens if the required unit is not enabled.

return nil
}
if err = issue.LoadAttributes(); err != nil {
ctx.Handle(500, "LoadAttributes", nil)
return nil
}
return issue
Expand All @@ -749,6 +754,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, "UnitEnabled", nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

return nil
}
if err = issue.LoadAttributes(); err != nil {
ctx.Handle(500, "LoadAttributes", nil)
return nil
}
}
return issues
}

Expand Down Expand Up @@ -884,9 +902,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"))
if err != nil {
ctx.NotFoundOrServerError("GetIssueByIndex", models.IsErrIssueNotExist, err)
issue := GetActionIssue(ctx)
if ctx.Written() {
return
}

Expand All @@ -913,7 +930,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)
Expand All @@ -935,7 +952,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)
Expand All @@ -962,7 +979,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
Expand Down Expand Up @@ -1032,10 +1049,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
Expand Down
4 changes: 0 additions & 4 deletions routers/repo/issue_label.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
22 changes: 12 additions & 10 deletions routers/repo/issue_stopwatch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,12 @@ import (

// IssueStopwatch creates or stops a stopwatch for the given issue.
func IssueStopwatch(c *context.Context) {
issueIndex := c.ParamsInt64("index")
issue, err := models.GetIssueByIndex(c.Repo.Repository.ID, issueIndex)

if err != nil {
c.Handle(http.StatusInternalServerError, "GetIssueByIndex", err)
issue := GetActionIssue(c)
if c.Written() {
return
}
if !c.Repo.CanUseTimetracker(issue, c.User) {
c.Handle(http.StatusNotFound, "CanUseTimetracker", nil)
return
}

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

// CancelStopwatch cancel the stopwatch
func CancelStopwatch(c *context.Context) {
issueIndex := c.ParamsInt64("index")
issue, err := models.GetIssueByIndex(c.Repo.Repository.ID, issueIndex)

if err != nil {
c.Handle(http.StatusInternalServerError, "GetIssueByIndex", err)
issue := GetActionIssue(c)
if c.Written() {
return
}
if !c.Repo.CanUseTimetracker(issue, c.User) {
c.Handle(http.StatusNotFound, "CanUseTimetracker", nil)
return
}

Expand Down
14 changes: 6 additions & 8 deletions routers/repo/issue_timetrack.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,12 @@ import (

// AddTimeManually tracks time manually
func AddTimeManually(c *context.Context, form auth.AddTimeManuallyForm) {
issueIndex := c.ParamsInt64("index")
issue, err := models.GetIssueByIndex(c.Repo.Repository.ID, issueIndex)
if err != nil {
if models.IsErrIssueNotExist(err) {
c.Handle(http.StatusNotFound, "GetIssueByIndex", err)
return
}
c.Handle(http.StatusInternalServerError, "GetIssueByIndex", err)
issue := GetActionIssue(c)
if c.Written() {
return
}
if !c.Repo.CanUseTimetracker(issue, c.User) {
c.Handle(http.StatusNotFound, "CanUseTimetracker", nil)
return
}
url := issue.HTMLURL()
Expand Down
8 changes: 3 additions & 5 deletions routers/repo/issue_watch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,8 @@ func IssueWatch(c *context.Context) {
return
}

issueIndex := c.ParamsInt64("index")
issue, err := models.GetIssueByIndex(c.Repo.Repository.ID, issueIndex)
if err != nil {
c.Handle(http.StatusInternalServerError, "GetIssueByIndex", err)
issue := GetActionIssue(c)
if c.Written() {
return
}

Expand All @@ -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)
}
31 changes: 13 additions & 18 deletions routers/routes/routes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,12 +474,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").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)
Expand All @@ -491,20 +492,14 @@ func RegisterRoutes(m *macaron.Macaron) {
m.Post("/toggle", repo.IssueStopwatch)
m.Post("/cancel", repo.CancelStopwatch)
})

}, func(ctx *context.Context) {
if !ctx.Repo.CanUseTimetracker(repo.GetActionIssue(ctx), ctx.User) {
ctx.Handle(404, ctx.Req.RequestURI, nil)
return
}
})
})

m.Post("/labels", repo.UpdateIssueLabel, reqRepoWriter)
m.Post("/milestone", repo.UpdateIssueMilestone, reqRepoWriter)
m.Post("/assignee", repo.UpdateIssueAssignee, reqRepoWriter)
m.Post("/status", repo.UpdateIssueStatus, reqRepoWriter)
}, context.CheckUnit(models.UnitTypeIssues))
m.Post("/labels", reqRepoWriter, repo.UpdateIssueLabel)
m.Post("/milestone", reqRepoWriter, repo.UpdateIssueMilestone)
m.Post("/assignee", reqRepoWriter, repo.UpdateIssueAssignee)
m.Post("/status", reqRepoWriter, repo.UpdateIssueStatus)
})
m.Group("/comments/:id", func() {
m.Post("", repo.UpdateCommentContent)
m.Post("/delete", repo.DeleteComment)
Expand All @@ -514,15 +509,15 @@ func RegisterRoutes(m *macaron.Macaron) {
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)
m.Get("/:id/edit", repo.EditMilestone)
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).
Expand Down Expand Up @@ -593,8 +588,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("/milestones", repo.Milestones)
m.Get("/labels/", context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests), repo.RetrieveLabels, repo.Labels)
m.Get("/milestones", context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests), repo.Milestones)
}, context.RepoRef())

m.Group("/wiki", func() {
Expand Down
10 changes: 5 additions & 5 deletions templates/repo/issue/list.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 red active basic button issue-action" data-action="close" data-url="{{$.Link}}/status">{{.i18n.Tr "repo.issues.action_close"}}</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="{{$.RepoLink}}/issues/status">{{.i18n.Tr "repo.issues.action_close"}}</div>
</div>

<div class="ui secondary filter menu floated right">
Expand All @@ -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}}
Expand All @@ -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}}
Expand All @@ -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}}
Expand Down