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.

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: new users see all orgs #397

Merged
merged 1 commit into from
Apr 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 12 additions & 3 deletions auth/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,16 +79,25 @@ func (u *User) IsTeamMember(teamID string) bool {

// Organizations returns the user's membership of organizations (indirectly via
// their membership of teams).
func (u *User) Organizations() (organizations []string) {
// De-dup organizations
//
// NOTE: always returns a non-nil slice
func (u *User) Organizations() []string {
// De-dup organizations using map
seen := make(map[string]bool)
for _, t := range u.Teams {
if _, ok := seen[t.Organization]; ok {
continue
}
organizations = append(organizations, t.Organization)
seen[t.Organization] = true
}

// Turn map into slice
organizations := make([]string, len(seen))
var i int
for org := range seen {
organizations[i] = org
i++
}
return organizations
}

Expand Down
13 changes: 6 additions & 7 deletions auth/user_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func TestUser_Organizations(t *testing.T) {
},
{
Name: "owners",
Organization: "big-tabacco",
Organization: "big-tobacco",
},
{
Name: "owners",
Expand All @@ -47,10 +47,9 @@ func TestUser_Organizations(t *testing.T) {
},
},
}
want := []string{
"acme-corp",
"big-tabacco",
"big-pharma",
}
assert.Equal(t, want, u.Organizations())
want := u.Organizations()
assert.Equal(t, 3, len(want), want)
assert.Contains(t, want, "acme-corp")
assert.Contains(t, want, "big-tobacco")
assert.Contains(t, want, "big-pharma")
}
12 changes: 12 additions & 0 deletions integration/organization_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,18 @@ func TestOrganization(t *testing.T) {
assert.Contains(t, got.Items, want2)
})

t.Run("new user should see zero orgs", func(t *testing.T) {
svc := setup(t, nil)
_ = svc.createOrganization(t, ctx)
_ = svc.createOrganization(t, ctx)

_, ctx := svc.createUserCtx(t, ctx)

got, err := svc.ListOrganizations(ctx, organization.OrganizationListOptions{})
require.NoError(t, err)
assert.Equal(t, 0, len(got.Items))
})

t.Run("get", func(t *testing.T) {
svc := setup(t, nil)
want := svc.createOrganization(t, ctx)
Expand Down
7 changes: 5 additions & 2 deletions organization/db.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,12 @@ func (db *pgdb) update(ctx context.Context, name string, fn func(*Organization)
}

func (db *pgdb) list(ctx context.Context, opts OrganizationListOptions) (*OrganizationList, error) {
names := []string{"%"}
if len(opts.Names) > 0 {
// optionally filter by organization name
var names []string
if opts.Names != nil {
names = opts.Names
} else {
names = []string{"%"} // return all organizations
}

batch := &pgx.Batch{}
Expand Down
2 changes: 1 addition & 1 deletion organization/organization.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ type (

// ListOptions represents the options for listing organizations.
OrganizationListOptions struct {
Names []string // filter organizations by name
Names []string // filter organizations by name if non-nil
otf.ListOptions
}

Expand Down
12 changes: 6 additions & 6 deletions run/spawner.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,12 +121,12 @@ func (h *spawner) handle(ctx context.Context, event cloud.VCSEvent) error {
return fmt.Errorf("retrieving repository tarball: %w", err)
}

// Determine which workspaces to spawn runs for. If its a PR then a
// (speculative) run is
// spawned for all workspaces. Otherwise each workspace's branch setting
// is checked and if it set then it must match the event's branch. If it is
// not set then the event's branch must match the repo's default branch. If
// neither of these conditions are true then the workspace is skipped.
// Determine which workspaces to spawn runs for. If it's a PR then a
// (speculative) run is spawned for all workspaces. Otherwise each
// workspace's branch setting is checked and if it set then it must match
// the event's branch. If it is not set then the event's branch must match
// the repo's default branch. If neither of these conditions are true then
// the workspace is skipped.
filterFunc := func(unfiltered []*workspace.Workspace) (filtered []*workspace.Workspace) {
for _, ws := range unfiltered {
if ws.Branch != "" && ws.Branch == branch {
Expand Down