diff --git a/models/issues/review.go b/models/issues/review.go index 78ef0d20c23..99366222198 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -483,6 +483,14 @@ func SubmitReview(ctx context.Context, doer *user_model.User, issue *Issue, revi if _, err := sess.ID(review.ID).Cols("content, type, official, commit_id, stale").Update(review); err != nil { return nil, nil, err } + + // make sure the leftover review request is cleared, consistent with CreateReview + if reviewType != ReviewTypePending { + if _, err := sess.Where(builder.Eq{"reviewer_id": doer.ID, "issue_id": issue.ID, "type": ReviewTypeRequest}). + Delete(new(Review)); err != nil { + return nil, nil, err + } + } } comm, err := CreateComment(ctx, &CreateCommentOptions{ diff --git a/models/issues/review_test.go b/models/issues/review_test.go index 092d88d1749..a384dbd30fe 100644 --- a/models/issues/review_test.go +++ b/models/issues/review_test.go @@ -303,6 +303,46 @@ func TestDeleteDismissedReview(t *testing.T) { unittest.AssertNotExistsBean(t, &issues_model.Comment{ID: comment.ID}) } +func TestSubmitReviewClearsStaleReviewRequest(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 3}) + assert.NoError(t, issue.LoadRepo(t.Context())) + assert.NoError(t, issue.Repo.LoadOwner(t.Context())) + reviewer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + // the reviewer is requested to review the pull request + requestReview, err := issues_model.CreateReview(t.Context(), issues_model.CreateReviewOptions{ + Type: issues_model.ReviewTypeRequest, + Issue: issue, + Reviewer: reviewer, + }) + assert.NoError(t, err) + + // the reviewer starts a pending review (e.g. by adding code comments) + pendingReview, err := issues_model.CreateReview(t.Context(), issues_model.CreateReviewOptions{ + Type: issues_model.ReviewTypePending, + Issue: issue, + Reviewer: reviewer, + }) + assert.NoError(t, err) + + // submitting the pending review must clear the leftover review request, + // otherwise the reviewer can no longer be re-requested afterwards + review, _, err := issues_model.SubmitReview(t.Context(), reviewer, issue, issues_model.ReviewTypeComment, "looks good", "", false, nil) + assert.NoError(t, err) + assert.Equal(t, pendingReview.ID, review.ID) + assert.Equal(t, issues_model.ReviewTypeComment, review.Type) + + unittest.AssertNotExistsBean(t, &issues_model.Review{ID: requestReview.ID}) + + // the reviewer can be re-requested afterwards (no-op before the fix) + comment, err := issues_model.AddReviewRequest(t.Context(), issue, reviewer, doer, false) + assert.NoError(t, err) + assert.NotNil(t, comment) +} + func TestAddReviewRequest(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase())