From d3846df1f996d6ae4f95ea51ad6f927234fddac3 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 13 Feb 2024 23:29:33 +0100 Subject: [PATCH 1/6] Dont load Review if Comment is CommentTypeReviewRequest (#28551) (#29160) Backport #28551 RequestReview get deleted on review. So we don't have to try to load them on comments. (cherry picked from commit 0ac3186267b717bce7076ef44f883df7720d7a2d) --- models/issues/comment.go | 7 +++++++ models/issues/comment_list.go | 3 ++- models/issues/review.go | 3 +++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/models/issues/comment.go b/models/issues/comment.go index 8dc8c7682d..40da4d88b1 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -688,8 +688,15 @@ func (c *Comment) LoadReactions(ctx context.Context, repo *repo_model.Repository } func (c *Comment) loadReview(ctx context.Context) (err error) { + if c.ReviewID == 0 { + return nil + } if c.Review == nil { if c.Review, err = GetReviewByID(ctx, c.ReviewID); err != nil { + // review request which has been replaced by actual reviews doesn't exist in database anymore, so ignorem them. + if c.Type == CommentTypeReviewRequest { + return nil + } return err } } diff --git a/models/issues/comment_list.go b/models/issues/comment_list.go index 93af45870e..cb7df3270d 100644 --- a/models/issues/comment_list.go +++ b/models/issues/comment_list.go @@ -430,7 +430,8 @@ func (comments CommentList) loadReviews(ctx context.Context) error { for _, comment := range comments { comment.Review = reviews[comment.ReviewID] if comment.Review == nil { - if comment.ReviewID > 0 { + // review request which has been replaced by actual reviews doesn't exist in database anymore, so don't log errors for them. + if comment.ReviewID > 0 && comment.Type != CommentTypeReviewRequest { log.Error("comment with review id [%d] but has no review record", comment.ReviewID) } continue diff --git a/models/issues/review.go b/models/issues/review.go index eae8c20e97..5bfb8b9dd2 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -622,6 +622,9 @@ func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_mo return nil, err } + // func caller use the created comment to retrieve created review too. + comment.Review = review + return comment, committer.Commit() } From aced7547c2fa88a2a626f2b0169741905e8b6dc8 Mon Sep 17 00:00:00 2001 From: KN4CK3R Date: Wed, 14 Feb 2024 18:51:51 +0100 Subject: [PATCH 2/6] Use ghost user if user was not found (#29161) (#29169) Backport #29161 (cherry picked from commit d823465d94b3b43945eace060000db9334eca52d) --- models/issues/comment_list.go | 4 ++++ models/issues/review.go | 8 ++++++++ 2 files changed, 12 insertions(+) diff --git a/models/issues/comment_list.go b/models/issues/comment_list.go index cb7df3270d..30a437ea50 100644 --- a/models/issues/comment_list.go +++ b/models/issues/comment_list.go @@ -225,6 +225,10 @@ func (comments CommentList) loadAssignees(ctx context.Context) error { for _, comment := range comments { comment.Assignee = assignees[comment.AssigneeID] + if comment.Assignee == nil { + comment.AssigneeID = user_model.GhostUserID + comment.Assignee = user_model.NewGhostUser() + } } return nil } diff --git a/models/issues/review.go b/models/issues/review.go index 5bfb8b9dd2..3fca30d7b6 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -159,6 +159,14 @@ func (r *Review) LoadReviewer(ctx context.Context) (err error) { return err } r.Reviewer, err = user_model.GetPossibleUserByID(ctx, r.ReviewerID) + if err != nil { + if !user_model.IsErrUserNotExist(err) { + return fmt.Errorf("GetPossibleUserByID [%d]: %w", r.ReviewerID, err) + } + r.ReviewerID = user_model.GhostUserID + r.Reviewer = user_model.NewGhostUser() + return nil + } return err } From 8782275c9c66ad6fc7c44503d7df9dae7196aa65 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 17 Feb 2024 20:06:55 +0800 Subject: [PATCH 3/6] Fix push to create with capitalize repo name (#29090) (#29206) Fix #29073 Backport #29090 Co-authored-by: KN4CK3R (cherry picked from commit 933cc4da642c13b18423be99574944c43cc558c7) --- cmd/serv.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/cmd/serv.go b/cmd/serv.go index 43e6eaba2b..15cbbe910e 100644 --- a/cmd/serv.go +++ b/cmd/serv.go @@ -216,16 +216,18 @@ func runServ(c *cli.Context) error { } } - // LowerCase and trim the repoPath as that's how they are stored. - repoPath = strings.ToLower(strings.TrimSpace(repoPath)) - rr := strings.SplitN(repoPath, "/", 2) if len(rr) != 2 { return fail(ctx, "Invalid repository path", "Invalid repository path: %v", repoPath) } - username := strings.ToLower(rr[0]) - reponame := strings.ToLower(strings.TrimSuffix(rr[1], ".git")) + username := rr[0] + reponame := strings.TrimSuffix(rr[1], ".git") + + // LowerCase and trim the repoPath as that's how they are stored. + // This should be done after splitting the repoPath into username and reponame + // so that username and reponame are not affected. + repoPath = strings.ToLower(strings.TrimSpace(repoPath)) if alphaDashDotPattern.MatchString(reponame) { return fail(ctx, "Invalid repo name", "Invalid repo name: %s", reponame) From a40762d929c20e4b480084c23eed9529f86ee2e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim-Nicas=20Oelschl=C3=A4ger?= <72873130+zokkis@users.noreply.github.com> Date: Sun, 18 Feb 2024 19:51:00 +0100 Subject: [PATCH 4/6] Convert visibility to number (#29226) (#29244) Backport #29226 Don't throw error while creating user (Fixes #29218) --- The backport info from Giteabot https://github.com/go-gitea/gitea/pull/29226#issuecomment-1951341322 needs to specify the version, because the default is v1.18 (cherry picked from commit 39735c43a8b6f7db3b3e3531ca9115a60335d524) --- templates/admin/user/new.tmpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/admin/user/new.tmpl b/templates/admin/user/new.tmpl index 81f70511d0..bcb53d8131 100644 --- a/templates/admin/user/new.tmpl +++ b/templates/admin/user/new.tmpl @@ -26,7 +26,7 @@
- +
{{if .DefaultUserVisibilityMode.IsPublic}}{{ctx.Locale.Tr "settings.visibility.public"}}{{end}} {{if .DefaultUserVisibilityMode.IsLimited}}{{ctx.Locale.Tr "settings.visibility.limited"}}{{end}} From 861d0b96893ab5174a9216fb352810f42b255f62 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Mon, 19 Feb 2024 18:09:36 +0800 Subject: [PATCH 5/6] Do not use lower tag names to find releases/tags (#29261) (#29262) Backport #29261. Fix #26090, see https://github.com/go-gitea/gitea/issues/26090#issuecomment-1952013206 Since `TagName` stores the original tag name and `LowerTagName` stores the lower tag name, it doesn't make sense to use lowercase tags as `TagNames` in `FindReleasesOptions`. https://github.com/go-gitea/gitea/blob/5e72526da4e915791f03af056890e16821bde052/services/repository/push.go#L396-L397 While the only other usage looks correct: https://github.com/go-gitea/gitea/blob/5e72526da4e915791f03af056890e16821bde052/routers/web/repo/repo.go#L416 (cherry picked from commit f79530c50ee1c7833cae13e56531e5d1fd66f5ba) --- services/repository/push.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/services/repository/push.go b/services/repository/push.go index 90aac95323..6e8f2d4640 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -315,12 +315,7 @@ func pushUpdateAddTags(ctx context.Context, repo *repo_model.Repository, gitRepo return nil } - lowerTags := make([]string, 0, len(tags)) - for _, tag := range tags { - lowerTags = append(lowerTags, strings.ToLower(tag)) - } - - releases, err := repo_model.GetReleasesByRepoIDAndNames(ctx, repo.ID, lowerTags) + releases, err := repo_model.GetReleasesByRepoIDAndNames(ctx, repo.ID, tags) if err != nil { return fmt.Errorf("GetReleasesByRepoIDAndNames: %w", err) } @@ -329,6 +324,11 @@ func pushUpdateAddTags(ctx context.Context, repo *repo_model.Repository, gitRepo relMap[rel.LowerTagName] = rel } + lowerTags := make([]string, 0, len(tags)) + for _, tag := range tags { + lowerTags = append(lowerTags, strings.ToLower(tag)) + } + newReleases := make([]*repo_model.Release, 0, len(lowerTags)-len(relMap)) emailToUser := make(map[string]*user_model.User) From 8377ecbfe1f2b72ec7d65c46cbc9022ad0ccd75f Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 19 Feb 2024 17:48:58 +0100 Subject: [PATCH 6/6] Workaround to clean up old reviews on creating a new one (#28554) (#29264) close #28542 backport #28554 --- *Sponsored by Kithara Software GmbH* (cherry picked from commit c01b266d8680a270b1e8067e757ed25be38eea24) --- models/issues/review.go | 40 ++++++- models/unittest/unit_tests.go | 8 +- tests/integration/api_pull_review_test.go | 126 ++++++++++++++++++++++ 3 files changed, 165 insertions(+), 9 deletions(-) diff --git a/models/issues/review.go b/models/issues/review.go index 3fca30d7b6..18296c0dda 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -293,8 +293,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, @@ -304,15 +310,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") } - return review, db.Insert(ctx, review) + + if _, err := sess.Insert(review); err != nil { + return nil, err + } + 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 d47bceea1e..75898436fc 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) } diff --git a/tests/integration/api_pull_review_test.go b/tests/integration/api_pull_review_test.go index 0e9bace44c..a84a12994e 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) { @@ -305,3 +308,126 @@ func TestAPIPullReviewRequest(t *testing.T) { req = NewRequestWithJSON(t, http.MethodDelete, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers?token=%s", repo3.OwnerName, repo3.Name, pullIssue12.Index, token), &api.PullReviewRequestOptions{}) 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?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, token2), &api.PullReviewRequestOptions{ + Reviewers: []string{user8.LoginName}, + }) + 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?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, token2), &api.PullReviewRequestOptions{ + Reviewers: []string{user8.LoginName}, + }) + 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?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, token8), &api.CreatePullReviewOptions{ + Event: "APPROVED", + Body: "lgtm", + }) + 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?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, token2), &api.PullReviewRequestOptions{ + Reviewers: []string{user8.LoginName}, + }) + 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?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, token8), &api.CreatePullReviewOptions{ + Event: "APPROVED", + Body: "lgtm", + }) + 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?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, token8), &api.CreatePullReviewOptions{ + Event: "REQUEST_CHANGES", + Body: "please change XYZ", + }) + 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) + }) +}