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

Add ListPendingOrgInvitations method and Invitation.Inviter field. #527

Closed
wants to merge 13 commits into from

Conversation

@varadarajana
Copy link
Contributor

@varadarajana varadarajana commented Jan 21, 2017

This change is made to incorporate changes to GitHub for Issue #514
Here I have tried to change the test case to include the entire response.
I have not been able to compare time.Time field in Invitation struct. I need to work on the Date time conversions.

Resolves #514.

@varadarajana
Copy link
Contributor Author

@varadarajana varadarajana commented Jan 21, 2017

@shurcooL @gmlewis Can you please review this?

Loading

@varadarajana
Copy link
Contributor Author

@varadarajana varadarajana commented Jan 21, 2017

The golang documentation of time, requires structs to access as time.Time and not *time.Time. So changed it as it.
The URL https://developer.github.com/v3/orgs/teams/#list-pending-team-invitations does not provide correct date format I think, it is not consistent with other API in the documentation. I have changed the return JSON to the type consistent and mentioned in the documentation of GitHub.
https://developer.github.com/v3/ states dates to be
All timestamps are returned in ISO 8601 format:

YYYY-MM-DDTHH:MM:SSZ

Loading

@varadarajana
Copy link
Contributor Author

@varadarajana varadarajana commented Jan 21, 2017

I have now added support for Orgs List Members Invitation also. Now this contains both List Members as well as Team Invitations.

Loading

Copy link
Member

@dmitshur dmitshur left a comment

Looks good. I've reviewed and left first pass comments.

Loading

//ListPendingOrgInvitations returns a list of contains a role field which refers to the
//Organization Invitation role and will be one of the following values:
//direct_member, admin, billing_manager, hiring_manager, or reinstate.
//If the invitee is not a GitHub member, the login field in the return hash will be null.
Copy link
Member

@dmitshur dmitshur Jan 26, 2017

Choose a reason for hiding this comment

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

Loading

Copy link
Member

@dmitshur dmitshur Jan 26, 2017

Choose a reason for hiding this comment

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

This documentation is hard to read and strangely detailed (it refers to specifics of the returned values inside Invitation struct).

We should either omit it altogether, just say // ListPendingOrgInvitations returns a list of pending organization invitations., or update it to be specific to go-github. The returned thing is []Invitation, a slice of invitations, not a hash. There's no null, it's nil. Etc.

It would make sense to me to move the documentation of role to the Invitation struct itself, not here. Unless it's specific to this method, is it?

Loading

Copy link
Contributor Author

@varadarajana varadarajana Jan 28, 2017

Choose a reason for hiding this comment

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

@shurcooL Yes, this has been changed.

Loading

//If the invitee is not a GitHub member, the login field in the return hash will be null.
//
// GitHub API docs:
// https://developer.github.com/v3/orgs/members/#list-pending-organization-invitations
Copy link
Member

@dmitshur dmitshur Jan 26, 2017

Choose a reason for hiding this comment

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

There's no benefit in putting the long URL on a new line. We're not hard-limited to the width of a punch card, it's fine to keep it on the same line.

Loading

Copy link
Contributor Author

@varadarajana varadarajana Jan 28, 2017

Choose a reason for hiding this comment

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

@shurcooL this is done.

Loading

Role *string `json:"role,omitempty"`
//golang documentation states
//Programs using times should typically store and pass them as values, not pointers.
//That is, time variables and struct fields should be of type time.Time, not *time.Time
Copy link
Member

@dmitshur dmitshur Jan 26, 2017

Choose a reason for hiding this comment

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

This comment shouldn't be here as part of the final PR.

I will respond to it.

While it's true that it's fine to use time.Time as a value, the reason we're using it as a point here is because of JSON encoding. We want to be able to tell (sometimes) that a field is absent by checking if its value is nil.

So, we want to keep CreatedAt as a pointer to *time.Time. If you want to discuss this further, I suggest opening a new issue, because it's outside of the scope of this PR.

Edit: I missed your #527 (comment), let me look into that. I'll update this comment afterwards.

Loading

Copy link
Member

@dmitshur dmitshur Jan 26, 2017

Choose a reason for hiding this comment

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

Okay, question, have you considered using *Timestamp here instead of *time.Time?

Can you be more specific about the problem you're seeing with *time.Time as is?

Loading

Copy link
Contributor Author

@varadarajana varadarajana Jan 28, 2017

Choose a reason for hiding this comment

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

@shurcooL in fact this time i tried pointer to time.Time, and it worked. i assigned it to a variable and then passed its address.

Loading

}

func (i Invitation) String() string {
return Stringify(i)
}

// Inviter represents the entity introduced in team and org invitations
Copy link
Member

@dmitshur dmitshur Jan 26, 2017

Choose a reason for hiding this comment

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

A period is missing at the end of this sentence.

Also, you can write the full word "organization", there's no need for a short form here.

Loading

Copy link
Contributor Author

@varadarajana varadarajana Jan 28, 2017

Choose a reason for hiding this comment

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

@shurcooL this is done.

Loading

t.Errorf("Organizations.ListPendingOrgInvitations returned error: %v", err)
}

tm := time.Date(2017, 01, 21, 0, 0, 0, 0, time.UTC)
Copy link
Member

@dmitshur dmitshur Jan 29, 2017

Choose a reason for hiding this comment

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

Nitpick, what does tm mean? It doesn't seem to be short for anything, and I don't think it's an acronym. Is it meant to stand for "time" except without letters i and e?

I'd suggest naming it something more clear. Just createdAt could work well.

Loading

Copy link
Contributor Author

@varadarajana varadarajana Jan 29, 2017

Choose a reason for hiding this comment

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

@shurcooL yes, tm was not proper, i have set it as createdAt

Loading

@@ -44,19 +44,46 @@ func (t Team) String() string {
return Stringify(t)
}

// Invitation represents a team member's inviation status
// Invitation represents a team member's invitation status.
// Role can be one of the values - direct_member, admin, billing_manager, hiring_manager, or reinstate.
Copy link
Member

@dmitshur dmitshur Jan 29, 2017

Choose a reason for hiding this comment

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

// Role can be one of the values ...

Move that comment near Role field:

// Role can be one of ...
Role *string `json:"role,omitempty"`

See

// The originating VCS type. Can be one of 'subversion', 'git',
// 'mercurial', or 'tfvc'. Without this parameter, the import job will
for an example.

Loading

Copy link
Contributor Author

@varadarajana varadarajana Jan 29, 2017

Choose a reason for hiding this comment

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

@shurcooL yes this is done.

Loading

Events_url *string `json:"events_url,omitempty"`
Received_events_url *string `json:"received_events_url,omitempty"`
Type *string `json:"type,omitempty"`
Site_admin bool `json:"site_admin,omitempty"`
Copy link
Member

@dmitshur dmitshur Jan 29, 2017

Choose a reason for hiding this comment

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

Site_admin should also be a pointer, i.e., *bool.

Loading

Copy link
Member

@dmitshur dmitshur Jan 29, 2017

Choose a reason for hiding this comment

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

Most of these variables need to be renamed to follow Go style.

See:

So, it should be SiteAdmin, not Site_admin. URL, not Url. HTMLURL, not Html_url, etc.

Loading

Copy link
Contributor Author

@varadarajana varadarajana Jan 29, 2017

Choose a reason for hiding this comment

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

@shurcooL this is also done now.

Loading

Copy link
Member

@dmitshur dmitshur left a comment

Thanks for addressing previous feedback, that part looks great.

I've spotted another simplification we can make. The new Inviter struct can be replaced by an existing one. See inline comments.

Loading

ReceivedEventsURL *string `json:"received_events_url,omitempty"`
Type *string `json:"type,omitempty"`
SiteAdmin *bool `json:"site_admin,omitempty"`
}
Copy link
Member

@dmitshur dmitshur Jan 29, 2017

Choose a reason for hiding this comment

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

I think this new struct is just a subset of User. I think we can and should use User instead of creating a new struct. So you can delete this struct.

Loading

Copy link
Contributor Author

@varadarajana varadarajana Jan 30, 2017

Choose a reason for hiding this comment

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

@shurcooL yes, thank you, i missed it. I have corrected it and checked in.

Loading

Role *string `json:"role,omitempty"`
CreatedAt *time.Time `json:"created_at,omitempty"`
Inviter *Inviter `json:"inviter,omitempty"`
Copy link
Member

@dmitshur dmitshur Jan 29, 2017

Choose a reason for hiding this comment

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

Use User instead of Inviter here:

Inviter   *User   `json:"inviter,omitempty"`

See here for precedent:

https://github.com/google/go-github/blob/c9c37fd6/github/repos_invitations.go#L15

Loading

Copy link
Contributor Author

@varadarajana varadarajana Jan 30, 2017

Choose a reason for hiding this comment

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

@shurcooL yes, thank you, i missed it. I have corrected it and checked in.

Loading

@dmitshur dmitshur changed the title #514: added inviter to Invitation for teams invite Add ListPendingOrgInvitations method and Invitation.Inviter field. Jan 30, 2017
Copy link
Member

@dmitshur dmitshur left a comment

LGTM. Thanks @varadarajana.

@gmlewis, mind looking it over before we merge?

Loading

@dmitshur dmitshur requested a review from gmlewis Jan 30, 2017
// 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
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

Loading

Copy link
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?

Loading

Copy link
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!

Loading

Copy link
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 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

Loading

Copy link
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. 😃

Loading

Copy link
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.

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.

Loading

Copy link
Collaborator

@gmlewis gmlewis left a comment

This is looking great, @varadarajana... thank you!
Just a few minor comments if you don't mind.

Loading

// 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
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?

Loading

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

pendingInvitations := new([]*Invitation)
Copy link
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.

Loading

Copy link
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.

Loading

Copy link
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.

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

Loading

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.
Copy link
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.

nit - would you mind putting the acceptable values in quotes to follow the style of the other documentation in the repo?

Loading

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

Loading

Copy link
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?

Loading

Copy link
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.

Loading

testMethod(t, r, "GET")
testFormValues(t, r, values{"page": "1"})
testHeader(t, r, "Accept", mediaTypeOrgMembershipPreview)
fmt.Fprint(w, `[
Copy link
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?

Loading

Copy link
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?

Loading

Copy link
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

Loading

Copy link
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!

Loading

gmlewis
gmlewis approved these changes Feb 1, 2017
@gmlewis
Copy link
Collaborator

@gmlewis gmlewis commented Feb 1, 2017

LGTM.
Merging.

Loading

@gmlewis gmlewis closed this in 0e7db46 Feb 1, 2017
@gmlewis
Copy link
Collaborator

@gmlewis gmlewis commented Feb 1, 2017

Integrated in 0e7db46

Loading

BOTBrad pushed a commit to BOTBrad/go-github that referenced this issue Jun 16, 2017
Fixes google#514.
Closes google#527.

Change-Id: Id42e4304aae2388af67dca345bf12aa042b773f1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants