diff --git a/models/fixtures/comment.yml b/models/fixtures/comment.yml index f4121284a6..2c47196c05 100644 --- a/models/fixtures/comment.yml +++ b/models/fixtures/comment.yml @@ -113,3 +113,43 @@ review_id: 22 assignee_id: 5 created_unix: 946684817 + +- + id: 13 + type: 29 # push + poster_id: 2 + issue_id: 19 # in repo_id 58 + content: '{"is_force_push":false,"commit_ids":["4ca8bcaf27e28504df7bf996819665986b01c847","96cef4a7b72b3c208340ae6f0cf55a93e9077c93","c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2"]}' + created_unix: 1688672373 + +- + id: 14 + type: 29 # push + poster_id: 2 + issue_id: 19 # in repo_id 58 + content: '{"is_force_push":false,"commit_ids":["23576dd018294e476c06e569b6b0f170d0558705"]}' + created_unix: 1688672374 + +- + id: 15 + type: 29 # push + poster_id: 2 + issue_id: 19 # in repo_id 58 + content: '{"is_force_push":false,"commit_ids":["3e64625bd6eb5bcba69ac97de6c8f507402df861", "c704db5794097441aa2d9dd834d5b7e2f8f08108"]}' + created_unix: 1688672375 + +- + id: 16 + type: 29 # push + poster_id: 2 + issue_id: 19 # in repo_id 58 + content: '{"is_force_push":false,"commit_ids":["811d46c7e518f4f180afb862c0db5cb8c80529ce", "747ddb3506a4fa04a7747808eb56ae16f9e933dc", "837d5c8125633d7d258f93b998e867eab0145520", "1978192d98bb1b65e11c2cf37da854fbf94bffd6"]}' + created_unix: 1688672376 + +- + id: 17 + type: 29 # push + poster_id: 2 + issue_id: 19 # in repo_id 58 + content: '{"is_force_push":true,"commit_ids":["1978192d98bb1b65e11c2cf37da854fbf94bffd6", "9b93963cf6de4dc33f915bb67f192d099c301f43"]}' + created_unix: 1749734240 diff --git a/options/locale_next/locale_en-US.json b/options/locale_next/locale_en-US.json index 356aaaead0..368152095e 100644 --- a/options/locale_next/locale_en-US.json +++ b/options/locale_next/locale_en-US.json @@ -89,6 +89,8 @@ "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_trigger": "Triggered because: %[1]s by: %[2]s", + "repo.diff.commit.next-short": "Next", + "repo.diff.commit.previous-short": "Prev", "discussion.locked": "This discussion has been locked. Commenting is limited to contributors.", "editor.textarea.tab_hint": "Line already indented. Press Tab again or Escape to leave the editor.", "editor.textarea.shift_tab_hint": "No indentation on this line. Press Shift + Tab again or Escape to leave the editor.", diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 2db982d3b6..8547bbcc03 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -10,13 +10,16 @@ import ( "errors" "fmt" "html" + "html/template" "net/http" "net/url" + "path" "strconv" "strings" "forgejo.org/models" activities_model "forgejo.org/models/activities" + asymkey_model "forgejo.org/models/asymkey" "forgejo.org/models/db" git_model "forgejo.org/models/git" issues_model "forgejo.org/models/issues" @@ -28,11 +31,13 @@ import ( "forgejo.org/models/unit" user_model "forgejo.org/models/user" "forgejo.org/modules/base" + "forgejo.org/modules/charset" "forgejo.org/modules/emoji" "forgejo.org/modules/git" "forgejo.org/modules/gitrepo" issue_template "forgejo.org/modules/issue/template" "forgejo.org/modules/log" + "forgejo.org/modules/markup" "forgejo.org/modules/optional" "forgejo.org/modules/setting" "forgejo.org/modules/structs" @@ -498,6 +503,7 @@ func PrepareMergedViewPullInfo(ctx *context.Context, issue *issues_model.Issue) ctx.Data["IsPullRequestBroken"] = true ctx.Data["BaseTarget"] = pull.BaseBranch ctx.Data["NumCommits"] = 0 + ctx.Data["CommitIDs"] = map[string]bool{} ctx.Data["NumFiles"] = 0 return nil } @@ -508,6 +514,12 @@ func PrepareMergedViewPullInfo(ctx *context.Context, issue *issues_model.Issue) ctx.Data["NumCommits"] = len(compareInfo.Commits) ctx.Data["NumFiles"] = compareInfo.NumFiles + commitIDs := map[string]bool{} + for _, commit := range compareInfo.Commits { + commitIDs[commit.ID.String()] = true + } + ctx.Data["CommitIDs"] = commitIDs + if len(compareInfo.Commits) != 0 { sha := compareInfo.Commits[0].ID.String() commitStatuses, _, err := git_model.GetLatestCommitStatus(ctx, ctx.Repo.Repository.ID, sha, db.ListOptionsAll) @@ -591,6 +603,7 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C ctx.Data["IsPullRequestBroken"] = true ctx.Data["BaseTarget"] = pull.BaseBranch ctx.Data["NumCommits"] = 0 + ctx.Data["CommitIDs"] = map[string]bool{} ctx.Data["NumFiles"] = 0 return nil } @@ -601,6 +614,13 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C ctx.Data["NumCommits"] = len(compareInfo.Commits) ctx.Data["NumFiles"] = compareInfo.NumFiles + + commitIDs := map[string]bool{} + for _, commit := range compareInfo.Commits { + commitIDs[commit.ID.String()] = true + } + ctx.Data["CommitIDs"] = commitIDs + return compareInfo } @@ -659,6 +679,7 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C } ctx.Data["BaseTarget"] = pull.BaseBranch ctx.Data["NumCommits"] = 0 + ctx.Data["CommitIDs"] = map[string]bool{} ctx.Data["NumFiles"] = 0 return nil } @@ -736,6 +757,7 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C ctx.Data["IsPullRequestBroken"] = true ctx.Data["BaseTarget"] = pull.BaseBranch ctx.Data["NumCommits"] = 0 + ctx.Data["CommitIDs"] = map[string]bool{} ctx.Data["NumFiles"] = 0 return nil } @@ -760,6 +782,13 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C ctx.Data["NumCommits"] = len(compareInfo.Commits) ctx.Data["NumFiles"] = compareInfo.NumFiles + + commitIDs := map[string]bool{} + for _, commit := range compareInfo.Commits { + commitIDs[commit.ID.String()] = true + } + ctx.Data["CommitIDs"] = commitIDs + return compareInfo } @@ -919,7 +948,81 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi ctx.Data["IsShowingOnlySingleCommit"] = willShowSpecifiedCommit - if willShowSpecifiedCommit || willShowSpecifiedCommitRange { + if willShowSpecifiedCommit { + commitID := specifiedEndCommit + + ctx.Data["CommitID"] = commitID + + var prevCommit, curCommit, nextCommit *git.Commit + + // Iterate in reverse to properly map "previous" and "next" buttons + for i := len(prInfo.Commits) - 1; i >= 0; i-- { + commit := prInfo.Commits[i] + + if curCommit != nil { + nextCommit = commit + break + } + + if commit.ID.String() == commitID { + curCommit = commit + } else { + prevCommit = commit + } + } + + if curCommit == nil { + ctx.ServerError("Repo.GitRepo.viewPullFiles", git.ErrNotExist{ID: commitID}) + return + } + + ctx.Data["Commit"] = curCommit + if prevCommit != nil { + ctx.Data["PrevCommitLink"] = path.Join(ctx.Repo.RepoLink, "pulls", strconv.FormatInt(issue.Index, 10), "commits", prevCommit.ID.String()) + } + if nextCommit != nil { + ctx.Data["NextCommitLink"] = path.Join(ctx.Repo.RepoLink, "pulls", strconv.FormatInt(issue.Index, 10), "commits", nextCommit.ID.String()) + } + + statuses, _, err := git_model.GetLatestCommitStatus(ctx, ctx.Repo.Repository.ID, commitID, db.ListOptionsAll) + if err != nil { + log.Error("GetLatestCommitStatus: %v", err) + } + if !ctx.Repo.CanRead(unit.TypeActions) { + git_model.CommitStatusesHideActionsURL(ctx, statuses) + } + + ctx.Data["CommitStatus"] = git_model.CalcCommitStatus(statuses) + ctx.Data["CommitStatuses"] = statuses + + verification := asymkey_model.ParseCommitWithSignature(ctx, curCommit) + ctx.Data["Verification"] = verification + ctx.Data["Author"] = user_model.ValidateCommitWithEmail(ctx, curCommit) + + note := &git.Note{} + err = git.GetNote(ctx, ctx.Repo.GitRepo, specifiedEndCommit, note) + if err == nil { + ctx.Data["NoteCommit"] = note.Commit + ctx.Data["NoteAuthor"] = user_model.ValidateCommitWithEmail(ctx, note.Commit) + ctx.Data["NoteRendered"], err = markup.RenderCommitMessage(&markup.RenderContext{ + Links: markup.Links{ + Base: ctx.Repo.RepoLink, + BranchPath: path.Join("commit", util.PathEscapeSegments(commitID)), + }, + Metas: ctx.Repo.Repository.ComposeMetas(ctx), + GitRepo: ctx.Repo.GitRepo, + Ctx: ctx, + }, template.HTMLEscapeString(string(charset.ToUTF8WithFallback(note.Message, charset.ConvertOpts{})))) + if err != nil { + ctx.ServerError("RenderCommitMessage", err) + return + } + } + + endCommitID = commitID + startCommitID = prInfo.MergeBase + ctx.Data["IsShowingAllCommits"] = false + } else if willShowSpecifiedCommitRange { if len(specifiedEndCommit) > 0 { endCommitID = specifiedEndCommit } else { @@ -930,6 +1033,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi } else { startCommitID = prInfo.MergeBase } + ctx.Data["IsShowingAllCommits"] = false } else { endCommitID = headCommitID @@ -937,10 +1041,10 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi ctx.Data["IsShowingAllCommits"] = true } - ctx.Data["Username"] = ctx.Repo.Owner.Name - ctx.Data["Reponame"] = ctx.Repo.Repository.Name ctx.Data["AfterCommitID"] = endCommitID ctx.Data["BeforeCommitID"] = startCommitID + ctx.Data["Username"] = ctx.Repo.Owner.Name + ctx.Data["Reponame"] = ctx.Repo.Repository.Name fileOnly := ctx.FormBool("file-only") diff --git a/routers/web/web.go b/routers/web/web.go index f8a13dab7e..4b39f22f7d 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -1510,7 +1510,10 @@ func registerRoutes(m *web.Route) { m.Group("/commits", func() { m.Get("", context.RepoRef(), repo.SetWhitespaceBehavior, repo.GetPullDiffStats, repo.ViewPullCommits) m.Get("/list", context.RepoRef(), repo.GetPullCommits) - m.Get("/{sha:[a-f0-9]{4,40}}", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForSingleCommit) + m.Group("/{sha:[a-f0-9]{4,40}}", func() { + 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.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) diff --git a/templates/repo/commit_header.tmpl b/templates/repo/commit_header.tmpl index 8a074e9545..9604daf2b0 100644 --- a/templates/repo/commit_header.tmpl +++ b/templates/repo/commit_header.tmpl @@ -14,10 +14,19 @@ {{end}} {{end}}
-
+

{{RenderCommitMessage $.Context .Commit.Message ($.Repository.ComposeMetas ctx)}}{{template "repo/commit_statuses" dict "Status" .CommitStatus "Statuses" .CommitStatuses}}

- {{if not $.PageIsWiki}} -
+
+ {{if .PageIsPullFiles}} +
+ + {{svg "octicon-chevron-left"}} {{ctx.Locale.Tr "repo.diff.commit.previous-short"}} + + + {{ctx.Locale.Tr "repo.diff.commit.next-short"}} {{svg "octicon-chevron-right"}} + +
+ {{else if not $.PageIsWiki}} {{ctx.Locale.Tr "repo.diff.browse_source"}} @@ -132,8 +141,8 @@
{{end}} -
- {{end}} + {{end}} +
{{if IsMultilineCommitMessage .Commit.Message}} 
{{RenderCommitBody $.Context .Commit.Message ($.Repository.ComposeMetas ctx)}}
@@ -185,9 +194,16 @@ {{end}}
{{ctx.Locale.Tr "repo.diff.commit"}} - - {{ShortSha .CommitID}} - + {{if .PageIsPullFiles}} + {{$commitShaLink := (printf "%s/commit/%s" $.RepoLink (PathEscape .CommitID))}} + + {{ShortSha .CommitID}} + + {{else}} + + {{ShortSha .CommitID}} + + {{end}}
diff --git a/templates/repo/commit_load_branches_and_tags.tmpl b/templates/repo/commit_load_branches_and_tags.tmpl index ffa0e530e8..25402ca2f4 100644 --- a/templates/repo/commit_load_branches_and_tags.tmpl +++ b/templates/repo/commit_load_branches_and_tags.tmpl @@ -1,4 +1,4 @@ -{{if not .PageIsWiki}} +{{if not (or .PageIsWiki .PageIsPullFiles)}}
{{if and (not $.Repository.IsArchived) (not .DiffNotAvailable)}} diff --git a/templates/repo/diff/new_review.tmpl b/templates/repo/diff/new_review.tmpl index 13d09babe1..ded7a6c5fc 100644 --- a/templates/repo/diff/new_review.tmpl +++ b/templates/repo/diff/new_review.tmpl @@ -1,17 +1,14 @@
-
- {{if $.IsShowingAllCommits}}
@@ -55,5 +52,4 @@
- {{end}}
diff --git a/tests/e2e/pr-review.test.e2e.ts b/tests/e2e/pr-review.test.e2e.ts index b619cdbcd1..577c8bf20a 100644 --- a/tests/e2e/pr-review.test.e2e.ts +++ b/tests/e2e/pr-review.test.e2e.ts @@ -11,7 +11,7 @@ import {save_visual, test} from './utils_e2e.ts'; test.use({user: 'user2'}); -test('PR: Finish review', async ({page}) => { +test('PR: Create review from files', async ({page}) => { const response = await page.goto('/user2/repo1/pulls/5/files'); expect(response?.status()).toBe(200); @@ -22,4 +22,93 @@ test('PR: Finish review', async ({page}) => { await page.locator('#review-box .js-btn-review').click(); await expect(page.locator('.tippy-box .review-box-panel')).toBeVisible(); await save_visual(page); + + await page.locator('.review-box-panel textarea#_combo_markdown_editor_0') + .fill('This is a review'); + await page.locator('.review-box-panel button.btn-submit[value="approve"]').click(); + await page.waitForURL(/.*\/user2\/repo1\/pulls\/5#issuecomment-\d+/); + await save_visual(page); +}); + +test('PR: Create review from commit', async ({page}) => { + const response = await page.goto('/user2/repo1/pulls/3/commits/4a357436d925b5c974181ff12a994538ddc5a269'); + expect(response?.status()).toBe(200); + + await page.locator('button.add-code-comment').click(); + const code_comment = page.locator('.comment-code-cloud form textarea.markdown-text-editor'); + await expect(code_comment).toBeVisible(); + + await code_comment.fill('This is a code comment'); + await save_visual(page); + + const start_button = page.locator('.comment-code-cloud form button.btn-start-review'); + // Workaround for #7152, where there might already be a pending review state from previous + // test runs (most likely to happen when debugging tests). + if (await start_button.isVisible({timeout: 100})) { + await start_button.click(); + } else { + await page.locator('.comment-code-cloud form button.btn-add-comment').click(); + } + + await expect(page.locator('.comment-list .comment-container')).toBeVisible(); + + // We need to wait for the review to be processed. Checking the comment counter + // conveniently does that. + await expect(page.locator('#review-box .js-btn-review > span.review-comments-counter')).toHaveText('1'); + + await page.locator('#review-box .js-btn-review').click(); + await expect(page.locator('.tippy-box .review-box-panel')).toBeVisible(); + await save_visual(page); + + await page.locator('.review-box-panel textarea.markdown-text-editor') + .fill('This is a review'); + await page.locator('.review-box-panel button.btn-submit[value="approve"]').click(); + await page.waitForURL(/.*\/user2\/repo1\/pulls\/3#issuecomment-\d+/); + await save_visual(page); + + // In addition to testing the ability to delete comments, this also + // performs clean up. If tests are run for multiple platforms, the data isn't reset + // in-between, and subsequent runs of this test would fail, because when there already is + // a comment, the on-hover button to start a conversation doesn't appear anymore. + await page.goto('/user2/repo1/pulls/3/commits/4a357436d925b5c974181ff12a994538ddc5a269'); + await page.locator('.comment-header-right.actions a.context-menu').click(); + + await expect(page.locator('.comment-header-right.actions div.menu').getByText(/Copy link.*/)).toBeVisible(); + // The button to delete a comment will prompt for confirmation using a browser alert. + page.on('dialog', (dialog) => dialog.accept()); + await page.locator('.comment-header-right.actions div.menu .delete-comment').click(); + + await expect(page.locator('.comment-list .comment-container')).toBeHidden(); + await save_visual(page); +}); + +test('PR: Navigate by single commit', async ({page}) => { + const response = await page.goto('/user2/repo1/pulls/3/commits'); + expect(response?.status()).toBe(200); + + await page.locator('tbody.commit-list td.message a').nth(1).click(); + await page.waitForURL(/.*\/user2\/repo1\/pulls\/3\/commits\/4a357436d925b5c974181ff12a994538ddc5a269/); + await save_visual(page); + + let prevButton = page.locator('.commit-header-buttons').getByText(/Prev/); + let nextButton = page.locator('.commit-header-buttons').getByText(/Next/); + await prevButton.waitFor(); + await nextButton.waitFor(); + + await expect(prevButton).toHaveClass(/disabled/); + await expect(nextButton).not.toHaveClass(/disabled/); + await expect(nextButton).toHaveAttribute('href', '/user2/repo1/pulls/3/commits/5f22f7d0d95d614d25a5b68592adb345a4b5c7fd'); + await nextButton.click(); + + await page.waitForURL(/.*\/user2\/repo1\/pulls\/3\/commits\/5f22f7d0d95d614d25a5b68592adb345a4b5c7fd/); + await save_visual(page); + + prevButton = page.locator('.commit-header-buttons').getByText(/Prev/); + nextButton = page.locator('.commit-header-buttons').getByText(/Next/); + await prevButton.waitFor(); + await nextButton.waitFor(); + + await expect(prevButton).not.toHaveClass(/disabled/); + await expect(nextButton).toHaveClass(/disabled/); + await expect(prevButton).toHaveAttribute('href', '/user2/repo1/pulls/3/commits/4a357436d925b5c974181ff12a994538ddc5a269'); }); diff --git a/tests/gitea-repositories-meta/user2/commitsonpr.git/logs/refs/heads/branch1 b/tests/gitea-repositories-meta/user2/commitsonpr.git/logs/refs/heads/branch1 index cf96195665..34c9ccecdd 100644 --- a/tests/gitea-repositories-meta/user2/commitsonpr.git/logs/refs/heads/branch1 +++ b/tests/gitea-repositories-meta/user2/commitsonpr.git/logs/refs/heads/branch1 @@ -1 +1,2 @@ 0000000000000000000000000000000000000000 1978192d98bb1b65e11c2cf37da854fbf94bffd6 Gitea 1688672383 +0200 push +1978192d98bb1b65e11c2cf37da854fbf94bffd6 9b93963cf6de4dc33f915bb67f192d099c301f43 Forgejo 1749737639 +0200 push diff --git a/tests/gitea-repositories-meta/user2/commitsonpr.git/objects/06/c5865eaeaaa2f92e8fce75a281f6272ee68e90 b/tests/gitea-repositories-meta/user2/commitsonpr.git/objects/06/c5865eaeaaa2f92e8fce75a281f6272ee68e90 new file mode 100644 index 0000000000..009c9849d9 Binary files /dev/null and b/tests/gitea-repositories-meta/user2/commitsonpr.git/objects/06/c5865eaeaaa2f92e8fce75a281f6272ee68e90 differ diff --git a/tests/gitea-repositories-meta/user2/commitsonpr.git/objects/2d/65d92dc800c6f448541240c18e82bf36b954bb b/tests/gitea-repositories-meta/user2/commitsonpr.git/objects/2d/65d92dc800c6f448541240c18e82bf36b954bb new file mode 100644 index 0000000000..e50c7ad5eb Binary files /dev/null and b/tests/gitea-repositories-meta/user2/commitsonpr.git/objects/2d/65d92dc800c6f448541240c18e82bf36b954bb differ diff --git a/tests/gitea-repositories-meta/user2/commitsonpr.git/objects/9b/93963cf6de4dc33f915bb67f192d099c301f43 b/tests/gitea-repositories-meta/user2/commitsonpr.git/objects/9b/93963cf6de4dc33f915bb67f192d099c301f43 new file mode 100644 index 0000000000..41814996f1 Binary files /dev/null and b/tests/gitea-repositories-meta/user2/commitsonpr.git/objects/9b/93963cf6de4dc33f915bb67f192d099c301f43 differ diff --git a/tests/gitea-repositories-meta/user2/commitsonpr.git/objects/ab/a2b386a1eb0b0273ada0fed4f7d075d6e343c1 b/tests/gitea-repositories-meta/user2/commitsonpr.git/objects/ab/a2b386a1eb0b0273ada0fed4f7d075d6e343c1 new file mode 100644 index 0000000000..2d936251cd Binary files /dev/null and b/tests/gitea-repositories-meta/user2/commitsonpr.git/objects/ab/a2b386a1eb0b0273ada0fed4f7d075d6e343c1 differ diff --git a/tests/gitea-repositories-meta/user2/commitsonpr.git/objects/b3/b178fe7c45986f6325aeac1b036c74825ae8f4 b/tests/gitea-repositories-meta/user2/commitsonpr.git/objects/b3/b178fe7c45986f6325aeac1b036c74825ae8f4 new file mode 100644 index 0000000000..7852ab9100 Binary files /dev/null and b/tests/gitea-repositories-meta/user2/commitsonpr.git/objects/b3/b178fe7c45986f6325aeac1b036c74825ae8f4 differ diff --git a/tests/gitea-repositories-meta/user2/commitsonpr.git/objects/c9/cf8a3095808af2425255056e01746fef420801 b/tests/gitea-repositories-meta/user2/commitsonpr.git/objects/c9/cf8a3095808af2425255056e01746fef420801 new file mode 100644 index 0000000000..32a51a2837 Binary files /dev/null and b/tests/gitea-repositories-meta/user2/commitsonpr.git/objects/c9/cf8a3095808af2425255056e01746fef420801 differ diff --git a/tests/gitea-repositories-meta/user2/commitsonpr.git/refs/heads/branch1 b/tests/gitea-repositories-meta/user2/commitsonpr.git/refs/heads/branch1 index 357fc9d6ed..6fc9179bdd 100644 --- a/tests/gitea-repositories-meta/user2/commitsonpr.git/refs/heads/branch1 +++ b/tests/gitea-repositories-meta/user2/commitsonpr.git/refs/heads/branch1 @@ -1 +1 @@ -1978192d98bb1b65e11c2cf37da854fbf94bffd6 +9b93963cf6de4dc33f915bb67f192d099c301f43 diff --git a/tests/gitea-repositories-meta/user2/commitsonpr.git/refs/pull/1/head b/tests/gitea-repositories-meta/user2/commitsonpr.git/refs/pull/1/head index 357fc9d6ed..6fc9179bdd 100644 --- a/tests/gitea-repositories-meta/user2/commitsonpr.git/refs/pull/1/head +++ b/tests/gitea-repositories-meta/user2/commitsonpr.git/refs/pull/1/head @@ -1 +1 @@ -1978192d98bb1b65e11c2cf37da854fbf94bffd6 +9b93963cf6de4dc33f915bb67f192d099c301f43 diff --git a/tests/integration/pull_commit_test.go b/tests/integration/pull_commit_test.go index 90d16d725d..8ca78f8147 100644 --- a/tests/integration/pull_commit_test.go +++ b/tests/integration/pull_commit_test.go @@ -31,3 +31,20 @@ func TestListPullCommits(t *testing.T) { } assert.Equal(t, "4a357436d925b5c974181ff12a994538ddc5a269", pullCommitList.LastReviewCommitSha) } + +func TestPullCommitLinks(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + req := NewRequest(t, "GET", "/user2/repo1/pulls/3/commits") + resp := MakeRequest(t, req, http.StatusOK) + + htmlDoc := NewHTMLParser(t, resp.Body) + + commitSha := htmlDoc.Find(".commit-list td.sha a.sha.label").First() + commitShaHref, _ := commitSha.Attr("href") + assert.Equal(t, "/user2/repo1/pulls/3/commits/5f22f7d0d95d614d25a5b68592adb345a4b5c7fd", commitShaHref) + + commitLink := htmlDoc.Find(".commit-list td.message a").First() + commitLinkHref, _ := commitLink.Attr("href") + assert.Equal(t, "/user2/repo1/pulls/3/commits/5f22f7d0d95d614d25a5b68592adb345a4b5c7fd", commitLinkHref) +} diff --git a/tests/integration/pull_diff_test.go b/tests/integration/pull_diff_test.go index 70e0d5d33a..6db9fc25d8 100644 --- a/tests/integration/pull_diff_test.go +++ b/tests/integration/pull_diff_test.go @@ -14,22 +14,22 @@ import ( ) func TestPullDiff_CompletePRDiff(t *testing.T) { - doTestPRDiff(t, "/user2/commitsonpr/pulls/1/files", false, []string{"test1.txt", "test10.txt", "test2.txt", "test3.txt", "test4.txt", "test5.txt", "test6.txt", "test7.txt", "test8.txt", "test9.txt"}) + doTestPRDiff(t, "/user2/commitsonpr/pulls/1/files", []string{"test1.txt", "test10.txt", "test2.txt", "test3.txt", "test4.txt", "test5.txt", "test6.txt", "test7.txt", "test8.txt", "test9.txt"}) } func TestPullDiff_SingleCommitPRDiff(t *testing.T) { - doTestPRDiff(t, "/user2/commitsonpr/pulls/1/commits/c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2", true, []string{"test3.txt"}) + doTestPRDiff(t, "/user2/commitsonpr/pulls/1/commits/c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2", []string{"test3.txt"}) } func TestPullDiff_CommitRangePRDiff(t *testing.T) { - doTestPRDiff(t, "/user2/commitsonpr/pulls/1/files/4ca8bcaf27e28504df7bf996819665986b01c847..23576dd018294e476c06e569b6b0f170d0558705", true, []string{"test2.txt", "test3.txt", "test4.txt"}) + doTestPRDiff(t, "/user2/commitsonpr/pulls/1/files/4ca8bcaf27e28504df7bf996819665986b01c847..23576dd018294e476c06e569b6b0f170d0558705", []string{"test2.txt", "test3.txt", "test4.txt"}) } func TestPullDiff_StartingFromBaseToCommitPRDiff(t *testing.T) { - doTestPRDiff(t, "/user2/commitsonpr/pulls/1/files/c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2", true, []string{"test1.txt", "test2.txt", "test3.txt"}) + doTestPRDiff(t, "/user2/commitsonpr/pulls/1/files/c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2", []string{"test1.txt", "test2.txt", "test3.txt"}) } -func doTestPRDiff(t *testing.T, prDiffURL string, reviewBtnDisabled bool, expectedFilenames []string) { +func doTestPRDiff(t *testing.T, prDiffURL string, expectedFilenames []string) { defer tests.PrepareTestEnv(t)() session := loginUser(t, "user2") @@ -52,7 +52,4 @@ func doTestPRDiff(t *testing.T, prDiffURL string, reviewBtnDisabled bool, expect filename, _ := s.Attr("data-old-filename") assert.Equal(t, expectedFilenames[i], filename) }) - - // Ensure the review button is enabled for full PR reviews - assert.Equal(t, reviewBtnDisabled, doc.doc.Find(".js-btn-review").HasClass("disabled")) } diff --git a/tests/integration/pull_files_test.go b/tests/integration/pull_files_test.go new file mode 100644 index 0000000000..eb7072a4e6 --- /dev/null +++ b/tests/integration/pull_files_test.go @@ -0,0 +1,106 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package integration + +import ( + "net/http" + "strings" + "testing" + "time" + + repo_model "forgejo.org/models/repo" + "forgejo.org/models/unittest" + "forgejo.org/modules/git" + "forgejo.org/tests" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPullFilesCommitHeader(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + t.Run("Verify commit info", func(t *testing.T) { + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + gitRepo, err := git.OpenRepository(git.DefaultContext, repo.RepoPath()) + require.NoError(t, err) + defer gitRepo.Close() + + commit, err := gitRepo.GetCommit("62fb502a7172d4453f0322a2cc85bddffa57f07a") + require.NoError(t, err) + + req := NewRequest(t, "GET", "/user2/repo1/pulls/5/commits/62fb502a7172d4453f0322a2cc85bddffa57f07a") + resp := MakeRequest(t, req, http.StatusOK) + + htmlDoc := NewHTMLParser(t, resp.Body) + header := htmlDoc.doc.Find("#diff-commit-header") + + summary := header.Find(".commit-header h3") + assert.Equal(t, commit.Summary(), strings.TrimSpace(summary.Text())) + + author := header.Find(".author strong") + assert.Equal(t, commit.Author.Name, author.Text()) + + date, _ := header.Find("#authored-time relative-time").Attr("datetime") + assert.Equal(t, commit.Author.When.Format(time.RFC3339), date) + + sha := header.Find(".commit-header-row .sha.label") + shaHref, _ := sha.Attr("href") + assert.Equal(t, commit.ID.String()[:10], sha.Find(".shortsha").Text()) + assert.Equal(t, "/user2/repo1/commit/62fb502a7172d4453f0322a2cc85bddffa57f07a", shaHref) + }) + + t.Run("Navigation", func(t *testing.T) { + t.Run("No previous on first commit", func(t *testing.T) { + req := NewRequest(t, "GET", "/user2/commitsonpr/pulls/1/commits/4ca8bcaf27e28504df7bf996819665986b01c847") + resp := MakeRequest(t, req, http.StatusOK) + + htmlDoc := NewHTMLParser(t, resp.Body) + buttons := htmlDoc.doc.Find(".commit-header-buttons a.small.button") + + assert.Equal(t, 2, buttons.Length(), "expected two buttons in commit header") + + assert.True(t, buttons.First().HasClass("disabled"), "'prev' button should be disabled") + assert.False(t, buttons.Last().HasClass("disabled"), "'next' button should not be disabled") + + href, _ := buttons.Last().Attr("href") + assert.Equal(t, "/user2/commitsonpr/pulls/1/commits/96cef4a7b72b3c208340ae6f0cf55a93e9077c93", href) + }) + + t.Run("No next on last commit", func(t *testing.T) { + req := NewRequest(t, "GET", "/user2/commitsonpr/pulls/1/commits/9b93963cf6de4dc33f915bb67f192d099c301f43") + resp := MakeRequest(t, req, http.StatusOK) + + htmlDoc := NewHTMLParser(t, resp.Body) + buttons := htmlDoc.doc.Find(".commit-header-buttons a.small.button") + + assert.Equal(t, 2, buttons.Length(), "expected two buttons in commit header") + + assert.False(t, buttons.First().HasClass("disabled"), "'prev' button should not be disabled") + assert.True(t, buttons.Last().HasClass("disabled"), "'next' button should be disabled") + + href, _ := buttons.First().Attr("href") + assert.Equal(t, "/user2/commitsonpr/pulls/1/commits/2d65d92dc800c6f448541240c18e82bf36b954bb", href) + }) + + t.Run("Both directions on middle commit", func(t *testing.T) { + req := NewRequest(t, "GET", "/user2/commitsonpr/pulls/1/commits/c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2") + resp := MakeRequest(t, req, http.StatusOK) + + htmlDoc := NewHTMLParser(t, resp.Body) + buttons := htmlDoc.doc.Find(".commit-header-buttons a.small.button") + + assert.Equal(t, 2, buttons.Length(), "expected two buttons in commit header") + + assert.False(t, buttons.First().HasClass("disabled"), "'prev' button should not be disabled") + assert.False(t, buttons.Last().HasClass("disabled"), "'next' button should not be disabled") + + href, _ := buttons.First().Attr("href") + assert.Equal(t, "/user2/commitsonpr/pulls/1/commits/96cef4a7b72b3c208340ae6f0cf55a93e9077c93", href) + + href, _ = buttons.Last().Attr("href") + assert.Equal(t, "/user2/commitsonpr/pulls/1/commits/23576dd018294e476c06e569b6b0f170d0558705", href) + }) + }) +} diff --git a/tests/integration/pull_test.go b/tests/integration/pull_test.go index d5321f6ae5..fd9dbf8888 100644 --- a/tests/integration/pull_test.go +++ b/tests/integration/pull_test.go @@ -30,6 +30,43 @@ func TestViewPulls(t *testing.T) { assert.Equal(t, "Search pulls…", placeholder) } +func TestPullViewConversation(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + req := NewRequest(t, "GET", "/user2/commitsonpr/pulls/1") + resp := MakeRequest(t, req, http.StatusOK) + + htmlDoc := NewHTMLParser(t, resp.Body) + + t.Run("Commits", func(t *testing.T) { + commitLists := htmlDoc.Find(".timeline-item.commits-list") + assert.Equal(t, 4, commitLists.Length()) + + commits := commitLists.Find(".singular-commit") + assert.Equal(t, 10, commits.Length()) + + // First one has not been affected by a force push, therefore it's still part of the + // PR and should link to the PR-scoped review tab + firstCommit := commits.Eq(0) + firstCommitMessageHref, _ := firstCommit.Find("a.default-link").Attr("href") + firstCommitShaHref, _ := firstCommit.Find("a.sha.label").Attr("href") + assert.Equal(t, "/user2/commitsonpr/pulls/1/commits/4ca8bcaf27e28504df7bf996819665986b01c847", firstCommitMessageHref) + assert.Equal(t, "/user2/commitsonpr/pulls/1/commits/4ca8bcaf27e28504df7bf996819665986b01c847", firstCommitShaHref) + + // The fifth commit has been overwritten by a force push. + // Attempting to view the old one in the review tab won't work: + req := NewRequest(t, "GET", "/user2/commitsonpr/pulls/1/commits/3e64625bd6eb5bcba69ac97de6c8f507402df861") + MakeRequest(t, req, http.StatusNotFound) + + // Therefore, this commit should link to the non-PR commit view instead + fifthCommit := commits.Eq(4) + fifthCommitMessageHref, _ := fifthCommit.Find("a.default-link").Attr("href") + fifthCommitShaHref, _ := fifthCommit.Find("a.sha.label").Attr("href") + assert.Equal(t, "/user2/commitsonpr/commit/3e64625bd6eb5bcba69ac97de6c8f507402df861", fifthCommitMessageHref) + assert.Equal(t, "/user2/commitsonpr/commit/3e64625bd6eb5bcba69ac97de6c8f507402df861", fifthCommitShaHref) + }) +} + func TestPullManuallyMergeWarning(t *testing.T) { defer tests.PrepareTestEnv(t)() diff --git a/web_src/css/repo.css b/web_src/css/repo.css index cf916f0361..144424937c 100644 --- a/web_src/css/repo.css +++ b/web_src/css/repo.css @@ -2468,8 +2468,21 @@ tbody.commit-list { display: flex; } -#diff-file-boxes { +#diff-content-container { flex: 1; +} + +#diff-commit-header { + /* Counteract the `+2px` for width in `.segment` */ + padding: 0 2px; +} + +#diff-commit-header + h4, +#diff-commit-header + #diff-file-boxes { + margin-top: 8px; +} + +#diff-file-boxes { max-width: 100%; display: flex; flex-direction: column;