Merge f484e3d44e into 68a3e6b5e6

routers/api/v1/repo/pull_review.go

@@ -372,7 +372,11 @@ func CreatePullReview(ctx *context.APIContext) {
 	// create review and associate all pending review comments
 	review, _, err := pull_service.SubmitReview(ctx, ctx.Doer, ctx.Repo.GitRepo, pr.Issue, reviewType, opts.Body, opts.CommitID, nil)
 	if err != nil {
-		ctx.Error(http.StatusInternalServerError, "SubmitReview", err)
+		if pull_service.IsErrSubmitReviewOnClosedPR(err) {
+			ctx.Error(http.StatusUnprocessableEntity, "", err)
+		} else {
+			ctx.Error(http.StatusInternalServerError, "SubmitReview", err)
+		}
 		return
 	}
 
@@ -460,7 +464,11 @@ func SubmitPullReview(ctx *context.APIContext) {
 	// create review and associate all pending review comments
 	review, _, err = pull_service.SubmitReview(ctx, ctx.Doer, ctx.Repo.GitRepo, pr.Issue, reviewType, opts.Body, headCommitID, nil)
 	if err != nil {
-		ctx.Error(http.StatusInternalServerError, "SubmitReview", err)
+		if pull_service.IsErrSubmitReviewOnClosedPR(err) {
+			ctx.Error(http.StatusUnprocessableEntity, "", err)
+		} else {
+			ctx.Error(http.StatusInternalServerError, "SubmitReview", err)
+		}
 		return
 	}
 

routers/web/repo/pull_review.go

@@ -264,6 +264,8 @@ func SubmitReview(ctx *context.Context) {
 		if issues_model.IsContentEmptyErr(err) {
 			ctx.Flash.Error(ctx.Tr("repo.issues.review.content.empty"))
 			ctx.JSONRedirect(fmt.Sprintf("%s/pulls/%d/files", ctx.Repo.RepoLink, issue.Index))
+		} else if pull_service.IsErrSubmitReviewOnClosedPR(err) {
+			ctx.Error(http.StatusUnprocessableEntity)
 		} else {
 			ctx.ServerError("SubmitReview", err)
 		}

services/pull/review.go

@@ -43,6 +43,23 @@ func (err ErrDismissRequestOnClosedPR) Unwrap() error {
 	return util.ErrPermissionDenied
 }
 
+// ErrSubmitReviewOnClosedPR represents an error when an user tries to submit an approve or reject review associated to a closed or merged PR.
+type ErrSubmitReviewOnClosedPR struct{}
+
+// IsErrSubmitReviewOnClosedPR checks if an error is an ErrSubmitReviewOnClosedPR.
+func IsErrSubmitReviewOnClosedPR(err error) bool {
+	_, ok := err.(ErrSubmitReviewOnClosedPR)
+	return ok
+}
+
+func (err ErrSubmitReviewOnClosedPR) Error() string {
+	return "can't submit review for a closed or merged PR"
+}
+
+func (err ErrSubmitReviewOnClosedPR) 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 {
@@ -293,6 +310,10 @@ func SubmitReview(ctx context.Context, doer *user_model.User, gitRepo *git.Repos
 	if reviewType != issues_model.ReviewTypeApprove && reviewType != issues_model.ReviewTypeReject {
 		stale = false
 	} else {
+		if issue.IsClosed || pr.HasMerged {
+			return nil, nil, ErrSubmitReviewOnClosedPR{}
+		}
+
 		headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName())
 		if err != nil {
 			return nil, nil, err

templates/repo/diff/new_review.tmpl

@@ -30,20 +30,25 @@
 				{{end}}
 				<div class="divider"></div>
 				{{$showSelfTooltip := (and $.IsSigned ($.Issue.IsPoster $.SignedUser.ID))}}
-				{{if $showSelfTooltip}}
-					<span class="tw-inline-block" data-tooltip-content="{{ctx.Locale.Tr "repo.diff.review.self_approve"}}">
-						<button type="submit" name="type" value="approve" disabled class="ui submit primary tiny button btn-submit">{{ctx.Locale.Tr "repo.diff.review.approve"}}</button>
-					</span>
-				{{else}}
-					<button type="submit" name="type" value="approve" class="ui submit primary tiny button btn-submit">{{ctx.Locale.Tr "repo.diff.review.approve"}}</button>
+				{{$isOpen := (not (or $.Issue.IsClosed (and $.Issue.IsPull $.Issue.PullRequest.HasMerged)))}}
+				{{if $isOpen}}
+					{{if $showSelfTooltip}}
+						<span class="tw-inline-block" data-tooltip-content="{{ctx.Locale.Tr "repo.diff.review.self_approve"}}">
+							<button type="submit" name="type" value="approve" disabled class="ui submit primary tiny button btn-submit">{{ctx.Locale.Tr "repo.diff.review.approve"}}</button>
+						</span>
+					{{else}}
+						<button type="submit" name="type" value="approve" class="ui submit primary tiny button btn-submit">{{ctx.Locale.Tr "repo.diff.review.approve"}}</button>
+					{{end}}
 				{{end}}
 				<button type="submit" name="type" value="comment" class="ui submit tiny basic button btn-submit">{{ctx.Locale.Tr "repo.diff.review.comment"}}</button>
-				{{if $showSelfTooltip}}
-					<span class="tw-inline-block" data-tooltip-content="{{ctx.Locale.Tr "repo.diff.review.self_reject"}}">
-						<button type="submit" name="type" value="reject" disabled class="ui submit red tiny button btn-submit">{{ctx.Locale.Tr "repo.diff.review.reject"}}</button>
-					</span>
-				{{else}}
-					<button type="submit" name="type" value="reject" class="ui submit red tiny button btn-submit">{{ctx.Locale.Tr "repo.diff.review.reject"}}</button>
+				{{if $isOpen}}
+					{{if $showSelfTooltip}}
+						<span class="tw-inline-block" data-tooltip-content="{{ctx.Locale.Tr "repo.diff.review.self_reject"}}">
+							<button type="submit" name="type" value="reject" disabled class="ui submit red tiny button btn-submit">{{ctx.Locale.Tr "repo.diff.review.reject"}}</button>
+						</span>
+					{{else}}
+						<button type="submit" name="type" value="reject" class="ui submit red tiny button btn-submit">{{ctx.Locale.Tr "repo.diff.review.reject"}}</button>
+					{{end}}
 				{{end}}
 			</form>
 		</div>