diff --git a/models/issues/review.go b/models/issues/review.go index 5370117a81..6ff36615db 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -457,6 +457,11 @@ func SubmitReview(ctx context.Context, doer *user_model.User, issue *Issue, revi if official, err = IsOfficialReviewer(ctx, issue, doer); err != nil { return nil, nil, err } + // delete previous review requests from the same user + reviewCond := builder.Eq{"reviewer_id": doer.ID, "issue_id": issue.ID} + if _, err := sess.Where(reviewCond.And(builder.Eq{"type": ReviewTypeRequest})).Delete(new(Review)); err != nil { + return nil, nil, err + } } review.Official = official @@ -511,10 +516,14 @@ func SubmitReview(ctx context.Context, doer *user_model.User, issue *Issue, revi // GetReviewByIssueIDAndUserID get the latest review of reviewer for a pull request func GetReviewByIssueIDAndUserID(ctx context.Context, issueID, userID int64) (*Review, error) { + return GetReviewByIssueIDUserIDAndTypes(ctx, issueID, userID, []ReviewType{ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest}) +} + +func GetReviewByIssueIDUserIDAndTypes(ctx context.Context, issueID, userID int64, types []ReviewType) (*Review, error) { review := new(Review) has, err := db.GetEngine(ctx).Where( - builder.In("type", ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest). + builder.In("type", types). And(builder.Eq{"issue_id": issueID, "reviewer_id": userID, "original_author_id": 0})). Desc("id"). Get(review) @@ -707,12 +716,12 @@ func RemoveReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user } defer committer.Close() - review, err := GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID) + review, err := GetReviewByIssueIDUserIDAndTypes(ctx, issue.ID, reviewer.ID, []ReviewType{ReviewTypeRequest}) if err != nil && !IsErrReviewNotExist(err) { return nil, err } - if review == nil || review.Type != ReviewTypeRequest { + if review == nil { return nil, nil } diff --git a/models/issues/review_test.go b/models/issues/review_test.go index bdeaae5ea3..e035be617b 100644 --- a/models/issues/review_test.go +++ b/models/issues/review_test.go @@ -321,6 +321,82 @@ func TestAddReviewRequest(t *testing.T) { assert.True(t, issues_model.IsErrReviewRequestOnClosedPR(err)) } +func TestSubmitPendingReviewDeletesReviewRequest(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + pull := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 1}) + require.NoError(t, pull.LoadIssue(db.DefaultContext)) + issue := pull.Issue + require.NoError(t, issue.LoadRepo(db.DefaultContext)) + reviewer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + reviewRequest, err := issues_model.CreateReview(db.DefaultContext, issues_model.CreateReviewOptions{ + Issue: issue, + Reviewer: reviewer, + Type: issues_model.ReviewTypeRequest, + }) + require.NoError(t, err) + + // creating a pending review should NOT remove review requests + reviewPending, err := issues_model.CreateReview(db.DefaultContext, issues_model.CreateReviewOptions{ + Issue: issue, + Reviewer: reviewer, + Type: issues_model.ReviewTypePending, + }) + require.NoError(t, err) + unittest.AssertExistsIf(t, true, &issues_model.Review{ID: reviewRequest.ID}) + // submitting a pending review to finish it SHOULD remove review requests + _, _, err = issues_model.SubmitReview( + db.DefaultContext, + reviewer, + issue, + issues_model.ReviewTypeReject, + "test content", + reviewPending.CommitID, + false, + []string{}, + ) + require.NoError(t, err) + unittest.AssertNotExistsBean(t, &issues_model.Review{ID: reviewRequest.ID}) +} + +// this test is for handling a state correctly that should never exist, but is representable and was +// achievable thanks to #12243 +func TestReviewRequestDeletesReviewRequestsBeforeRejectedReviews(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + sess := db.GetEngine(db.DefaultContext) + + pull := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 1}) + require.NoError(t, pull.LoadIssue(db.DefaultContext)) + issue := pull.Issue + require.NoError(t, issue.LoadRepo(db.DefaultContext)) + reviewer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) + + // this one will end up being a ReviewTypeRequest. We are initially creating it as + // ReviewTypeReject to avoid it being deleted on making the actual rejected review + reviewRequest, err := issues_model.CreateReview(db.DefaultContext, issues_model.CreateReviewOptions{ + Issue: issue, + Reviewer: reviewer, + Type: issues_model.ReviewTypeReject, + }) + require.NoError(t, err) + // this review is an actual rejected review that somehow managed to be saved without deleting + // reviewRequest. This is a state that is representable and is/was achievable thanks to #12243 + _, err = issues_model.CreateReview(db.DefaultContext, issues_model.CreateReviewOptions{ + Issue: issue, + Reviewer: reviewer, + Type: issues_model.ReviewTypeReject, + }) + require.NoError(t, err) + reviewRequest.Type = issues_model.ReviewTypeRequest + _, err = sess.ID(reviewRequest.ID).Cols("type").Update(reviewRequest) + require.NoError(t, err) + + _, err = issues_model.RemoveReviewRequest(db.DefaultContext, issue, reviewer, doer) + require.NoError(t, err) + unittest.AssertNotExistsBean(t, &issues_model.Review{ID: reviewRequest.ID}) +} + func TestAddTeamReviewRequest(t *testing.T) { defer unittest.OverrideFixtures("models/fixtures/TestAddTeamReviewRequest")() require.NoError(t, unittest.PrepareTestDatabase()) diff --git a/release-notes/12302.md b/release-notes/12302.md new file mode 100644 index 0000000000..b51bab427d --- /dev/null +++ b/release-notes/12302.md @@ -0,0 +1 @@ +fix: When a review was created as pending and then submitted, the review request wasn't deleted. These review requests couldn't be removed, as the now existing review shadowed the review request. Now, review requests get deleted when a pending review from that reviewer gets submitted, and broken review requests in already existing data can be normally removed via the UI.