[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 <contact@earl-warren.org>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8455
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Co-authored-by: forgejo-backport-action <forgejo-backport-action@noreply.codeberg.org>
Co-committed-by: forgejo-backport-action <forgejo-backport-action@noreply.codeberg.org>

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",

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`
 

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

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

templates/mail/actions/now_done.tmpl

@@ -21,7 +21,7 @@
 		<br />
 
 		{{.locale.Tr "mail.actions.run_info_cur_status" .Run.Status .PriorStatus}}<br />
-		{{.locale.Tr "mail.actions.run_info_ref" .Branch .CommitSHA}}<br />
+		{{.locale.Tr "mail.actions.run_info_sha" .CommitSHA}}<br />
 		{{if .LastRun}}
 			{{.locale.Tr "mail.actions.run_info_previous_status" .LastRun.Status}}<br />
 		{{end}}

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