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 <art27@cantab.net>
Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com>
This commit is contained in:
Kyle Evans 2020-05-04 17:44:30 -05:00 committed by GitHub
parent 680dfab2ee
commit 38cd9ba47b
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 48 additions and 40 deletions

View file

@ -547,23 +547,27 @@ func RepoAssignment() macaron.Handler {
ctx.Data["CommitID"] = ctx.Repo.CommitID ctx.Data["CommitID"] = ctx.Repo.CommitID
// People who have push access or have forked repository can propose a new pull request. // 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)) { 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 // Pull request is allowed if this is a fork repository
// and base repository accepts pull requests. // and base repository accepts pull requests.
if repo.BaseRepo != nil && repo.BaseRepo.AllowsPulls() { if repo.BaseRepo != nil && repo.BaseRepo.AllowsPulls() {
canCompare = true
ctx.Data["BaseRepo"] = repo.BaseRepo ctx.Data["BaseRepo"] = repo.BaseRepo
ctx.Repo.PullRequest.BaseRepo = repo.BaseRepo ctx.Repo.PullRequest.BaseRepo = repo.BaseRepo
ctx.Repo.PullRequest.Allowed = true ctx.Repo.PullRequest.Allowed = canPush
ctx.Repo.PullRequest.HeadInfo = ctx.Repo.Owner.Name + ":" + ctx.Repo.BranchName ctx.Repo.PullRequest.HeadInfo = ctx.Repo.Owner.Name + ":" + ctx.Repo.BranchName
} else if repo.AllowsPulls() { } else if repo.AllowsPulls() {
// Or, this is repository accepts pull requests between branches. // Or, this is repository accepts pull requests between branches.
canCompare = true
ctx.Data["BaseRepo"] = repo ctx.Data["BaseRepo"] = repo
ctx.Repo.PullRequest.BaseRepo = repo ctx.Repo.PullRequest.BaseRepo = repo
ctx.Repo.PullRequest.Allowed = true ctx.Repo.PullRequest.Allowed = canPush
ctx.Repo.PullRequest.SameRepo = true ctx.Repo.PullRequest.SameRepo = true
ctx.Repo.PullRequest.HeadInfo = ctx.Repo.BranchName ctx.Repo.PullRequest.HeadInfo = ctx.Repo.BranchName
} }
} ctx.Data["CanCompareOrPull"] = canCompare
ctx.Data["PullRequestCtx"] = ctx.Repo.PullRequest ctx.Data["PullRequestCtx"] = ctx.Repo.PullRequest
if ctx.Query("go-get") == "1" { if ctx.Query("go-get") == "1" {

View file

@ -2270,6 +2270,7 @@ transfer_repo = transferred repository <code>%s</code> to <a href="%s">%s</a>
push_tag = pushed tag <a href="%s/src/tag/%s">%[2]s</a> to <a href="%[1]s">%[3]s</a> push_tag = pushed tag <a href="%s/src/tag/%s">%[2]s</a> to <a href="%[1]s">%[3]s</a>
delete_tag = deleted tag %[2]s from <a href="%[1]s">%[3]s</a> delete_tag = deleted tag %[2]s from <a href="%[1]s">%[3]s</a>
delete_branch = deleted branch %[2]s from <a href="%[1]s">%[3]s</a> delete_branch = deleted branch %[2]s from <a href="%[1]s">%[3]s</a>
compare_branch = Compare
compare_commits = Compare %d commits compare_commits = Compare %d commits
compare_commits_general = Compare commits compare_commits_general = Compare commits
mirror_sync_push = synced commits to <a href="%[1]s/src/%[2]s">%[3]s</a> at <a href="%[1]s">%[4]s</a> from mirror mirror_sync_push = synced commits to <a href="%[1]s/src/%[2]s">%[3]s</a> at <a href="%[1]s">%[4]s</a> from mirror

View file

@ -46,6 +46,7 @@ func Branches(ctx *context.Context) {
ctx.Data["AllowsPulls"] = ctx.Repo.Repository.AllowsPulls() ctx.Data["AllowsPulls"] = ctx.Repo.Repository.AllowsPulls()
ctx.Data["IsWriter"] = ctx.Repo.CanWrite(models.UnitTypeCode) ctx.Data["IsWriter"] = ctx.Repo.CanWrite(models.UnitTypeCode)
ctx.Data["IsMirror"] = ctx.Repo.Repository.IsMirror 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["PageIsViewCode"] = true
ctx.Data["PageIsBranches"] = true ctx.Data["PageIsBranches"] = true

View file

@ -709,6 +709,9 @@ func RegisterRoutes(m *macaron.Macaron) {
m.Group("/milestone", func() { m.Group("/milestone", func() {
m.Get("/:id", repo.MilestoneIssuesAndPulls) m.Get("/:id", repo.MilestoneIssuesAndPulls)
}, reqRepoIssuesOrPullsReader, context.RepoRef()) }, 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()) }, context.RepoAssignment(), context.UnitTypes())
// Grouping for those endpoints that do require authentication // 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("/:id/:action", repo.ChangeMilestonStatus)
m.Post("/delete", repo.DeleteMilestone) m.Post("/delete", repo.DeleteMilestone)
}, context.RepoMustNotBeArchived(), reqRepoIssuesOrPullsWriter, context.RepoRef()) }, 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.Group("/pull", func() {
m.Post("/:index/target_branch", repo.UpdatePullRequestTarget) m.Post("/:index/target_branch", repo.UpdatePullRequestTarget)
}, context.RepoMustNotBeArchived()) }, context.RepoMustNotBeArchived())

View file

@ -81,13 +81,13 @@
</a> </a>
{{else if and (not .IsDeleted) $.AllowsPulls (gt .CommitsAhead 0)}} {{else if and (not .IsDeleted) $.AllowsPulls (gt .CommitsAhead 0)}}
<a href="{{$.RepoLink}}/compare/{{$.DefaultBranch | EscapePound}}...{{if ne $.Repository.Owner.Name $.Owner.Name}}{{$.Owner.Name}}:{{end}}{{.Name | EscapePound}}"> <a href="{{$.RepoLink}}/compare/{{$.DefaultBranch | EscapePound}}...{{if ne $.Repository.Owner.Name $.Owner.Name}}{{$.Owner.Name}}:{{end}}{{.Name | EscapePound}}">
<button id="new-pull-request" class="ui compact basic button">{{$.i18n.Tr "repo.pulls.compare_changes"}}</button> <button id="new-pull-request" class="ui compact basic button">{{if $.CanPull}}{{$.i18n.Tr "repo.pulls.compare_changes"}}{{else}}{{$.i18n.Tr "action.compare_branch"}}{{end}}</button>
</a> </a>
{{end}} {{end}}
{{else if and .LatestPullRequest.HasMerged .MergeMovedOn}} {{else if and .LatestPullRequest.HasMerged .MergeMovedOn}}
{{if and (not .IsDeleted) $.AllowsPulls (gt .CommitsAhead 0)}} {{if and (not .IsDeleted) $.AllowsPulls (gt .CommitsAhead 0)}}
<a href="{{$.RepoLink}}/compare/{{$.DefaultBranch | EscapePound}}...{{if ne $.Repository.Owner.Name $.Owner.Name}}{{$.Owner.Name}}:{{end}}{{.Name | EscapePound}}"> <a href="{{$.RepoLink}}/compare/{{$.DefaultBranch | EscapePound}}...{{if ne $.Repository.Owner.Name $.Owner.Name}}{{$.Owner.Name}}:{{end}}{{.Name | EscapePound}}">
<button id="new-pull-request" class="ui compact basic button">{{$.i18n.Tr "repo.pulls.compare_changes"}}</button> <button id="new-pull-request" class="ui compact basic button">{{if $.CanPull}}{{$.i18n.Tr "repo.pulls.compare_changes"}}{{else}}{{$.i18n.Tr "action.compare_branch"}}{{end}}</button>
</a> </a>
{{end}} {{end}}
{{else}} {{else}}

View file

@ -5,7 +5,7 @@
{{if .PageIsComparePull}} {{if .PageIsComparePull}}
<h2 class="ui header"> <h2 class="ui header">
{{if not .Repository.IsArchived}} {{if and $.IsSigned (not .Repository.IsArchived)}}
{{.i18n.Tr "repo.pulls.compare_changes"}} {{.i18n.Tr "repo.pulls.compare_changes"}}
<div class="sub header">{{.i18n.Tr "repo.pulls.compare_changes_desc"}}</div> <div class="sub header">{{.i18n.Tr "repo.pulls.compare_changes_desc"}}</div>
{{ else }} {{ else }}
@ -65,18 +65,20 @@
{{.i18n.Tr "repo.pulls.has_pull_request" $.RepoLink $.RepoRelPath .PullRequest.Index | Safe}} {{.i18n.Tr "repo.pulls.has_pull_request" $.RepoLink $.RepoRelPath .PullRequest.Index | Safe}}
</div> </div>
{{else}} {{else}}
{{if not .Repository.IsArchived}} {{if and $.IsSigned (not .Repository.IsArchived)}}
<div class="ui info message show-form-container"> <div class="ui info message show-form-container">
<button class="ui button green show-form">{{.i18n.Tr "repo.pulls.new"}}</button> <button class="ui button green show-form">{{.i18n.Tr "repo.pulls.new"}}</button>
</div> </div>
{{ else }} {{else if .Repository.IsArchived}}
<div class="ui warning message"> <div class="ui warning message">
{{.i18n.Tr "repo.archive.title"}} {{.i18n.Tr "repo.archive.title"}}
</div> </div>
{{end}} {{end}}
{{if $.IsSigned}}
<div class="pullrequest-form" style="display: none"> <div class="pullrequest-form" style="display: none">
{{template "repo/issue/new_form" .}} {{template "repo/issue/new_form" .}}
</div> </div>
{{end}}
{{template "repo/commits_table" .}} {{template "repo/commits_table" .}}
{{template "repo/diff/box" .}} {{template "repo/diff/box" .}}
{{end}} {{end}}

View file

@ -62,10 +62,10 @@
{{ $l := Subtract $n 1}} {{ $l := Subtract $n 1}}
<!-- If home page, show new PR. If not, show breadcrumb --> <!-- If home page, show new PR. If not, show breadcrumb -->
{{if eq $n 0}} {{if eq $n 0}}
{{if and .PullRequestCtx.Allowed .IsViewBranch (not .Repository.IsArchived)}} {{if and .CanCompareOrPull .IsViewBranch (not .Repository.IsArchived)}}
<div class="fitted item"> <div class="fitted item">
<a href="{{.BaseRepo.Link}}/compare/{{.BaseRepo.DefaultBranch | EscapePound}}...{{if ne .Repository.Owner.Name .BaseRepo.Owner.Name}}{{.Repository.Owner.Name}}:{{end}}{{.BranchName | EscapePound}}"> <a href="{{.BaseRepo.Link}}/compare/{{.BaseRepo.DefaultBranch | EscapePound}}...{{if ne .Repository.Owner.Name .BaseRepo.Owner.Name}}{{.Repository.Owner.Name}}:{{end}}{{.BranchName | EscapePound}}">
<button id="new-pull-request" class="ui compact basic button">{{.i18n.Tr "repo.pulls.compare_changes"}}</button> <button id="new-pull-request" class="ui compact basic button">{{if .PullRequestCtx.Allowed}}{{.i18n.Tr "repo.pulls.compare_changes"}}{{else}}{{.i18n.Tr "action.compare_branch"}}{{end}}</button>
</a> </a>
</div> </div>
{{end}} {{end}}