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) 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..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 } @@ -430,7 +434,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..18296c0dda 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 } @@ -285,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, @@ -296,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 @@ -622,6 +660,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() } 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/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) 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}} 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) + }) +}