Prevent re-review and dismiss review actions on closed and merged PRs (#30065)

Resolves #29965.

---
Manually tested this by:
- Following the
[installation](https://docs.gitea.com/next/installation/install-with-docker#basics)
guide (but built a local Docker image instead)
- Creating 2 users, one who is the `Owner` of a newly-created repository
and the other a `Collaborator`
- Had the `Collaborator` create a PR that the `Owner` reviews
- `Collaborator` resolves conversation and `Owner` merges PR

And with this change we see that we can no longer see re-request review
button for the `Owner`:

<img width="1351" alt="Screenshot 2024-03-25 at 12 39 18 AM"
src="https://github.com/go-gitea/gitea/assets/60799661/bcd9c579-3cf7-474f-a51e-b436fe1a39a4">

(cherry picked from commit 242b331260925e604150346e61329097d5731e77)
This commit is contained in:
Kemal Zebari 2024-03-28 08:19:24 -07:00 committed by Earl Warren
parent 3ccb0c2512
commit 23676bfea7
No known key found for this signature in database
GPG key ID: 0579CB2928A78A00
9 changed files with 170 additions and 11 deletions

View file

@ -66,6 +66,23 @@ func (err ErrNotValidReviewRequest) Unwrap() error {
return util.ErrInvalidArgument 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 // ReviewType defines the sort of feedback a review gives
type ReviewType int type ReviewType int
@ -618,11 +635,26 @@ func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_mo
return nil, err return nil, err
} }
if review != nil {
// skip it when reviewer hase been request to review // skip it when reviewer hase been request to review
if review != nil && review.Type == ReviewTypeRequest { 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. 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, // if the reviewer is an official reviewer,
// remove the official flag in the all previous reviews // remove the official flag in the all previous reviews
official, err := IsOfficialReviewer(ctx, issue, reviewer) official, err := IsOfficialReviewer(ctx, issue, reviewer)

View file

@ -288,3 +288,33 @@ func TestDeleteDismissedReview(t *testing.T) {
assert.NoError(t, issues_model.DeleteReview(db.DefaultContext, review)) assert.NoError(t, issues_model.DeleteReview(db.DefaultContext, review))
unittest.AssertNotExistsBean(t, &issues_model.Comment{ID: comment.ID}) unittest.AssertNotExistsBean(t, &issues_model.Comment{ID: comment.ID})
} }
func TestAddReviewRequest(t *testing.T) {
assert.NoError(t, unittest.PrepareTestDatabase())
pull := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 1})
assert.NoError(t, pull.LoadIssue(db.DefaultContext))
issue := pull.Issue
assert.NoError(t, issue.LoadRepo(db.DefaultContext))
reviewer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
_, err := issues_model.CreateReview(db.DefaultContext, issues_model.CreateReviewOptions{
Issue: issue,
Reviewer: reviewer,
Type: issues_model.ReviewTypeReject,
})
assert.NoError(t, err)
pull.HasMerged = false
assert.NoError(t, pull.UpdateCols(db.DefaultContext, "has_merged"))
issue.IsClosed = true
_, err = issues_model.AddReviewRequest(db.DefaultContext, issue, reviewer, &user_model.User{})
assert.Error(t, err)
assert.True(t, issues_model.IsErrReviewRequestOnClosedPR(err))
pull.HasMerged = true
assert.NoError(t, pull.UpdateCols(db.DefaultContext, "has_merged"))
issue.IsClosed = false
_, err = issues_model.AddReviewRequest(db.DefaultContext, issue, reviewer, &user_model.User{})
assert.Error(t, err)
assert.True(t, issues_model.IsErrReviewRequestOnClosedPR(err))
}

View file

@ -787,6 +787,8 @@ func DeleteReviewRequests(ctx *context.APIContext) {
// "$ref": "#/responses/empty" // "$ref": "#/responses/empty"
// "422": // "422":
// "$ref": "#/responses/validationError" // "$ref": "#/responses/validationError"
// "403":
// "$ref": "#/responses/forbidden"
// "404": // "404":
// "$ref": "#/responses/notFound" // "$ref": "#/responses/notFound"
opts := web.GetForm(ctx).(*api.PullReviewRequestOptions) opts := web.GetForm(ctx).(*api.PullReviewRequestOptions)
@ -855,6 +857,10 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions
for _, reviewer := range reviewers { for _, reviewer := range reviewers {
comment, err := issue_service.ReviewRequest(ctx, pr.Issue, ctx.Doer, reviewer, isAdd) comment, err := issue_service.ReviewRequest(ctx, pr.Issue, ctx.Doer, reviewer, isAdd)
if err != nil { if err != nil {
if issues_model.IsErrReviewRequestOnClosedPR(err) {
ctx.Error(http.StatusForbidden, "", err)
return
}
ctx.Error(http.StatusInternalServerError, "ReviewRequest", err) ctx.Error(http.StatusInternalServerError, "ReviewRequest", err)
return return
} }
@ -1068,7 +1074,7 @@ func dismissReview(ctx *context.APIContext, msg string, isDismiss, dismissPriors
ctx.Error(http.StatusForbidden, "", "Must be repo admin") ctx.Error(http.StatusForbidden, "", "Must be repo admin")
return return
} }
review, pr, isWrong := prepareSingleReview(ctx) review, _, isWrong := prepareSingleReview(ctx)
if isWrong { if isWrong {
return return
} }
@ -1078,13 +1084,12 @@ func dismissReview(ctx *context.APIContext, msg string, isDismiss, dismissPriors
return return
} }
if pr.Issue.IsClosed {
ctx.Error(http.StatusForbidden, "", "not need to dismiss this review because this pr is closed")
return
}
_, err := pull_service.DismissReview(ctx, review.ID, ctx.Repo.Repository.ID, msg, ctx.Doer, isDismiss, dismissPriors) _, err := pull_service.DismissReview(ctx, review.ID, ctx.Repo.Repository.ID, msg, ctx.Doer, isDismiss, dismissPriors)
if err != nil { if err != nil {
if pull_service.IsErrDismissRequestOnClosedPR(err) {
ctx.Error(http.StatusForbidden, "", err)
return
}
ctx.Error(http.StatusInternalServerError, "pull_service.DismissReview", err) ctx.Error(http.StatusInternalServerError, "pull_service.DismissReview", err)
return return
} }

View file

@ -2494,6 +2494,10 @@ func UpdatePullReviewRequest(ctx *context.Context) {
_, err = issue_service.ReviewRequest(ctx, issue, ctx.Doer, reviewer, action == "attach") _, err = issue_service.ReviewRequest(ctx, issue, ctx.Doer, reviewer, action == "attach")
if err != nil { if err != nil {
if issues_model.IsErrReviewRequestOnClosedPR(err) {
ctx.Status(http.StatusForbidden)
return
}
ctx.ServerError("ReviewRequest", err) ctx.ServerError("ReviewRequest", err)
return return
} }

View file

@ -263,6 +263,10 @@ func DismissReview(ctx *context.Context) {
form := web.GetForm(ctx).(*forms.DismissReviewForm) form := web.GetForm(ctx).(*forms.DismissReviewForm)
comm, err := pull_service.DismissReview(ctx, form.ReviewID, ctx.Repo.Repository.ID, form.Message, ctx.Doer, true, true) comm, err := pull_service.DismissReview(ctx, form.ReviewID, ctx.Repo.Repository.ID, form.Message, ctx.Doer, true, true)
if err != nil { if err != nil {
if pull_service.IsErrDismissRequestOnClosedPR(err) {
ctx.Status(http.StatusForbidden)
return
}
ctx.ServerError("pull_service.DismissReview", err) ctx.ServerError("pull_service.DismissReview", err)
return return
} }

View file

@ -20,11 +20,29 @@ import (
"code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/optional"
"code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/util"
notify_service "code.gitea.io/gitea/services/notify" notify_service "code.gitea.io/gitea/services/notify"
) )
var notEnoughLines = regexp.MustCompile(`fatal: file .* has only \d+ lines?`) 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. // 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. // 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 { func checkInvalidation(ctx context.Context, c *issues_model.Comment, doer *user_model.User, repo *git.Repository, branch string) error {
@ -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") 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 { if err := issues_model.DismissReview(ctx, review, isDismiss); err != nil {
return nil, err return nil, err
} }

View file

@ -0,0 +1,48 @@
// Copyright 2024 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT
package pull_test
import (
"testing"
"code.gitea.io/gitea/models/db"
issues_model "code.gitea.io/gitea/models/issues"
"code.gitea.io/gitea/models/unittest"
user_model "code.gitea.io/gitea/models/user"
pull_service "code.gitea.io/gitea/services/pull"
"github.com/stretchr/testify/assert"
)
func TestDismissReview(t *testing.T) {
assert.NoError(t, unittest.PrepareTestDatabase())
pull := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{})
assert.NoError(t, pull.LoadIssue(db.DefaultContext))
issue := pull.Issue
assert.NoError(t, issue.LoadRepo(db.DefaultContext))
reviewer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
review, err := issues_model.CreateReview(db.DefaultContext, issues_model.CreateReviewOptions{
Issue: issue,
Reviewer: reviewer,
Type: issues_model.ReviewTypeReject,
})
assert.NoError(t, err)
issue.IsClosed = true
pull.HasMerged = false
assert.NoError(t, issues_model.UpdateIssueCols(db.DefaultContext, issue, "is_closed"))
assert.NoError(t, pull.UpdateCols(db.DefaultContext, "has_merged"))
_, err = pull_service.DismissReview(db.DefaultContext, review.ID, issue.RepoID, "", &user_model.User{}, false, false)
assert.Error(t, err)
assert.True(t, pull_service.IsErrDismissRequestOnClosedPR(err))
pull.HasMerged = true
pull.Issue.IsClosed = false
assert.NoError(t, issues_model.UpdateIssueCols(db.DefaultContext, issue, "is_closed"))
assert.NoError(t, pull.UpdateCols(db.DefaultContext, "has_merged"))
_, err = pull_service.DismissReview(db.DefaultContext, review.ID, issue.RepoID, "", &user_model.User{}, false, false)
assert.Error(t, err)
assert.True(t, pull_service.IsErrDismissRequestOnClosedPR(err))
}

View file

@ -59,7 +59,7 @@
{{end}} {{end}}
</div> </div>
<div class="tw-flex tw-items-center tw-gap-2"> <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}}"> <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}} {{svg "octicon-x" 20}}
</a> </a>
@ -91,7 +91,7 @@
{{svg "octicon-hourglass" 16}} {{svg "octicon-hourglass" 16}}
</span> </span>
{{end}} {{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> <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}} {{end}}
{{svg (printf "octicon-%s" .Review.Type.Icon) 16 (printf "text %s" (.Review.HTMLTypeColorName))}} {{svg (printf "octicon-%s" .Review.Type.Icon) 16 (printf "text %s" (.Review.HTMLTypeColorName))}}

View file

@ -11422,6 +11422,9 @@
"204": { "204": {
"$ref": "#/responses/empty" "$ref": "#/responses/empty"
}, },
"403": {
"$ref": "#/responses/forbidden"
},
"404": { "404": {
"$ref": "#/responses/notFound" "$ref": "#/responses/notFound"
}, },