From 2bc47e81a39c64d9712627cb9fe154932fdd14bd Mon Sep 17 00:00:00 2001 From: Jason Song Date: Mon, 30 Sep 2024 10:28:09 +0800 Subject: [PATCH 1/9] Ensure `GetCSRF` doesn't return an empty token (#32130) Since page templates keep changing, some pages that contained forms with CSRF token no longer have them. It leads to some calls of `GetCSRF` returning an empty string, which fails the tests. Like https://github.com/go-gitea/gitea/blob/3269b04d61ffe6a7ce462cd05ee150e4491124e8/tests/integration/attachment_test.go#L62-L63 The test did try to get the CSRF token and provided it, but it was empty. (cherry picked from commit 13283873e9d523d5a5557f55d64f702c1a9f76ec) Conflicts: tests/integration/integration_test.go trivial context conflict --- tests/integration/attachment_test.go | 9 +++------ tests/integration/integration_test.go | 7 ++++++- tests/integration/org_test.go | 4 ---- 3 files changed, 9 insertions(+), 11 deletions(-) diff --git a/tests/integration/attachment_test.go b/tests/integration/attachment_test.go index 7cbc2545d5..7bd3e680f7 100644 --- a/tests/integration/attachment_test.go +++ b/tests/integration/attachment_test.go @@ -30,7 +30,7 @@ func generateImg() bytes.Buffer { return buff } -func createAttachment(t *testing.T, session *TestSession, repoURL, filename string, buff bytes.Buffer, expectedStatus int) string { +func createAttachment(t *testing.T, session *TestSession, csrf, repoURL, filename string, buff bytes.Buffer, expectedStatus int) string { body := &bytes.Buffer{} // Setup multi-part @@ -42,8 +42,6 @@ func createAttachment(t *testing.T, session *TestSession, repoURL, filename stri err = writer.Close() require.NoError(t, err) - csrf := GetCSRF(t, session, repoURL) - req := NewRequestWithBody(t, "POST", repoURL+"/issues/attachments", body) req.Header.Add("X-Csrf-Token", csrf) req.Header.Add("Content-Type", writer.FormDataContentType()) @@ -60,15 +58,14 @@ func createAttachment(t *testing.T, session *TestSession, repoURL, filename stri func TestCreateAnonymousAttachment(t *testing.T) { defer tests.PrepareTestEnv(t)() session := emptyTestSession(t) - // this test is not right because it just doesn't pass the CSRF validation - createAttachment(t, session, "user2/repo1", "image.png", generateImg(), http.StatusBadRequest) + createAttachment(t, session, GetCSRF(t, session, "/user/login"), "user2/repo1", "image.png", generateImg(), http.StatusSeeOther) } func TestCreateIssueAttachment(t *testing.T) { defer tests.PrepareTestEnv(t)() const repoURL = "user2/repo1" session := loginUser(t, "user2") - uuid := createAttachment(t, session, repoURL, "image.png", generateImg(), http.StatusOK) + uuid := createAttachment(t, session, GetCSRF(t, session, repoURL), repoURL, "image.png", generateImg(), http.StatusOK) req := NewRequest(t, "GET", repoURL+"/issues/new") resp := session.MakeRequest(t, req, http.StatusOK) diff --git a/tests/integration/integration_test.go b/tests/integration/integration_test.go index e43200f4cb..606df2ed1c 100644 --- a/tests/integration/integration_test.go +++ b/tests/integration/integration_test.go @@ -663,12 +663,17 @@ func VerifyJSONSchema(t testing.TB, resp *httptest.ResponseRecorder, schemaFile require.NoError(t, schemaValidation) } +// GetCSRF returns CSRF token from body +// If it fails, it means the CSRF token is not found in the response body returned by the url with the given session. +// In this case, you should find a better url to get it. func GetCSRF(t testing.TB, session *TestSession, urlStr string) string { t.Helper() req := NewRequest(t, "GET", urlStr) resp := session.MakeRequest(t, req, http.StatusOK) doc := NewHTMLParser(t, resp.Body) - return doc.GetCSRF() + csrf := doc.GetCSRF() + require.NotEmpty(t, csrf) + return csrf } func GetHTMLTitle(t testing.TB, session *TestSession, urlStr string) string { diff --git a/tests/integration/org_test.go b/tests/integration/org_test.go index a1e448be8a..f907b75c72 100644 --- a/tests/integration/org_test.go +++ b/tests/integration/org_test.go @@ -204,9 +204,7 @@ func TestTeamSearch(t *testing.T) { var results TeamSearchResults session := loginUser(t, user.Name) - csrf := GetCSRF(t, session, "/"+org.Name) req := NewRequestf(t, "GET", "/org/%s/teams/-/search?q=%s", org.Name, "_team") - req.Header.Add("X-Csrf-Token", csrf) resp := session.MakeRequest(t, req, http.StatusOK) DecodeJSON(t, resp, &results) assert.NotEmpty(t, results.Data) @@ -217,9 +215,7 @@ func TestTeamSearch(t *testing.T) { // no access if not organization member user5 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5}) session = loginUser(t, user5.Name) - csrf = GetCSRF(t, session, "/"+org.Name) req = NewRequestf(t, "GET", "/org/%s/teams/-/search?q=%s", org.Name, "team") - req.Header.Add("X-Csrf-Token", csrf) session.MakeRequest(t, req, http.StatusNotFound) } From 388e7c9719da39cd4761a8c5bf58c43ee4127ec3 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sun, 6 Oct 2024 07:35:19 +0200 Subject: [PATCH 2/9] Ensure `GetCSRF` doesn't return an empty token (#32130) (followup) Modifies the calls to createAttachment() for tests that do not exist in Gitea. --- tests/integration/issue_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/issue_test.go b/tests/integration/issue_test.go index 909242efc6..c5e24b0c00 100644 --- a/tests/integration/issue_test.go +++ b/tests/integration/issue_test.go @@ -500,7 +500,7 @@ func TestIssueCommentAttachment(t *testing.T) { link, exists := htmlDoc.doc.Find("#comment-form").Attr("action") assert.True(t, exists, "The template has changed") - uuid := createAttachment(t, session, repoURL, "image.png", generateImg(), http.StatusOK) + uuid := createAttachment(t, session, GetCSRF(t, session, repoURL), repoURL, "image.png", generateImg(), http.StatusOK) commentCount := htmlDoc.doc.Find(".comment-list .comment .render-content").Length() From b67b7c12385059898fc8cb7997755a88b3afa483 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Tue, 1 Oct 2024 09:58:55 +0800 Subject: [PATCH 3/9] Fix the logic of finding the latest pull review commit ID (#32139) Fix #31423 (cherry picked from commit f4b8f6fc40ce2869135372a5c6ec6418d27ebfba) Conflicts: models/fixtures/comment.yml comment fixtures have to be shifted because there is one more in Forgejo --- models/fixtures/comment.yml | 19 +++++++++++++ models/fixtures/review.yml | 19 +++++++++++++ models/issues/pull.go | 2 +- models/issues/review.go | 2 +- models/issues/review_list.go | 6 ++-- models/issues/review_test.go | 4 +-- routers/api/v1/repo/pull_review.go | 1 - services/pull/pull.go | 8 +++++- services/pull/review.go | 2 +- tests/integration/api_pull_review_test.go | 2 +- tests/integration/pull_commit_test.go | 34 +++++++++++++++++++++++ 11 files changed, 88 insertions(+), 11 deletions(-) create mode 100644 tests/integration/pull_commit_test.go diff --git a/models/fixtures/comment.yml b/models/fixtures/comment.yml index d2a1de6559..f4121284a6 100644 --- a/models/fixtures/comment.yml +++ b/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 diff --git a/models/fixtures/review.yml b/models/fixtures/review.yml index ac97e24c2b..0438ceadae 100644 --- a/models/fixtures/review.yml +++ b/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 diff --git a/models/issues/pull.go b/models/issues/pull.go index a035cad649..45e2e19434 100644 --- a/models/issues/pull.go +++ b/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, }) diff --git a/models/issues/review.go b/models/issues/review.go index ca6fd6035b..a39c12069b 100644 --- a/models/issues/review.go +++ b/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, }) diff --git a/models/issues/review_list.go b/models/issues/review_list.go index 0ee28874ec..a5ceb21791 100644 --- a/models/issues/review_list.go +++ b/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}) diff --git a/models/issues/review_test.go b/models/issues/review_test.go index 43dc9ed2c1..51cb940579 100644 --- a/models/issues/review_test.go +++ b/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) diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index 39e1d487fa..8fba085ff7 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/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, } diff --git a/services/pull/pull.go b/services/pull/pull.go index 82ca0d7047..6af7d8ba0c 100644 --- a/services/pull/pull.go +++ b/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) { diff --git a/services/pull/review.go b/services/pull/review.go index 011c2a3058..927c43150b 100644 --- a/services/pull/review.go +++ b/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 { diff --git a/tests/integration/api_pull_review_test.go b/tests/integration/api_pull_review_test.go index b7e5130cb8..9a395968b0 100644 --- a/tests/integration/api_pull_review_test.go +++ b/tests/integration/api_pull_review_test.go @@ -160,7 +160,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 { diff --git a/tests/integration/pull_commit_test.go b/tests/integration/pull_commit_test.go new file mode 100644 index 0000000000..477f01725d --- /dev/null +++ b/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) + }) +} From a681daaca291bc78f305b14c3efdfd84c1639999 Mon Sep 17 00:00:00 2001 From: Bruno Sofiato Date: Thu, 3 Oct 2024 13:03:36 -0300 Subject: [PATCH 4/9] Fixed race condition when deleting documents by repoId in ElasticSearch (#32185) Resolves #32184 --------- Signed-off-by: Bruno Sofiato (cherry picked from commit d266d190bd744b7b6f572bf69a42013e21b9be62) --- .../code/elasticsearch/elasticsearch.go | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/modules/indexer/code/elasticsearch/elasticsearch.go b/modules/indexer/code/elasticsearch/elasticsearch.go index d64d99433d..5c01034450 100644 --- a/modules/indexer/code/elasticsearch/elasticsearch.go +++ b/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) From 1dfe58ad11bc6fdc73a2b5ffb3c1481fbddbf46b Mon Sep 17 00:00:00 2001 From: Job Date: Fri, 4 Oct 2024 19:12:48 +0200 Subject: [PATCH 5/9] Fix PR creation on forked repositories (#31863) Resolves #20475 (cherry picked from commit 7e68bc88238104d2ee8b5a877fc1ad437f1778a4) Conflicts: tests/integration/pull_create_test.go add missing testPullCreateDirectly from c63060b130d34e3f03f28f4dccbf04d381a95c17 Fix code owners will not be mentioned when a pull request comes from a forked repository (#30476) --- routers/api/v1/repo/pull.go | 17 +++++++-- tests/integration/pull_create_test.go | 51 +++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 97937fa937..bcb6ef2581 100644 --- a/routers/api/v1/repo/pull.go +++ b/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 diff --git a/tests/integration/pull_create_test.go b/tests/integration/pull_create_test.go index 8bf48f4f6e..b814642bc7 100644 --- a/tests/integration/pull_create_test.go +++ b/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) + }) +} From af901ac7bb03d27f175f2292581fc67fa9c8d567 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Sat, 5 Oct 2024 02:45:06 +0900 Subject: [PATCH 6/9] Add support for searching users by email (#30908) Fix #30898 we have an option `SearchByEmail`, so enable it, then we can search user by email. Also added a test for it. (cherry picked from commit 5d6d025c9b8d2abca9ec2bfdc795d1f0c1c6592d) --- models/user/search.go | 14 ++++++++- routers/api/v1/user/user.go | 11 +++---- tests/integration/api_user_search_test.go | 36 +++++++++++++++++++++++ 3 files changed, 55 insertions(+), 6 deletions(-) diff --git a/models/user/search.go b/models/user/search.go index 04c434e4fa..558daf2e6a 100644 --- a/models/user/search.go +++ b/models/user/search.go @@ -69,7 +69,19 @@ func (opts *SearchUserOptions) toSearchQueryBase(ctx context.Context) *xorm.Sess builder.Like{"LOWER(full_name)", lowerKeyword}, ) if opts.SearchByEmail { - keywordCond = keywordCond.Or(builder.Like{"LOWER(email)", lowerKeyword}) + var emailCond builder.Cond + emailCond = builder.Like{"LOWER(email)", lowerKeyword} + if opts.Actor == nil { + emailCond = emailCond.And(builder.Eq{"keep_email_private": false}) + } else if !opts.Actor.IsAdmin { + emailCond = emailCond.And( + builder.Or( + builder.Eq{"keep_email_private": false}, + builder.Eq{"id": opts.Actor.ID}, + ), + ) + } + keywordCond = keywordCond.Or(emailCond) } cond = cond.And(keywordCond) diff --git a/routers/api/v1/user/user.go b/routers/api/v1/user/user.go index 831c1436da..0847b4079b 100644 --- a/routers/api/v1/user/user.go +++ b/routers/api/v1/user/user.go @@ -69,11 +69,12 @@ func Search(ctx *context.APIContext) { users = []*user_model.User{user_model.NewActionsUser()} default: users, maxResults, err = user_model.SearchUsers(ctx, &user_model.SearchUserOptions{ - Actor: ctx.Doer, - Keyword: ctx.FormTrim("q"), - UID: uid, - Type: user_model.UserTypeIndividual, - ListOptions: listOptions, + Actor: ctx.Doer, + Keyword: ctx.FormTrim("q"), + UID: uid, + Type: user_model.UserTypeIndividual, + SearchByEmail: true, + ListOptions: listOptions, }) if err != nil { ctx.JSON(http.StatusInternalServerError, map[string]any{ diff --git a/tests/integration/api_user_search_test.go b/tests/integration/api_user_search_test.go index 2ecd338987..eb2b6f45f5 100644 --- a/tests/integration/api_user_search_test.go +++ b/tests/integration/api_user_search_test.go @@ -130,3 +130,39 @@ func TestAPIUserSearchNotLoggedInUserHidden(t *testing.T) { DecodeJSON(t, resp, &results) assert.Empty(t, results.Data) } + +func TestAPIUserSearchByEmail(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + // admin can search user with private email + adminUsername := "user1" + session := loginUser(t, adminUsername) + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadUser) + query := "user2@example.com" + req := NewRequestf(t, "GET", "/api/v1/users/search?q=%s", query). + AddTokenAuth(token) + resp := MakeRequest(t, req, http.StatusOK) + + var results SearchResults + DecodeJSON(t, resp, &results) + assert.Equal(t, 1, len(results.Data)) + assert.Equal(t, query, results.Data[0].Email) + + // no login user can not search user with private email + req = NewRequestf(t, "GET", "/api/v1/users/search?q=%s", query) + resp = MakeRequest(t, req, http.StatusOK) + DecodeJSON(t, resp, &results) + assert.Empty(t, results.Data) + + // user can search self with private email + user2 := "user2" + session = loginUser(t, user2) + token = getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadUser) + req = NewRequestf(t, "GET", "/api/v1/users/search?q=%s", query). + AddTokenAuth(token) + resp = MakeRequest(t, req, http.StatusOK) + + DecodeJSON(t, resp, &results) + assert.Equal(t, 1, len(results.Data)) + assert.Equal(t, query, results.Data[0].Email) +} From 122baf4de82da1b54d4883a8d738344f8ff065c3 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sun, 6 Oct 2024 08:21:41 +0200 Subject: [PATCH 7/9] chore(release-notes): weekly cherry-pick week 2024-41 --- release-notes/5477.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 release-notes/5477.md diff --git a/release-notes/5477.md b/release-notes/5477.md new file mode 100644 index 0000000000..3f735e043f --- /dev/null +++ b/release-notes/5477.md @@ -0,0 +1,3 @@ +feat: [commit](https://codeberg.org/forgejo/forgejo/commit/af901ac7bb03d27f175f2292581fc67fa9c8d567) Add support for searching users by email. +fix: [commit](https://codeberg.org/forgejo/forgejo/commit/1dfe58ad11bc6fdc73a2b5ffb3c1481fbddbf46b) PR creation on forked repositories. +fix: [commit](https://codeberg.org/forgejo/forgejo/commit/b67b7c12385059898fc8cb7997755a88b3afa483) the logic of finding the latest pull review commit ID is incorrect. From 1bee6fa839e0d2000abef39bb6c9fbd6483f864f Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sun, 6 Oct 2024 08:28:43 +0200 Subject: [PATCH 8/9] Add support for searching users by email (#30908) (testifylint) --- tests/integration/api_user_search_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/api_user_search_test.go b/tests/integration/api_user_search_test.go index eb2b6f45f5..63c1f86f27 100644 --- a/tests/integration/api_user_search_test.go +++ b/tests/integration/api_user_search_test.go @@ -145,7 +145,7 @@ func TestAPIUserSearchByEmail(t *testing.T) { var results SearchResults DecodeJSON(t, resp, &results) - assert.Equal(t, 1, len(results.Data)) + assert.Len(t, results.Data, 1) assert.Equal(t, query, results.Data[0].Email) // no login user can not search user with private email @@ -163,6 +163,6 @@ func TestAPIUserSearchByEmail(t *testing.T) { resp = MakeRequest(t, req, http.StatusOK) DecodeJSON(t, resp, &results) - assert.Equal(t, 1, len(results.Data)) + assert.Len(t, results.Data, 1) assert.Equal(t, query, results.Data[0].Email) } From 6488d15860948f8e65a51ec4ecc43f2da20391a2 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sun, 6 Oct 2024 10:00:09 +0200 Subject: [PATCH 9/9] Fix the logic of finding the latest pull review commit ID (#32139) (followup) Adjust the tests for review deletion to ignore a newly inserted fixture. It is a review request and cannot be deleted. --- tests/integration/api_pull_review_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/integration/api_pull_review_test.go b/tests/integration/api_pull_review_test.go index 9a395968b0..b66e65ee41 100644 --- a/tests/integration/api_pull_review_test.go +++ b/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) }) } }