diff --git a/Makefile b/Makefile index e6416d5a6f..852c85ccd6 100644 --- a/Makefile +++ b/Makefile @@ -47,7 +47,6 @@ GO_LICENSES_PACKAGE ?= github.com/google/go-licenses@v1.6.0 # renovate: datasour GOVULNCHECK_PACKAGE ?= golang.org/x/vuln/cmd/govulncheck@v1 # renovate: datasource=go DEADCODE_PACKAGE ?= golang.org/x/tools/cmd/deadcode@v0.34.0 # renovate: datasource=go GOMOCK_PACKAGE ?= go.uber.org/mock/mockgen@v0.5.2 # renovate: datasource=go -GOPLS_PACKAGE ?= golang.org/x/tools/gopls@v0.18.1 # renovate: datasource=go RENOVATE_NPM_PACKAGE ?= renovate@40.57.1 # renovate: datasource=docker packageName=data.forgejo.org/renovate/renovate # https://github.com/disposable-email-domains/disposable-email-domains/commits/main/ @@ -222,7 +221,6 @@ help: @echo " - lint-go lint go files" @echo " - lint-go-fix lint go files and fix issues" @echo " - lint-go-vet lint go files with vet" - @echo " - lint-go-gopls lint go files with gopls" @echo " - lint-js lint js files" @echo " - lint-js-fix lint js files and fix issues" @echo " - lint-css lint css files" @@ -487,11 +485,6 @@ lint-go-vet: @echo "Running go vet..." @$(GO) vet ./... -.PHONY: lint-go-gopls -lint-go-gopls: - @echo "Running gopls check..." - @GO=$(GO) GOPLS_PACKAGE=$(GOPLS_PACKAGE) tools/lint-go-gopls.sh $(GO_SOURCES_NO_BINDATA) - .PHONY: lint-editorconfig lint-editorconfig: $(GO) run $(EDITORCONFIG_CHECKER_PACKAGE) templates .forgejo/workflows @@ -932,7 +925,6 @@ deps-tools: $(GO) install $(GO_LICENSES_PACKAGE) $(GO) install $(GOVULNCHECK_PACKAGE) $(GO) install $(GOMOCK_PACKAGE) - $(GO) install $(GOPLS_PACKAGE) node_modules: package-lock.json npm install --no-save 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/models/fixtures/pull_auto_merge.yml b/models/fixtures/pull_auto_merge.yml new file mode 100644 index 0000000000..ca780a73aa --- /dev/null +++ b/models/fixtures/pull_auto_merge.yml @@ -0,0 +1 @@ +[] # empty diff --git a/models/issues/issue_search.go b/models/issues/issue_search.go index bf4b89ee0b..91a69c26a7 100644 --- a/models/issues/issue_search.go +++ b/models/issues/issue_search.go @@ -66,6 +66,8 @@ func applySorts(sess *xorm.Session, sortType string, priorityRepoID int64) { sess.Asc("issue.created_unix").Asc("issue.id") case "recentupdate": sess.Desc("issue.updated_unix").Desc("issue.created_unix").Desc("issue.id") + case "recentclose": + sess.Desc("issue.closed_unix").Desc("issue.created_unix").Desc("issue.id") case "leastupdate": sess.Asc("issue.updated_unix").Asc("issue.created_unix").Asc("issue.id") case "mostcomment": diff --git a/models/issues/pull_list.go b/models/issues/pull_list.go index a448673454..8fc0491026 100644 --- a/models/issues/pull_list.go +++ b/models/issues/pull_list.go @@ -152,7 +152,8 @@ func PullRequests(ctx context.Context, baseRepoID int64, opts *PullRequestsOptio applySorts(findSession, opts.SortType, 0) findSession = db.SetSessionPagination(findSession, opts) prs := make([]*PullRequest, 0, opts.PageSize) - return prs, maxResults, findSession.Find(&prs) + found := findSession.Find(&prs) + return prs, maxResults, found } // PullRequestList defines a list of pull requests diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index 5a8e8d8aaf..3682f6fd25 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -79,6 +79,47 @@ func TestPullRequestsNewest(t *testing.T) { } } +func TestPullRequests_Closed_RecentSortType(t *testing.T) { + // Issue ID | Closed At. | Updated At + // 2 | 1707270001 | 1707270001 + // 3 | 1707271000 | 1707279999 + // 11 | 1707279999 | 1707275555 + tests := []struct { + sortType string + expectedIssueIDOrder []int64 + }{ + {"recentupdate", []int64{3, 11, 2}}, + {"recentclose", []int64{11, 3, 2}}, + } + + require.NoError(t, unittest.PrepareTestDatabase()) + _, err := db.Exec(db.DefaultContext, "UPDATE issue SET closed_unix = 1707270001, updated_unix = 1707270001, is_closed = true WHERE id = 2") + require.NoError(t, err) + _, err = db.Exec(db.DefaultContext, "UPDATE issue SET closed_unix = 1707271000, updated_unix = 1707279999, is_closed = true WHERE id = 3") + require.NoError(t, err) + _, err = db.Exec(db.DefaultContext, "UPDATE issue SET closed_unix = 1707279999, updated_unix = 1707275555, is_closed = true WHERE id = 11") + require.NoError(t, err) + + for _, test := range tests { + t.Run(test.sortType, func(t *testing.T) { + prs, _, err := issues_model.PullRequests(db.DefaultContext, 1, &issues_model.PullRequestsOptions{ + ListOptions: db.ListOptions{ + Page: 1, + }, + State: "closed", + SortType: test.sortType, + }) + require.NoError(t, err) + + if assert.Len(t, prs, len(test.expectedIssueIDOrder)) { + for i := range test.expectedIssueIDOrder { + assert.Equal(t, test.expectedIssueIDOrder[i], prs[i].IssueID) + } + } + }) + } +} + func TestLoadRequestedReviewers(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) diff --git a/models/pull/automerge.go b/models/pull/automerge.go index 63f572309b..dcc1f39271 100644 --- a/models/pull/automerge.go +++ b/models/pull/automerge.go @@ -10,6 +10,7 @@ import ( "forgejo.org/models/db" repo_model "forgejo.org/models/repo" user_model "forgejo.org/models/user" + "forgejo.org/modules/log" "forgejo.org/modules/timeutil" ) @@ -58,13 +59,15 @@ func ScheduleAutoMerge(ctx context.Context, doer *user_model.User, pullID int64, return ErrAlreadyScheduledToAutoMerge{PullID: pullID} } - _, err := db.GetEngine(ctx).Insert(&AutoMerge{ + scheduledPRM, err := db.GetEngine(ctx).Insert(&AutoMerge{ DoerID: doer.ID, PullID: pullID, MergeStyle: style, Message: message, DeleteBranchAfterMerge: deleteBranch, }) + log.Trace("ScheduleAutoMerge %+v for PR %d", scheduledPRM, pullID) + return err } @@ -81,6 +84,8 @@ func GetScheduledMergeByPullID(ctx context.Context, pullID int64) (bool, *AutoMe return false, nil, err } + log.Trace("GetScheduledMergeByPullID found %+v for PR %d", scheduledPRM, pullID) + scheduledPRM.Doer = doer return true, scheduledPRM, nil } @@ -94,6 +99,8 @@ func DeleteScheduledAutoMerge(ctx context.Context, pullID int64) error { return db.ErrNotExist{Resource: "auto_merge", ID: pullID} } + log.Trace("DeleteScheduledAutoMerge %+v for PR %d", scheduledPRM, pullID) + _, err = db.GetEngine(ctx).ID(scheduledPRM.ID).Delete(&AutoMerge{}) return err } 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/package-lock.json b/package-lock.json index 4a91eeecc2..40601d8536 100644 --- a/package-lock.json +++ b/package-lock.json @@ -16,7 +16,7 @@ "@primer/octicons": "19.14.0", "ansi_up": "6.0.5", "asciinema-player": "3.8.2", - "chart.js": "4.4.9", + "chart.js": "4.5.0", "chartjs-adapter-dayjs-4": "1.0.4", "chartjs-plugin-zoom": "2.2.0", "clippie": "4.1.7", @@ -5373,9 +5373,9 @@ } }, "node_modules/chart.js": { - "version": "4.4.9", - "resolved": "https://registry.npmjs.org/chart.js/-/chart.js-4.4.9.tgz", - "integrity": "sha512-EyZ9wWKgpAU0fLJ43YAEIF8sr5F2W3LqbS40ZJyHIner2lY14ufqv2VMp69MAiZ2rpwxEUxEhIH/0U3xyRynxg==", + "version": "4.5.0", + "resolved": "https://registry.npmjs.org/chart.js/-/chart.js-4.5.0.tgz", + "integrity": "sha512-aYeC/jDgSEx8SHWZvANYMioYMZ2KX02W6f6uVfyteuCGcadDLcYVHdfdygsTQkQ4TKn5lghoojAsPj5pu0SnvQ==", "license": "MIT", "dependencies": { "@kurkle/color": "^0.3.0" diff --git a/package.json b/package.json index f7f04b1c0b..b4d170fdb9 100644 --- a/package.json +++ b/package.json @@ -15,7 +15,7 @@ "@primer/octicons": "19.14.0", "ansi_up": "6.0.5", "asciinema-player": "3.8.2", - "chart.js": "4.4.9", + "chart.js": "4.5.0", "chartjs-adapter-dayjs-4": "1.0.4", "chartjs-plugin-zoom": "2.2.0", "clippie": "4.1.7", diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 75b4870e51..c9dda124de 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -71,7 +71,7 @@ func ListPullRequests(ctx *context.APIContext) { // in: query // description: Type of sort // type: string - // enum: [oldest, recentupdate, leastupdate, mostcomment, leastcomment, priority] + // enum: [oldest, recentupdate, recentclose, leastupdate, mostcomment, leastcomment, priority] // - name: milestone // in: query // description: ID of the milestone diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 2db982d3b6..fd18646211 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,78 @@ 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) + } + + 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 +1030,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi } else { startCommitID = prInfo.MergeBase } + ctx.Data["IsShowingAllCommits"] = false } else { endCommitID = headCommitID @@ -937,10 +1038,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 2568c6a853..534c37e918 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -1511,7 +1511,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/services/automerge/automerge.go b/services/automerge/automerge.go index f183136907..cbfe3bd54e 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -107,6 +107,7 @@ func handlePullRequestAutoMerge(pullID int64, sha string) { return } if !exists { + log.Trace("GetScheduledMergeByPullID found nothing for PR %d", pullID) return } @@ -204,6 +205,10 @@ func handlePullRequestAutoMerge(pullID int64, sha string) { return } + if err := pull_model.DeleteScheduledAutoMerge(ctx, pr.ID); err != nil && !db.IsErrNotExist(err) { + log.Error("DeleteScheduledAutoMerge[%d]: %v", pr.ID, err) + } + if err := pull_service.Merge(ctx, pr, doer, baseGitRepo, scheduledPRM.MergeStyle, "", scheduledPRM.Message, true); err != nil { log.Error("pull_service.Merge: %v", err) // FIXME: if merge failed, we should display some error message to the pull request page. diff --git a/services/pull/check.go b/services/pull/check.go index afeb7e675e..6002e2ae26 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -28,6 +28,7 @@ import ( "forgejo.org/modules/timeutil" asymkey_service "forgejo.org/services/asymkey" notify_service "forgejo.org/services/notify" + shared_automerge "forgejo.org/services/shared/automerge" ) // prPatchCheckerQueue represents a queue to handle update pull request tests @@ -170,7 +171,7 @@ func isSignedIfRequired(ctx context.Context, pr *issues_model.PullRequest, doer // checkAndUpdateStatus checks if pull request is possible to leaving checking status, // and set to be either conflict or mergeable. -func checkAndUpdateStatus(ctx context.Context, pr *issues_model.PullRequest) { +func checkAndUpdateStatus(ctx context.Context, pr *issues_model.PullRequest) bool { // If status has not been changed to conflict by testPatch then we are mergeable if pr.Status == issues_model.PullRequestStatusChecking { pr.Status = issues_model.PullRequestStatusMergeable @@ -184,12 +185,15 @@ func checkAndUpdateStatus(ctx context.Context, pr *issues_model.PullRequest) { if has { log.Trace("Not updating status for %-v as it is due to be rechecked", pr) - return + return false } if err := pr.UpdateColsIfNotMerged(ctx, "merge_base", "status", "conflicted_files", "changed_protected_files"); err != nil { log.Error("Update[%-v]: %v", pr, err) + return false } + + return true } // getMergeCommit checks if a pull request has been merged @@ -339,15 +343,22 @@ func handler(items ...string) []string { } func testPR(id int64) { - pullWorkingPool.CheckIn(fmt.Sprint(id)) - defer pullWorkingPool.CheckOut(fmt.Sprint(id)) ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("Test PR[%d] from patch checking queue", id)) defer finished() + if pr, updated := testPRProtected(ctx, id); pr != nil && updated { + shared_automerge.AddToQueueIfMergeable(ctx, pr) + } +} + +func testPRProtected(ctx context.Context, id int64) (*issues_model.PullRequest, bool) { + pullWorkingPool.CheckIn(fmt.Sprint(id)) + defer pullWorkingPool.CheckOut(fmt.Sprint(id)) + pr, err := issues_model.GetPullRequestByID(ctx, id) if err != nil { log.Error("Unable to GetPullRequestByID[%d] for testPR: %v", id, err) - return + return nil, false } log.Trace("Testing %-v", pr) @@ -357,12 +368,12 @@ func testPR(id int64) { if pr.HasMerged { log.Trace("%-v is already merged (status: %s, merge commit: %s)", pr, pr.Status, pr.MergedCommitID) - return + return nil, false } if manuallyMerged(ctx, pr) { log.Trace("%-v is manually merged (status: %s, merge commit: %s)", pr, pr.Status, pr.MergedCommitID) - return + return nil, false } if err := TestPatch(pr); err != nil { @@ -371,9 +382,10 @@ func testPR(id int64) { if err := pr.UpdateCols(ctx, "status"); err != nil { log.Error("update pr [%-v] status to PullRequestStatusError failed: %v", pr, err) } - return + return nil, false } - checkAndUpdateStatus(ctx, pr) + + return pr, checkAndUpdateStatus(ctx, pr) } // CheckPRsForBaseBranch check all pulls with baseBrannch diff --git a/services/shared/automerge/automerge.go b/services/shared/automerge/automerge.go index 1dc309f4b3..be7b2f6eb4 100644 --- a/services/shared/automerge/automerge.go +++ b/services/shared/automerge/automerge.go @@ -21,9 +21,9 @@ import ( var PRAutoMergeQueue *queue.WorkerPoolQueue[string] func addToQueue(pr *issues_model.PullRequest, sha string) { - log.Trace("Adding pullID: %d to the pull requests patch checking queue with sha %s", pr.ID, sha) + log.Trace("Adding pullID: %d to the automerge queue with sha %s", pr.ID, sha) if err := PRAutoMergeQueue.Push(fmt.Sprintf("%d_%s", pr.ID, sha)); err != nil { - log.Error("Error adding pullID: %d to the pull requests patch checking queue %v", pr.ID, err) + log.Error("Error adding pullID: %d to the automerge queue %v", pr.ID, err) } } @@ -43,32 +43,29 @@ func StartPRCheckAndAutoMergeBySHA(ctx context.Context, sha string, repo *repo_m return nil } -// StartPRCheckAndAutoMerge start an automerge check and auto merge task for a pull request func StartPRCheckAndAutoMerge(ctx context.Context, pull *issues_model.PullRequest) { if pull == nil || pull.HasMerged || !pull.CanAutoMerge() { return } - if err := pull.LoadBaseRepo(ctx); err != nil { - log.Error("LoadBaseRepo: %v", err) - return + commitID := pull.HeadCommitID + if commitID == "" { + commitID = getCommitIDFromRefName(ctx, pull) } - gitRepo, err := gitrepo.OpenRepository(ctx, pull.BaseRepo) - if err != nil { - log.Error("OpenRepository: %v", err) - return - } - defer gitRepo.Close() - commitID, err := gitRepo.GetRefCommitID(pull.GetGitRefName()) - if err != nil { - log.Error("GetRefCommitID: %v", err) + if commitID == "" { return } addToQueue(pull, commitID) } +var AddToQueueIfMergeable = func(ctx context.Context, pull *issues_model.PullRequest) { + if pull.Status == issues_model.PullRequestStatusMergeable { + StartPRCheckAndAutoMerge(ctx, pull) + } +} + func getPullRequestsByHeadSHA(ctx context.Context, sha string, repo *repo_model.Repository, filter func(*issues_model.PullRequest) bool) (map[int64]*issues_model.PullRequest, error) { gitRepo, err := gitrepo.OpenRepository(ctx, repo) if err != nil { @@ -118,3 +115,24 @@ func getPullRequestsByHeadSHA(ctx context.Context, sha string, repo *repo_model. return pulls, nil } + +func getCommitIDFromRefName(ctx context.Context, pull *issues_model.PullRequest) string { + if err := pull.LoadBaseRepo(ctx); err != nil { + log.Error("LoadBaseRepo: %v", err) + return "" + } + + gitRepo, err := gitrepo.OpenRepository(ctx, pull.BaseRepo) + if err != nil { + log.Error("OpenRepository: %v", err) + return "" + } + defer gitRepo.Close() + commitID, err := gitRepo.GetRefCommitID(pull.GetGitRefName()) + if err != nil { + log.Error("GetRefCommitID: %v", err) + return "" + } + + return commitID +} 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/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 124d5bd1bd..bba4ac4949 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -12994,6 +12994,7 @@ "enum": [ "oldest", "recentupdate", + "recentclose", "leastupdate", "mostcomment", "leastcomment", diff --git a/tests/e2e/declare_repos_test.go b/tests/e2e/declare_repos_test.go index 0ccb776ff7..4c68566a48 100644 --- a/tests/e2e/declare_repos_test.go +++ b/tests/e2e/declare_repos_test.go @@ -82,7 +82,7 @@ func newRepo(t *testing.T, userID int64, repoName string, fileChanges []FileChan for _, file := range fileChanges { for i, version := range file.Versions { operation := "update" - if i == 0 && file.Filename != "README.md" { + if i == 0 { operation = "create" } 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/README.md b/tests/integration/README.md index d83685388e..a6be7fe72c 100644 --- a/tests/integration/README.md +++ b/tests/integration/README.md @@ -71,11 +71,11 @@ TEST_MYSQL_HOST=localhost:3306 TEST_MYSQL_DBNAME=test?multiStatements=true TEST_ ### Run pgsql integration tests Setup a pgsql database inside docker ``` -docker run -e "POSTGRES_DB=test" -e POSTGRES_PASSWORD=postgres -p 5432:5432 --rm --name pgsql postgres:latest #(Ctrl-c to stop the database) +docker run -e "POSTGRES_DB=test" -e POSTGRES_PASSWORD=postgres POSTGRESQL_FSYNC=off POSTGRESQL_EXTRA_FLAGS="-c full_page_writes=off" -p 5432:5432 --rm --name pgsql data.forgejo.org/oci/bitnami/postgresql:16 #(Ctrl-c to stop the database) ``` Start tests based on the database container ``` -TEST_STORAGE_TYPE=local TEST_PGSQL_HOST=localhost:5432 TEST_PGSQL_DBNAME=test TEST_PGSQL_USERNAME=postgres TEST_PGSQL_PASSWORD=postgres make test-pgsql +TEST_STORAGE_TYPE=local TEST_PGSQL_HOST=localhost:5432 TEST_PGSQL_DBNAME=test TEST_PGSQL_USERNAME=postgres TEST_PGSQL_PASSWORD=postgres make 'test-pgsql#Test' ``` ### Running individual tests diff --git a/tests/integration/patch_status_test.go b/tests/integration/patch_status_test.go index 078051fe63..3ce1dc4cb9 100644 --- a/tests/integration/patch_status_test.go +++ b/tests/integration/patch_status_test.go @@ -4,6 +4,7 @@ package integration import ( + "context" "fmt" "net/http" "net/url" @@ -20,7 +21,10 @@ import ( user_model "forgejo.org/models/user" "forgejo.org/modules/git" "forgejo.org/modules/optional" + "forgejo.org/modules/test" + pull_service "forgejo.org/services/pull" files_service "forgejo.org/services/repository/files" + shared_automerge "forgejo.org/services/shared/automerge" "forgejo.org/tests" "github.com/stretchr/testify/assert" @@ -51,6 +55,20 @@ func TestPatchStatus(t *testing.T) { }) defer f() + testAutomergeQueued := func(t *testing.T, pr *issues_model.PullRequest, expected issues_model.PullRequestStatus) { + t.Helper() + + var actual issues_model.PullRequestStatus = -1 + defer test.MockVariableValue(&shared_automerge.AddToQueueIfMergeable, func(ctx context.Context, pull *issues_model.PullRequest) { + actual = pull.Status + })() + + pull_service.AddToTaskQueue(t.Context(), pr) + assert.Eventually(t, func() bool { + return expected == actual + }, time.Second*5, time.Millisecond*200) + } + testRepoFork(t, session, "user2", repo.Name, "org3", "forked-repo") forkRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "org3", Name: "forked-repo"}) @@ -91,7 +109,9 @@ func TestPatchStatus(t *testing.T) { require.NoError(t, git.NewCommand(t.Context(), "push", "fork", "HEAD:normal").Run(&git.RunOpts{Dir: dstPath})) testPullCreateDirectly(t, session, repo.OwnerName, repo.Name, repo.DefaultBranch, forkRepo.OwnerName, forkRepo.Name, "normal", "across repo normal") - test(t, unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadRepoID: forkRepo.ID, HeadBranch: "normal"}, "flow = 0")) + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadRepoID: forkRepo.ID, HeadBranch: "normal"}, "flow = 0") + test(t, pr) + testAutomergeQueued(t, pr, issues_model.PullRequestStatusMergeable) }) t.Run("Same repository", func(t *testing.T) { @@ -144,7 +164,9 @@ func TestPatchStatus(t *testing.T) { t.Run("Existing", func(t *testing.T) { defer tests.PrintCurrentTest(t)() - test(t, unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadRepoID: forkRepo.ID, HeadBranch: "normal"}, "flow = 0")) + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadRepoID: forkRepo.ID, HeadBranch: "normal"}, "flow = 0") + test(t, pr) + testAutomergeQueued(t, pr, issues_model.PullRequestStatusConflict) }) t.Run("New", func(t *testing.T) { 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/tests/integration/repo_generate_test.go b/tests/integration/repo_generate_test.go index d22e6ecc74..44987c14b0 100644 --- a/tests/integration/repo_generate_test.go +++ b/tests/integration/repo_generate_test.go @@ -15,6 +15,7 @@ import ( repo_model "forgejo.org/models/repo" "forgejo.org/models/unittest" user_model "forgejo.org/models/user" + "forgejo.org/modules/git" "forgejo.org/modules/optional" "forgejo.org/modules/setting" "forgejo.org/modules/test" @@ -43,9 +44,13 @@ func assertRepoCreateForm(t *testing.T, htmlDoc *HTMLDoc, owner *user_model.User // the template menu is loaded client-side, so don't assert the option exists assert.Equal(t, templateID, htmlDoc.GetInputValueByName("repo_template"), "Unexpected repo_template selection") - for _, name := range []string{"issue_labels", "gitignores", "license", "object_format_name"} { + for _, name := range []string{"issue_labels", "gitignores", "license"} { htmlDoc.AssertDropdownHasOptions(t, name) } + + if git.SupportHashSha256 { + htmlDoc.AssertDropdownHasOptions(t, "object_format_name") + } } func testRepoGenerate(t *testing.T, session *TestSession, templateID, templateOwnerName, templateRepoName string, user, generateOwner *user_model.User, generateRepoName string) { diff --git a/tools/lint-go-gopls.sh b/tools/lint-go-gopls.sh deleted file mode 100755 index a222ea14d7..0000000000 --- a/tools/lint-go-gopls.sh +++ /dev/null @@ -1,23 +0,0 @@ -#!/bin/bash -set -uo pipefail - -cd "$(dirname -- "${BASH_SOURCE[0]}")" && cd .. - -IGNORE_PATTERNS=( - "is deprecated" # TODO: fix these -) - -# lint all go files with 'gopls check' and look for lines starting with the -# current absolute path, indicating a error was found. This is necessary -# because the tool does not set non-zero exit code when errors are found. -# ref: https://github.com/golang/go/issues/67078 -ERROR_LINES=$("$GO" run "$GOPLS_PACKAGE" check "$@" 2>/dev/null | grep -E "^$PWD" | grep -vFf <(printf '%s\n' "${IGNORE_PATTERNS[@]}")); -NUM_ERRORS=$(echo -n "$ERROR_LINES" | wc -l) - -if [ "$NUM_ERRORS" -eq "0" ]; then - exit 0; -else - echo "$ERROR_LINES" - echo "Found $NUM_ERRORS 'gopls check' errors" - exit 1; -fi 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;