From e86f5ef7d488b8d40656e58d35fdb74aa262ab3a Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Sat, 27 Dec 2025 15:57:25 +0100 Subject: [PATCH] fix: allow Actions trust management on conflicted PRs (#10594) Fixes #10589. ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests - I added test coverage for Go changes... - [ ] in their respective `*_test.go` for unit tests. - [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] I do not want this change to show in the release notes. - [x] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/.md` to be be used for the release notes instead of the title. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10594 Reviewed-by: Michael Kriese Co-authored-by: Mathieu Fenniak Co-committed-by: Mathieu Fenniak --- templates/repo/issue/view_content/pull.tmpl | 1 + tests/integration/actions_trust_test.go | 91 ++++++++++++++++++++- tests/integration/html_helper.go | 8 ++ 3 files changed, 96 insertions(+), 4 deletions(-) diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 21f0d613ed..3a6a306b12 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -77,6 +77,7 @@
  • {{.}}
    • {{end}} + {{template "repo/pulls/trust" .}} {{else if .IsPullRequestBroken}}
    {{svg "octicon-x"}} diff --git a/tests/integration/actions_trust_test.go b/tests/integration/actions_trust_test.go index 7a683d5625..651700cd26 100644 --- a/tests/integration/actions_trust_test.go +++ b/tests/integration/actions_trust_test.go @@ -20,6 +20,7 @@ import ( actions_module "forgejo.org/modules/actions" "forgejo.org/modules/git" "forgejo.org/modules/structs" + "forgejo.org/modules/translation" actions_service "forgejo.org/services/actions" pull_service "forgejo.org/services/pull" repo_service "forgejo.org/services/repository" @@ -68,6 +69,32 @@ func actionsTrustTestAssertNoTrustPanel(t *testing.T, session *TestSession, url actionsTrustTestAssertTrustPanelPresence(t, session, url, false) } +func actionsTrustTestAssertPRIsWIP(t *testing.T, session *TestSession, url string) { + t.Helper() + + req := NewRequest(t, "GET", url) + resp := session.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + + locale := translation.NewLocale("en-US") + assert.Equal(t, 1, htmlDoc.FindByTextTrim("div", locale.TrString("repo.pulls.cannot_merge_work_in_progress")).Length()) +} + +func actionsTrustTestAssertPRConflicted(t *testing.T, session *TestSession, url string) { + t.Helper() + + req := NewRequest(t, "GET", url) + resp := session.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + + locale := translation.NewLocale("en-US") + + // ....Eventually is used because conflict checking is async and may not complete immediately. + require.Eventually(t, func() bool { + return htmlDoc.FindByTextTrim("div", locale.TrString("repo.pulls.files_conflicted")).Length() == 1 + }, 5*time.Second, time.Millisecond*100) +} + func actionsTrustTestCreateBaseRepo(t *testing.T, owner *user_model.User) (*repo_model.Repository, func()) { t.Helper() @@ -127,13 +154,19 @@ func actionsTrustTestRequireRun(t *testing.T, repo *repo_model.Repository, modif func actionsTrustTestRepoCreateBranch(t *testing.T, doer *user_model.User, repo *repo_model.Repository) *structs.FilesResponse { t.Helper() - return actionsTrustTestModifyRepo(t, doer, repo, "file_in_fork.txt", "main", "fork-branch-1") + return actionsTrustTestModifyRepo(t, doer, repo, "file_in_fork.txt", "main", "fork-branch-1", "content") +} + +func actionsTrustMakePRConflicted(t *testing.T, doer *user_model.User, repo *repo_model.Repository) *structs.FilesResponse { + t.Helper() + + return actionsTrustTestModifyRepo(t, doer, repo, "file_in_fork.txt", "main", "main", "conflicting content") } func actionsTrustTestRepoModify(t *testing.T, doer *user_model.User, baseRepo, headRepo *repo_model.Repository, filename string) *structs.FilesResponse { t.Helper() - modified := actionsTrustTestModifyRepo(t, doer, headRepo, filename, "fork-branch-1", "fork-branch-1") + modified := actionsTrustTestModifyRepo(t, doer, headRepo, filename, "fork-branch-1", "fork-branch-1", "content") // the creation of the run is not synchronous require.Eventually(t, func() bool { return unittest.BeanExists(t, &actions_model.ActionRun{RepoID: baseRepo.ID, CommitSHA: modified.Commit.SHA}) @@ -141,7 +174,7 @@ func actionsTrustTestRepoModify(t *testing.T, doer *user_model.User, baseRepo, h return modified } -func actionsTrustTestModifyRepo(t *testing.T, doer *user_model.User, repo *repo_model.Repository, filename, oldBranch, newBranch string) *structs.FilesResponse { +func actionsTrustTestModifyRepo(t *testing.T, doer *user_model.User, repo *repo_model.Repository, filename, oldBranch, newBranch, content string) *structs.FilesResponse { t.Helper() // add a new file to the forked repo @@ -150,7 +183,7 @@ func actionsTrustTestModifyRepo(t *testing.T, doer *user_model.User, repo *repo_ { Operation: "create", TreePath: filename, - ContentReader: strings.NewReader("content"), + ContentReader: strings.NewReader(content), }, }, Message: "add " + filename, @@ -220,6 +253,23 @@ func actionsTrustTestCreatePullRequestFromForkedRepo(t *testing.T, baseUser *use return forkedRepo, pullRequest, addFileToForkedResp } +// Mark the PR as a work-in-progress PR +func actionsTrustTestSetPullRequestWIP(t *testing.T, pullRequest *issues_model.PullRequest, wip bool) { + t.Helper() + newTitle := pullRequest.Issue.Title + if wip && !pullRequest.IsWorkInProgress(t.Context()) { + newTitle = fmt.Sprintf("WIP: %s", pullRequest.Issue.Title) + } else if !wip { + prefix := pullRequest.GetWorkInProgressPrefix(t.Context()) + newTitle = pullRequest.Issue.Title[len(prefix):] + } + pullRequest.Issue.Title = newTitle + require.NoError(t, issues_model.UpdateIssueCols(t.Context(), pullRequest.Issue, "name")) + + pullRequest.Issue = nil + require.NoError(t, pullRequest.LoadIssue(t.Context())) +} + func TestActionsPullRequestTrustPanel(t *testing.T) { onApplicationRun(t, func(t *testing.T, u *url.URL) { ownerUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) // owner of the repo @@ -353,3 +403,36 @@ func TestActionsPullRequestTrustPanel(t *testing.T) { }) }) } + +func TestActionsPullRequestTrustPanelWIPConflicts(t *testing.T) { + onApplicationRun(t, func(t *testing.T, u *url.URL) { + ownerUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) // owner of the repo + + regularUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5}) // a regular user with no specific permission + regularSession := loginUser(t, regularUser.Name) + + userAdmin := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) // the instance admin + adminSession := loginUser(t, userAdmin.Name) + + baseRepo, f := actionsTrustTestCreateBaseRepo(t, ownerUser) + defer f() + + _, pullRequest, _ := actionsTrustTestCreatePullRequestFromForkedRepo(t, ownerUser, baseRepo, regularUser) + pullRequestLink := pullRequest.Issue.Link() + + actionsTrustTestSetPullRequestWIP(t, pullRequest, true) + actionsTrustTestAssertPRIsWIP(t, adminSession, pullRequestLink) + + t.Run("Regular user sees pending approval even though PR is a WIP PR", func(t *testing.T) { + actionsTrustTestAssertTrustPanel(t, regularSession, pullRequestLink) + }) + + actionsTrustTestSetPullRequestWIP(t, pullRequest, false) + _ = actionsTrustMakePRConflicted(t, userAdmin, baseRepo) + actionsTrustTestAssertPRConflicted(t, adminSession, pullRequestLink) + + t.Run("Regular user sees pending approval even though PR is conflicted", func(t *testing.T) { + actionsTrustTestAssertTrustPanel(t, regularSession, pullRequestLink) + }) + }) +} diff --git a/tests/integration/html_helper.go b/tests/integration/html_helper.go index bcf63e9730..e793009ce0 100644 --- a/tests/integration/html_helper.go +++ b/tests/integration/html_helper.go @@ -6,6 +6,7 @@ package integration import ( "bytes" "fmt" + "strings" "testing" "github.com/PuerkitoBio/goquery" @@ -108,6 +109,13 @@ func (doc *HTMLDoc) FindByText(selector, text string) *goquery.Selection { }) } +// FindByText gets all elements by selector that also has the given text, w/ leading & trailing whitespace trimmed +func (doc *HTMLDoc) FindByTextTrim(selector, text string) *goquery.Selection { + return doc.doc.Find(selector).FilterFunction(func(i int, s *goquery.Selection) bool { + return strings.TrimSpace(s.Text()) == text + }) +} + // AssertSelection check if selection exists or does not exist depending on checkExists func (doc *HTMLDoc) AssertSelection(t testing.TB, selection *goquery.Selection, checkExists bool) { if checkExists {