diff --git a/release-notes/11365.md b/release-notes/11365.md new file mode 100644 index 0000000000..15e17a2054 --- /dev/null +++ b/release-notes/11365.md @@ -0,0 +1,3 @@ +fix: edit raw instead of rendered Git notes when editing notes on commit pages +fix: make editing Git notes from single-commit PR page actually work +feat: add cancel button to Git note adding and editing diff --git a/routers/web/repo/commit.go b/routers/web/repo/commit.go index 9968ccd018..3db8a091b0 100644 --- a/routers/web/repo/commit.go +++ b/routers/web/repo/commit.go @@ -407,6 +407,7 @@ func Diff(ctx *context.Context) { if err == nil { ctx.Data["NoteCommit"] = note.Commit ctx.Data["NoteAuthor"] = user_model.ValidateCommitWithEmail(ctx, note.Commit) + ctx.Data["NoteRaw"] = string(charset.ToUTF8WithFallback(note.Message, charset.ConvertOpts{})) ctx.Data["NoteRendered"], err = markup.RenderCommitMessage(&markup.RenderContext{ Links: markup.Links{ Base: ctx.Repo.RepoLink, @@ -463,7 +464,7 @@ func RawDiff(ctx *context.Context) { } } -func SetCommitNotes(ctx *context.Context) { +func setCommitNotes(ctx *context.Context, redirectURL string) { form := web.GetForm(ctx).(*forms.CommitNotesForm) commitID := ctx.Params(":sha") @@ -474,10 +475,14 @@ func SetCommitNotes(ctx *context.Context) { return } - ctx.Redirect(fmt.Sprintf("%s/commit/%s", ctx.Repo.Repository.HTMLURL(), commitID)) + ctx.Redirect(redirectURL) } -func RemoveCommitNotes(ctx *context.Context) { +func SetCommitNotes(ctx *context.Context) { + setCommitNotes(ctx, ctx.Repo.Repository.CommitLink(ctx.Params(":sha"))) +} + +func removeCommitNotes(ctx *context.Context, redirectURL string) { commitID := ctx.Params(":sha") err := git.RemoveNote(ctx, ctx.Repo.GitRepo, commitID) @@ -486,5 +491,9 @@ func RemoveCommitNotes(ctx *context.Context) { return } - ctx.Redirect(fmt.Sprintf("%s/commit/%s", ctx.Repo.Repository.HTMLURL(), commitID)) + ctx.Redirect(redirectURL) +} + +func RemoveCommitNotes(ctx *context.Context) { + removeCommitNotes(ctx, ctx.Repo.Repository.CommitLink(ctx.Params(":sha"))) } diff --git a/routers/web/repo/commit_test.go b/routers/web/repo/commit_test.go new file mode 100644 index 0000000000..eb4c1e3367 --- /dev/null +++ b/routers/web/repo/commit_test.go @@ -0,0 +1,58 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package repo + +import ( + "net/http" + "testing" + + "forgejo.org/models/unittest" + "forgejo.org/modules/git" + "forgejo.org/modules/test" + "forgejo.org/modules/web" + "forgejo.org/services/contexttest" + "forgejo.org/services/forms" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSetCommitNotes(t *testing.T) { + unittest.PrepareTestEnv(t) + commitID := "65f1bf27bc3bf70f64657658635e66094edbcb4d" + path := "/user2/repo1/commit/" + commitID + ctx, _ := contexttest.MockContext(t, path) + ctx.SetParams(":sha", commitID) + contexttest.LoadUser(t, ctx, 2) + contexttest.LoadRepo(t, ctx, 1) + contexttest.LoadGitRepo(t, ctx) + notes := `This is a new note.\nSee https://frogejo.org.` + web.SetForm(ctx, &forms.CommitNotesForm{ + Notes: notes, + }) + SetCommitNotes(ctx) + assert.Equal(t, http.StatusSeeOther, ctx.Resp.Status()) + assert.Equal(t, path, test.RedirectURL(ctx.Resp)) + note, err := git.GetNote(ctx, ctx.Repo.GitRepo, commitID) + require.NoError(t, err) + assert.Equal(t, []byte(notes+"\n"), note.Message) +} + +func TestRemoveCommitNotes(t *testing.T) { + unittest.PrepareTestEnv(t) + commitID := "65f1bf27bc3bf70f64657658635e66094edbcb4d" + path := "/user2/repo1/commit/" + commitID + ctx, _ := contexttest.MockContext(t, path) + ctx.SetParams(":sha", commitID) + contexttest.LoadUser(t, ctx, 2) + contexttest.LoadRepo(t, ctx, 1) + contexttest.LoadGitRepo(t, ctx) + RemoveCommitNotes(ctx) + assert.Equal(t, http.StatusSeeOther, ctx.Resp.Status()) + assert.Equal(t, path, test.RedirectURL(ctx.Resp)) + note, err := git.GetNote(ctx, ctx.Repo.GitRepo, commitID) + require.Error(t, err) + assert.True(t, git.IsErrNotExist(err)) + assert.Nil(t, note) +} diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 781a001fba..83e3a3cef3 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1008,6 +1008,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi if err == nil { ctx.Data["NoteCommit"] = note.Commit ctx.Data["NoteAuthor"] = user_model.ValidateCommitWithEmail(ctx, note.Commit) + ctx.Data["NoteRaw"] = string(charset.ToUTF8WithFallback(note.Message, charset.ConvertOpts{})) ctx.Data["NoteRendered"], err = markup.RenderCommitMessage(&markup.RenderContext{ Links: markup.Links{ Base: ctx.Repo.RepoLink, @@ -2018,3 +2019,11 @@ func UpdateTrustWithPullRequestActions(ctx *context.Context) { ctx.Redirect(fmt.Sprintf("%s#pull-request-trust-panel", pr.Issue.Link())) } + +func SetCommitNotesPullRequest(ctx *context.Context) { + setCommitNotes(ctx, fmt.Sprintf("%s/pulls/%s/commits/%s", ctx.Repo.Repository.Link(), ctx.Params(":index"), ctx.Params(":sha"))) +} + +func RemoveCommitNotesPullRequest(ctx *context.Context) { + removeCommitNotes(ctx, fmt.Sprintf("%s/pulls/%s/commits/%s", ctx.Repo.Repository.Link(), ctx.Params(":index"), ctx.Params(":sha"))) +} diff --git a/routers/web/repo/pull_test.go b/routers/web/repo/pull_test.go new file mode 100644 index 0000000000..ae7f78f810 --- /dev/null +++ b/routers/web/repo/pull_test.go @@ -0,0 +1,62 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package repo + +import ( + "net/http" + "testing" + + "forgejo.org/models/unittest" + "forgejo.org/modules/git" + "forgejo.org/modules/test" + "forgejo.org/modules/web" + "forgejo.org/services/contexttest" + "forgejo.org/services/forms" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSetCommitNotesPullRequest(t *testing.T) { + unittest.PrepareTestEnv(t) + commitID := "65f1bf27bc3bf70f64657658635e66094edbcb4d" + pullID := "5" + path := "/user2/repo1/pulls/" + pullID + "/commits/" + commitID + ctx, _ := contexttest.MockContext(t, path) + ctx.SetParams(":sha", commitID) + ctx.SetParams(":index", pullID) + contexttest.LoadUser(t, ctx, 2) + contexttest.LoadRepo(t, ctx, 1) + contexttest.LoadGitRepo(t, ctx) + notes := `This is a new note.\nSee https://frogejo.org.` + web.SetForm(ctx, &forms.CommitNotesForm{ + Notes: notes, + }) + SetCommitNotesPullRequest(ctx) + assert.Equal(t, http.StatusSeeOther, ctx.Resp.Status()) + assert.Equal(t, path, test.RedirectURL(ctx.Resp)) + note, err := git.GetNote(ctx, ctx.Repo.GitRepo, commitID) + require.NoError(t, err) + assert.Equal(t, []byte(notes+"\n"), note.Message) +} + +func TestRemoveCommitNotesPullRequest(t *testing.T) { + unittest.PrepareTestEnv(t) + commitID := "65f1bf27bc3bf70f64657658635e66094edbcb4d" + pullID := "5" + path := "/user2/repo1/pulls/" + pullID + "/commits/" + commitID + ctx, _ := contexttest.MockContext(t, path) + ctx.SetParams(":sha", commitID) + ctx.SetParams(":index", pullID) + contexttest.LoadUser(t, ctx, 2) + contexttest.LoadRepo(t, ctx, 1) + contexttest.LoadGitRepo(t, ctx) + RemoveCommitNotesPullRequest(ctx) + assert.Equal(t, http.StatusSeeOther, ctx.Resp.Status()) + assert.Equal(t, path, test.RedirectURL(ctx.Resp)) + note, err := git.GetNote(ctx, ctx.Repo.GitRepo, commitID) + require.Error(t, err) + assert.True(t, git.IsErrNotExist(err)) + assert.Nil(t, note) +} diff --git a/routers/web/web.go b/routers/web/web.go index cd5d65b912..6cddc1d0b0 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -1564,6 +1564,10 @@ func registerRoutes(m *web.Route) { m.Get("", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForSingleCommit) m.Post("/reviews/submit", context.RepoMustNotBeArchived(), web.Bind(forms.SubmitReviewForm{}), repo.SubmitReview) }) + m.Group("/{sha:([a-f0-9]{4,64})$}/notes", func() { + m.Post("", context.RepoMustNotBeArchived(), web.Bind(forms.CommitNotesForm{}), repo.SetCommitNotesPullRequest) + m.Post("/remove", context.RepoMustNotBeArchived(), repo.RemoveCommitNotesPullRequest) + }, reqSignIn, reqRepoCodeWriter) }) m.Post("/merge", context.RepoMustNotBeArchived(), web.Bind(forms.MergePullRequestForm{}), context.EnforceQuotaWeb(quota_model.LimitSubjectSizeGitAll, context.QuotaTargetRepo), repo.MergePullRequest) m.Post("/cancel_auto_merge", context.RepoMustNotBeArchived(), repo.CancelAutoMergePullRequest) @@ -1626,8 +1630,8 @@ func registerRoutes(m *web.Route) { m.Get("/commit/{sha:([a-f0-9]{4,64})$}", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.Diff) m.Get("/commit/{sha:([a-f0-9]{4,64})$}/load-branches-and-tags", repo.LoadBranchesAndTags) m.Group("/commit/{sha:([a-f0-9]{4,64})$}/notes", func() { - m.Post("", web.Bind(forms.CommitNotesForm{}), repo.SetCommitNotes) - m.Post("/remove", repo.RemoveCommitNotes) + m.Post("", context.RepoMustNotBeArchived(), web.Bind(forms.CommitNotesForm{}), repo.SetCommitNotes) + m.Post("/remove", context.RepoMustNotBeArchived(), repo.RemoveCommitNotes) }, reqSignIn, reqRepoCodeWriter) m.Get("/cherry-pick/{sha:([a-f0-9]{4,64})$}", repo.SetEditorconfigIfExists, repo.CherryPick) }, repo.MustBeNotEmpty, context.RepoRef(), reqRepoCodeReader) diff --git a/templates/repo/commit_header.tmpl b/templates/repo/commit_header.tmpl index 3426d5feac..dacbf43b59 100644 --- a/templates/repo/commit_header.tmpl +++ b/templates/repo/commit_header.tmpl @@ -293,7 +293,7 @@
{{.NoteRendered | SanitizeHTML}}