diff --git a/models/fixtures/review.yml b/models/fixtures/review.yml index 5b8bbceca9edf..74111fa0d657e 100644 --- a/models/fixtures/review.yml +++ b/models/fixtures/review.yml @@ -207,6 +207,17 @@ - id: 22 + type: 1 + reviewer_id: 5 + issue_id: 3 + content: "review accepted of user5 but dismissed" + original_author_id: 0 + updated_unix: 946684000 + created_unix: 946684000 + dismissed: true + +- + id: 23 type: 4 reviewer_id: 5 issue_id: 3 @@ -214,3 +225,26 @@ original_author_id: 0 updated_unix: 946684817 created_unix: 946684817 + +- + id: 24 + type: 3 + reviewer_id: 5 + issue_id: 3 + content: "review rejected of user5 but dismissed" + original_author_id: 0 + updated_unix: 946684827 + created_unix: 946684827 + dismissed: true + +- + id: 25 + type: 3 + reviewer_id: 15 + reviewer_team_id: 0 + issue_id: 3 + content: "review rejected by user15 but dismissed" + original_author_id: 0 + updated_unix: 946684899 + created_unix: 946684899 + dismissed: true diff --git a/models/issues/pull_list_test.go b/models/issues/pull_list_test.go index 437830701c671..072bbda1e07a7 100644 --- a/models/issues/pull_list_test.go +++ b/models/issues/pull_list_test.go @@ -52,12 +52,12 @@ func TestPullRequestList_LoadReviews(t *testing.T) { } reviewList, err := prs.LoadReviews(t.Context()) assert.NoError(t, err) - // 1, 7, 8, 9, 10, 22 + // 1, 7, 8, 9, 10, 23 assert.Len(t, reviewList, 6) assert.EqualValues(t, 1, reviewList[0].ID) assert.EqualValues(t, 7, reviewList[1].ID) assert.EqualValues(t, 8, reviewList[2].ID) assert.EqualValues(t, 9, reviewList[3].ID) assert.EqualValues(t, 10, reviewList[4].ID) - assert.EqualValues(t, 22, reviewList[5].ID) + assert.EqualValues(t, 23, reviewList[5].ID) } diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index 7089af253b7a4..d154c9dd7f1b7 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -12,6 +12,7 @@ import ( "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/test" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -344,10 +345,25 @@ func TestGetApprovers(t *testing.T) { pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 5}) // Official reviews are already deduplicated. Allow unofficial reviews // to assert that there are no duplicated approvers. - setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly = false + defer test.MockVariableValue(&setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly, false)() approvers := pr.GetApprovers(t.Context()) expected := "Reviewed-by: User Five \nReviewed-by: Org Six \n" assert.Equal(t, expected, approvers) + + // comment-type and pending reviews should be ignored + pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) + assert.EqualValues(t, 3, pr.IssueID) + approvers = pr.GetApprovers(t.Context()) + expected = "Reviewed-by: User Five \nReviewed-by: user4 \n" + assert.Equal(t, expected, approvers) + + // un-official reviews should now be ignored too + pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) + assert.EqualValues(t, 3, pr.IssueID) + setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly = true + approvers = pr.GetApprovers(t.Context()) + expected = "" + assert.Equal(t, expected, approvers) } func TestGetPullRequestByMergedCommit(t *testing.T) { diff --git a/models/issues/review.go b/models/issues/review.go index b758fa5ffac63..f14e22394b70d 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -17,6 +17,7 @@ import ( access_model "code.gitea.io/gitea/models/perm/access" "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/util" @@ -390,6 +391,7 @@ func GetCurrentReview(ctx context.Context, reviewer *user_model.User, issue *Iss Types: []ReviewType{ReviewTypePending}, IssueID: issue.ID, ReviewerID: reviewer.ID, + Dismissed: optional.Some(false), }) if err != nil { return nil, err @@ -532,12 +534,18 @@ 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) { +func GetReviewByIssueIDAndUserID(ctx context.Context, issueID, userID int64, dismissed optional.Option[bool]) (*Review, error) { review := new(Review) - has, err := db.GetEngine(ctx).Where( - builder.In("type", ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest). - And(builder.Eq{"issue_id": issueID, "reviewer_id": userID, "original_author_id": 0})). + cond := builder.In("type", ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest). + And(builder.Eq{"issue_id": issueID, "reviewer_id": userID, "original_author_id": 0}) + + // apply optional filter for dismissed + if dismissed.Has() { + cond = cond.And(builder.Eq{"dismissed": dismissed.Value()}) + } + + has, err := db.GetEngine(ctx).Where(cond). Desc("id"). Get(review) if err != nil { @@ -647,7 +655,7 @@ func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_mo return db.WithTx2(ctx, func(ctx context.Context) (*Comment, error) { sess := db.GetEngine(ctx) - review, err := GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID) + review, err := GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID, optional.None[bool]()) if err != nil && !IsErrReviewNotExist(err) { return nil, err } @@ -717,7 +725,7 @@ func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_mo // RemoveReviewRequest remove a review request from one reviewer func RemoveReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_model.User) (*Comment, error) { return db.WithTx2(ctx, func(ctx context.Context) (*Comment, error) { - review, err := GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID) + review, err := GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID, optional.Some(false)) if err != nil && !IsErrReviewNotExist(err) { return nil, err } @@ -752,7 +760,7 @@ func RemoveReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user // Recalculate the latest official review for reviewer func restoreLatestOfficialReview(ctx context.Context, issueID, reviewerID int64) error { - review, err := GetReviewByIssueIDAndUserID(ctx, issueID, reviewerID) + review, err := GetReviewByIssueIDAndUserID(ctx, issueID, reviewerID, optional.None[bool]()) if err != nil && !IsErrReviewNotExist(err) { return err } @@ -844,7 +852,7 @@ func RemoveTeamReviewRequest(ctx context.Context, issue *Issue, reviewer *organi if official { // recalculate which is the latest official review from that team - review, err := GetReviewByIssueIDAndUserID(ctx, issue.ID, -reviewer.ID) + review, err := GetReviewByIssueIDAndUserID(ctx, issue.ID, -reviewer.ID, optional.None[bool]()) if err != nil && !IsErrReviewNotExist(err) { return nil, err } diff --git a/models/issues/review_list.go b/models/issues/review_list.go index bbb8c489fa133..6dbbea0515a29 100644 --- a/models/issues/review_list.go +++ b/models/issues/review_list.go @@ -163,7 +163,7 @@ func GetReviewsByIssueID(ctx context.Context, issueID int64) (latestReviews, mig reviews := make([]*Review, 0, 10) // Get all reviews for the issue id - if err := db.GetEngine(ctx).Where("issue_id=?", issueID).OrderBy("updated_unix ASC").Find(&reviews); err != nil { + if err := db.GetEngine(ctx).Where("issue_id=? AND dismissed=?", issueID, false).OrderBy("updated_unix ASC").Find(&reviews); err != nil { return nil, nil, err } @@ -175,7 +175,7 @@ func GetReviewsByIssueID(ctx context.Context, issueID int64) (latestReviews, mig reviewTeamsMap := make(map[int64][]*Review) // key is reviewer team id countedReivewTypes := []ReviewType{ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest} for _, review := range reviews { - if review.ReviewerTeamID == 0 && slices.Contains(countedReivewTypes, review.Type) && !review.Dismissed { + if review.ReviewerTeamID == 0 && slices.Contains(countedReivewTypes, review.Type) { if review.OriginalAuthorID != 0 { originalReviewersMap[review.OriginalAuthorID] = append(originalReviewersMap[review.OriginalAuthorID], review) } else { diff --git a/services/issue/assignee.go b/services/issue/assignee.go index ba9c91e0edc4a..93a7d1d5eac54 100644 --- a/services/issue/assignee.go +++ b/services/issue/assignee.go @@ -14,6 +14,7 @@ import ( "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/optional" notify_service "code.gitea.io/gitea/services/notify" ) @@ -116,7 +117,7 @@ func isValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User, } } - lastReview, err := issues_model.GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID) + lastReview, err := issues_model.GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID, optional.None[bool]()) if err != nil && !issues_model.IsErrReviewNotExist(err) { return err } diff --git a/services/pull/pull.go b/services/pull/pull.go index 619347055b7d3..0f37f9bb6373a 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -30,6 +30,7 @@ import ( "code.gitea.io/gitea/modules/globallock" "code.gitea.io/gitea/modules/graceful" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/optional" repo_module "code.gitea.io/gitea/modules/repository" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" @@ -1096,6 +1097,7 @@ func GetPullCommits(ctx context.Context, baseGitRepo *git.Repository, doer *user issues_model.ReviewTypeComment, issues_model.ReviewTypeReject, }, + Dismissed: optional.Some(false), }) if err != nil && !issues_model.IsErrReviewNotExist(err) { diff --git a/tests/integration/api_pull_review_test.go b/tests/integration/api_pull_review_test.go index 9ffbd9a2673d4..994cf740e2af0 100644 --- a/tests/integration/api_pull_review_test.go +++ b/tests/integration/api_pull_review_test.go @@ -40,23 +40,28 @@ func TestAPIPullReview(t *testing.T) { var reviews []*api.PullReview DecodeJSON(t, resp, &reviews) - require.Len(t, reviews, 8) + require.Len(t, reviews, 11) for _, r := range reviews { assert.Equal(t, pullIssue.HTMLURL(t.Context()), r.HTMLPullURL) } - assert.EqualValues(t, 8, reviews[3].ID) - assert.EqualValues(t, "APPROVED", reviews[3].State) - assert.Equal(t, 0, reviews[3].CodeCommentsCount) - assert.True(t, reviews[3].Stale) - assert.False(t, reviews[3].Official) - - assert.EqualValues(t, 10, reviews[5].ID) - assert.EqualValues(t, "REQUEST_CHANGES", reviews[5].State) - assert.Equal(t, 1, reviews[5].CodeCommentsCount) - assert.EqualValues(t, -1, reviews[5].Reviewer.ID) // ghost user - assert.False(t, reviews[5].Stale) - assert.True(t, reviews[5].Official) + if assert.EqualValues(t, 8, reviews[4].ID) { + assert.EqualValues(t, "APPROVED", reviews[4].State) + assert.Equal(t, 0, reviews[4].CodeCommentsCount) + assert.True(t, reviews[4].Stale) + assert.False(t, reviews[4].Official) + } + + if assert.EqualValues(t, 10, reviews[6].ID) { + assert.EqualValues(t, "REQUEST_CHANGES", reviews[6].State) + assert.Equal(t, 1, reviews[6].CodeCommentsCount) + assert.EqualValues(t, -1, reviews[6].Reviewer.ID) // ghost user + assert.False(t, reviews[6].Stale) + assert.True(t, reviews[6].Official) + } + if assert.EqualValues(t, 25, reviews[10].ID) { + assert.True(t, reviews[10].Dismissed) + } // test GetPullReview req = NewRequestf(t, http.MethodGet, "/api/v1/repos/%s/%s/pulls/%d/reviews/%d", repo.OwnerName, repo.Name, pullIssue.Index, reviews[3].ID).