fix: Prevent unremovable review requests after submitting pending reviews (#12302)

Some notes:
- I didn't write integration tests because it's a pure bugfix that addresses implementation details of the model layer.
  - I can see interpretations of "it involves interactions with a live Forgejo server" that would cover this PR, but they don't make sense to me in context.
- I didn't add anything to the documentation because it's a pure bugfix - the system should always have worked this way
  - there's no value in confusing people trying to figure out how the system works now with how it didn't work in the past
- However, there IS value in informing people who may have gotten bitten by this in the past, so I think a release note makes sense
- These fixes are closely related, and the changes small, so I decided to make just one PR.
  - From a user perspective, this is just one issue, and I think in terms of release notes, it makes more sense to have just this one.
- Technically, fixing only one of the underlying issues would be enough. Since this is a case of invalid states being representable, it makes sense to both try to prevent it happening in the first place, and deal with it gracefully if it does happen.
  - At the very least, fixing #12245 is required unless we want to live with data generated in the past being broken

Fixes #12243
Fixes #12245

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12302
Reviewed-by: limiting-factor <limiting-factor@noreply.codeberg.org>

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
 	}
 

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())

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.