From a1e3bace727653787eb05a4797bf1e1360ea9c2e Mon Sep 17 00:00:00 2001 From: forgejo-backport-action Date: Wed, 9 Jul 2025 08:13:05 +0200 Subject: [PATCH 1/2] [v12.0/forgejo] fix: correctly mark reviews as stale for AGit PRs (#8454) **Backport:** https://codeberg.org/forgejo/forgejo/pulls/8450 The argument order for `ValidatePullRequest` is to first give the new commitID and then the old commit ID. This results in reviews not being marked as stale when they are not stale and reviews as not stale when they are stale. The test will fail if the fix is not present. Add testing for the following three scenarios: 1. A review is made, the PR is updated and as a consequence the PR's diff is changed. The review is now marked as stale. 2. A review is made but in the meantime the PR is updated and the review is submitted on a older commit ID. If the diff changed the review is marked as stale. 3. A review that was made against a older commit ID is no longer marked as stale if the PR is force-pushed to that older commit ID. Co-authored-by: Gusted Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8454 Reviewed-by: Earl Warren Co-authored-by: forgejo-backport-action Co-committed-by: forgejo-backport-action --- services/agit/agit.go | 2 +- tests/integration/pull_review_test.go | 159 ++++++++++++++++++++++++++ 2 files changed, 160 insertions(+), 1 deletion(-) diff --git a/services/agit/agit.go b/services/agit/agit.go index 20e87642c3..8ef641629a 100644 --- a/services/agit/agit.go +++ b/services/agit/agit.go @@ -221,7 +221,7 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git. } // Validate pull request. - pull_service.ValidatePullRequest(ctx, pr, oldCommitID, opts.NewCommitIDs[i], pusher) + pull_service.ValidatePullRequest(ctx, pr, opts.NewCommitIDs[i], oldCommitID, pusher) // TODO: call `InvalidateCodeComments` diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 603252f45f..4af0a1100e 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -5,19 +5,23 @@ package integration import ( + "bytes" "fmt" "io" "net/http" "net/http/httptest" "net/url" + "os" "path" "strconv" "strings" "testing" + "time" "forgejo.org/models/db" issues_model "forgejo.org/models/issues" repo_model "forgejo.org/models/repo" + unit_model "forgejo.org/models/unit" "forgejo.org/models/unittest" user_model "forgejo.org/models/user" "forgejo.org/modules/git" @@ -761,3 +765,158 @@ func updateFileInBranch(user *user_model.User, repo *repo_model.Repository, tree _, err = files_service.ChangeRepoFiles(git.DefaultContext, repo, user, opts) return err } + +func TestPullRequestStaleReview(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + session := loginUser(t, user2.Name) + + // Create temporary repository. + repo, _, f := tests.CreateDeclarativeRepo(t, user2, "", + []unit_model.Type{unit_model.TypePullRequests}, nil, + []*files_service.ChangeRepoFile{ + { + Operation: "create", + TreePath: "FUNFACT", + ContentReader: strings.NewReader("Smithy was the runner up to be Forgejo's name"), + }, + }, + ) + defer f() + + // Clone it. + dstPath := t.TempDir() + r := fmt.Sprintf("%suser2/%s.git", u.String(), repo.Name) + cloneURL, _ := url.Parse(r) + cloneURL.User = url.UserPassword("user2", userPassword) + require.NoError(t, git.CloneWithArgs(t.Context(), nil, cloneURL.String(), dstPath, git.CloneRepoOptions{})) + + // Create first commit. + require.NoError(t, os.WriteFile(path.Join(dstPath, "README.md"), []byte("## test content"), 0o600)) + require.NoError(t, git.AddChanges(dstPath, true)) + require.NoError(t, git.CommitChanges(dstPath, git.CommitChangesOptions{ + Committer: &git.Signature{ + Email: "user2@example.com", + Name: "user2", + When: time.Now(), + }, + Author: &git.Signature{ + Email: "user2@example.com", + Name: "user2", + When: time.Now(), + }, + Message: "Add README.", + })) + stdout := &bytes.Buffer{} + require.NoError(t, git.NewCommand(t.Context(), "rev-parse", "HEAD").Run(&git.RunOpts{Dir: dstPath, Stdout: stdout})) + firstCommitID := strings.TrimSpace(stdout.String()) + + // Create agit PR. + require.NoError(t, git.NewCommand(t.Context(), "push", "origin", "HEAD:refs/for/main", "-o", "topic=agit-pr").Run(&git.RunOpts{Dir: dstPath})) + + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{Index: 1, BaseRepoID: repo.ID}) + + req := NewRequest(t, "GET", "/"+repo.FullName()+"/pulls/1/files/reviews/new_comment") + resp := session.MakeRequest(t, req, http.StatusOK) + doc := NewHTMLParser(t, resp.Body) + + t.Run("Mark review as stale", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + // Create a approved review against against this commit. + req = NewRequestWithValues(t, "POST", "/"+repo.FullName()+"/pulls/1/files/reviews/comments", map[string]string{ + "_csrf": doc.GetCSRF(), + "origin": doc.GetInputValueByName("origin"), + "latest_commit_id": firstCommitID, + "side": "proposed", + "line": "1", + "path": "FUNFACT", + "diff_start_cid": doc.GetInputValueByName("diff_start_cid"), + "diff_end_cid": doc.GetInputValueByName("diff_end_cid"), + "diff_base_cid": doc.GetInputValueByName("diff_base_cid"), + "content": "nitpicking comment", + "pending_review": "", + }) + session.MakeRequest(t, req, http.StatusOK) + + req = NewRequestWithValues(t, "POST", "/"+repo.FullName()+"/pulls/1/files/reviews/submit", map[string]string{ + "_csrf": doc.GetCSRF(), + "commit_id": firstCommitID, + "content": "looks good", + "type": "comment", + }) + session.MakeRequest(t, req, http.StatusOK) + + // Review is not stale. + review := unittest.AssertExistsAndLoadBean(t, &issues_model.Review{IssueID: pr.IssueID}) + assert.False(t, review.Stale) + + // Create second commit + require.NoError(t, os.WriteFile(path.Join(dstPath, "README.md"), []byte("## I prefer this heading"), 0o600)) + require.NoError(t, git.AddChanges(dstPath, true)) + require.NoError(t, git.CommitChanges(dstPath, git.CommitChangesOptions{ + Committer: &git.Signature{ + Email: "user2@example.com", + Name: "user2", + When: time.Now(), + }, + Author: &git.Signature{ + Email: "user2@example.com", + Name: "user2", + When: time.Now(), + }, + Message: "Add README.", + })) + + // Push to agit PR. + require.NoError(t, git.NewCommand(t.Context(), "push", "origin", "HEAD:refs/for/main", "-o", "topic=agit-pr").Run(&git.RunOpts{Dir: dstPath})) + + // Review is stale. + review = unittest.AssertExistsAndLoadBean(t, &issues_model.Review{IssueID: pr.IssueID}) + assert.True(t, review.Stale) + }) + + t.Run("Create stale review", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + // Review based on the first commit, which is a stale review because the + // PR's head is at the seconnd commit. + req := NewRequestWithValues(t, "POST", "/"+repo.FullName()+"/pulls/1/files/reviews/submit", map[string]string{ + "_csrf": doc.GetCSRF(), + "commit_id": firstCommitID, + "content": "looks good", + "type": "approve", + }) + session.MakeRequest(t, req, http.StatusOK) + + // There does not exist a review that is not stale, because all reviews + // are based on the first commit and the PR's head is at the second commit. + unittest.AssertExistsIf(t, false, &issues_model.Review{IssueID: pr.IssueID}, "stale = false") + }) + + t.Run("Mark unstale", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + // Force push the PR to the first commit. + require.NoError(t, git.NewCommand(t.Context(), "reset", "--hard", "HEAD~1").Run(&git.RunOpts{Dir: dstPath})) + require.NoError(t, git.NewCommand(t.Context(), "push", "origin", "HEAD:refs/for/main", "-o", "topic=agit-pr", "-o", "force-push").Run(&git.RunOpts{Dir: dstPath})) + + // There does not exist a review that is stale, because all reviews + // are based on the first commit and thus all reviews are no longer marked + // as stale. + unittest.AssertExistsIf(t, false, &issues_model.Review{IssueID: pr.IssueID}, "stale = true") + }) + + t.Run("Diff did not change", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + // Create a empty commit and push it to the PR. + require.NoError(t, git.NewCommand(t.Context(), "commit", "--allow-empty", "-m", "Empty commit").Run(&git.RunOpts{Dir: dstPath})) + require.NoError(t, git.NewCommand(t.Context(), "push", "origin", "HEAD:refs/for/main", "-o", "topic=agit-pr").Run(&git.RunOpts{Dir: dstPath})) + + // There does not exist a review that is stale, because the diff did not + // change. + unittest.AssertExistsIf(t, false, &issues_model.Review{IssueID: pr.IssueID}, "stale = true") + }) + }) +} From eb543dcbdb7dcce4bbcedea17cd55299aa05cf71 Mon Sep 17 00:00:00 2001 From: forgejo-backport-action Date: Wed, 9 Jul 2025 10:20:54 +0200 Subject: [PATCH 2/2] [v12.0/forgejo] fix(email): actions notification template confuses branch with PR (#8455) **Backport:** https://codeberg.org/forgejo/forgejo/pulls/8448 When a mail notification is sent because of a failed pull request run, the body will show: Branch: #661 (f57df45) where #661 is the number of the pull request and not the branch. This is because run.PrettyRef() is used and has a misleading special case returning a PR number instead of a ref. Remove the textual description as it can easily be discovered from the run web page linked in the mail. Co-authored-by: Earl Warren Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8455 Reviewed-by: Earl Warren Co-authored-by: forgejo-backport-action Co-committed-by: forgejo-backport-action --- options/locale_next/locale_en-US.json | 2 +- services/mailer/mail_actions.go | 2 -- services/mailer/mail_actions_now_done_test.go | 2 +- templates/mail/actions/now_done.tmpl | 2 +- 4 files changed, 3 insertions(+), 5 deletions(-) diff --git a/options/locale_next/locale_en-US.json b/options/locale_next/locale_en-US.json index b1c98e4551..4f1c3904e2 100644 --- a/options/locale_next/locale_en-US.json +++ b/options/locale_next/locale_en-US.json @@ -92,7 +92,7 @@ "mail.actions.not_successful_run": "Workflow %[1]s failed in repository %[2]s", "mail.actions.run_info_cur_status": "This Run's Status: %[1]s (just updated from %[2]s)", "mail.actions.run_info_previous_status": "Previous Run's Status: %[1]s", - "mail.actions.run_info_ref": "Branch: %[1]s (%[2]s)", + "mail.actions.run_info_sha": "Commit: %[1]s", "mail.actions.run_info_trigger": "Triggered because: %[1]s by: %[2]s", "repo.diff.commit.next-short": "Next", "repo.diff.commit.previous-short": "Prev", diff --git a/services/mailer/mail_actions.go b/services/mailer/mail_actions.go index fa0d2635f1..a99af823b3 100644 --- a/services/mailer/mail_actions.go +++ b/services/mailer/mail_actions.go @@ -60,7 +60,6 @@ func sendMailActionRun(to *user_model.User, run *actions_model.ActionRun, priorS if len(commitSHA) > 7 { commitSHA = commitSHA[:7] } - branch := run.PrettyRef() data := map[string]any{ "locale": locale, @@ -73,7 +72,6 @@ func sendMailActionRun(to *user_model.User, run *actions_model.ActionRun, priorS "LastRun": lastRun, "PriorStatus": priorStatus, "CommitSHA": commitSHA, - "Branch": branch, "IsSuccess": run.Status.IsSuccess(), } diff --git a/services/mailer/mail_actions_now_done_test.go b/services/mailer/mail_actions_now_done_test.go index f4c597c99c..e84441f460 100644 --- a/services/mailer/mail_actions_now_done_test.go +++ b/services/mailer/mail_actions_now_done_test.go @@ -54,7 +54,7 @@ func getActionsNowDoneTestOrg(t *testing.T, name, email string, owner *user_mode } func assertTranslatedLocaleMailActionsNowDone(t *testing.T, msgBody string) { - AssertTranslatedLocale(t, msgBody, "mail.actions.successful_run_after_failure", "mail.actions.not_successful_run", "mail.actions.run_info_cur_status", "mail.actions.run_info_ref", "mail.actions.run_info_previous_status", "mail.actions.run_info_trigger", "mail.view_it_on") + AssertTranslatedLocale(t, msgBody, "mail.actions.successful_run_after_failure", "mail.actions.not_successful_run", "mail.actions.run_info_cur_status", "mail.actions.run_info_sha", "mail.actions.run_info_previous_status", "mail.actions.run_info_trigger", "mail.view_it_on") } func TestActionRunNowDoneStatusMatrix(t *testing.T) { diff --git a/templates/mail/actions/now_done.tmpl b/templates/mail/actions/now_done.tmpl index a890411055..adc990c545 100644 --- a/templates/mail/actions/now_done.tmpl +++ b/templates/mail/actions/now_done.tmpl @@ -21,7 +21,7 @@
{{.locale.Tr "mail.actions.run_info_cur_status" .Run.Status .PriorStatus}}
- {{.locale.Tr "mail.actions.run_info_ref" .Branch .CommitSHA}}
+ {{.locale.Tr "mail.actions.run_info_sha" .CommitSHA}}
{{if .LastRun}} {{.locale.Tr "mail.actions.run_info_previous_status" .LastRun.Status}}
{{end}}