From 38cd9ba47b0f9b70e8638f8caf77b3effe50eef4 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Mon, 4 May 2020 17:44:30 -0500 Subject: [PATCH] Allow unauthenticated users to compare (#11240) * routers: make /compare route available to unauthenticated users Remove some bits of the compare interface if the user isn't signed in. Notably, they don't need to see the "New Pull Request" button box nor the hidden form that would fail to submit due to the POST request continuing to require proper privileges. Follow-up commits will improve the UI a bit around this, removing some "Pull Request" verbiage in favor of "Compare." * ui: home: show "compare" button for unauthenticated users This change requires pulling in the BaseRepo unconditionally and recording if the pull request is in-fact not allowed (.PullRequestCtx.Allowed). If the user isn't allowed to create a pull request, either because this isn't a fork or same-fork branch PRs aren't allowed, then we'll name the button "Compare" instead of "Pull Request." * ui: branch list: use the new Compare language when available When viewing the branch listing as an unauthenticated user, you'll get "Pull Request" buttons. use the new "Compare" verbiage instead, which matches GitHub behavior when you can't issue a pull request from the branches. Co-authored-by: zeripath Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com> --- modules/context/repo.go | 36 ++++++++++++++++++-------------- options/locale/locale_en-US.ini | 1 + routers/repo/branch.go | 1 + routers/routes/routes.go | 6 +++--- templates/repo/branch/list.tmpl | 4 ++-- templates/repo/diff/compare.tmpl | 36 +++++++++++++++++--------------- templates/repo/home.tmpl | 4 ++-- 7 files changed, 48 insertions(+), 40 deletions(-) diff --git a/modules/context/repo.go b/modules/context/repo.go index 841dcd960..5ebed0eb7 100644 --- a/modules/context/repo.go +++ b/modules/context/repo.go @@ -547,23 +547,27 @@ func RepoAssignment() macaron.Handler { ctx.Data["CommitID"] = ctx.Repo.CommitID // People who have push access or have forked repository can propose a new pull request. - if ctx.Repo.CanWrite(models.UnitTypeCode) || (ctx.IsSigned && ctx.User.HasForkedRepo(ctx.Repo.Repository.ID)) { - // Pull request is allowed if this is a fork repository - // and base repository accepts pull requests. - if repo.BaseRepo != nil && repo.BaseRepo.AllowsPulls() { - ctx.Data["BaseRepo"] = repo.BaseRepo - ctx.Repo.PullRequest.BaseRepo = repo.BaseRepo - ctx.Repo.PullRequest.Allowed = true - ctx.Repo.PullRequest.HeadInfo = ctx.Repo.Owner.Name + ":" + ctx.Repo.BranchName - } else if repo.AllowsPulls() { - // Or, this is repository accepts pull requests between branches. - ctx.Data["BaseRepo"] = repo - ctx.Repo.PullRequest.BaseRepo = repo - ctx.Repo.PullRequest.Allowed = true - ctx.Repo.PullRequest.SameRepo = true - ctx.Repo.PullRequest.HeadInfo = ctx.Repo.BranchName - } + canPush := ctx.Repo.CanWrite(models.UnitTypeCode) || (ctx.IsSigned && ctx.User.HasForkedRepo(ctx.Repo.Repository.ID)) + canCompare := false + + // Pull request is allowed if this is a fork repository + // and base repository accepts pull requests. + if repo.BaseRepo != nil && repo.BaseRepo.AllowsPulls() { + canCompare = true + ctx.Data["BaseRepo"] = repo.BaseRepo + ctx.Repo.PullRequest.BaseRepo = repo.BaseRepo + ctx.Repo.PullRequest.Allowed = canPush + ctx.Repo.PullRequest.HeadInfo = ctx.Repo.Owner.Name + ":" + ctx.Repo.BranchName + } else if repo.AllowsPulls() { + // Or, this is repository accepts pull requests between branches. + canCompare = true + ctx.Data["BaseRepo"] = repo + ctx.Repo.PullRequest.BaseRepo = repo + ctx.Repo.PullRequest.Allowed = canPush + ctx.Repo.PullRequest.SameRepo = true + ctx.Repo.PullRequest.HeadInfo = ctx.Repo.BranchName } + ctx.Data["CanCompareOrPull"] = canCompare ctx.Data["PullRequestCtx"] = ctx.Repo.PullRequest if ctx.Query("go-get") == "1" { diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index fe6857352..37efecbff 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -2270,6 +2270,7 @@ transfer_repo = transferred repository %s to %s push_tag = pushed tag %[2]s to %[3]s delete_tag = deleted tag %[2]s from %[3]s delete_branch = deleted branch %[2]s from %[3]s +compare_branch = Compare compare_commits = Compare %d commits compare_commits_general = Compare commits mirror_sync_push = synced commits to %[3]s at %[4]s from mirror diff --git a/routers/repo/branch.go b/routers/repo/branch.go index 615450b36..1664f68ec 100644 --- a/routers/repo/branch.go +++ b/routers/repo/branch.go @@ -46,6 +46,7 @@ func Branches(ctx *context.Context) { ctx.Data["AllowsPulls"] = ctx.Repo.Repository.AllowsPulls() ctx.Data["IsWriter"] = ctx.Repo.CanWrite(models.UnitTypeCode) ctx.Data["IsMirror"] = ctx.Repo.Repository.IsMirror + ctx.Data["CanPull"] = ctx.Repo.CanWrite(models.UnitTypeCode) || (ctx.IsSigned && ctx.User.HasForkedRepo(ctx.Repo.Repository.ID)) ctx.Data["PageIsViewCode"] = true ctx.Data["PageIsBranches"] = true diff --git a/routers/routes/routes.go b/routers/routes/routes.go index f3bd42f02..ac0f3f4f1 100644 --- a/routers/routes/routes.go +++ b/routers/routes/routes.go @@ -709,6 +709,9 @@ func RegisterRoutes(m *macaron.Macaron) { m.Group("/milestone", func() { m.Get("/:id", repo.MilestoneIssuesAndPulls) }, reqRepoIssuesOrPullsReader, context.RepoRef()) + m.Combo("/compare/*", repo.MustBeNotEmpty, reqRepoCodeReader, repo.SetEditorconfigIfExists). + Get(repo.SetDiffViewStyle, repo.CompareDiff). + Post(reqSignIn, context.RepoMustNotBeArchived(), reqRepoPullsReader, repo.MustAllowPulls, bindIgnErr(auth.CreateIssueForm{}), repo.CompareAndPullRequestPost) }, context.RepoAssignment(), context.UnitTypes()) // Grouping for those endpoints that do require authentication @@ -769,9 +772,6 @@ func RegisterRoutes(m *macaron.Macaron) { m.Post("/:id/:action", repo.ChangeMilestonStatus) m.Post("/delete", repo.DeleteMilestone) }, context.RepoMustNotBeArchived(), reqRepoIssuesOrPullsWriter, context.RepoRef()) - m.Combo("/compare/*", repo.MustBeNotEmpty, reqRepoCodeReader, repo.SetEditorconfigIfExists). - Get(repo.SetDiffViewStyle, repo.CompareDiff). - Post(context.RepoMustNotBeArchived(), reqRepoPullsReader, repo.MustAllowPulls, bindIgnErr(auth.CreateIssueForm{}), repo.CompareAndPullRequestPost) m.Group("/pull", func() { m.Post("/:index/target_branch", repo.UpdatePullRequestTarget) }, context.RepoMustNotBeArchived()) diff --git a/templates/repo/branch/list.tmpl b/templates/repo/branch/list.tmpl index b310edfbd..d2eedef49 100644 --- a/templates/repo/branch/list.tmpl +++ b/templates/repo/branch/list.tmpl @@ -81,13 +81,13 @@ {{else if and (not .IsDeleted) $.AllowsPulls (gt .CommitsAhead 0)}} - + {{end}} {{else if and .LatestPullRequest.HasMerged .MergeMovedOn}} {{if and (not .IsDeleted) $.AllowsPulls (gt .CommitsAhead 0)}} - + {{end}} {{else}} diff --git a/templates/repo/diff/compare.tmpl b/templates/repo/diff/compare.tmpl index b08863643..35a37ab76 100644 --- a/templates/repo/diff/compare.tmpl +++ b/templates/repo/diff/compare.tmpl @@ -5,7 +5,7 @@ {{if .PageIsComparePull}}

- {{if not .Repository.IsArchived}} + {{if and $.IsSigned (not .Repository.IsArchived)}} {{.i18n.Tr "repo.pulls.compare_changes"}}
{{.i18n.Tr "repo.pulls.compare_changes_desc"}}
{{ else }} @@ -64,22 +64,24 @@
{{.i18n.Tr "repo.pulls.has_pull_request" $.RepoLink $.RepoRelPath .PullRequest.Index | Safe}}
- {{else}} - {{if not .Repository.IsArchived}} -
- -
- {{ else }} -
- {{.i18n.Tr "repo.archive.title"}} -
- {{ end }} - - {{template "repo/commits_table" .}} - {{template "repo/diff/box" .}} - {{end}} + {{else}} + {{if and $.IsSigned (not .Repository.IsArchived)}} +
+ +
+ {{else if .Repository.IsArchived}} +
+ {{.i18n.Tr "repo.archive.title"}} +
+ {{end}} + {{if $.IsSigned}} + + {{end}} + {{template "repo/commits_table" .}} + {{template "repo/diff/box" .}} + {{end}} {{else}} {{template "repo/commits_table" .}} {{template "repo/diff/box" .}} diff --git a/templates/repo/home.tmpl b/templates/repo/home.tmpl index 51e924747..3a0d364f3 100644 --- a/templates/repo/home.tmpl +++ b/templates/repo/home.tmpl @@ -62,10 +62,10 @@ {{ $l := Subtract $n 1}} {{if eq $n 0}} - {{if and .PullRequestCtx.Allowed .IsViewBranch (not .Repository.IsArchived)}} + {{if and .CanCompareOrPull .IsViewBranch (not .Repository.IsArchived)}}
- +
{{end}}