Skip to content

Commit 4ca7b0f

Browse files
committed
Prevent re-review and dismiss review actions on merged PRs
1 parent 475b6e8 commit 4ca7b0f

File tree

5 files changed

+76
-5
lines changed

5 files changed

+76
-5
lines changed

models/issues/review.go

+38-3
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,24 @@ func (err ErrNotValidReviewRequest) Unwrap() error {
6666
return util.ErrInvalidArgument
6767
}
6868

69+
// ErrReReviewRequestOnMergedPR represents an error when an user tries to request a re-review on a merged PR.
70+
type ErrReReviewRequestOnMergedPR struct {
71+
}
72+
73+
// IsErrReReviewRequestOnMergedPR checks if an error is an ErrRequestedReReviewOnMergedPR.
74+
func IsErrReReviewRequestOnMergedPR(err error) bool {
75+
_, ok := err.(ErrReReviewRequestOnMergedPR)
76+
return ok
77+
}
78+
79+
func (err ErrReReviewRequestOnMergedPR) Error() string {
80+
return "cannot request a re-review on a merged PR"
81+
}
82+
83+
func (err ErrReReviewRequestOnMergedPR) Unwrap() error {
84+
return util.ErrPermissionDenied
85+
}
86+
6987
// ReviewType defines the sort of feedback a review gives
7088
type ReviewType int
7189

@@ -618,9 +636,26 @@ func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_mo
618636
return nil, err
619637
}
620638

621-
// skip it when reviewer hase been request to review
622-
if review != nil && review.Type == ReviewTypeRequest {
623-
return nil, committer.Commit() // still commit the transaction, or committer.Close() will rollback it, even if it's a reused transaction.
639+
if review != nil {
640+
// NOTE: in case of failure we still commit the transaction here, else committer.Close() will rollback it,
641+
// even if it's a reused transaction.
642+
643+
// skip it when reviewer hase been request to review
644+
if review.Type == ReviewTypeRequest {
645+
return nil, committer.Commit()
646+
}
647+
648+
if err := issue.LoadPullRequest(ctx); err != nil {
649+
return nil, err
650+
}
651+
652+
if issue.PullRequest.HasMerged {
653+
var err error = ErrReReviewRequestOnMergedPR{}
654+
if err2 := committer.Commit(); err2 != nil {
655+
err = err2
656+
}
657+
return nil, err
658+
}
624659
}
625660

626661
// if the reviewer is an official reviewer,

routers/web/repo/issue.go

+4
Original file line numberDiff line numberDiff line change
@@ -2498,6 +2498,10 @@ func UpdatePullReviewRequest(ctx *context.Context) {
24982498

24992499
_, err = issue_service.ReviewRequest(ctx, issue, ctx.Doer, reviewer, action == "attach")
25002500
if err != nil {
2501+
if issues_model.IsErrReReviewRequestOnMergedPR(err) {
2502+
ctx.Status(http.StatusForbidden)
2503+
return
2504+
}
25012505
ctx.ServerError("ReviewRequest", err)
25022506
return
25032507
}

routers/web/repo/pull_review.go

+4
Original file line numberDiff line numberDiff line change
@@ -277,6 +277,10 @@ func DismissReview(ctx *context.Context) {
277277
form := web.GetForm(ctx).(*forms.DismissReviewForm)
278278
comm, err := pull_service.DismissReview(ctx, form.ReviewID, ctx.Repo.Repository.ID, form.Message, ctx.Doer, true, true)
279279
if err != nil {
280+
if pull_service.IsErrReReviewRequestOnMergedPR(err) {
281+
ctx.Status(http.StatusForbidden)
282+
return
283+
}
280284
ctx.ServerError("pull_service.DismissReview", err)
281285
return
282286
}

services/pull/review.go

+28
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,30 @@ import (
2020
"code.gitea.io/gitea/modules/log"
2121
"code.gitea.io/gitea/modules/optional"
2222
"code.gitea.io/gitea/modules/setting"
23+
"code.gitea.io/gitea/modules/util"
2324
notify_service "code.gitea.io/gitea/services/notify"
2425
)
2526

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

29+
// ErrDismissRequestOnMergedPR represents an error when an user tries to dismiss a review associated to a merged PR.
30+
type ErrDismissRequestOnMergedPR struct {
31+
}
32+
33+
// IsErrReReviewRequestOnMergedPR checks if an error is an ErrDismissRequestOnMergedPR.
34+
func IsErrReReviewRequestOnMergedPR(err error) bool {
35+
_, ok := err.(ErrDismissRequestOnMergedPR)
36+
return ok
37+
}
38+
39+
func (err ErrDismissRequestOnMergedPR) Error() string {
40+
return "can't dismiss a review associated to a merged PR"
41+
}
42+
43+
func (err ErrDismissRequestOnMergedPR) Unwrap() error {
44+
return util.ErrPermissionDenied
45+
}
46+
2847
// checkInvalidation checks if the line of code comment got changed by another commit.
2948
// If the line got changed the comment is going to be invalidated.
3049
func checkInvalidation(ctx context.Context, c *issues_model.Comment, doer *user_model.User, repo *git.Repository, branch string) error {
@@ -382,6 +401,15 @@ func DismissReview(ctx context.Context, reviewID, repoID int64, message string,
382401
return nil, fmt.Errorf("reviews's repository is not the same as the one we expect")
383402
}
384403

404+
issue := review.Issue
405+
if err := issue.LoadPullRequest(ctx); err != nil {
406+
return nil, err
407+
}
408+
409+
if issue.PullRequest.HasMerged {
410+
return nil, ErrDismissRequestOnMergedPR{}
411+
}
412+
385413
if err := issues_model.DismissReview(ctx, review, isDismiss); err != nil {
386414
return nil, err
387415
}

templates/repo/issue/view_content/sidebar.tmpl

+2-2
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@
5959
{{end}}
6060
</div>
6161
<div class="tw-flex tw-items-center tw-gap-2">
62-
{{if (and $.Permission.IsAdmin (or (eq .Review.Type 1) (eq .Review.Type 3)) (not $.Issue.IsClosed))}}
62+
{{if (and $.Permission.IsAdmin (or (eq .Review.Type 1) (eq .Review.Type 3)) (not $.Issue.IsClosed) (not $.Issue.PullRequest.HasMerged))}}
6363
<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}}">
6464
{{svg "octicon-x" 20}}
6565
</a>
@@ -91,7 +91,7 @@
9191
{{svg "octicon-hourglass" 16}}
9292
</span>
9393
{{end}}
94-
{{if .CanChange}}
94+
{{if and .CanChange (or .Checked (not $.Issue.PullRequest.HasMerged))}}
9595
<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>
9696
{{end}}
9797
{{svg (printf "octicon-%s" .Review.Type.Icon) 16 (printf "text %s" (.Review.HTMLTypeColorName))}}

0 commit comments

Comments
 (0)