saltyming/forgejo
1
0
Fork 0
mirror of https://codeberg.org/forgejo/forgejo.git synced 2025-03-21 21:10:14 +00:00
Code Activity

Revert "Prevent allow/reject reviews on merged/closed PRs"

Browse source 
This reverts commit 4ed372af13.
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
This commit is contained in:
Caesar Schinas 2024-08-09 16:32:44 +01:00
parent 8359b26d3f
commit 65c2595f26
No known key found for this signature in database
GPG key ID: AE9108461BEA5ACF
5 changed files with 45 additions and 41 deletions
routers
api/v1/repo
pull_review.go
web/repo
pull_review.go
services/pull
review.go
templates/repo/diff
new_review.tmpl
tests/integration
pull_review_test.go

13
routers/api/v1/repo/pull_review.go
View file

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

2
routers/web/repo/pull_review.go
View file

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

8
services/pull/review.go
View file

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

28
templates/repo/diff/new_review.tmpl
View file

@ -30,24 +30,20 @@
{{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>
</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}}
{{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}}
<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>
</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}}
{{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}}
</form>
</div>

35
tests/integration/pull_review_test.go
View file

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