From 65c2595f266060a2efc8ec4541c48ea61da2036e Mon Sep 17 00:00:00 2001 From: Caesar Schinas Date: Fri, 9 Aug 2024 16:32:44 +0100 Subject: [PATCH 1/2] Revert "Prevent allow/reject reviews on merged/closed PRs" This reverts commit 4ed372af13a10e5a45464ac38f16c634707267c4. This change from Gitea was not considered by the Forgejo UI team and there is a consensus that it feels like a regression. The test which was added in that commit is kept and modified to test that reviews can successfully be submitted on closed and merged PRs. Closes forgejo/design#11 --- routers/api/v1/repo/pull_review.go | 13 ++-------- routers/web/repo/pull_review.go | 2 -- services/pull/review.go | 8 ------ templates/repo/diff/new_review.tmpl | 28 +++++++++------------ tests/integration/pull_review_test.go | 35 ++++++++++++++++++++++++--- 5 files changed, 45 insertions(+), 41 deletions(-) diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index 6799e43c73..39e1d487fa 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/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) - } + 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) - } + ctx.Error(http.StatusInternalServerError, "SubmitReview", err) return } diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index 24763668d0..e8a3c48d7f 100644 --- a/routers/web/repo/pull_review.go +++ b/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) } diff --git a/services/pull/review.go b/services/pull/review.go index 7a7f140602..011c2a3058 100644 --- a/services/pull/review.go +++ b/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 diff --git a/templates/repo/diff/new_review.tmpl b/templates/repo/diff/new_review.tmpl index 1b74a230f4..a2eae007a5 100644 --- a/templates/repo/diff/new_review.tmpl +++ b/templates/repo/diff/new_review.tmpl @@ -30,24 +30,20 @@ {{end}}
{{$showSelfTooltip := (and $.IsSigned ($.Issue.IsPoster $.SignedUser.ID))}} - {{if not $.Issue.IsClosed}} - {{if $showSelfTooltip}} - - - - {{else}} - - {{end}} + {{if $showSelfTooltip}} + + + + {{else}} + {{end}} - {{if not $.Issue.IsClosed}} - {{if $showSelfTooltip}} - - - - {{else}} - - {{end}} + {{if $showSelfTooltip}} + + + + {{else}} + {{end}} diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 3ce1f34844..d15a7bd130 100644 --- a/tests/integration/pull_review_test.go +++ b/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) }) }) } From 24418da690b378552a2e70849ab0451391be77ea Mon Sep 17 00:00:00 2001 From: Caesar Schinas Date: Tue, 13 Aug 2024 22:14:46 +0100 Subject: [PATCH 2/2] add release notes --- release-notes/4907.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 release-notes/4907.md diff --git a/release-notes/4907.md b/release-notes/4907.md new file mode 100644 index 0000000000..7c6cbdd7fc --- /dev/null +++ b/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.