From 296e6a284e04f010498221092c97d29955a4eb3e Mon Sep 17 00:00:00 2001 From: Robert Wolff Date: Tue, 10 Mar 2026 23:49:18 +0100 Subject: [PATCH] fix(ui): improve Git notes editing (#11365) Closes #11355, namely: 1. bug: editing the note does not edit the orginal content, but the rendered content - https://codeberg.org/forgejo/forgejo/pulls/11365/commits/16368c4ccb7c5e4711599abe5a607d0a9da81f9b - edit raw notes instead of rendered notes 2. bug: editing existing note on single-commit PR page leads to 404 page because it sends a POST request to `/OWNER/REPO/pulls/ID/commits/COMMIT_HASH/notes` - https://codeberg.org/forgejo/forgejo/pulls/11365/commits/f036fc55db6b32975f6b0d78d0a7b0e34cd5e866 - add new paths for the actions on pull request pages for `/OWNER/REPO/pulls/ID/commits/COMMIT_HASH/notes` and `/OWNER/REPO/pulls/ID/commits/COMMIT_HASH/notes/remove` 3. feat: both for adding and editing there is no `Cancel` button - https://codeberg.org/forgejo/forgejo/pulls/11365/commits/58d8c7cc872f34ddb092fe2c28d757580d16a320 - moved both the `Cancel` and the `Save`/`Edit` button to the right for better consistency how, e.g., issue comments are edited/created. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11365 Reviewed-by: Gusted Co-authored-by: Robert Wolff Co-committed-by: Robert Wolff --- release-notes/11365.md | 3 ++ routers/web/repo/commit.go | 17 ++++++-- routers/web/repo/commit_test.go | 58 ++++++++++++++++++++++++++++ routers/web/repo/pull.go | 9 +++++ routers/web/repo/pull_test.go | 62 ++++++++++++++++++++++++++++++ routers/web/web.go | 8 +++- templates/repo/commit_header.tmpl | 33 +++++----------- tests/e2e/git-notes.test.e2e.ts | 34 ++++++++++++++-- web_src/js/features/repo-commit.js | 26 ++++++------- 9 files changed, 204 insertions(+), 46 deletions(-) create mode 100644 release-notes/11365.md create mode 100644 routers/web/repo/commit_test.go create mode 100644 routers/web/repo/pull_test.go 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 @@
- +
@@ -303,32 +303,19 @@ 
{{.NoteRendered | SanitizeHTML}}
- {{if and ($.Permission.CanWrite $.UnitTypeCode) (not $.Repository.IsArchived) (not .IsDeleted)}} -
-
- -
- -
- -
- -
-
-
- {{end}} -{{else if and ($.Permission.CanWrite $.UnitTypeCode) (not $.Repository.IsArchived) (not .IsDeleted)}} -
-
- +{{end}} +{{if and ($.Permission.CanWrite $.UnitTypeCode) (not $.Repository.IsArchived) (not .IsDeleted)}} +
+
- +
-
- +
+ + +
{{end}} - diff --git a/tests/e2e/git-notes.test.e2e.ts b/tests/e2e/git-notes.test.e2e.ts index f1f4d07c1d..2e2948a43e 100644 --- a/tests/e2e/git-notes.test.e2e.ts +++ b/tests/e2e/git-notes.test.e2e.ts @@ -6,26 +6,52 @@ import {screenshot} from './shared/screenshots.ts'; test.use({user: 'user2'}); test('Change git note', async ({page}) => { + const text = 'This is a new note .\nSee https://frogejo.org.'; + let response = await page.goto('/user2/repo1/commit/65f1bf27bc3bf70f64657658635e66094edbcb4d'); expect(response?.status()).toBe(200); // An add button should not be present, because the commit already has a commit note await expect(page.locator('#commit-notes-add-button')).toHaveCount(0); + let renderedarea = page.locator('#commit-notes-display-area pre.commit-body'); + await expect(renderedarea).toBeVisible(); + let textarea = page.locator('textarea[name="notes"]'); + await expect(textarea).toBeHidden(); + await page.locator('#commit-notes-edit-button').click(); - let textarea = page.locator('textarea[name="notes"]'); + await expect(renderedarea).toBeHidden(); await expect(textarea).toBeVisible(); - await textarea.fill('This is a new note'); + await textarea.fill(text); await screenshot(page, page.locator('.ui.container.fluid.padded')); - await page.locator('#notes-save-button').click(); + await page.locator('#commit-notes-save-button').click(); + + await expect(renderedarea).toBeVisible(); + await expect(textarea).toBeHidden(); + await expect(renderedarea).toHaveText(text); + await expect(renderedarea.locator('a')).toHaveAttribute('href', 'https://frogejo.org'); await screenshot(page, page.locator('.ui.container.fluid.padded')); + // Check edited note response = await page.goto('/user2/repo1/commit/65f1bf27bc3bf70f64657658635e66094edbcb4d'); expect(response?.status()).toBe(200); + renderedarea = page.locator('#commit-notes-display-area pre.commit-body'); + await expect(renderedarea).toHaveText(text); + await expect(renderedarea.locator('a')).toHaveAttribute('href', 'https://frogejo.org'); textarea = page.locator('textarea[name="notes"]'); - await expect(textarea).toHaveText('This is a new note'); + await expect(textarea).toHaveText(text); + await expect(textarea.locator('a')).toHaveCount(0); await screenshot(page, page.locator('.ui.container.fluid.padded')); + + // Cancel note editing + await page.locator('#commit-notes-edit-button').click(); + await textarea.fill('Edited note'); + await page.locator('#commit-notes-cancel-button').click(); + await expect(renderedarea).toBeVisible(); + await expect(renderedarea).toHaveText(text); + await expect(textarea).toBeHidden(); + await expect(textarea).toHaveText(text); }); diff --git a/web_src/js/features/repo-commit.js b/web_src/js/features/repo-commit.js index c546cfe0c2..5f8d0e7393 100644 --- a/web_src/js/features/repo-commit.js +++ b/web_src/js/features/repo-commit.js @@ -28,18 +28,18 @@ export function initCommitStatuses() { } export function initCommitNotes() { - const notesEditButton = document.getElementById('commit-notes-edit-button'); - if (notesEditButton !== null) { - notesEditButton.addEventListener('click', () => { - document.getElementById('commit-notes-display-area').classList.add('tw-hidden'); - document.getElementById('commit-notes-edit-area').classList.remove('tw-hidden'); - }); - } + document.getElementById('commit-notes-edit-button')?.addEventListener('click', () => { + document.getElementById('commit-notes-display-area').classList.add('tw-hidden'); + document.getElementById('commit-notes-edit-area').classList.remove('tw-hidden'); + }); - const notesAddButton = document.getElementById('commit-notes-add-button'); - if (notesAddButton !== null) { - notesAddButton.addEventListener('click', () => { - document.getElementById('commit-notes-add-area').classList.remove('tw-hidden'); - }); - } + document.getElementById('commit-notes-add-button')?.addEventListener('click', () => { + document.getElementById('commit-notes-edit-area').classList.remove('tw-hidden'); + }); + + document.getElementById('commit-notes-cancel-button')?.addEventListener('click', () => { + document.getElementById('commit-notes-edit-form').reset(); + document.getElementById('commit-notes-display-area')?.classList.remove('tw-hidden'); + document.getElementById('commit-notes-edit-area').classList.add('tw-hidden'); + }); }