Compare commits

...

3 commits

Author SHA1 Message Date
Earl Warren
32e8610b20 fix(email): actions notification template confuses branch with PR (#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.

## 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.
  - [ ] 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.
- [ ] 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.
- [ ] 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/<pull request number>.md` to be be used for the release notes instead of the title.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8448
Reviewed-by: Christopher Besch <mail@chris-besch.com>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>
2025-07-09 09:43:33 +02:00
Gusted
11934670ec fix: correctly mark reviews as stale for AGit PRs (#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.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8450
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Co-authored-by: Gusted <postmaster@gusted.xyz>
Co-committed-by: Gusted <postmaster@gusted.xyz>
2025-07-09 07:38:00 +02:00
Renovate Bot
0636b3087f Update dependency go to v1.24.5 (forgejo) (#8449)
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [go](https://go.dev/) ([source](https://github.com/golang/go)) | toolchain | patch | `1.24.4` -> `1.24.5` |

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - Between 12:00 AM and 03:59 AM ( * 0-3 * * * ) (UTC).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0MS4yMy4yIiwidXBkYXRlZEluVmVyIjoiNDEuMjMuMiIsInRhcmdldEJyYW5jaCI6ImZvcmdlam8iLCJsYWJlbHMiOlsiZGVwZW5kZW5jeS11cGdyYWRlIiwidGVzdC9ub3QtbmVlZGVkIl19-->

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8449
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Co-authored-by: Renovate Bot <forgejo-renovate-action@forgejo.org>
Co-committed-by: Renovate Bot <forgejo-renovate-action@forgejo.org>
2025-07-09 07:32:03 +02:00
7 changed files with 164 additions and 7 deletions

2
go.mod
View file

@ -2,7 +2,7 @@ module forgejo.org
go 1.24
toolchain go1.24.4
toolchain go1.24.5
require (
code.forgejo.org/f3/gof3/v3 v3.11.0

View file

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

View file

@ -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`

View file

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

View file

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

View file

@ -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}}

View file

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