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

lafriks · 2017-10-13T22:41:42Z

Allows forking to organization even when user owns repository or has already forked to his user but still limits single fork per user/organization

codecov-io · 2017-10-13T23:07:42Z

Codecov Report

Merging #2699 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master   #2699      +/-   ##
=========================================
- Coverage   27.12%   27.1%   -0.03%     
=========================================
  Files          87      87              
  Lines       17176   17191      +15     
=========================================
  Hits         4659    4659              
- Misses      11837   11852      +15     
  Partials      680     680

Impacted Files	Coverage Δ
models/repo.go	`12.95% <0%> (-0.12%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32ca299...ea6d8eb. Read the comment docs.

lunny · 2017-10-14T03:22:54Z

models/repo.go

+		return false, err
+	}
+	for _, org := range user.Orgs {
+		if !org.HasForkedRepo(repo.ID) {


maybe

if org.HasForkedRepo(repo.ID) { return false } } return true

Not possible, since it would stop looping of all left organizations if first has already forked the repository.

@lunny No, that would not work correct. We are looking in this code to check if user has at least one organization that has not forked repository yet. And if user has forked && all orgs has forks than it is not possible to fork anymore - so return false

@lafriks OK. I see.

lunny · 2017-10-14T08:52:42Z

Otherwise LGTM

daviian · 2017-10-14T08:59:31Z

models/repo.go

+	if repo.OwnerID != user.ID && !user.HasForkedRepo(repo.ID) {
+		return true, nil
+	}
+	if err := user.GetOrganizations(true); err != nil {


This does retrieve all organizations not respecting access rights to organization.
There should also be a check that user is owner of org at least.

I think user.GetOwnedOrganizations() can be used.

daviian · 2017-10-14T09:03:04Z

routers/repo/pull.go

@@ -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 := !ctx.User.HasForkedRepo(forkRepo.ID)
+	ctx.Data["CanForkToUser"] = canForkToUser


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

Morlinest · 2017-10-14T12:00:20Z

models/repo.go

+	if err := user.GetOrganizations(true); err != nil {
+		return false, err
+	}
+	for _, org := range user.Orgs {


user.OwnedOrgs (with previous use of user.GetOwnedOrganizations())

Morlinest · 2017-10-14T13:17:51Z

routers/repo/pull.go

@@ -73,7 +75,19 @@ func getForkRepository(ctx *context.Context) *models.Repository {
 		ctx.Handle(500, "GetOrganizations", err)
 		return nil
 	}
-	ctx.Data["Orgs"] = ctx.User.Orgs
+	var orgs []*models.User
+	for _, org := range ctx.User.Orgs {


Same as above (https://github.com/go-gitea/gitea/pull/2699/files#diff-d4e2553a87eb8b329efd7335f028903cR648) as Orgs includes also organizations where you are in team that have only read rights.

ethantkoenig · 2017-10-14T18:34:22Z

Can we please add a test to prevent another regression?

lafriks · 2017-10-14T19:02:29Z

@lunny @Morlinest @daviian fixed all suggestions

lafriks · 2017-10-14T21:05:52Z

@ethantkoenig integration test added

ethantkoenig · 2017-10-14T21:14:47Z

integrations/repo_fork_test.go

 	"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 {


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

@ethantkoenig thanks, done

ethantkoenig · 2017-10-14T21:17:01Z

routers/repo/pull.go

+	ctx.Data["Orgs"] = orgs
+
+	if canForkToUser {
+		ctx.Data["ContextUser"] = ctx.User


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.

@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.

Oh, I missed that you had removed that line

ethantkoenig · 2017-10-14T21:48:57Z

routers/repo/pull.go

+	ctx.Data["Orgs"] = orgs
+
+	if canForkToUser {
+		ctx.Data["ContextUser"] = ctx.User


Oh, I missed that you had removed that line

ethantkoenig · 2017-10-14T21:49:09Z

LGTM

lunny · 2017-10-15T01:22:08Z

@daviian @Morlinest needs your approvals.

lafriks · 2017-10-15T14:57:14Z

Make LG-TM work again

…ions (go-gitea#2699) * Fix so that user can still fork his own repository to his organizations * Fix to only use owned organizations * Add integration test for forking own repository to owned organization

…ions (#2699) (#2707) * Fix so that user can still fork his own repository to his organizations * Fix to only use owned organizations * Add integration test for forking own repository to owned organization

lafriks added issue/regression Issue needs no code to be fixed, only a description on how to fix it yourself backport/v1.2 labels Oct 13, 2017

lafriks added this to the 1.3.0 milestone Oct 13, 2017

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 13, 2017

lunny requested changes Oct 14, 2017

View reviewed changes

tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 14, 2017

daviian suggested changes Oct 14, 2017

View reviewed changes

Morlinest suggested changes Oct 14, 2017

View reviewed changes

lafriks force-pushed the feat/fork_own_to_org branch from f7ddb53 to 4e27da8 Compare October 14, 2017 16:59

lafriks added 3 commits October 14, 2017 21:55

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

f09a125

Add nil check

d13427d

Fix to only use owned organizations

0f894db

lafriks force-pushed the feat/fork_own_to_org branch from 4e27da8 to 0f894db Compare October 14, 2017 19:01

lafriks added 2 commits October 14, 2017 22:07

Small code improvement

2b0d671

Add integration test for forkin own repository to owned organization

f86bd84

Fix typo

fa0d64e

ethantkoenig suggested changes Oct 14, 2017

View reviewed changes

Fix fork tests to load user ID by user name

f102aad

ethantkoenig approved these changes Oct 14, 2017

View reviewed changes

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 14, 2017

Merge branch 'master' into feat/fork_own_to_org

c1ca44a

lunny approved these changes Oct 15, 2017

View reviewed changes

Morlinest approved these changes Oct 15, 2017

View reviewed changes

daviian approved these changes Oct 15, 2017

View reviewed changes

Merge branch 'master' into feat/fork_own_to_org

ea6d8eb

lafriks merged commit 1ec4dc6 into go-gitea:master Oct 15, 2017

lafriks deleted the feat/fork_own_to_org branch October 15, 2017 15:06

lafriks mentioned this pull request Oct 15, 2017

Fix so that user can still fork his own repository to owned organizations (#2699) #2707

Merged

lafriks added the backport/done All backports for this PR have been created label Oct 15, 2017

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lafriks commented Oct 13, 2017

codecov-io commented Oct 13, 2017 •

edited

Loading

lunny Oct 14, 2017

daviian Oct 14, 2017

lafriks Oct 14, 2017

lunny Oct 14, 2017

lunny commented Oct 14, 2017

daviian Oct 14, 2017

Morlinest Oct 14, 2017

daviian Oct 14, 2017

Morlinest Oct 14, 2017

Morlinest Oct 14, 2017

ethantkoenig commented Oct 14, 2017

lafriks commented Oct 14, 2017

lafriks commented Oct 14, 2017 •

edited

Loading

ethantkoenig Oct 14, 2017

lafriks Oct 14, 2017

ethantkoenig Oct 14, 2017

lafriks Oct 14, 2017

ethantkoenig Oct 14, 2017

ethantkoenig Oct 14, 2017

ethantkoenig commented Oct 14, 2017

lunny commented Oct 15, 2017

lafriks commented Oct 15, 2017

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

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

Conversation

lafriks commented Oct 13, 2017

codecov-io commented Oct 13, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented Oct 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethantkoenig commented Oct 14, 2017

lafriks commented Oct 14, 2017

lafriks commented Oct 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethantkoenig commented Oct 14, 2017

lunny commented Oct 15, 2017

lafriks commented Oct 15, 2017

codecov-io commented Oct 13, 2017 •

edited

Loading

lafriks commented Oct 14, 2017 •

edited

Loading