Skip to content

Commit

Permalink
Prevent re-review and dismiss review actions on closed or merged PRs
Browse files Browse the repository at this point in the history
  • Loading branch information
kemzeb committed Mar 27, 2024
1 parent 475b6e8 commit 12139f5
Show file tree
Hide file tree
Showing 5 changed files with 78 additions and 5 deletions.
38 changes: 35 additions & 3 deletions models/issues/review.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,23 @@ func (err ErrNotValidReviewRequest) Unwrap() error {
return util.ErrInvalidArgument
}

// ErrReviewRequestOnClosedPR represents an error when an user tries to request a re-review on a closed or merged PR.
type ErrReviewRequestOnClosedPR struct{}

// IsErrReviewRequestOnClosedPR checks if an error is an ErrReviewRequestOnClosedPR.
func IsErrReviewRequestOnClosedPR(err error) bool {
_, ok := err.(ErrReviewRequestOnClosedPR)
return ok
}

func (err ErrReviewRequestOnClosedPR) Error() string {
return "cannot request a re-review on a closed or merged PR"
}

func (err ErrReviewRequestOnClosedPR) Unwrap() error {
return util.ErrPermissionDenied
}

// ReviewType defines the sort of feedback a review gives
type ReviewType int

Expand Down Expand Up @@ -618,9 +635,24 @@ func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_mo
return nil, err
}

// skip it when reviewer hase been request to review
if review != nil && review.Type == ReviewTypeRequest {
return nil, committer.Commit() // still commit the transaction, or committer.Close() will rollback it, even if it's a reused transaction.
if review != nil {
// skip it when reviewer hase been request to review
if review.Type == ReviewTypeRequest {
return nil, committer.Commit() // still commit the transaction, or committer.Close() will rollback it, even if it's a reused transaction.
}

if issue.IsClosed {
return nil, ErrReviewRequestOnClosedPR{}
}

if issue.IsPull {
if err := issue.LoadPullRequest(ctx); err != nil {
return nil, err
}
if issue.PullRequest.HasMerged {
return nil, ErrReviewRequestOnClosedPR{}
}
}
}

// if the reviewer is an official reviewer,
Expand Down
4 changes: 4 additions & 0 deletions routers/web/repo/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -2498,6 +2498,10 @@ func UpdatePullReviewRequest(ctx *context.Context) {

_, err = issue_service.ReviewRequest(ctx, issue, ctx.Doer, reviewer, action == "attach")
if err != nil {
if issues_model.IsErrReviewRequestOnClosedPR(err) {
ctx.Status(http.StatusForbidden)
return
}
ctx.ServerError("ReviewRequest", err)
return
}
Expand Down
4 changes: 4 additions & 0 deletions routers/web/repo/pull_review.go
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,10 @@ func DismissReview(ctx *context.Context) {
form := web.GetForm(ctx).(*forms.DismissReviewForm)
comm, err := pull_service.DismissReview(ctx, form.ReviewID, ctx.Repo.Repository.ID, form.Message, ctx.Doer, true, true)
if err != nil {
if pull_service.IsErrDismissRequestOnClosedPR(err) {
ctx.Status(http.StatusForbidden)
return
}
ctx.ServerError("pull_service.DismissReview", err)
return
}
Expand Down
33 changes: 33 additions & 0 deletions services/pull/review.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,29 @@ import (
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/optional"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/util"
notify_service "code.gitea.io/gitea/services/notify"
)

var notEnoughLines = regexp.MustCompile(`fatal: file .* has only \d+ lines?`)

// ErrDismissRequestOnClosedPR represents an error when an user tries to dismiss a review associated to a closed or merged PR.
type ErrDismissRequestOnClosedPR struct{}

// IsErrDismissRequestOnClosedPR checks if an error is an ErrDismissRequestOnClosedPR.
func IsErrDismissRequestOnClosedPR(err error) bool {
_, ok := err.(ErrDismissRequestOnClosedPR)
return ok
}

func (err ErrDismissRequestOnClosedPR) Error() string {
return "can't dismiss a review associated to a closed or merged PR"
}

func (err ErrDismissRequestOnClosedPR) Unwrap() error {
return util.ErrPermissionDenied
}

// checkInvalidation checks if the line of code comment got changed by another commit.
// If the line got changed the comment is going to be invalidated.
func checkInvalidation(ctx context.Context, c *issues_model.Comment, doer *user_model.User, repo *git.Repository, branch string) error {
Expand Down Expand Up @@ -382,6 +400,21 @@ func DismissReview(ctx context.Context, reviewID, repoID int64, message string,
return nil, fmt.Errorf("reviews's repository is not the same as the one we expect")
}

issue := review.Issue

if issue.IsClosed {
return nil, ErrDismissRequestOnClosedPR{}
}

if issue.IsPull {
if err := issue.LoadPullRequest(ctx); err != nil {
return nil, err
}
if issue.PullRequest.HasMerged {
return nil, ErrDismissRequestOnClosedPR{}
}
}

if err := issues_model.DismissReview(ctx, review, isDismiss); err != nil {
return nil, err
}
Expand Down
4 changes: 2 additions & 2 deletions templates/repo/issue/view_content/sidebar.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
{{end}}
</div>
<div class="tw-flex tw-items-center tw-gap-2">
{{if (and $.Permission.IsAdmin (or (eq .Review.Type 1) (eq .Review.Type 3)) (not $.Issue.IsClosed))}}
{{if (and $.Permission.IsAdmin (or (eq .Review.Type 1) (eq .Review.Type 3)) (not $.Issue.IsClosed) (not $.Issue.PullRequest.HasMerged))}}
<a href="#" class="ui muted icon tw-flex tw-items-center show-modal" data-tooltip-content="{{ctx.Locale.Tr "repo.issues.dismiss_review"}}" data-modal="#dismiss-review-modal-{{.Review.ID}}">
{{svg "octicon-x" 20}}
</a>
Expand Down Expand Up @@ -91,7 +91,7 @@
{{svg "octicon-hourglass" 16}}
</span>
{{end}}
{{if .CanChange}}
{{if and .CanChange (or .Checked (and (not $.Issue.IsClosed) (not $.Issue.PullRequest.HasMerged)))}}
<a href="#" class="ui muted icon re-request-review{{if .Checked}} checked{{end}}" data-tooltip-content="{{if .Checked}}{{ctx.Locale.Tr "repo.issues.remove_request_review"}}{{else}}{{ctx.Locale.Tr "repo.issues.re_request_review"}}{{end}}" data-issue-id="{{$.Issue.ID}}" data-id="{{.ItemID}}" data-update-url="{{$.RepoLink}}/issues/request_review">{{if .Checked}}{{svg "octicon-trash"}}{{else}}{{svg "octicon-sync"}}{{end}}</a>
{{end}}
{{svg (printf "octicon-%s" .Review.Type.Icon) 16 (printf "text %s" (.Review.HTMLTypeColorName))}}
Expand Down

0 comments on commit 12139f5

Please sign in to comment.