Merge pull request 'Revert "Prevent allow/reject reviews on merged/closed PRs"' (#4907) from caesar/forgejo:revert-no-closed-pr-review into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/4907
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Reviewed-by: Otto <otto@codeberg.org>

release-notes/4907.md

@@ -0,0 +1 @@
+Reverted a change from Gitea which prevented allow/reject reviews on merged or closed PRs. This change was not considered by the Forgejo UI team and there is a consensus that it feels like a regression, since it interferes with workflows known to be used by Forgejo users without providing a tangible benefit.

routers/api/v1/repo/pull_review.go

@@ -4,7 +4,6 @@
 package repo
 
 import (
-	"errors"
 	"fmt"
 	"net/http"
 	"strings"
@@ -520,11 +519,7 @@ 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 {
-		if errors.Is(err, pull_service.ErrSubmitReviewOnClosedPR) {
-			ctx.Error(http.StatusUnprocessableEntity, "", err)
-		} else {
 		ctx.Error(http.StatusInternalServerError, "SubmitReview", err)
-		}
 		return
 	}
 
@@ -612,11 +607,7 @@ 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 {
-		if errors.Is(err, pull_service.ErrSubmitReviewOnClosedPR) {
-			ctx.Error(http.StatusUnprocessableEntity, "", err)
-		} else {
 		ctx.Error(http.StatusInternalServerError, "SubmitReview", err)
-		}
 		return
 	}
 

routers/web/repo/pull_review.go

@@ -248,8 +248,6 @@ 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 errors.Is(err, pull_service.ErrSubmitReviewOnClosedPR) {
-			ctx.Status(http.StatusUnprocessableEntity)
 		} else {
 			ctx.ServerError("SubmitReview", err)
 		}

services/pull/review.go

@@ -6,7 +6,6 @@ package pull
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"io"
 	"regexp"
@@ -44,9 +43,6 @@ 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.
-var ErrSubmitReviewOnClosedPR = errors.New("can't submit review for a closed or merged PR")
-
 // 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, repo *git.Repository, branch string) error {
@@ -297,10 +293,6 @@ 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 {
-			return nil, nil, ErrSubmitReviewOnClosedPR
-		}
-
 		headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName())
 		if err != nil {
 			return nil, nil, err

templates/repo/diff/new_review.tmpl

@@ -30,7 +30,6 @@
 				{{end}}
 				<div class="divider"></div>
 				{{$showSelfTooltip := (and $.IsSigned ($.Issue.IsPoster $.SignedUser.ID))}}
-				{{if not $.Issue.IsClosed}}
 				{{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>
@@ -38,9 +37,7 @@
 				{{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 not $.Issue.IsClosed}}
 				{{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>
@@ -48,7 +45,6 @@
 				{{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>
 	</div>

tests/integration/pull_review_test.go

@@ -19,6 +19,7 @@ import (
 	"code.gitea.io/gitea/models/unittest"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/git"
+	"code.gitea.io/gitea/modules/gitrepo"
 	"code.gitea.io/gitea/modules/test"
 	issue_service "code.gitea.io/gitea/services/issue"
 	repo_service "code.gitea.io/gitea/services/repository"
@@ -412,6 +413,12 @@ func TestPullView_GivenApproveOrRejectReviewOnClosedPR(t *testing.T) {
 		// Have user1 create a fork of repo1.
 		testRepoFork(t, user1Session, "user2", "repo1", "user1", "repo1")
 
+		baseRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user2", Name: "repo1"})
+		forkedRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: "repo1"})
+		baseGitRepo, err := gitrepo.OpenRepository(db.DefaultContext, baseRepo)
+		require.NoError(t, err)
+		defer baseGitRepo.Close()
+
 		t.Run("Submit approve/reject review on merged PR", func(t *testing.T) {
 			// Create a merged PR (made by user1) in the upstream repo1.
 			testEditFile(t, user1Session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n")
@@ -420,16 +427,26 @@ func TestPullView_GivenApproveOrRejectReviewOnClosedPR(t *testing.T) {
 			assert.EqualValues(t, "pulls", elem[3])
 			testPullMerge(t, user1Session, elem[1], elem[2], elem[4], repo_model.MergeStyleMerge, false)
 
+			// Get the commit SHA
+			pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{
+				BaseRepoID: baseRepo.ID,
+				BaseBranch: "master",
+				HeadRepoID: forkedRepo.ID,
+				HeadBranch: "master",
+			})
+			sha, err := baseGitRepo.GetRefCommitID(pr.GetGitRefName())
+			require.NoError(t, err)
+
 			// Grab the CSRF token.
 			req := NewRequest(t, "GET", path.Join(elem[1], elem[2], "pulls", elem[4]))
 			resp = user2Session.MakeRequest(t, req, http.StatusOK)
 			htmlDoc := NewHTMLParser(t, resp.Body)
 
 			// Submit an approve review on the PR.
-			testSubmitReview(t, user2Session, htmlDoc.GetCSRF(), "user2", "repo1", elem[4], "", "approve", http.StatusUnprocessableEntity)
+			testSubmitReview(t, user2Session, htmlDoc.GetCSRF(), "user2", "repo1", elem[4], sha, "approve", http.StatusOK)
 
 			// Submit a reject review on the PR.
-			testSubmitReview(t, user2Session, htmlDoc.GetCSRF(), "user2", "repo1", elem[4], "", "reject", http.StatusUnprocessableEntity)
+			testSubmitReview(t, user2Session, htmlDoc.GetCSRF(), "user2", "repo1", elem[4], sha, "reject", http.StatusOK)
 		})
 
 		t.Run("Submit approve/reject review on closed PR", func(t *testing.T) {
@@ -440,16 +457,26 @@ func TestPullView_GivenApproveOrRejectReviewOnClosedPR(t *testing.T) {
 			assert.EqualValues(t, "pulls", elem[3])
 			testIssueClose(t, user1Session, elem[1], elem[2], elem[4])
 
+			// Get the commit SHA
+			pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{
+				BaseRepoID: baseRepo.ID,
+				BaseBranch: "master",
+				HeadRepoID: forkedRepo.ID,
+				HeadBranch: "a-test-branch",
+			})
+			sha, err := baseGitRepo.GetRefCommitID(pr.GetGitRefName())
+			require.NoError(t, err)
+
 			// Grab the CSRF token.
 			req := NewRequest(t, "GET", path.Join(elem[1], elem[2], "pulls", elem[4]))
 			resp = user2Session.MakeRequest(t, req, http.StatusOK)
 			htmlDoc := NewHTMLParser(t, resp.Body)
 
 			// Submit an approve review on the PR.
-			testSubmitReview(t, user2Session, htmlDoc.GetCSRF(), "user2", "repo1", elem[4], "", "approve", http.StatusUnprocessableEntity)
+			testSubmitReview(t, user2Session, htmlDoc.GetCSRF(), "user2", "repo1", elem[4], sha, "approve", http.StatusOK)
 
 			// Submit a reject review on the PR.
-			testSubmitReview(t, user2Session, htmlDoc.GetCSRF(), "user2", "repo1", elem[4], "", "reject", http.StatusUnprocessableEntity)
+			testSubmitReview(t, user2Session, htmlDoc.GetCSRF(), "user2", "repo1", elem[4], sha, "reject", http.StatusOK)
 		})
 	})
 }