Merge pull request '[gitea] week 2024-41-v9.0 cherry pick (gitea/main -> v9.0/forgejo)' (#5480) from earl-warren/wcp/2024-41-v9.0 into v9.0/forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/5480
Reviewed-by: Otto <otto@codeberg.org>

models/fixtures/comment.yml

@@ -94,3 +94,22 @@
   content: "test markup light/dark-mode-only ![GitHub-Mark-Light](https://user-images.githubusercontent.com/3369400/139447912-e0f43f33-6d9f-45f8-be46-2df5bbc91289.png#gh-dark-mode-only)![GitHub-Mark-Dark](https://user-images.githubusercontent.com/3369400/139448065-39a229ba-4b06-434b-bc67-616e2ed80c8f.png#gh-light-mode-only)"
   created_unix: 946684813
   updated_unix: 946684813
+
+-
+  id: 11
+  type: 22 # review
+  poster_id: 5
+  issue_id: 3 # in repo_id 1
+  content: "reviewed by user5"
+  review_id: 21
+  created_unix: 946684816
+
+-
+  id: 12
+  type: 27 # review request
+  poster_id: 2
+  issue_id: 3 # in repo_id 1
+  content: "review request for user5"
+  review_id: 22
+  assignee_id: 5
+  created_unix: 946684817

models/fixtures/review.yml

@@ -179,3 +179,22 @@
   content: "Review Comment"
   updated_unix: 946684810
   created_unix: 946684810
+
+-
+  id: 21
+  type: 2
+  reviewer_id: 5
+  issue_id: 3
+  content: "reviewed by user5"
+  commit_id: 4a357436d925b5c974181ff12a994538ddc5a269
+  updated_unix: 946684816
+  created_unix: 946684816
+
+-
+  id: 22
+  type: 4
+  reviewer_id: 5
+  issue_id: 3
+  content: "review request for user5"
+  updated_unix: 946684817
+  created_unix: 946684817

models/issues/pull.go

@@ -408,7 +408,7 @@ func (pr *PullRequest) getReviewedByLines(ctx context.Context, writer io.Writer)
 
 	// Note: This doesn't page as we only expect a very limited number of reviews
 	reviews, err := FindLatestReviews(ctx, FindReviewOptions{
-		Type:         ReviewTypeApprove,
+		Types:        []ReviewType{ReviewTypeApprove},
 		IssueID:      pr.IssueID,
 		OfficialOnly: setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly,
 	})

models/issues/review.go

@@ -364,7 +364,7 @@ func GetCurrentReview(ctx context.Context, reviewer *user_model.User, issue *Iss
 		return nil, nil
 	}
 	reviews, err := FindReviews(ctx, FindReviewOptions{
-		Type:       ReviewTypePending,
+		Types:      []ReviewType{ReviewTypePending},
 		IssueID:    issue.ID,
 		ReviewerID: reviewer.ID,
 	})

models/issues/review_list.go

@@ -92,7 +92,7 @@ func (reviews ReviewList) LoadIssues(ctx context.Context) error {
 // FindReviewOptions represent possible filters to find reviews
 type FindReviewOptions struct {
 	db.ListOptions
-	Type         ReviewType
+	Types        []ReviewType
 	IssueID      int64
 	ReviewerID   int64
 	OfficialOnly bool
@@ -107,8 +107,8 @@ func (opts *FindReviewOptions) toCond() builder.Cond {
 	if opts.ReviewerID > 0 {
 		cond = cond.And(builder.Eq{"reviewer_id": opts.ReviewerID})
 	}
-	if opts.Type != ReviewTypeUnknown {
-		cond = cond.And(builder.Eq{"type": opts.Type})
+	if len(opts.Types) > 0 {
+		cond = cond.And(builder.In("type", opts.Types))
 	}
 	if opts.OfficialOnly {
 		cond = cond.And(builder.Eq{"official": true})

models/issues/review_test.go

@@ -64,7 +64,7 @@ func TestReviewType_Icon(t *testing.T) {
 func TestFindReviews(t *testing.T) {
 	require.NoError(t, unittest.PrepareTestDatabase())
 	reviews, err := issues_model.FindReviews(db.DefaultContext, issues_model.FindReviewOptions{
-		Type:       issues_model.ReviewTypeApprove,
+		Types:      []issues_model.ReviewType{issues_model.ReviewTypeApprove},
 		IssueID:    2,
 		ReviewerID: 1,
 	})
@@ -76,7 +76,7 @@ func TestFindReviews(t *testing.T) {
 func TestFindLatestReviews(t *testing.T) {
 	require.NoError(t, unittest.PrepareTestDatabase())
 	reviews, err := issues_model.FindLatestReviews(db.DefaultContext, issues_model.FindReviewOptions{
-		Type:    issues_model.ReviewTypeApprove,
+		Types:   []issues_model.ReviewType{issues_model.ReviewTypeApprove},
 		IssueID: 11,
 	})
 	require.NoError(t, err)

modules/indexer/code/elasticsearch/elasticsearch.go

@@ -20,6 +20,7 @@ import (
 	indexer_internal "code.gitea.io/gitea/modules/indexer/internal"
 	inner_elasticsearch "code.gitea.io/gitea/modules/indexer/internal/elasticsearch"
 	"code.gitea.io/gitea/modules/json"
+	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/timeutil"
 	"code.gitea.io/gitea/modules/typesniffer"
@@ -197,8 +198,33 @@ func (b *Indexer) Index(ctx context.Context, repo *repo_model.Repository, sha st
 	return nil
 }
 
-// Delete deletes indexes by ids
+// Delete entries by repoId
 func (b *Indexer) Delete(ctx context.Context, repoID int64) error {
+	if err := b.doDelete(ctx, repoID); err != nil {
+		// Maybe there is a conflict during the delete operation, so we should retry after a refresh
+		log.Warn("Deletion of entries of repo %v within index %v was erroneus. Trying to refresh index before trying again", repoID, b.inner.VersionedIndexName(), err)
+		if err := b.refreshIndex(ctx); err != nil {
+			return err
+		}
+		if err := b.doDelete(ctx, repoID); err != nil {
+			log.Error("Could not delete entries of repo %v within index %v", repoID, b.inner.VersionedIndexName())
+			return err
+		}
+	}
+	return nil
+}
+
+func (b *Indexer) refreshIndex(ctx context.Context) error {
+	if _, err := b.inner.Client.Refresh(b.inner.VersionedIndexName()).Do(ctx); err != nil {
+		log.Error("Error while trying to refresh index %v", b.inner.VersionedIndexName(), err)
+		return err
+	}
+
+	return nil
+}
+
+// Delete entries by repoId
+func (b *Indexer) doDelete(ctx context.Context, repoID int64) error {
 	_, err := b.inner.Client.DeleteByQuery(b.inner.VersionedIndexName()).
 		Query(elastic.NewTermsQuery("repo_id", repoID)).
 		Do(ctx)

release-notes/5480.md

@@ -0,0 +1,2 @@
+fix: [commit](https://codeberg.org/forgejo/forgejo/commit/7d3a013e5e81bbc054f4a730923e08f61814bf66) PR creation on forked repositories.
+fix: [commit](https://codeberg.org/forgejo/forgejo/commit/700e9f027bce8c783b74de07b3f29e09be045fa7) the logic of finding the latest pull review commit ID is incorrect.

routers/api/v1/repo/pull.go

@@ -1112,9 +1112,20 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption)
 	// Check if current user has fork of repository or in the same repository.
 	headRepo := repo_model.GetForkedRepo(ctx, headUser.ID, baseRepo.ID)
 	if headRepo == nil && !isSameRepo {
-		log.Trace("parseCompareInfo[%d]: does not have fork or in same repository", baseRepo.ID)
-		ctx.NotFound("GetForkedRepo")
-		return nil, nil, nil, "", ""
+		err := baseRepo.GetBaseRepo(ctx)
+		if err != nil {
+			ctx.Error(http.StatusInternalServerError, "GetBaseRepo", err)
+			return nil, nil, nil, "", ""
+		}
+
+		// Check if baseRepo's base repository is the same as headUser's repository.
+		if baseRepo.BaseRepo == nil || baseRepo.BaseRepo.OwnerID != headUser.ID {
+			log.Trace("parseCompareInfo[%d]: does not have fork or in same repository", baseRepo.ID)
+			ctx.NotFound("GetBaseRepo")
+			return nil, nil, nil, "", ""
+		}
+		// Assign headRepo so it can be used below.
+		headRepo = baseRepo.BaseRepo
 	}
 
 	var headGitRepo *git.Repository

routers/api/v1/repo/pull_review.go

@@ -82,7 +82,6 @@ func ListPullReviews(ctx *context.APIContext) {
 
 	opts := issues_model.FindReviewOptions{
 		ListOptions: utils.GetListOptions(ctx),
-		Type:        issues_model.ReviewTypeUnknown,
 		IssueID:     pr.IssueID,
 	}
 

services/pull/pull.go

@@ -966,6 +966,8 @@ type CommitInfo struct {
 }
 
 // GetPullCommits returns all commits on given pull request and the last review commit sha
+// Attention: The last review commit sha must be from the latest review whose commit id is not empty.
+// So the type of the latest review cannot be "ReviewTypeRequest".
 func GetPullCommits(ctx *gitea_context.Context, issue *issues_model.Issue) ([]CommitInfo, string, error) {
 	pull := issue.PullRequest
 
@@ -1011,7 +1013,11 @@ func GetPullCommits(ctx *gitea_context.Context, issue *issues_model.Issue) ([]Co
 		lastreview, err := issues_model.FindLatestReviews(ctx, issues_model.FindReviewOptions{
 			IssueID:    issue.ID,
 			ReviewerID: ctx.Doer.ID,
-			Type:       issues_model.ReviewTypeUnknown,
+			Types: []issues_model.ReviewType{
+				issues_model.ReviewTypeApprove,
+				issues_model.ReviewTypeComment,
+				issues_model.ReviewTypeReject,
+			},
 		})
 
 		if err != nil && !issues_model.IsErrReviewNotExist(err) {

services/pull/review.go

@@ -340,7 +340,7 @@ func DismissApprovalReviews(ctx context.Context, doer *user_model.User, pull *is
 	reviews, err := issues_model.FindReviews(ctx, issues_model.FindReviewOptions{
 		ListOptions: db.ListOptionsAll,
 		IssueID:     pull.IssueID,
-		Type:        issues_model.ReviewTypeApprove,
+		Types:       []issues_model.ReviewType{issues_model.ReviewTypeApprove},
 		Dismissed:   optional.Some(false),
 	})
 	if err != nil {

tests/integration/api_pull_review_test.go

@@ -60,6 +60,9 @@ func TestAPIPullReviewCreateDeleteComment(t *testing.T) {
 				var reviews []*api.PullReview
 				DecodeJSON(t, resp, &reviews)
 				for _, review := range reviews {
+					if review.State == api.ReviewStateRequestReview {
+						continue
+					}
 					req := NewRequestf(t, http.MethodDelete, "/api/v1/repos/%s/pulls/%d/reviews/%d", repo.FullName(), pullIssue.Index, review.ID).
 						AddTokenAuth(token)
 					MakeRequest(t, req, http.StatusNoContent)
@@ -93,7 +96,7 @@ func TestAPIPullReviewCreateDeleteComment(t *testing.T) {
 				DecodeJSON(t, resp, &getReview)
 				require.EqualValues(t, getReview, review)
 			}
-			requireReviewCount(1)
+			requireReviewCount(2)
 
 			newCommentBody := "first new line"
 			var reviewComment api.PullReviewComment
@@ -140,7 +143,7 @@ func TestAPIPullReviewCreateDeleteComment(t *testing.T) {
 					AddTokenAuth(token)
 				MakeRequest(t, req, http.StatusNoContent)
 			}
-			requireReviewCount(0)
+			requireReviewCount(1)
 		})
 	}
 }
@@ -160,7 +163,7 @@ func TestAPIPullReview(t *testing.T) {
 
 	var reviews []*api.PullReview
 	DecodeJSON(t, resp, &reviews)
-	if !assert.Len(t, reviews, 6) {
+	if !assert.Len(t, reviews, 8) {
 		return
 	}
 	for _, r := range reviews {

tests/integration/pull_commit_test.go

@@ -0,0 +1,34 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package integration
+
+import (
+	"net/http"
+	"net/url"
+	"testing"
+
+	pull_service "code.gitea.io/gitea/services/pull"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestListPullCommits(t *testing.T) {
+	onGiteaRun(t, func(t *testing.T, u *url.URL) {
+		session := loginUser(t, "user5")
+		req := NewRequest(t, "GET", "/user2/repo1/pulls/3/commits/list")
+		resp := session.MakeRequest(t, req, http.StatusOK)
+
+		var pullCommitList struct {
+			Commits             []pull_service.CommitInfo `json:"commits"`
+			LastReviewCommitSha string                    `json:"last_review_commit_sha"`
+		}
+		DecodeJSON(t, resp, &pullCommitList)
+
+		if assert.Len(t, pullCommitList.Commits, 2) {
+			assert.Equal(t, "5f22f7d0d95d614d25a5b68592adb345a4b5c7fd", pullCommitList.Commits[0].ID)
+			assert.Equal(t, "4a357436d925b5c974181ff12a994538ddc5a269", pullCommitList.Commits[1].ID)
+		}
+		assert.Equal(t, "4a357436d925b5c974181ff12a994538ddc5a269", pullCommitList.LastReviewCommitSha)
+	})
+}

tests/integration/pull_create_test.go

@@ -72,6 +72,30 @@ func testPullCreate(t *testing.T, session *TestSession, user, repo string, toSel
 	return resp
 }
 
+func testPullCreateDirectly(t *testing.T, session *TestSession, baseRepoOwner, baseRepoName, baseBranch, headRepoOwner, headRepoName, headBranch, title string) *httptest.ResponseRecorder {
+	headCompare := headBranch
+	if headRepoOwner != "" {
+		if headRepoName != "" {
+			headCompare = fmt.Sprintf("%s/%s:%s", headRepoOwner, headRepoName, headBranch)
+		} else {
+			headCompare = fmt.Sprintf("%s:%s", headRepoOwner, headBranch)
+		}
+	}
+	req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/compare/%s...%s", baseRepoOwner, baseRepoName, baseBranch, headCompare))
+	resp := session.MakeRequest(t, req, http.StatusOK)
+
+	// Submit the form for creating the pull
+	htmlDoc := NewHTMLParser(t, resp.Body)
+	link, exists := htmlDoc.doc.Find("form.ui.form").Attr("action")
+	assert.True(t, exists, "The template has changed")
+	req = NewRequestWithValues(t, "POST", link, map[string]string{
+		"_csrf": htmlDoc.GetCSRF(),
+		"title": title,
+	})
+	resp = session.MakeRequest(t, req, http.StatusOK)
+	return resp
+}
+
 func TestPullCreate(t *testing.T) {
 	onGiteaRun(t, func(t *testing.T, u *url.URL) {
 		session := loginUser(t, "user1")
@@ -505,3 +529,30 @@ func TestRecentlyPushed(t *testing.T) {
 		})
 	})
 }
+
+/*
+Setup:
+The base repository is: user2/repo1
+Fork repository to: user1/repo1
+Push extra commit to: user2/repo1, which changes README.md
+Create a PR on user1/repo1
+
+Test checks:
+Check if pull request can be created from base to the fork repository.
+*/
+func TestPullCreatePrFromBaseToFork(t *testing.T) {
+	onGiteaRun(t, func(t *testing.T, u *url.URL) {
+		sessionFork := loginUser(t, "user1")
+		testRepoFork(t, sessionFork, "user2", "repo1", "user1", "repo1")
+
+		// Edit base repository
+		sessionBase := loginUser(t, "user2")
+		testEditFile(t, sessionBase, "user2", "repo1", "master", "README.md", "Hello, World (Edited)\n")
+
+		// Create a PR
+		resp := testPullCreateDirectly(t, sessionFork, "user1", "repo1", "master", "user2", "repo1", "master", "This is a pull title")
+		// check the redirected URL
+		url := test.RedirectURL(resp)
+		assert.Regexp(t, "^/user1/repo1/pulls/[0-9]*$", url)
+	})
+}