Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix so that user can still fork his own repository to his organizations #2699

Merged
merged 9 commits into from
Oct 15, 2017
Merged

Fix so that user can still fork his own repository to his organizations #2699

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f09a125
Fix so that user can still fork his own repository to his organizations
lafriks Oct 13, 2017
d13427d
Add nil check
lafriks Oct 13, 2017
0f894db
Fix to only use owned organizations
lafriks Oct 14, 2017
2b0d671
Small code improvement
lafriks Oct 14, 2017
f86bd84
Add integration test for forkin own repository to owned organization
lafriks Oct 14, 2017
fa0d64e
Fix typo
lafriks Oct 14, 2017
f102aad
Fix fork tests to load user ID by user name
lafriks Oct 14, 2017
c1ca44a
Merge branch 'master' into feat/fork_own_to_org
lafriks Oct 14, 2017
ea6d8eb
Merge branch 'master' into feat/fork_own_to_org
lafriks Oct 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion integrations/pull_create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func testPullCreate(t *testing.T, session *TestSession, user, repo, branch strin
func TestPullCreate(t *testing.T) {
prepareTestEnv(t)
session := loginUser(t, "user1")
testRepoFork(t, session)
testRepoFork(t, session, "user2", "repo1", "user1", "1", "repo1")
testEditFile(t, session, "user1", "repo1", "master", "README.md")
testPullCreate(t, session, "user1", "repo1", "master")
}
4 changes: 2 additions & 2 deletions integrations/pull_merge_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func testPullCleanUp(t *testing.T, session *TestSession, user, repo, pullnum str
func TestPullMerge(t *testing.T) {
prepareTestEnv(t)
session := loginUser(t, "user1")
testRepoFork(t, session)
testRepoFork(t, session, "user2", "repo1", "user1", "1", "repo1")
testEditFile(t, session, "user1", "repo1", "master", "README.md")

resp := testPullCreate(t, session, "user1", "repo1", "master")
Expand All @@ -61,7 +61,7 @@ func TestPullMerge(t *testing.T) {
func TestPullCleanUpAfterMerge(t *testing.T) {
prepareTestEnv(t)
session := loginUser(t, "user1")
testRepoFork(t, session)
testRepoFork(t, session, "user2", "repo1", "user1", "1", "repo1")
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "feature/test", "README.md")

resp := testPullCreate(t, session, "user1", "repo1", "feature/test")
Expand Down
31 changes: 24 additions & 7 deletions integrations/repo_fork_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,20 @@
package integrations

import (
"fmt"
"net/http"
"testing"

"github.com/stretchr/testify/assert"
)

func testRepoFork(t *testing.T, session *TestSession) *TestResponse {
func testRepoFork(t *testing.T, session *TestSession, ownerName, repoName, forkOwnerName, forkOwnerID, forkRepoName string) *TestResponse {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: instead of requiring caller to specify forkOwnerID, we can infer it from forkOwnerName:

forkOwner := AssertExistsAndLoadBean(t, &models.User{Name: forkOwnerName}).(*models.User)
...
forkOwner.ID

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ethantkoenig thanks, done

// Step0: check the existence of the to-fork repo
req := NewRequest(t, "GET", "/user1/repo1")
req := NewRequestf(t, "GET", "/%s/%s", forkOwnerName, forkRepoName)
resp := session.MakeRequest(t, req, http.StatusNotFound)

// Step1: go to the main page of repo
req = NewRequest(t, "GET", "/user2/repo1")
req = NewRequestf(t, "GET", "/%s/%s", ownerName, repoName)
resp = session.MakeRequest(t, req, http.StatusOK)

// Step2: click the fork button
Expand All @@ -31,15 +32,17 @@ func testRepoFork(t *testing.T, session *TestSession) *TestResponse {
htmlDoc = NewHTMLParser(t, resp.Body)
link, exists = htmlDoc.doc.Find("form.ui.form[action^=\"/repo/fork/\"]").Attr("action")
assert.True(t, exists, "The template has changed")
_, exists = htmlDoc.doc.Find(fmt.Sprintf(".owner.dropdown .item[data-value=\"%s\"]", forkOwnerID)).Attr("data-value")
assert.True(t, exists, fmt.Sprintf("Fork owner '%s' is not present in select box", forkOwnerName))
req = NewRequestWithValues(t, "POST", link, map[string]string{
"_csrf": htmlDoc.GetCSRF(),
"uid": "1",
"repo_name": "repo1",
"uid": forkOwnerID,
"repo_name": forkRepoName,
})
resp = session.MakeRequest(t, req, http.StatusFound)

// Step4: check the existence of the forked repo
req = NewRequest(t, "GET", "/user1/repo1")
req = NewRequestf(t, "GET", "/%s/%s", forkOwnerName, forkRepoName)
resp = session.MakeRequest(t, req, http.StatusOK)

return resp
Expand All @@ -48,5 +51,19 @@ func testRepoFork(t *testing.T, session *TestSession) *TestResponse {
func TestRepoFork(t *testing.T) {
prepareTestEnv(t)
session := loginUser(t, "user1")
testRepoFork(t, session)
testRepoFork(t, session, "user2", "repo1", "user1", "1", "repo1")
}

func TestRepoForkToOrg(t *testing.T) {
prepareTestEnv(t)
session := loginUser(t, "user2")
testRepoFork(t, session, "user2", "repo1", "user3", "3", "repo1")

// Check that no more forking is allowed as user2 owns repository
// and user3 organization that owner user2 is also now has forked this repository
req := NewRequest(t, "GET", "/user2/repo1")
resp := session.MakeRequest(t, req, http.StatusOK)
htmlDoc := NewHTMLParser(t, resp.Body)
_, exists := htmlDoc.doc.Find("a.ui.button[href^=\"/repo/fork/\"]").Attr("href")
assert.False(t, exists, "Forking should not be allowed anymore")
}
19 changes: 19 additions & 0 deletions models/repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,25 @@ func (repo *Repository) CanBeForked() bool {
return !repo.IsBare && repo.UnitEnabled(UnitTypeCode)
}

// CanUserFork returns true if specified user can fork repository.
func (repo *Repository) CanUserFork(user *User) (bool, error) {
if user == nil {
return false, nil
}
if repo.OwnerID != user.ID && !user.HasForkedRepo(repo.ID) {
return true, nil
}
if err := user.GetOwnedOrganizations(); err != nil {
return false, err
}
for _, org := range user.OwnedOrgs {
if repo.OwnerID != org.ID && !org.HasForkedRepo(repo.ID) {
return true, nil
}
}
return false, nil
}

// CanEnablePulls returns true if repository meets the requirements of accepting pulls.
func (repo *Repository) CanEnablePulls() bool {
return !repo.IsMirror && !repo.IsBare
Expand Down
5 changes: 5 additions & 0 deletions modules/context/repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,11 @@ func RepoAssignment() macaron.Handler {
ctx.Data["IsRepositoryAdmin"] = ctx.Repo.IsAdmin()
ctx.Data["IsRepositoryWriter"] = ctx.Repo.IsWriter()

if ctx.Data["CanSignedUserFork"], err = ctx.Repo.Repository.CanUserFork(ctx.User); err != nil {
ctx.Handle(500, "CanUserFork", err)
return
}

ctx.Data["DisableSSH"] = setting.SSH.Disabled
ctx.Data["ExposeAnonSSH"] = setting.SSH.ExposeAnonymous
ctx.Data["DisableHTTP"] = setting.Repository.DisableHTTPGit
Expand Down
26 changes: 20 additions & 6 deletions routers/repo/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ func getForkRepository(ctx *context.Context) *models.Repository {
ctx.Data["repo_name"] = forkRepo.Name
ctx.Data["description"] = forkRepo.Description
ctx.Data["IsPrivate"] = forkRepo.IsPrivate
canForkToUser := forkRepo.OwnerID != ctx.User.ID && !ctx.User.HasForkedRepo(forkRepo.ID)
ctx.Data["CanForkToUser"] = canForkToUser
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should a user be able to fork his own repo into his user?


if err = forkRepo.GetOwner(); err != nil {
ctx.Handle(500, "GetOwner", err)
Expand All @@ -69,11 +71,23 @@ func getForkRepository(ctx *context.Context) *models.Repository {
ctx.Data["ForkFrom"] = forkRepo.Owner.Name + "/" + forkRepo.Name
ctx.Data["ForkFromOwnerID"] = forkRepo.Owner.ID

if err := ctx.User.GetOrganizations(true); err != nil {
ctx.Handle(500, "GetOrganizations", err)
if err := ctx.User.GetOwnedOrganizations(); err != nil {
ctx.Handle(500, "GetOwnedOrganizations", err)
return nil
}
ctx.Data["Orgs"] = ctx.User.Orgs
var orgs []*models.User
for _, org := range ctx.User.OwnedOrgs {
if forkRepo.OwnerID != org.ID && !org.HasForkedRepo(forkRepo.ID) {
orgs = append(orgs, org)
}
}
ctx.Data["Orgs"] = orgs

if canForkToUser {
ctx.Data["ContextUser"] = ctx.User
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In both places where getForkRepository() is called (Fork and ForkPost in routers/repo/pull.go), ctx.Data["ContextUser"] is immediately overwritten after this function is called.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ethantkoenig it is not overwritten in Fork handler only in ForkPost. It is intentional so - getForkRepository fills in default ctx.Data["ContextUser"] value and in ForkPost it is overwritten by actually selected one from dropdown box.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I missed that you had removed that line

} else if len(orgs) > 0 {
ctx.Data["ContextUser"] = orgs[0]
}

return forkRepo
}
Expand All @@ -87,23 +101,23 @@ func Fork(ctx *context.Context) {
return
}

ctx.Data["ContextUser"] = ctx.User
ctx.HTML(200, tplFork)
}

// ForkPost response for forking a repository
func ForkPost(ctx *context.Context, form auth.CreateRepoForm) {
ctx.Data["Title"] = ctx.Tr("new_fork")

forkRepo := getForkRepository(ctx)
ctxUser := checkContextUser(ctx, form.UID)
if ctx.Written() {
return
}

ctxUser := checkContextUser(ctx, form.UID)
forkRepo := getForkRepository(ctx)
if ctx.Written() {
return
}

ctx.Data["ContextUser"] = ctxUser

if ctx.HasError() {
Expand Down
2 changes: 1 addition & 1 deletion templates/repo/header.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
</div>
{{if .CanBeForked}}
<div class="ui compact labeled button" tabindex="0">
<a class="ui compact button {{if eq .OwnerID $.SignedUserID}}poping up{{end}}" {{if not (eq .OwnerID $.SignedUserID)}}href="{{AppSubUrl}}/repo/fork/{{.ID}}"{{else}} data-content="{{$.i18n.Tr "repo.fork_from_self"}}" data-position="top center" data-variation="tiny"{{end}}>
<a class="ui compact button {{if not $.CanSignedUserFork}}poping up{{end}}" {{if $.CanSignedUserFork}}href="{{AppSubUrl}}/repo/fork/{{.ID}}"{{else}} data-content="{{$.i18n.Tr "repo.fork_from_self"}}" data-position="top center" data-variation="tiny"{{end}}>
<i class="octicon octicon-repo-forked"></i>{{$.i18n.Tr "repo.fork"}}
</a>
<a class="ui basic label" href="{{.Link}}/forks">
Expand Down
20 changes: 10 additions & 10 deletions templates/repo/pulls/fork.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,17 @@
</span>
<i class="dropdown icon"></i>
<div class="menu">
<div class="item" data-value="{{.SignedUser.ID}}">
<img class="ui mini image" src="{{.SignedUser.RelAvatarLink}}">
{{.SignedUser.ShortName 20}}
</div>
{{if .CanForkToUser}}
<div class="item" data-value="{{.SignedUser.ID}}">
<img class="ui mini image" src="{{.SignedUser.RelAvatarLink}}">
{{.SignedUser.ShortName 20}}
</div>
{{end}}
{{range .Orgs}}
{{if and (.IsOwnedBy $.SignedUser.ID) (ne .ID $.ForkFromOwnerID)}}
<div class="item" data-value="{{.ID}}">
<img class="ui mini image" src="{{.RelAvatarLink}}">
{{.ShortName 20}}
</div>
{{end}}
<div class="item" data-value="{{.ID}}">
<img class="ui mini image" src="{{.RelAvatarLink}}">
{{.ShortName 20}}
</div>
{{end}}
</div>
</div>
Expand Down