Merge pull request '[gitea] v1.21 cherry-pick' (#2407) from earl-warren/forgejo:wip-v1.21-gitea-cherry-pick into v1.21/forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/2407
Reviewed-by: oliverpool <oliverpool@noreply.codeberg.org>
Reviewed-by: Gusted <gusted@noreply.codeberg.org>

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)

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
 		}
 	}

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

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()
 }
 

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)
 }

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)

templates/admin/user/new.tmpl

@@ -26,7 +26,7 @@
 				<div class="inline field {{if .Err_Visibility}}error{{end}}">
 					<span class="inline required field"><label for="visibility">{{ctx.Locale.Tr "settings.visibility"}}</label></span>
 					<div class="ui selection type dropdown">
-						<input type="hidden" id="visibility" name="visibility" value="{{if .visibility}}{{.visibility}}{{else}}{{printf "%d" .DefaultUserVisibilityMode}}{{end}}">
+						<input type="hidden" id="visibility" name="visibility" value="{{if .visibility}}{{printf "%d" .visibility}}{{else}}{{printf "%d" .DefaultUserVisibilityMode}}{{end}}">
 						<div class="text">
 							{{if .DefaultUserVisibilityMode.IsPublic}}{{ctx.Locale.Tr "settings.visibility.public"}}{{end}}
 							{{if .DefaultUserVisibilityMode.IsLimited}}{{ctx.Locale.Tr "settings.visibility.limited"}}{{end}}

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)
+	})
+}