Skip to content

Add ListPendingOrgInvitations method and Invitation.Inviter field. #527

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

Closed
varadarajana wants to merge 13 commits into from
Closed

Add ListPendingOrgInvitations method and Invitation.Inviter field. #527

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
74b3db5
#514: added inviter to Invitation for orgs and teams invite
varadarajana Jan 21, 2017
4a4a270
#514: go fmt
varadarajana Jan 21, 2017
3f2f969
#514: removed prints
varadarajana Jan 21, 2017
1d7107a
#514: changed time data types and included them in test
varadarajana Jan 21, 2017
f5b2eeb
#514 Added Org List Pending Invitations
varadarajana Jan 21, 2017
112d718
#514 removed typos in error handling
varadarajana Jan 21, 2017
2a95ca6
#514: Modified to pointer time.Time in Invitation
varadarajana Jan 28, 2017
6305685
#514: Set up Naming conventions for multi words, pointer to bool
varadarajana Jan 29, 2017
1840a00
#514: multi word variable format
varadarajana Jan 29, 2017
5c4cab8
#514 : used User instead of new struct
varadarajana Jan 30, 2017
e732a04
#514 : removed Inviter struct
varadarajana Jan 30, 2017
7e8a9ee
Merge remote-tracking branch 'upstream/master'
varadarajana Jan 31, 2017
1bd4328
#514: Added quotes to list values in comment
varadarajana Jan 31, 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
26 changes: 26 additions & 0 deletions github/orgs_members.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,3 +270,29 @@ func (s *OrganizationsService) RemoveOrgMembership(user, org string) (*Response,

return s.client.Do(req, nil)
}

// ListPendingOrgInvitations returns a list of pending invitations.
//
// GitHub API docs: https://developer.github.com/v3/orgs/members/#list-pending-organization-invitations
func (s *OrganizationsService) ListPendingOrgInvitations(org int, opt *ListOptions) ([]*Invitation, *Response, error) {
Copy link
Copy Markdown
Member

@dmitshur dmitshur Jan 30, 2017

Choose a reason for hiding this comment

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

One more thought. Should we spell out the entire word "Organization"? I don't think we shorten "Organization" to "Org" anywhere else. E.g.:

https://godoc.org/github.com/google/go-github/github#ActivityService.ListEventsForOrganization

Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis Jan 31, 2017

Choose a reason for hiding this comment

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

Correct me if I'm wrong, but an invite only relates to Organizations anyway, right?
Also, this is the OrganizationsService, so I'm thinking that a more idiomatic-Go-way to handle this (specifically to reduce stutter) would be to drop the Org entirely... so when a client uses the call, it would look like:

... := c.Organizations.ListPendingInvitations(...)

Thoughts?

Copy link
Copy Markdown
Member

@dmitshur dmitshur Jan 31, 2017

Choose a reason for hiding this comment

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

Good catch, that's even better!

Copy link
Copy Markdown
Member

@dmitshur dmitshur Jan 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we shorten "Organization" to "Org" anywhere else.

Well, I just found a counterexample of that:

https://github.com/google/go-github/blob/4baf08e3bbebb0f/github/orgs_members.go#L178-L181

Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis Jan 31, 2017

Choose a reason for hiding this comment

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

Yeah... I don't like that as much. ☹️
Your call, @shurcooL as a tie-breaker. 😃

Copy link
Copy Markdown
Member

@dmitshur dmitshur Jan 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm. Looking at the methods of OrganizationsService more closely, it seems there's many, many uses of Org. So I'm afraid it might make sense to stay consistent with that and keep this as is.

image

It seems valuable to keep Org because it makes it more clear that the pending invitation applies to org, rather than team (which also exists).

Notice there's RemoveOrgMembership and RemoveTeamMembership.

After this PR, there will be ListPendingOrgInvitations and ListPendingTeamInvitations, which is very consistent.

u := fmt.Sprintf("orgs/%v/invitations", org)
u, err := addOptions(u, opt)
if err != nil {
return nil, nil, err
}

req, err := s.client.NewRequest("GET", u, nil)
if err != nil {
return nil, nil, err
}

// TODO: remove custom Accept header when this API fully launches.
req.Header.Set("Accept", mediaTypeOrgMembershipPreview)

pendingInvitations := new([]*Invitation)
Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis Jan 31, 2017

Choose a reason for hiding this comment

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

Just for the record, I don't like calling new to make a pointer to a slice (since it is already a reference type)... but I see this happens all over in this repo, so let's leave it... and maybe some day I can throw together a PR to replace it and see what the other authors think about the change.

Copy link
Copy Markdown
Member

@dmitshur dmitshur Jan 31, 2017

Choose a reason for hiding this comment

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

Agree that this would be best to tackle in a separate PR since it happens everywhere.

I feel similarly about the whole pattern of returning err instead of nil in situations where err is known to be nil; I will make a PR for that sometime.

Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis Feb 1, 2017 •
edited by dmitshur
Loading

Choose a reason for hiding this comment

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

OK, I can address this change in a separate PR.
For the record, it will become:

var pendingInvitations []*Invitation
resp, err := s.client.Do(req, &pendingInvitations)
if err != nil {
	return nil, resp, err
}
return pendingInvitations, resp, nil

resp, err := s.client.Do(req, pendingInvitations)
if err != nil {
return nil, resp, err
}
return *pendingInvitations, resp, err
}
79 changes: 79 additions & 0 deletions github/orgs_members_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"net/http"
"reflect"
"testing"
"time"
)

func TestOrganizationsService_ListMembers(t *testing.T) {
Expand Down Expand Up @@ -353,3 +354,81 @@ func TestOrganizationsService_RemoveOrgMembership(t *testing.T) {
t.Errorf("Organizations.RemoveOrgMembership returned error: %v", err)
}
}

func TestOrganizationsService_ListPendingOrgInvitations(t *testing.T) {
setup()
defer teardown()

mux.HandleFunc("/orgs/1/invitations", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "GET")
testFormValues(t, r, values{"page": "1"})
testHeader(t, r, "Accept", mediaTypeOrgMembershipPreview)
fmt.Fprint(w, `[
Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis Jan 31, 2017

Choose a reason for hiding this comment

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

Can we please reduce this test to just fields of interest?

Copy link
Copy Markdown
Contributor Author

@varadarajana varadarajana Jan 31, 2017

Choose a reason for hiding this comment

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

@gmlewis, @shurcooL, As I mentioned above, I felt all fields need to be tested for assignment to the struct and compare values. Will it be ok only to test id field as before?

Copy link
Copy Markdown
Contributor Author

@varadarajana varadarajana Feb 1, 2017

Choose a reason for hiding this comment

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

@shurcooL @gmlewis thank you

Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis Feb 1, 2017

Choose a reason for hiding this comment

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

Thank you, @varadarajana!

{
"id": 1,
"login": "monalisa",
"email": "octocat@github.com",
"role": "direct_member",
"created_at": "2017-01-21T00:00:00Z",
"inviter": {
"login": "other_user",
"id": 1,
"avatar_url": "https://github.com/images/error/other_user_happy.gif",
"gravatar_id": "",
"url": "https://api.github.com/users/other_user",
"html_url": "https://github.com/other_user",
"followers_url": "https://api.github.com/users/other_user/followers",
"following_url": "https://api.github.com/users/other_user/following/other_user",
"gists_url": "https://api.github.com/users/other_user/gists/gist_id",
"starred_url": "https://api.github.com/users/other_user/starred/owner/repo",
"subscriptions_url": "https://api.github.com/users/other_user/subscriptions",
"organizations_url": "https://api.github.com/users/other_user/orgs",
"repos_url": "https://api.github.com/users/other_user/repos",
"events_url": "https://api.github.com/users/other_user/events/privacy",
"received_events_url": "https://api.github.com/users/other_user/received_events/privacy",
"type": "User",
"site_admin": false
}
}
]`)
})

opt := &ListOptions{Page: 1}
invitations, _, err := client.Organizations.ListPendingOrgInvitations(1, opt)
if err != nil {
t.Errorf("Organizations.ListPendingOrgInvitations returned error: %v", err)
}

createdAt := time.Date(2017, 01, 21, 0, 0, 0, 0, time.UTC)
want := []*Invitation{
{
ID: Int(1),
Login: String("monalisa"),
Email: String("octocat@github.com"),
Role: String("direct_member"),
CreatedAt: &createdAt,
Inviter: &User{
Login: String("other_user"),
ID: Int(1),
AvatarURL: String("https://github.com/images/error/other_user_happy.gif"),
GravatarID: String(""),
URL: String("https://api.github.com/users/other_user"),
HTMLURL: String("https://github.com/other_user"),
FollowersURL: String("https://api.github.com/users/other_user/followers"),
FollowingURL: String("https://api.github.com/users/other_user/following/other_user"),
GistsURL: String("https://api.github.com/users/other_user/gists/gist_id"),
StarredURL: String("https://api.github.com/users/other_user/starred/owner/repo"),
SubscriptionsURL: String("https://api.github.com/users/other_user/subscriptions"),
OrganizationsURL: String("https://api.github.com/users/other_user/orgs"),
ReposURL: String("https://api.github.com/users/other_user/repos"),
EventsURL: String("https://api.github.com/users/other_user/events/privacy"),
ReceivedEventsURL: String("https://api.github.com/users/other_user/received_events/privacy"),
Type: String("User"),
SiteAdmin: Bool(false),
},
}}

if !reflect.DeepEqual(invitations, want) {
t.Errorf("Organizations.ListPendingOrgInvitations returned %+v, want %+v", invitations, want)
}
}
10 changes: 6 additions & 4 deletions github/orgs_teams.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,15 @@ func (t Team) String() string {
return Stringify(t)
}

// Invitation represents a team member's inviation status
// Invitation represents a team member's invitation status.
type Invitation struct {
ID *int `json:"id,omitempty"`
Login *string `json:"login,omitempty"`
Email *string `json:"email,omitempty"`
ID *int `json:"id,omitempty"`
Login *string `json:"login,omitempty"`
Email *string `json:"email,omitempty"`
// Role can be one of the values - 'direct_member', 'admin', 'billing_manager', 'hiring_manager', or 'reinstate'.
Role *string `json:"role,omitempty"`
CreatedAt *time.Time `json:"created_at,omitempty"`
Inviter *User `json:"inviter,omitempty"`
}

func (i Invitation) String() string {
Expand Down
60 changes: 58 additions & 2 deletions github/orgs_teams_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"net/http"
"reflect"
"testing"
"time"
)

func TestOrganizationsService_ListTeams(t *testing.T) {
Expand Down Expand Up @@ -508,7 +509,34 @@ func TestOrganizationsService_ListPendingTeamInvitations(t *testing.T) {
testMethod(t, r, "GET")
testFormValues(t, r, values{"page": "1"})
testHeader(t, r, "Accept", mediaTypeOrgMembershipPreview)
fmt.Fprint(w, `[{"id":1}]`)
fmt.Fprint(w, `[
Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis Jan 31, 2017

Choose a reason for hiding this comment

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

Can we please reduce this test to just fields of interest?

Copy link
Copy Markdown
Contributor Author

@varadarajana varadarajana Jan 31, 2017

Choose a reason for hiding this comment

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

@gmlewis, @shurcooL I agree I made the test case big with the full test data. I have used all values that are returned by this API call, i felt test coverage will be better that way. I was not sure which field to omit, as JSON will return all of them.
Is there a guideline to choose some of these fields from from the response?

Copy link
Copy Markdown
Member

@dmitshur dmitshur Jan 31, 2017

Choose a reason for hiding this comment

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

I personally don't mind having all the fields here, since they come directly from GitHub API examples.

If we reduce it to just IDs, keep the new inviter field with an ID too.

{
"id": 1,
"login": "monalisa",
"email": "octocat@github.com",
"role": "direct_member",
"created_at": "2017-01-21T00:00:00Z",
"inviter": {
"login": "other_user",
"id": 1,
"avatar_url": "https://github.com/images/error/other_user_happy.gif",
"gravatar_id": "",
"url": "https://api.github.com/users/other_user",
"html_url": "https://github.com/other_user",
"followers_url": "https://api.github.com/users/other_user/followers",
"following_url": "https://api.github.com/users/other_user/following/other_user",
"gists_url": "https://api.github.com/users/other_user/gists/gist_id",
"starred_url": "https://api.github.com/users/other_user/starred/owner/repo",
"subscriptions_url": "https://api.github.com/users/other_user/subscriptions",
"organizations_url": "https://api.github.com/users/other_user/orgs",
"repos_url": "https://api.github.com/users/other_user/repos",
"events_url": "https://api.github.com/users/other_user/events/privacy",
"received_events_url": "https://api.github.com/users/other_user/received_events/privacy",
"type": "User",
"site_admin": false
}
}
]`)
})

opt := &ListOptions{Page: 1}
Expand All @@ -517,7 +545,35 @@ func TestOrganizationsService_ListPendingTeamInvitations(t *testing.T) {
t.Errorf("Organizations.ListPendingTeamInvitations returned error: %v", err)
}

want := []*Invitation{{ID: Int(1)}}
createdAt := time.Date(2017, 01, 21, 0, 0, 0, 0, time.UTC)
want := []*Invitation{
{
ID: Int(1),
Login: String("monalisa"),
Email: String("octocat@github.com"),
Role: String("direct_member"),
CreatedAt: &createdAt,
Inviter: &User{
Login: String("other_user"),
ID: Int(1),
AvatarURL: String("https://github.com/images/error/other_user_happy.gif"),
GravatarID: String(""),
URL: String("https://api.github.com/users/other_user"),
HTMLURL: String("https://github.com/other_user"),
FollowersURL: String("https://api.github.com/users/other_user/followers"),
FollowingURL: String("https://api.github.com/users/other_user/following/other_user"),
GistsURL: String("https://api.github.com/users/other_user/gists/gist_id"),
StarredURL: String("https://api.github.com/users/other_user/starred/owner/repo"),
SubscriptionsURL: String("https://api.github.com/users/other_user/subscriptions"),
OrganizationsURL: String("https://api.github.com/users/other_user/orgs"),
ReposURL: String("https://api.github.com/users/other_user/repos"),
EventsURL: String("https://api.github.com/users/other_user/events/privacy"),
ReceivedEventsURL: String("https://api.github.com/users/other_user/received_events/privacy"),
Type: String("User"),
SiteAdmin: Bool(false),
},
}}

if !reflect.DeepEqual(invitations, want) {
t.Errorf("Organizations.ListPendingTeamInvitations returned %+v, want %+v", invitations, want)
}
Expand Down