Make sure API endpoints handle re-review/dismiss on close/merged PRs

Author: kemzeb

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
 	}