Skip to content

hide issues from org private repos w/o team assignment #4034

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

Merged
merged 16 commits into from
Jun 21, 2018
Merged

hide issues from org private repos w/o team assignment #4034

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
6f46ee3
hide issues from org private repos w/o team assignment
daviian May 24, 2018
0e3c279
add comments to altered tests
daviian May 24, 2018
0b4402b
fix test
daviian May 24, 2018
2d77d18
fix null value on bool field
daviian May 24, 2018
17b946e
add suggested changes
daviian May 24, 2018
8ea95f6
further improvements
daviian May 24, 2018
a122731
move team unittypes to new team_unit table
daviian Jun 3, 2018
cab5121
limit GetUserRepositoryIDs by accessible unittypes
daviian Jun 3, 2018
a1fdf48
check unit types on action and notification creation
daviian Jun 5, 2018
7b990f7
adjust test to take new permission checks into account
daviian Jun 5, 2018
4ea560c
load repo before actually using it
daviian Jun 5, 2018
abdaa54
fix some things
daviian Jun 5, 2018
4e21fc7
undo mistakenly moved code
daviian Jun 5, 2018
498357b
add unique indexes
daviian Jun 17, 2018
d7b0a49
remove team-units if team gets deleted
daviian Jun 18, 2018
f5a3465
remove team-units if org gets deleted
daviian Jun 18, 2018
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
6 changes: 3 additions & 3 deletions integrations/api_repo_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,9 @@ func TestAPISearchRepo(t *testing.T) {
expectedResults
}{
{name: "RepositoriesMax50", requestURL: "/api/v1/repos/search?limit=50", expectedResults: expectedResults{
nil: {count: 15},
user: {count: 15},
user2: {count: 15}},
nil: {count: 16},
user: {count: 16},
user2: {count: 16}},
},
{name: "RepositoriesMax10", requestURL: "/api/v1/repos/search?limit=10", expectedResults: expectedResults{
nil: {count: 10},
Expand Down
16 changes: 12 additions & 4 deletions models/access_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,12 @@ func TestAccessLevel(t *testing.T) {

user1 := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
user2 := AssertExistsAndLoadBean(t, &User{ID: 5}).(*User)
repo1 := AssertExistsAndLoadBean(t, &Repository{OwnerID: 2, IsPrivate: false}).(*Repository)
repo2 := AssertExistsAndLoadBean(t, &Repository{OwnerID: 3, IsPrivate: true}).(*Repository)
// A public repository owned by User 2
repo1 := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove the IsPrivate: false part?

assert.False(t, repo1.IsPrivate)
// A private repository owned by Org 3
repo2 := AssertExistsAndLoadBean(t, &Repository{ID: 3}).(*Repository)
assert.True(t, repo2.IsPrivate)

level, err := AccessLevel(user1.ID, repo1)
assert.NoError(t, err)
Expand All @@ -47,8 +51,12 @@ func TestHasAccess(t *testing.T) {

user1 := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
user2 := AssertExistsAndLoadBean(t, &User{ID: 5}).(*User)
repo1 := AssertExistsAndLoadBean(t, &Repository{OwnerID: 2, IsPrivate: false}).(*Repository)
repo2 := AssertExistsAndLoadBean(t, &Repository{OwnerID: 3, IsPrivate: true}).(*Repository)
// A public repository owned by User 2
repo1 := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove the IsPrivate: false part?

Copy link
Member

@jonasfranz jonasfranz May 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

[13:16] lunny: JonasFranz because bool has default value false, Don’t know it means false or null
[13:16] lunny: Sess.Get has the same behavior
[13:16] JonasFranz: Okay, so we need to check it afterwards?

Since it is not supported to use bools in xorm, I propose to add an check if IsPrivate is set right:

assert.False(t, repo1.IsPrivate)

or

assert.True(t, repo1.IsPrivate)

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed it such that it really loads the expected repository. The resulting repository is the same, just an assurance.
That's applicable to the other changes in access_test.go too.

Copy link
Member

Choose a reason for hiding this comment

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

But it would be good to check that the repository is really private/public a comment does not ensure that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can add it of course, just think that it's nothing that adds value since its predefined by the fixtures anyway.

assert.False(t, repo1.IsPrivate)
// A private repository owned by Org 3
repo2 := AssertExistsAndLoadBean(t, &Repository{ID: 3}).(*Repository)
assert.True(t, repo2.IsPrivate)

for _, accessMode := range accessModes {
has, err := HasAccess(user1.ID, repo1, accessMode)
Expand Down
15 changes: 13 additions & 2 deletions models/fixtures/repository.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@
is_mirror: true
num_forks: 1
is_fork: false

-
id: 29
fork_id: 27
Expand All @@ -365,7 +365,7 @@
num_closed_pulls: 0
is_mirror: false
is_fork: true

-
id: 30
fork_id: 28
Expand All @@ -389,3 +389,14 @@
num_forks: 0
num_issues: 0
is_mirror: false

-
id: 32
owner_id: 3
lower_name: repo21
name: repo21
is_private: false
num_stars: 0
num_forks: 0
num_issues: 0
is_mirror: false
8 changes: 1 addition & 7 deletions models/fixtures/team.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,8 @@
lower_name: owners
name: Owners
authorize: 4 # owner
num_repos: 2
num_repos: 3
num_members: 1
unit_types: '[1,2,3,4,5,6,7]'

-
id: 2
Expand All @@ -16,7 +15,6 @@
authorize: 2 # write
num_repos: 1
num_members: 2
unit_types: '[1,2,3,4,5,6,7]'

-
id: 3
Expand All @@ -26,7 +24,6 @@
authorize: 4 # owner
num_repos: 0
num_members: 1
unit_types: '[1,2,3,4,5,6,7]'

-
id: 4
Expand All @@ -36,7 +33,6 @@
authorize: 4 # owner
num_repos: 0
num_members: 1
unit_types: '[1,2,3,4,5,6,7]'

-
id: 5
Expand All @@ -46,7 +42,6 @@
authorize: 4 # owner
num_repos: 2
num_members: 2
unit_types: '[1,2,3,4,5,6,7]'

-
id: 6
Expand All @@ -56,4 +51,3 @@
authorize: 4 # owner
num_repos: 2
num_members: 1
unit_types: '[1,2,3,4,5,6,7]'
10 changes: 8 additions & 2 deletions models/fixtures/team_repo.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,15 @@
org_id: 19
team_id: 6
repo_id: 27

-
id: 7
org_id: 19
team_id: 6
repo_id: 28
repo_id: 28

-
id: 8
org_id: 3
team_id: 1
repo_id: 32
209 changes: 209 additions & 0 deletions models/fixtures/team_unit.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,209 @@
-
id: 1
team_id: 1
type: 1

-
id: 2
team_id: 1
type: 2

-
id: 3
team_id: 1
type: 3

-
id: 4
team_id: 1
type: 4

-
id: 5
team_id: 1
type: 5

-
id: 6
team_id: 1
type: 6

-
id: 7
team_id: 1
type: 7

-
id: 8
team_id: 2
type: 1

-
id: 9
team_id: 2
type: 2

-
id: 10
team_id: 2
type: 3

-
id: 11
team_id: 2
type: 4

-
id: 12
team_id: 2
type: 5

-
id: 13
team_id: 2
type: 6

-
id: 14
team_id: 2
type: 7

-
id: 15
team_id: 3
type: 1

-
id: 16
team_id: 3
type: 2

-
id: 17
team_id: 3
type: 3

-
id: 18
team_id: 3
type: 4

-
id: 19
team_id: 3
type: 5

-
id: 20
team_id: 3
type: 6

-
id: 21
team_id: 3
type: 7

-
id: 22
team_id: 4
type: 1

-
id: 23
team_id: 4
type: 2

-
id: 24
team_id: 4
type: 3

-
id: 25
team_id: 4
type: 4

-
id: 26
team_id: 4
type: 5

-
id: 27
team_id: 4
type: 6

-
id: 28
team_id: 4
type: 7

-
id: 29
team_id: 5
type: 1

-
id: 30
team_id: 5
type: 2

-
id: 31
team_id: 5
type: 3

-
id: 32
team_id: 5
type: 4

-
id: 33
team_id: 5
type: 5

-
id: 34
team_id: 5
type: 6

-
id: 35
team_id: 5
type: 7

-
id: 36
team_id: 6
type: 1

-
id: 37
team_id: 6
type: 2

-
id: 38
team_id: 6
type: 3

-
id: 39
team_id: 6
type: 4

-
id: 40
team_id: 6
type: 5

-
id: 41
team_id: 6
type: 6

-
id: 42
team_id: 6
type: 7
2 changes: 1 addition & 1 deletion models/fixtures/user.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
is_admin: false
avatar: avatar3
avatar_email: [email protected]
num_repos: 2
num_repos: 3
num_members: 2
num_teams: 2

Expand Down
2 changes: 2 additions & 0 deletions models/migrations/migrations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,8 @@ var migrations = []Migration{
NewMigration("remove stale watches", removeStaleWatches),
// v68 -> V69
NewMigration("Reformat and remove incorrect topics", reformatAndRemoveIncorrectTopics),
// v69 -> v70
NewMigration("move team units to team_unit table", moveTeamUnitsToTeamUnitTable),
}

// Migrate database to current version
Expand Down
9 changes: 7 additions & 2 deletions models/migrations/v38.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,18 +25,23 @@ func removeCommitsUnitType(x *xorm.Engine) (err error) {
Created time.Time `xorm:"-"`
}

type Team struct {
ID int64
UnitTypes []int `xorm:"json"`
}

// Update team unit types
const batchSize = 100
for start := 0; ; start += batchSize {
teams := make([]*models.Team, 0, batchSize)
teams := make([]*Team, 0, batchSize)
if err := x.Limit(batchSize, start).Find(&teams); err != nil {
return err
}
if len(teams) == 0 {
break
}
for _, team := range teams {
ut := make([]models.UnitType, 0, len(team.UnitTypes))
ut := make([]int, 0, len(team.UnitTypes))
for _, u := range team.UnitTypes {
if u < V16UnitTypeCommits {
ut = append(ut, u)
Expand Down
Loading