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">

models/issues/review.go

@@ -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
 
@@ -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,

models/issues/review_test.go

@@ -288,3 +288,33 @@ func TestDeleteDismissedReview(t *testing.T) {
 	assert.NoError(t, issues_model.DeleteReview(db.DefaultContext, review))
 	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))
+}

routers/api/v1/repo/pull_review.go

@@ -640,6 +640,8 @@ func DeleteReviewRequests(ctx *context.APIContext) {
 	//     "$ref": "#/responses/empty"
 	//   "422":
 	//     "$ref": "#/responses/validationError"
+	//   "403":
+	//     "$ref": "#/responses/forbidden"
 	//   "404":
 	//     "$ref": "#/responses/notFound"
 	opts := web.GetForm(ctx).(*api.PullReviewRequestOptions)
@@ -708,6 +710,10 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions
 	for _, reviewer := range reviewers {
 		comment, err := issue_service.ReviewRequest(ctx, pr.Issue, ctx.Doer, reviewer, isAdd)
 		if err != nil {
+			if issues_model.IsErrReviewRequestOnClosedPR(err) {
+				ctx.Error(http.StatusForbidden, "", err)
+				return
+			}
 			ctx.Error(http.StatusInternalServerError, "ReviewRequest", err)
 			return
 		}
@@ -874,7 +880,7 @@ func dismissReview(ctx *context.APIContext, msg string, isDismiss, dismissPriors
 		ctx.Error(http.StatusForbidden, "", "Must be repo admin")
 		return
 	}
-	review, pr, isWrong := prepareSingleReview(ctx)
+	review, _, isWrong := prepareSingleReview(ctx)
 	if isWrong {
 		return
 	}
@@ -884,13 +890,12 @@ func dismissReview(ctx *context.APIContext, msg string, isDismiss, dismissPriors
 		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)
 	if err != nil {
+		if pull_service.IsErrDismissRequestOnClosedPR(err) {
+			ctx.Error(http.StatusForbidden, "", err)
+			return
+		}
 		ctx.Error(http.StatusInternalServerError, "pull_service.DismissReview", err)
 		return
 	}

routers/web/repo/issue.go

@@ -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
 		}

routers/web/repo/pull_review.go

@@ -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
 	}

services/pull/review.go

@@ -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 {
@@ -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
 	}

services/pull/review_test.go

@@ -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))
+}

templates/repo/issue/view_content/sidebar.tmpl

@@ -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>
@@ -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))}}