From cf0f77dcf3fa6e4bc041bba7a0cac25aa7688760 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Wed, 20 Dec 2023 12:28:25 +0100 Subject: [PATCH 1/7] Clean up old reviews on creating a new one --- models/issues/review.go | 40 ++++++++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/models/issues/review.go b/models/issues/review.go index 3db73a09ebcb7..c28a2d1078364 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -284,8 +284,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, @@ -295,15 +301,37 @@ func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error CommitID: opts.CommitID, Stale: opts.Stale, } - if opts.Reviewer != nil { + + switch { + case 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 _, 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 + 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 + } + } + + case opts.ReviewerTeam != nil: + review.Type = ReviewTypeRequest review.ReviewerTeamID = opts.ReviewerTeam.ID + + default: + 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 From e08e30685ba2cadfaf777884e55379ac6360e170 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Thu, 21 Dec 2023 09:58:03 +0100 Subject: [PATCH 2/7] else if --- models/issues/review.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/models/issues/review.go b/models/issues/review.go index b1962925d544b..6b6dec7323d6c 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -302,8 +302,7 @@ func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error Stale: opts.Stale, } - switch { - case opts.Reviewer != nil: + if opts.Reviewer != nil { review.Type = opts.Type review.ReviewerID = opts.Reviewer.ID @@ -320,11 +319,11 @@ func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error } } - case opts.ReviewerTeam != nil: + } else if opts.ReviewerTeam != nil { review.Type = ReviewTypeRequest review.ReviewerTeamID = opts.ReviewerTeam.ID - default: + } else { return nil, fmt.Errorf("provide either reviewer or reviewer team") } From 8a06ccb37dfbe8104e9a555899d8edd539739814 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sat, 23 Dec 2023 14:09:36 +0100 Subject: [PATCH 3/7] handle ReviewTypePending --- models/issues/review.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/models/issues/review.go b/models/issues/review.go index 6b6dec7323d6c..7ec39e59fc58d 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -308,8 +308,10 @@ func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error reviewCond := builder.Eq{"reviewer_id": opts.Reviewer.ID, "issue_id": opts.Issue.ID} // make sure user review requests are cleared - if _, err := sess.Where(reviewCond.And(builder.Eq{"type": ReviewTypeRequest})).Delete(new(Review)); err != nil { - return nil, err + if ots.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 if opts.Type == ReviewTypeApprove || opts.Type == ReviewTypeReject { From 9fcea4ca0a3df53e3a89e8ce1efc1922df927fae Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sat, 23 Dec 2023 14:14:18 +0100 Subject: [PATCH 4/7] Update models/issues/review.go --- models/issues/review.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/issues/review.go b/models/issues/review.go index 7ec39e59fc58d..f66f305fdc7f0 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -308,7 +308,7 @@ func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error reviewCond := builder.Eq{"reviewer_id": opts.Reviewer.ID, "issue_id": opts.Issue.ID} // make sure user review requests are cleared - if ots.Type != ReviewTypePending { + if opts.Type != ReviewTypePending { if _, err := sess.Where(reviewCond.And(builder.Eq{"type": ReviewTypeRequest})).Delete(new(Review)); err != nil { return nil, err } From d4049d9c3c7eccb2a0e7b5786d6916e10b4dbfdf Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Thu, 15 Feb 2024 18:15:34 +0100 Subject: [PATCH 5/7] return result for unittest.AssertCount* helper func --- models/unittest/unit_tests.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/models/unittest/unit_tests.go b/models/unittest/unit_tests.go index d47bceea1ea13..75898436fc52f 100644 --- 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) } From 2db7e7c02a3f0b74db52932a2c54a00915250a42 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Thu, 15 Feb 2024 19:32:08 +0100 Subject: [PATCH 6/7] Add TestAPIPullReviewStayDismissed test --- tests/integration/api_pull_review_test.go | 116 ++++++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/tests/integration/api_pull_review_test.go b/tests/integration/api_pull_review_test.go index daa136b21e45c..634e43e24ca37 100644 --- 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,116 @@ 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 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"}, + }).AddTokenAuth(token2) + MakeRequest(t, req, http.StatusCreated) + + reviewsCountCheck(t, + "check we have only one review request", + 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"}, + }).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) + }) +} From b3d062d3a0aa1bfaae292aebae6970ce0831cbd0 Mon Sep 17 00:00:00 2001 From: "m.huber" Date: Mon, 19 Feb 2024 13:55:07 +0100 Subject: [PATCH 7/7] test against rerequest and add some codecomments --- models/issues/review.go | 1 + tests/integration/api_pull_review_test.go | 16 +++++++++++++--- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/models/issues/review.go b/models/issues/review.go index 108efc21b95a6..fc110630e0fa8 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -322,6 +322,7 @@ func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error } } // 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 { diff --git a/tests/integration/api_pull_review_test.go b/tests/integration/api_pull_review_test.go index 634e43e24ca37..ab6d33cd5bee9 100644 --- a/tests/integration/api_pull_review_test.go +++ b/tests/integration/api_pull_review_test.go @@ -332,9 +332,9 @@ func TestAPIPullReviewStayDismissed(t *testing.T) { session8 := loginUser(t, user8.LoginName) token8 := getTokenForLoggedInUser(t, session8, auth_model.AccessTokenScopeWriteRepository) - // user2 request user8 again + // 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"}, + Reviewers: []string{user8.LoginName}, }).AddTokenAuth(token2) MakeRequest(t, req, http.StatusCreated) @@ -342,6 +342,16 @@ func TestAPIPullReviewStayDismissed(t *testing.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", @@ -360,7 +370,7 @@ func TestAPIPullReviewStayDismissed(t *testing.T) { // 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"}, + Reviewers: []string{user8.LoginName}, }).AddTokenAuth(token2) MakeRequest(t, req, http.StatusCreated)