From 823518f8ae073ed9b8760ceb5e8ab6a2ab32da90 Mon Sep 17 00:00:00 2001 From: "m.huber" Date: Wed, 22 Jan 2025 16:18:26 +0100 Subject: [PATCH 01/15] filter dismissed via sql more & make it possible to have review requests falsly stayed in DB deleted --- models/issues/pull.go | 2 ++ models/issues/review.go | 18 +++++++++++++----- models/issues/review_list.go | 4 ++-- services/pull/pull.go | 2 ++ 4 files changed, 19 insertions(+), 7 deletions(-) diff --git a/models/issues/pull.go b/models/issues/pull.go index e3af00224dedb..0f3ddeaac3a7c 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -20,6 +20,7 @@ import ( user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/util" @@ -401,6 +402,7 @@ func (pr *PullRequest) getReviewedByLines(ctx context.Context, writer io.Writer) Types: []ReviewType{ReviewTypeApprove}, IssueID: pr.IssueID, OfficialOnly: setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly, + Dismissed: optional.Some(true), }) if err != nil { log.Error("Unable to FindReviews for PR ID %d: %v", pr.ID, err) diff --git a/models/issues/review.go b/models/issues/review.go index 3e787273be8e1..577d93052ed28 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -16,6 +16,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" @@ -392,6 +393,7 @@ func GetCurrentReview(ctx context.Context, reviewer *user_model.User, issue *Iss Types: []ReviewType{ReviewTypePending}, IssueID: issue.ID, ReviewerID: reviewer.ID, + Dismissed: optional.Some(true), }) if err != nil { return nil, err @@ -534,12 +536,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 ...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 len(dismissed) != 0 { + cond = cond.And(builder.Eq{"dismissed": dismissed[0]}) + } + + has, err := db.GetEngine(ctx).Where(cond). Desc("id"). Get(review) if err != nil { @@ -731,7 +739,7 @@ func RemoveReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user } defer committer.Close() - review, err := GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID) + review, err := GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID, false) if err != nil && !IsErrReviewNotExist(err) { return nil, err } diff --git a/models/issues/review_list.go b/models/issues/review_list.go index 928f24fb2dcd7..3f03347ac960c 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/pull/pull.go b/services/pull/pull.go index 5d3758eca6d14..7d89e503fe828 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -30,6 +30,7 @@ import ( "code.gitea.io/gitea/modules/graceful" "code.gitea.io/gitea/modules/json" "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" @@ -1102,6 +1103,7 @@ func GetPullCommits(ctx *gitea_context.Context, issue *issues_model.Issue) ([]Co issues_model.ReviewTypeComment, issues_model.ReviewTypeReject, }, + Dismissed: optional.Some(false), }) if err != nil && !issues_model.IsErrReviewNotExist(err) { From e28f23a0720ddef4fe7f086be460bf950fc69eb9 Mon Sep 17 00:00:00 2001 From: "m.huber" Date: Mon, 18 Aug 2025 16:57:20 +0200 Subject: [PATCH 02/15] fix --- models/issues/pull.go | 2 +- models/issues/review.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/models/issues/pull.go b/models/issues/pull.go index c657e167f1c76..65ff5423aa8b5 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -374,7 +374,7 @@ func (pr *PullRequest) getReviewedByLines(ctx context.Context, writer io.Writer) Types: []ReviewType{ReviewTypeApprove}, IssueID: pr.IssueID, OfficialOnly: setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly, - Dismissed: optional.Some(true), + Dismissed: optional.Some(false), }) if err != nil { log.Error("Unable to FindReviews for PR ID %d: %v", pr.ID, err) diff --git a/models/issues/review.go b/models/issues/review.go index 198fd50b5d23d..a9a57491216bc 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -391,7 +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(true), + Dismissed: optional.Some(false), }) if err != nil { return nil, err From ce22adfc52451c3b1578c657c526fa05439b40c3 Mon Sep 17 00:00:00 2001 From: "m.huber" Date: Mon, 1 Sep 2025 14:44:59 +0200 Subject: [PATCH 03/15] fixtures add dismissed review case --- models/fixtures/review.yml | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/models/fixtures/review.yml b/models/fixtures/review.yml index 5b8bbceca9edf..5ad7af0ee9e2c 100644 --- a/models/fixtures/review.yml +++ b/models/fixtures/review.yml @@ -207,6 +207,17 @@ - id: 22 + type: 3 + reviewer_id: 5 + issue_id: 3 + content: "review request for user5" + 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,15 @@ original_author_id: 0 updated_unix: 946684817 created_unix: 946684817 + +- + id: 24 + 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 From 1f70222aec6f84bd881b144b0ff932ae29f0f730 Mon Sep 17 00:00:00 2001 From: "m.huber" Date: Mon, 1 Sep 2025 14:59:24 +0200 Subject: [PATCH 04/15] add error edgecase where dismissed is before an review request --- models/fixtures/review.yml | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/models/fixtures/review.yml b/models/fixtures/review.yml index 5ad7af0ee9e2c..3248a4ec30af7 100644 --- a/models/fixtures/review.yml +++ b/models/fixtures/review.yml @@ -229,6 +229,17 @@ - id: 24 type: 3 + reviewer_id: 5 + issue_id: 3 + content: "review request for user5" + 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 From 3d5913bdfd427a9a07399ab4a3244a6e6cb530c2 Mon Sep 17 00:00:00 2001 From: "m.huber" Date: Mon, 1 Sep 2025 15:42:37 +0200 Subject: [PATCH 05/15] adjust tests (1) --- models/issues/pull_test.go | 7 +++++++ tests/integration/api_pull_review_test.go | 3 ++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index 7089af253b7a4..2d1082b58d53f 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -348,6 +348,13 @@ func TestGetApprovers(t *testing.T) { approvers := pr.GetApprovers(t.Context()) expected := "Reviewed-by: User Five \nReviewed-by: Org Six \n" assert.Equal(t, expected, approvers) + + // dismissed reviews should be ignored + pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) + setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly = false + approvers = pr.GetApprovers(t.Context()) + expected = "Reviewed-by: TEST TODO\n" + assert.Equal(t, expected, approvers) } func TestGetPullRequestByMergedCommit(t *testing.T) { diff --git a/tests/integration/api_pull_review_test.go b/tests/integration/api_pull_review_test.go index 9ffbd9a2673d4..3b251e44e1402 100644 --- a/tests/integration/api_pull_review_test.go +++ b/tests/integration/api_pull_review_test.go @@ -40,7 +40,7 @@ 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) @@ -57,6 +57,7 @@ func TestAPIPullReview(t *testing.T) { assert.EqualValues(t, -1, reviews[5].Reviewer.ID) // ghost user assert.False(t, reviews[5].Stale) assert.True(t, reviews[5].Official) + 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). From 64800836c09968e70311d8ea176bf5a8b8a9ad37 Mon Sep 17 00:00:00 2001 From: "m.huber" Date: Mon, 1 Sep 2025 15:44:24 +0200 Subject: [PATCH 06/15] adjust tests (2) --- models/issues/pull_list_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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) } From bfb4f02a1d800bca5b76a2e21c1bfbeffa44bbc6 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 1 Sep 2025 15:47:35 +0200 Subject: [PATCH 07/15] Apply suggestions from code review Signed-off-by: 6543 <6543@obermui.de> --- models/fixtures/review.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/models/fixtures/review.yml b/models/fixtures/review.yml index 3248a4ec30af7..74111fa0d657e 100644 --- a/models/fixtures/review.yml +++ b/models/fixtures/review.yml @@ -207,10 +207,10 @@ - id: 22 - type: 3 + type: 1 reviewer_id: 5 issue_id: 3 - content: "review request for user5" + content: "review accepted of user5 but dismissed" original_author_id: 0 updated_unix: 946684000 created_unix: 946684000 @@ -231,7 +231,7 @@ type: 3 reviewer_id: 5 issue_id: 3 - content: "review request for user5" + content: "review rejected of user5 but dismissed" original_author_id: 0 updated_unix: 946684827 created_unix: 946684827 From 9afe1711138f8bea248c4bace178b1898c7fa70f Mon Sep 17 00:00:00 2001 From: "m.huber" Date: Mon, 1 Sep 2025 15:57:06 +0200 Subject: [PATCH 08/15] adjust tests (3) --- models/issues/pull_test.go | 3 ++- tests/integration/api_pull_review_test.go | 30 +++++++++++++---------- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index 2d1082b58d53f..61192d2e423a9 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -351,9 +351,10 @@ func TestGetApprovers(t *testing.T) { // dismissed reviews should be ignored pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) + assert.EqualValues(t, 3, pr.IssueID) setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly = false approvers = pr.GetApprovers(t.Context()) - expected = "Reviewed-by: TEST TODO\n" + expected = "Reviewed-by: user4 \n" assert.Equal(t, expected, approvers) } diff --git a/tests/integration/api_pull_review_test.go b/tests/integration/api_pull_review_test.go index 3b251e44e1402..8a87e731d2af6 100644 --- a/tests/integration/api_pull_review_test.go +++ b/tests/integration/api_pull_review_test.go @@ -45,19 +45,23 @@ func TestAPIPullReview(t *testing.T) { 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) - assert.True(t, reviews[10].Dismissed) + 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, 1000, 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). From a9c3366f44026f88d85712ebc0978bd3e4d02aa8 Mon Sep 17 00:00:00 2001 From: "m.huber" Date: Mon, 1 Sep 2025 16:20:04 +0200 Subject: [PATCH 09/15] adjust tests (4) --- tests/integration/api_pull_review_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/api_pull_review_test.go b/tests/integration/api_pull_review_test.go index 8a87e731d2af6..994cf740e2af0 100644 --- a/tests/integration/api_pull_review_test.go +++ b/tests/integration/api_pull_review_test.go @@ -59,7 +59,7 @@ func TestAPIPullReview(t *testing.T) { assert.False(t, reviews[6].Stale) assert.True(t, reviews[6].Official) } - if assert.EqualValues(t, 1000, reviews[10].ID) { + if assert.EqualValues(t, 25, reviews[10].ID) { assert.True(t, reviews[10].Dismissed) } From 9154b6c9c2ba0df226dc044ec92bf832766217ab Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 1 Sep 2025 16:21:34 +0200 Subject: [PATCH 10/15] Update models/issues/pull_test.go Signed-off-by: 6543 <6543@obermui.de> --- models/issues/pull_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index 61192d2e423a9..5b2ade3ed6aec 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -349,7 +349,7 @@ func TestGetApprovers(t *testing.T) { expected := "Reviewed-by: User Five \nReviewed-by: Org Six \n" assert.Equal(t, expected, approvers) - // dismissed reviews should be ignored + // dismissed, comment-type and pending reviews should be ignored pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) assert.EqualValues(t, 3, pr.IssueID) setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly = false From 7b2bb24d32948aab80a716841d45aa46ef930383 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 16 Sep 2025 16:24:01 +0200 Subject: [PATCH 11/15] moved to https://github.com/go-gitea/gitea/pull/35500 --- models/issues/pull.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/models/issues/pull.go b/models/issues/pull.go index f17b6838da215..7a37b627e1bd0 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -20,7 +20,6 @@ import ( user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" - "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/util" @@ -380,7 +379,6 @@ func (pr *PullRequest) getReviewedByLines(ctx context.Context, writer io.Writer) Types: []ReviewType{ReviewTypeApprove}, IssueID: pr.IssueID, OfficialOnly: setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly, - Dismissed: optional.Some(false), }) if err != nil { log.Error("Unable to FindReviews for PR ID %d: %v", pr.ID, err) From 14bd6af4540b5d98388b7489d231fcffac530c28 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 16 Sep 2025 20:14:03 +0200 Subject: [PATCH 12/15] make tests pass again --- models/issues/pull_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index 5b2ade3ed6aec..2430a774e6117 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -349,12 +349,13 @@ func TestGetApprovers(t *testing.T) { expected := "Reviewed-by: User Five \nReviewed-by: Org Six \n" assert.Equal(t, expected, approvers) - // dismissed, comment-type and pending reviews should be ignored + // ( TODO(#35500): dismissed, ) comment-type and pending reviews should be ignored pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) assert.EqualValues(t, 3, pr.IssueID) - setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly = false + setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly = true approvers = pr.GetApprovers(t.Context()) - expected = "Reviewed-by: user4 \n" + expected = "Reviewed-by: User Five \n" + // TODO(#35500): remove user 5 + "Reviewed-by: user4 \n" assert.Equal(t, expected, approvers) } From dbfba370a746bf46c1d83b1f4617d8131dfd4d14 Mon Sep 17 00:00:00 2001 From: "m.huber" Date: Wed, 17 Sep 2025 01:23:48 +0200 Subject: [PATCH 13/15] fix and extend test --- models/issues/pull_test.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index 2430a774e6117..20572afff5d04 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -349,13 +349,20 @@ func TestGetApprovers(t *testing.T) { expected := "Reviewed-by: User Five \nReviewed-by: Org Six \n" assert.Equal(t, expected, approvers) - // ( TODO(#35500): dismissed, ) comment-type and pending reviews should be ignored + // comment-type and pending reviews should be ignored + pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) + assert.EqualValues(t, 3, pr.IssueID) + setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly = false + 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 = "Reviewed-by: User Five \n" + // TODO(#35500): remove user 5 - "Reviewed-by: user4 \n" + expected = "" assert.Equal(t, expected, approvers) } From a48eb669ecdeeb4ae7fb9a54e90bf06f90cb163d Mon Sep 17 00:00:00 2001 From: "m.huber" Date: Thu, 2 Oct 2025 04:42:44 +0200 Subject: [PATCH 14/15] optional --- models/issues/review.go | 14 +++++++------- services/issue/assignee.go | 3 ++- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/models/issues/review.go b/models/issues/review.go index a9a57491216bc..f14e22394b70d 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -534,15 +534,15 @@ 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, dismissed ...bool) (*Review, error) { +func GetReviewByIssueIDAndUserID(ctx context.Context, issueID, userID int64, dismissed optional.Option[bool]) (*Review, error) { review := new(Review) 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 len(dismissed) != 0 { - cond = cond.And(builder.Eq{"dismissed": dismissed[0]}) + if dismissed.Has() { + cond = cond.And(builder.Eq{"dismissed": dismissed.Value()}) } has, err := db.GetEngine(ctx).Where(cond). @@ -655,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 } @@ -725,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, false) + review, err := GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID, optional.Some(false)) if err != nil && !IsErrReviewNotExist(err) { return nil, err } @@ -760,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 } @@ -852,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/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 } From 3326fa5976f721523f85badc6bf778188ee43ca5 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 6 Oct 2025 16:45:52 +0200 Subject: [PATCH 15/15] test var --- models/issues/pull_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index 20572afff5d04..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,7 +345,7 @@ 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) @@ -352,7 +353,6 @@ func TestGetApprovers(t *testing.T) { // comment-type and pending reviews should be ignored pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) assert.EqualValues(t, 3, pr.IssueID) - setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly = false approvers = pr.GetApprovers(t.Context()) expected = "Reviewed-by: User Five \nReviewed-by: user4 \n" assert.Equal(t, expected, approvers)