Workaround to clean up old reviews on creating a new one (#28554)

close  #28542

blocks  #28544

---
*Sponsored by Kithara Software GmbH*

diff --git a/models/issues/review.go b/models/issues/review.go
index 3aa9d3e2a86973c67d4a6b41627c1631e59df2de..fc110630e0fa8cb574215df567a311d0d32aa277 100644 (file)
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -292,8 +292,14 @@ func IsOfficialReviewerTeam(ctx context.Context, issue *Issue, team *organizatio
 
 // CreateReview creates a new review based on opts
 func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error) {
+       ctx, committer, err := db.TxContext(ctx)
+       if err != nil {
+               return nil, err
+       }
+       defer committer.Close()
+       sess := db.GetEngine(ctx)
+
        review := &Review{
-               Type:         opts.Type,
                Issue:        opts.Issue,
                IssueID:      opts.Issue.ID,
                Reviewer:     opts.Reviewer,
@@ -303,15 +309,39 @@ func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error
                CommitID:     opts.CommitID,
                Stale:        opts.Stale,
        }
+
        if opts.Reviewer != nil {
+               review.Type = opts.Type
                review.ReviewerID = opts.Reviewer.ID
-       } else {
-               if review.Type != ReviewTypeRequest {
-                       review.Type = ReviewTypeRequest
+
+               reviewCond := builder.Eq{"reviewer_id": opts.Reviewer.ID, "issue_id": opts.Issue.ID}
+               // make sure user review requests are cleared
+               if opts.Type != ReviewTypePending {
+                       if _, err := sess.Where(reviewCond.And(builder.Eq{"type": ReviewTypeRequest})).Delete(new(Review)); err != nil {
+                               return nil, err
+                       }
                }
+               // make sure if the created review gets dismissed no old review surface
+               // other types can be ignored, as they don't affect branch protection
+               if opts.Type == ReviewTypeApprove || opts.Type == ReviewTypeReject {
+                       if _, err := sess.Where(reviewCond.And(builder.In("type", ReviewTypeApprove, ReviewTypeReject))).
+                               Cols("dismissed").Update(&Review{Dismissed: true}); err != nil {
+                               return nil, err
+                       }
+               }
+
+       } else if opts.ReviewerTeam != nil {
+               review.Type = ReviewTypeRequest
                review.ReviewerTeamID = opts.ReviewerTeam.ID
+
+       } else {
+               return nil, fmt.Errorf("provide either reviewer or reviewer team")
+       }
+
+       if _, err := sess.Insert(review); err != nil {
+               return nil, err
        }
-       return review, db.Insert(ctx, review)
+       return review, committer.Commit()
 }
 
 // GetCurrentReview returns the current pending review of reviewer for given issue

diff --git a/models/unittest/unit_tests.go b/models/unittest/unit_tests.go
index d47bceea1ea1358b9ed0083322529a9080b95e18..75898436fc52f9ad46f724261b9787e44bc40a19 100644 (file)
--- a/models/unittest/unit_tests.go
+++ b/models/unittest/unit_tests.go
@@ -131,8 +131,8 @@ func AssertSuccessfulInsert(t assert.TestingT, beans ...any) {
 }
 
 // AssertCount assert the count of a bean
-func AssertCount(t assert.TestingT, bean, expected any) {
-       assert.EqualValues(t, expected, GetCount(t, bean))
+func AssertCount(t assert.TestingT, bean, expected any) bool {
+       return assert.EqualValues(t, expected, GetCount(t, bean))
 }
 
 // AssertInt64InRange assert value is in range [low, high]
@@ -150,7 +150,7 @@ func GetCountByCond(t assert.TestingT, tableName string, cond builder.Cond) int6
 }
 
 // AssertCountByCond test the count of database entries matching bean
-func AssertCountByCond(t assert.TestingT, tableName string, cond builder.Cond, expected int) {
-       assert.EqualValues(t, expected, GetCountByCond(t, tableName, cond),
+func AssertCountByCond(t assert.TestingT, tableName string, cond builder.Cond, expected int) bool {
+       return assert.EqualValues(t, expected, GetCountByCond(t, tableName, cond),
                "Failed consistency test, the counted bean (of table %s) was %+v", tableName, cond)
 }

diff --git a/tests/integration/api_pull_review_test.go b/tests/integration/api_pull_review_test.go
index daa136b21e45cc5e8a9ac85353c3d68d0163997f..ab6d33cd5bee967bd2c75369d5c48725dd4a6f58 100644 (file)
--- a/tests/integration/api_pull_review_test.go
+++ b/tests/integration/api_pull_review_test.go
@@ -13,11 +13,14 @@ import (
        issues_model "code.gitea.io/gitea/models/issues"
        repo_model "code.gitea.io/gitea/models/repo"
        "code.gitea.io/gitea/models/unittest"
+       user_model "code.gitea.io/gitea/models/user"
        "code.gitea.io/gitea/modules/json"
        api "code.gitea.io/gitea/modules/structs"
+       issue_service "code.gitea.io/gitea/services/issue"
        "code.gitea.io/gitea/tests"
 
        "github.com/stretchr/testify/assert"
+       "xorm.io/builder"
 )
 
 func TestAPIPullReview(t *testing.T) {
@@ -314,3 +317,126 @@ func TestAPIPullReviewRequest(t *testing.T) {
                AddTokenAuth(token)
        MakeRequest(t, req, http.StatusNoContent)
 }
+
+func TestAPIPullReviewStayDismissed(t *testing.T) {
+       // This test against issue https://github.com/go-gitea/gitea/issues/28542
+       // where old reviews surface after a review request got dismissed.
+       defer tests.PrepareTestEnv(t)()
+       pullIssue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 3})
+       assert.NoError(t, pullIssue.LoadAttributes(db.DefaultContext))
+       repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: pullIssue.RepoID})
+       user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+       session2 := loginUser(t, user2.LoginName)
+       token2 := getTokenForLoggedInUser(t, session2, auth_model.AccessTokenScopeWriteRepository)
+       user8 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 8})
+       session8 := loginUser(t, user8.LoginName)
+       token8 := getTokenForLoggedInUser(t, session8, auth_model.AccessTokenScopeWriteRepository)
+
+       // user2 request user8
+       req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", repo.OwnerName, repo.Name, pullIssue.Index), &api.PullReviewRequestOptions{
+               Reviewers: []string{user8.LoginName},
+       }).AddTokenAuth(token2)
+       MakeRequest(t, req, http.StatusCreated)
+
+       reviewsCountCheck(t,
+               "check we have only one review request",
+               pullIssue.ID, user8.ID, 0, 1, 1, false)
+
+       // user2 request user8 again, it is expected to be ignored
+       req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", repo.OwnerName, repo.Name, pullIssue.Index), &api.PullReviewRequestOptions{
+               Reviewers: []string{user8.LoginName},
+       }).AddTokenAuth(token2)
+       MakeRequest(t, req, http.StatusCreated)
+
+       reviewsCountCheck(t,
+               "check we have only one review request, even after re-request it again",
+               pullIssue.ID, user8.ID, 0, 1, 1, false)
+
+       // user8 reviews it as accept
+       req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/reviews", repo.OwnerName, repo.Name, pullIssue.Index), &api.CreatePullReviewOptions{
+               Event: "APPROVED",
+               Body:  "lgtm",
+       }).AddTokenAuth(token8)
+       MakeRequest(t, req, http.StatusOK)
+
+       reviewsCountCheck(t,
+               "check we have one valid approval",
+               pullIssue.ID, user8.ID, 0, 0, 1, true)
+
+       // emulate of auto-dismiss lgtm on a protected branch that where a pull just got an update
+       _, err := db.GetEngine(db.DefaultContext).Where("issue_id = ? AND reviewer_id = ?", pullIssue.ID, user8.ID).
+               Cols("dismissed").Update(&issues_model.Review{Dismissed: true})
+       assert.NoError(t, err)
+
+       // user2 request user8 again
+       req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", repo.OwnerName, repo.Name, pullIssue.Index), &api.PullReviewRequestOptions{
+               Reviewers: []string{user8.LoginName},
+       }).AddTokenAuth(token2)
+       MakeRequest(t, req, http.StatusCreated)
+
+       reviewsCountCheck(t,
+               "check we have no valid approval and one review request",
+               pullIssue.ID, user8.ID, 1, 1, 2, false)
+
+       // user8 dismiss review
+       _, err = issue_service.ReviewRequest(db.DefaultContext, pullIssue, user8, user8, false)
+       assert.NoError(t, err)
+
+       reviewsCountCheck(t,
+               "check new review request is now dismissed",
+               pullIssue.ID, user8.ID, 1, 0, 1, false)
+
+       // add a new valid approval
+       req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/reviews", repo.OwnerName, repo.Name, pullIssue.Index), &api.CreatePullReviewOptions{
+               Event: "APPROVED",
+               Body:  "lgtm",
+       }).AddTokenAuth(token8)
+       MakeRequest(t, req, http.StatusOK)
+
+       reviewsCountCheck(t,
+               "check that old reviews requests are deleted",
+               pullIssue.ID, user8.ID, 1, 0, 2, true)
+
+       // now add a change request witch should dismiss the approval
+       req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/reviews", repo.OwnerName, repo.Name, pullIssue.Index), &api.CreatePullReviewOptions{
+               Event: "REQUEST_CHANGES",
+               Body:  "please change XYZ",
+       }).AddTokenAuth(token8)
+       MakeRequest(t, req, http.StatusOK)
+
+       reviewsCountCheck(t,
+               "check that old reviews are dismissed",
+               pullIssue.ID, user8.ID, 2, 0, 3, false)
+}
+
+func reviewsCountCheck(t *testing.T, name string, issueID, reviewerID int64, expectedDismissed, expectedRequested, expectedTotal int, expectApproval bool) {
+       t.Run(name, func(t *testing.T) {
+               unittest.AssertCountByCond(t, "review", builder.Eq{
+                       "issue_id":    issueID,
+                       "reviewer_id": reviewerID,
+                       "dismissed":   true,
+               }, expectedDismissed)
+
+               unittest.AssertCountByCond(t, "review", builder.Eq{
+                       "issue_id":    issueID,
+                       "reviewer_id": reviewerID,
+               }, expectedTotal)
+
+               unittest.AssertCountByCond(t, "review", builder.Eq{
+                       "issue_id":    issueID,
+                       "reviewer_id": reviewerID,
+                       "type":        issues_model.ReviewTypeRequest,
+               }, expectedRequested)
+
+               approvalCount := 0
+               if expectApproval {
+                       approvalCount = 1
+               }
+               unittest.AssertCountByCond(t, "review", builder.Eq{
+                       "issue_id":    issueID,
+                       "reviewer_id": reviewerID,
+                       "type":        issues_model.ReviewTypeApprove,
+                       "dismissed":   false,
+               }, approvalCount)
+       })
+}