upstream/forgejo
1
0
Fork 0
mirror of https://codeberg.org/forgejo/forgejo.git synced 2026-06-08 20:32:12 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

fix(ui): improve Git notes editing (#11365)

Browse source 
Closes #11355, namely:

1. bug: editing the note does not edit the orginal content, but the rendered content
    - 16368c4ccb
    - 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`
    - f036fc55db
    - 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
    - 58d8c7cc87
    - 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 <gusted@noreply.codeberg.org>
Co-authored-by: Robert Wolff <mahlzahn@posteo.de>
Co-committed-by: Robert Wolff <mahlzahn@posteo.de>
This commit is contained in:
Robert Wolff 2026-03-10 23:49:18 +01:00 committed by Gusted
parent f1a08a7ab1
commit 296e6a284e
9 changed files with 204 additions and 46 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
release-notes/11365.md Normal file
View file

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

17
routers/web/repo/commit.go
View file

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

58
routers/web/repo/commit_test.go Normal file
View file

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

9
routers/web/repo/pull.go
View file

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

62
routers/web/repo/pull_test.go Normal file
View file

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

8
routers/web/web.go
View file

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

33
templates/repo/commit_header.tmpl
View file

@ -293,7 +293,7 @@
<div class="text right actions">
<form action="{{.Link}}/notes/remove" method="post">
<button type="button" class="ui cancel button">{{ctx.Locale.Tr "settings.cancel"}}</button>
<button type="submit" class="ui red button" href="{{.Link}}/notes/remove">{{ctx.Locale.Tr "remove"}}</button>
<button type="submit" class="ui red button">{{ctx.Locale.Tr "remove"}}</button>
</form>
</div>
</div>
@ -303,32 +303,19 @@
<div id="commit-notes-display-area" class="ui bottom attached info segment git-notes">
<pre class="commit-body">{{.NoteRendered | SanitizeHTML}}</pre>
</div>
{{if and ($.Permission.CanWrite $.UnitTypeCode) (not $.Repository.IsArchived) (not .IsDeleted)}}
<div id="commit-notes-edit-area" class="ui bottom attached info segment git-notes tw-hidden">
<form class="ui form" action="{{.Link}}/notes" method="post">
<div class="field">
<textarea name="notes">{{.NoteRendered | SanitizeHTML}}</textarea>
</div>
<div class="field">
<button id="notes-save-button" class="ui primary button">{{ctx.Locale.Tr "save"}}</button>
</div>
</form>
</div>
{{end}}
{{else if and ($.Permission.CanWrite $.UnitTypeCode) (not $.Repository.IsArchived) (not .IsDeleted)}}
<div id="commit-notes-add-area" class="ui tw-mt-3 segment tw-hidden">
<form class="ui form" action="{{.Link}}/notes" method="post">
{{end}}
{{if and ($.Permission.CanWrite $.UnitTypeCode) (not $.Repository.IsArchived) (not .IsDeleted)}}
<div id="commit-notes-edit-area" class="ui bottom attached info segment git-notes tw-hidden">
<form id="commit-notes-edit-form" class="ui form" action="{{.Link}}/notes" method="post">
<div class="field">
<textarea name="notes"></textarea>
<textarea name="notes">{{.NoteRaw}}</textarea>
</div>
<div class="field">
<button class="ui primary button">{{ctx.Locale.Tr "add"}}</button>
<div class="text right edit">
<button id="commit-notes-cancel-button" type="button" class="ui cancel button">{{ctx.Locale.Tr "cancel"}}</button>
<button id="commit-notes-save-button" type="submit" class="ui primary button">{{if .NoteRaw}}{{ctx.Locale.Tr "save"}}{{else}}{{ctx.Locale.Tr "add"}}{{end}}</button>
</div>
</div>
</form>
</div>
{{end}}

34
tests/e2e/git-notes.test.e2e.ts
View file

@ -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 <script>alert("xss")</script>.\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);
});

26
web_src/js/features/repo-commit.js
View file

@ -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');
});
}