Resolves #29965. --- Manually tested this by: - Following the [installation](https://docs.gitea.com/next/installation/install-with-docker#basics) guide (but built a local Docker image instead) - Creating 2 users, one who is the `Owner` of a newly-created repository and the other a `Collaborator` - Had the `Collaborator` create a PR that the `Owner` reviews - `Collaborator` resolves conversation and `Owner` merges PR And with this change we see that we can no longer see re-request review button for the `Owner`: <img width="1351" alt="Screenshot 2024-03-25 at 12 39 18 AM" src="https://github.com/go-gitea/gitea/assets/60799661/bcd9c579-3cf7-474f-a51e-b436fe1a39a4">tags/v1.22.0-rc0
return util.ErrInvalidArgument | return util.ErrInvalidArgument | ||||
} | } | ||||
// ErrReviewRequestOnClosedPR represents an error when an user tries to request a re-review on a closed or merged PR. | |||||
type ErrReviewRequestOnClosedPR struct{} | |||||
// IsErrReviewRequestOnClosedPR checks if an error is an ErrReviewRequestOnClosedPR. | |||||
func IsErrReviewRequestOnClosedPR(err error) bool { | |||||
_, ok := err.(ErrReviewRequestOnClosedPR) | |||||
return ok | |||||
} | |||||
func (err ErrReviewRequestOnClosedPR) Error() string { | |||||
return "cannot request a re-review on a closed or merged PR" | |||||
} | |||||
func (err ErrReviewRequestOnClosedPR) Unwrap() error { | |||||
return util.ErrPermissionDenied | |||||
} | |||||
// ReviewType defines the sort of feedback a review gives | // ReviewType defines the sort of feedback a review gives | ||||
type ReviewType int | type ReviewType int | ||||
return nil, err | return nil, err | ||||
} | } | ||||
// skip it when reviewer hase been request to review | |||||
if review != nil && review.Type == ReviewTypeRequest { | |||||
return nil, committer.Commit() // still commit the transaction, or committer.Close() will rollback it, even if it's a reused transaction. | |||||
if review != nil { | |||||
// skip it when reviewer hase been request to review | |||||
if review.Type == ReviewTypeRequest { | |||||
return nil, committer.Commit() // still commit the transaction, or committer.Close() will rollback it, even if it's a reused transaction. | |||||
} | |||||
if issue.IsClosed { | |||||
return nil, ErrReviewRequestOnClosedPR{} | |||||
} | |||||
if issue.IsPull { | |||||
if err := issue.LoadPullRequest(ctx); err != nil { | |||||
return nil, err | |||||
} | |||||
if issue.PullRequest.HasMerged { | |||||
return nil, ErrReviewRequestOnClosedPR{} | |||||
} | |||||
} | |||||
} | } | ||||
// if the reviewer is an official reviewer, | // if the reviewer is an official reviewer, |
assert.NoError(t, issues_model.DeleteReview(db.DefaultContext, review)) | assert.NoError(t, issues_model.DeleteReview(db.DefaultContext, review)) | ||||
unittest.AssertNotExistsBean(t, &issues_model.Comment{ID: comment.ID}) | unittest.AssertNotExistsBean(t, &issues_model.Comment{ID: comment.ID}) | ||||
} | } | ||||
func TestAddReviewRequest(t *testing.T) { | |||||
assert.NoError(t, unittest.PrepareTestDatabase()) | |||||
pull := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 1}) | |||||
assert.NoError(t, pull.LoadIssue(db.DefaultContext)) | |||||
issue := pull.Issue | |||||
assert.NoError(t, issue.LoadRepo(db.DefaultContext)) | |||||
reviewer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) | |||||
_, err := issues_model.CreateReview(db.DefaultContext, issues_model.CreateReviewOptions{ | |||||
Issue: issue, | |||||
Reviewer: reviewer, | |||||
Type: issues_model.ReviewTypeReject, | |||||
}) | |||||
assert.NoError(t, err) | |||||
pull.HasMerged = false | |||||
assert.NoError(t, pull.UpdateCols(db.DefaultContext, "has_merged")) | |||||
issue.IsClosed = true | |||||
_, err = issues_model.AddReviewRequest(db.DefaultContext, issue, reviewer, &user_model.User{}) | |||||
assert.Error(t, err) | |||||
assert.True(t, issues_model.IsErrReviewRequestOnClosedPR(err)) | |||||
pull.HasMerged = true | |||||
assert.NoError(t, pull.UpdateCols(db.DefaultContext, "has_merged")) | |||||
issue.IsClosed = false | |||||
_, err = issues_model.AddReviewRequest(db.DefaultContext, issue, reviewer, &user_model.User{}) | |||||
assert.Error(t, err) | |||||
assert.True(t, issues_model.IsErrReviewRequestOnClosedPR(err)) | |||||
} |
// "$ref": "#/responses/empty" | // "$ref": "#/responses/empty" | ||||
// "422": | // "422": | ||||
// "$ref": "#/responses/validationError" | // "$ref": "#/responses/validationError" | ||||
// "403": | |||||
// "$ref": "#/responses/forbidden" | |||||
// "404": | // "404": | ||||
// "$ref": "#/responses/notFound" | // "$ref": "#/responses/notFound" | ||||
opts := web.GetForm(ctx).(*api.PullReviewRequestOptions) | opts := web.GetForm(ctx).(*api.PullReviewRequestOptions) | ||||
for _, reviewer := range reviewers { | for _, reviewer := range reviewers { | ||||
comment, err := issue_service.ReviewRequest(ctx, pr.Issue, ctx.Doer, reviewer, isAdd) | comment, err := issue_service.ReviewRequest(ctx, pr.Issue, ctx.Doer, reviewer, isAdd) | ||||
if err != nil { | if err != nil { | ||||
if issues_model.IsErrReviewRequestOnClosedPR(err) { | |||||
ctx.Error(http.StatusForbidden, "", err) | |||||
return | |||||
} | |||||
ctx.Error(http.StatusInternalServerError, "ReviewRequest", err) | ctx.Error(http.StatusInternalServerError, "ReviewRequest", err) | ||||
return | return | ||||
} | } | ||||
ctx.Error(http.StatusForbidden, "", "Must be repo admin") | ctx.Error(http.StatusForbidden, "", "Must be repo admin") | ||||
return | return | ||||
} | } | ||||
review, pr, isWrong := prepareSingleReview(ctx) | |||||
review, _, isWrong := prepareSingleReview(ctx) | |||||
if isWrong { | if isWrong { | ||||
return | return | ||||
} | } | ||||
return | 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) | _, err := pull_service.DismissReview(ctx, review.ID, ctx.Repo.Repository.ID, msg, ctx.Doer, isDismiss, dismissPriors) | ||||
if err != nil { | if err != nil { | ||||
if pull_service.IsErrDismissRequestOnClosedPR(err) { | |||||
ctx.Error(http.StatusForbidden, "", err) | |||||
return | |||||
} | |||||
ctx.Error(http.StatusInternalServerError, "pull_service.DismissReview", err) | ctx.Error(http.StatusInternalServerError, "pull_service.DismissReview", err) | ||||
return | return | ||||
} | } |
_, err = issue_service.ReviewRequest(ctx, issue, ctx.Doer, reviewer, action == "attach") | _, err = issue_service.ReviewRequest(ctx, issue, ctx.Doer, reviewer, action == "attach") | ||||
if err != nil { | if err != nil { | ||||
if issues_model.IsErrReviewRequestOnClosedPR(err) { | |||||
ctx.Status(http.StatusForbidden) | |||||
return | |||||
} | |||||
ctx.ServerError("ReviewRequest", err) | ctx.ServerError("ReviewRequest", err) | ||||
return | return | ||||
} | } |
form := web.GetForm(ctx).(*forms.DismissReviewForm) | form := web.GetForm(ctx).(*forms.DismissReviewForm) | ||||
comm, err := pull_service.DismissReview(ctx, form.ReviewID, ctx.Repo.Repository.ID, form.Message, ctx.Doer, true, true) | comm, err := pull_service.DismissReview(ctx, form.ReviewID, ctx.Repo.Repository.ID, form.Message, ctx.Doer, true, true) | ||||
if err != nil { | if err != nil { | ||||
if pull_service.IsErrDismissRequestOnClosedPR(err) { | |||||
ctx.Status(http.StatusForbidden) | |||||
return | |||||
} | |||||
ctx.ServerError("pull_service.DismissReview", err) | ctx.ServerError("pull_service.DismissReview", err) | ||||
return | return | ||||
} | } |
"code.gitea.io/gitea/modules/log" | "code.gitea.io/gitea/modules/log" | ||||
"code.gitea.io/gitea/modules/optional" | "code.gitea.io/gitea/modules/optional" | ||||
"code.gitea.io/gitea/modules/setting" | "code.gitea.io/gitea/modules/setting" | ||||
"code.gitea.io/gitea/modules/util" | |||||
notify_service "code.gitea.io/gitea/services/notify" | notify_service "code.gitea.io/gitea/services/notify" | ||||
) | ) | ||||
var notEnoughLines = regexp.MustCompile(`fatal: file .* has only \d+ lines?`) | var notEnoughLines = regexp.MustCompile(`fatal: file .* has only \d+ lines?`) | ||||
// ErrDismissRequestOnClosedPR represents an error when an user tries to dismiss a review associated to a closed or merged PR. | |||||
type ErrDismissRequestOnClosedPR struct{} | |||||
// IsErrDismissRequestOnClosedPR checks if an error is an ErrDismissRequestOnClosedPR. | |||||
func IsErrDismissRequestOnClosedPR(err error) bool { | |||||
_, ok := err.(ErrDismissRequestOnClosedPR) | |||||
return ok | |||||
} | |||||
func (err ErrDismissRequestOnClosedPR) Error() string { | |||||
return "can't dismiss a review associated to a closed or merged PR" | |||||
} | |||||
func (err ErrDismissRequestOnClosedPR) Unwrap() error { | |||||
return util.ErrPermissionDenied | |||||
} | |||||
// checkInvalidation checks if the line of code comment got changed by another commit. | // 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. | // 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 { | func checkInvalidation(ctx context.Context, c *issues_model.Comment, doer *user_model.User, repo *git.Repository, branch string) error { | ||||
return nil, fmt.Errorf("reviews's repository is not the same as the one we expect") | return nil, fmt.Errorf("reviews's repository is not the same as the one we expect") | ||||
} | } | ||||
issue := review.Issue | |||||
if issue.IsClosed { | |||||
return nil, ErrDismissRequestOnClosedPR{} | |||||
} | |||||
if issue.IsPull { | |||||
if err := issue.LoadPullRequest(ctx); err != nil { | |||||
return nil, err | |||||
} | |||||
if issue.PullRequest.HasMerged { | |||||
return nil, ErrDismissRequestOnClosedPR{} | |||||
} | |||||
} | |||||
if err := issues_model.DismissReview(ctx, review, isDismiss); err != nil { | if err := issues_model.DismissReview(ctx, review, isDismiss); err != nil { | ||||
return nil, err | return nil, err | ||||
} | } |
// Copyright 2024 The Gitea Authors. All rights reserved. | |||||
// SPDX-License-Identifier: MIT | |||||
package pull_test | |||||
import ( | |||||
"testing" | |||||
"code.gitea.io/gitea/models/db" | |||||
issues_model "code.gitea.io/gitea/models/issues" | |||||
"code.gitea.io/gitea/models/unittest" | |||||
user_model "code.gitea.io/gitea/models/user" | |||||
pull_service "code.gitea.io/gitea/services/pull" | |||||
"github.com/stretchr/testify/assert" | |||||
) | |||||
func TestDismissReview(t *testing.T) { | |||||
assert.NoError(t, unittest.PrepareTestDatabase()) | |||||
pull := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{}) | |||||
assert.NoError(t, pull.LoadIssue(db.DefaultContext)) | |||||
issue := pull.Issue | |||||
assert.NoError(t, issue.LoadRepo(db.DefaultContext)) | |||||
reviewer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) | |||||
review, err := issues_model.CreateReview(db.DefaultContext, issues_model.CreateReviewOptions{ | |||||
Issue: issue, | |||||
Reviewer: reviewer, | |||||
Type: issues_model.ReviewTypeReject, | |||||
}) | |||||
assert.NoError(t, err) | |||||
issue.IsClosed = true | |||||
pull.HasMerged = false | |||||
assert.NoError(t, issues_model.UpdateIssueCols(db.DefaultContext, issue, "is_closed")) | |||||
assert.NoError(t, pull.UpdateCols(db.DefaultContext, "has_merged")) | |||||
_, err = pull_service.DismissReview(db.DefaultContext, review.ID, issue.RepoID, "", &user_model.User{}, false, false) | |||||
assert.Error(t, err) | |||||
assert.True(t, pull_service.IsErrDismissRequestOnClosedPR(err)) | |||||
pull.HasMerged = true | |||||
pull.Issue.IsClosed = false | |||||
assert.NoError(t, issues_model.UpdateIssueCols(db.DefaultContext, issue, "is_closed")) | |||||
assert.NoError(t, pull.UpdateCols(db.DefaultContext, "has_merged")) | |||||
_, err = pull_service.DismissReview(db.DefaultContext, review.ID, issue.RepoID, "", &user_model.User{}, false, false) | |||||
assert.Error(t, err) | |||||
assert.True(t, pull_service.IsErrDismissRequestOnClosedPR(err)) | |||||
} |
{{end}} | {{end}} | ||||
</div> | </div> | ||||
<div class="tw-flex tw-items-center tw-gap-2"> | <div class="tw-flex tw-items-center tw-gap-2"> | ||||
{{if (and $.Permission.IsAdmin (or (eq .Review.Type 1) (eq .Review.Type 3)) (not $.Issue.IsClosed))}} | |||||
{{if (and $.Permission.IsAdmin (or (eq .Review.Type 1) (eq .Review.Type 3)) (not $.Issue.IsClosed) (not $.Issue.PullRequest.HasMerged))}} | |||||
<a href="#" class="ui muted icon tw-flex tw-items-center show-modal" data-tooltip-content="{{ctx.Locale.Tr "repo.issues.dismiss_review"}}" data-modal="#dismiss-review-modal-{{.Review.ID}}"> | <a href="#" class="ui muted icon tw-flex tw-items-center show-modal" data-tooltip-content="{{ctx.Locale.Tr "repo.issues.dismiss_review"}}" data-modal="#dismiss-review-modal-{{.Review.ID}}"> | ||||
{{svg "octicon-x" 20}} | {{svg "octicon-x" 20}} | ||||
</a> | </a> | ||||
{{svg "octicon-hourglass" 16}} | {{svg "octicon-hourglass" 16}} | ||||
</span> | </span> | ||||
{{end}} | {{end}} | ||||
{{if .CanChange}} | |||||
{{if and .CanChange (or .Checked (and (not $.Issue.IsClosed) (not $.Issue.PullRequest.HasMerged)))}} | |||||
<a href="#" class="ui muted icon re-request-review{{if .Checked}} checked{{end}}" data-tooltip-content="{{if .Checked}}{{ctx.Locale.Tr "repo.issues.remove_request_review"}}{{else}}{{ctx.Locale.Tr "repo.issues.re_request_review"}}{{end}}" data-issue-id="{{$.Issue.ID}}" data-id="{{.ItemID}}" data-update-url="{{$.RepoLink}}/issues/request_review">{{if .Checked}}{{svg "octicon-trash"}}{{else}}{{svg "octicon-sync"}}{{end}}</a> | <a href="#" class="ui muted icon re-request-review{{if .Checked}} checked{{end}}" data-tooltip-content="{{if .Checked}}{{ctx.Locale.Tr "repo.issues.remove_request_review"}}{{else}}{{ctx.Locale.Tr "repo.issues.re_request_review"}}{{end}}" data-issue-id="{{$.Issue.ID}}" data-id="{{.ItemID}}" data-update-url="{{$.RepoLink}}/issues/request_review">{{if .Checked}}{{svg "octicon-trash"}}{{else}}{{svg "octicon-sync"}}{{end}}</a> | ||||
{{end}} | {{end}} | ||||
{{svg (printf "octicon-%s" .Review.Type.Icon) 16 (printf "text %s" (.Review.HTMLTypeColorName))}} | {{svg (printf "octicon-%s" .Review.Type.Icon) 16 (printf "text %s" (.Review.HTMLTypeColorName))}} |
"204": { | "204": { | ||||
"$ref": "#/responses/empty" | "$ref": "#/responses/empty" | ||||
}, | }, | ||||
"403": { | |||||
"$ref": "#/responses/forbidden" | |||||
}, | |||||
"404": { | "404": { | ||||
"$ref": "#/responses/notFound" | "$ref": "#/responses/notFound" | ||||
}, | }, |