Repository: google/go-github
Pull request #812 (2017-12-16 15:19:02)

Add Outside Collaborators API for OrganizationsService
I have implemented the two methods on `OrganizationsService` corresponding to missing
endpoints of the Outside Collaborators API, namely:
1. [`RemoveOutsideCollaborator`](https://developer.github.com/v3/orgs/outside_collaborators/#remove-outside-collaborator)
2. [`ConvertMemberToOutsideCollaborator`](https://developer.github.com/v3/orgs/outside_collaborators/#convert-member-to-outside-collaborator)

### TODO
- [x] Add unit tests

This was my first attempt at wrapping endpoints so it is likely that the code can be improved upon.
Meanwhile, I will work on adding unit tests for these endpoints.

Refer #524 

kshitij10496: I have added two tests corresponding to the accepted behavior of the endpoints.
However, there are other possibilities for writing unit tests which I would like to address:

- `RemoveOutsideCollaborator`
  - [x] Removing a outside collaborator from the organization: 204
  - [ ] Removing a member from the organization: 422
  - [ ] Removing a non-member from the organization: 404 (from: Manual experimentation)

- `ConvertMemberToOutsideCollaborator`
  - [x] Converting a member of the organization to outside collaborator: 204
  - [ ] Converting a non-member to outside collaborator: 403
  - [ ] Converting the last owner of the organization to outside collaborator: 403

Refer: [Outside Collaborators](https://developer.github.com/v3/orgs/outside_collaborators/#remove-outside-collaborator)
sahildua2305: Very nice, thoughtful PR, @kshitij10496.

I feel that it makes sense to include the test cases for those "invalid" scenarios as well. 👍 
kshitij10496: > Very nice, thoughtful PR, @kshitij10496.

Thank you, yet again for the appreciation. 😄 

> I feel that it makes sense to include the test cases for those "invalid" scenarios as well. 

I am working on adding those cases. Since this is my first time writing unit tests in Go, they will need a thorough review.

kshitij10496: I would like to share a question which irked me while I was writing test cases for the methods.

_What are the guidelines we use to decide between returning a non-nil `error` value or an "expected" unsuccessful(302, 404, 403, 422) response with a `nil` error?_

For example:
- [Get Team Member](https://developer.github.com/v3/orgs/teams/#get-team-member) endpoint implemented via [`OrganizationsService.IsTeamMember`](https://github.com/google/go-github/blob/fbfee053c26dab3772adfc7799d995eed379133e/github/orgs_teams.go#L273) method.
This endpoint returns a 404 response in certain cases which are passed as `nil` error values by our client. I understand that this makes sense because a boolean nature response is expected here.

However, I found it difficult to formulate a general approach to writing code for an endpoint which can return multiple types of response.

Currently, the [endpoints](https://developer.github.com/v3/orgs/outside_collaborators/#remove-outside-collaborator) I have implemented in this PR return only the 204 response with a `nil` error. In all other cases,(including the 403 mentioned in the spec) some non-nil error value is returned. The reason behind this particular approach is the correspondence between `ErrorResponse` type and the API response received in such cases.(In particular, the fields `Message` and `DocumentUrl` resonate with the `message` and `document_url` JSON fields of the response from the API.)

Should the implementation return all the different responses as a `*Response` value as opposed to an `error` value? In general, how can one decide which approach to take?

Patch:
diff --git a/AUTHORS b/AUTHORS
index c82c7cf5..00151daa 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -17,6 +17,7 @@ Alec Thomas <alec@swapoff.org>
 Aleks Clark <aleks.clark@gmail.com>
 Alex Bramley <a.bramley@gmail.com>
 Alexander Harkness <me@bearbin.net>
+Allen Sun <shlallen1990@gmail.com>
 Amey Sakhadeo <me@ameyms.com>
 Andreas Garnæs <https://github.com/andreas>
 Andrew Ryabchun <aryabchun@mail.ua>
@@ -88,6 +89,7 @@ Jihoon Chung <j.c@navercorp.com>
 Jimmi Dyson <jimmidyson@gmail.com>
 Joe Tsai <joetsai@digital-static.net>
 John Engelman <john.r.engelman@gmail.com>
+jpbelanger-mtl <jp.belanger@gmail.com>
 Juan Basso <jrbasso@gmail.com>
 Julien Rostand <jrostand@users.noreply.github.com>
 Justin Abrahms <justin@abrah.ms>
@@ -139,6 +141,7 @@ Sahil Dua <sahildua2305@gmail.com>
 saisi <saisi@users.noreply.github.com>
 Sam Minnée <sam@silverstripe.com>
 Sander van Harmelen <svanharmelen@schubergphilis.com>
+Sarasa Kisaragi <lingsamuelgrace@gmail.com>
 Sean Wang <sean@decrypted.org>
 Sebastian Mandrean <sebastian.mandrean@gmail.com>
 Sebastian Mæland Pedersen <sem.pedersen@stud.uis.no>
diff --git a/github/github-accessors.go b/github/github-accessors.go
index cc696325..694d7784 100644
--- a/github/github-accessors.go
+++ b/github/github-accessors.go
@@ -5676,6 +5676,14 @@ func (p *PullRequest) GetMergeable() bool {
 	return *p.Mergeable
 }
 
+// GetMergeableState returns the MergeableState field if it's non-nil, zero value otherwise.
+func (p *PullRequest) GetMergeableState() string {
+	if p == nil || p.MergeableState == nil {
+		return ""
+	}
+	return *p.MergeableState
+}
+
 // GetMergeCommitSHA returns the MergeCommitSHA field if it's non-nil, zero value otherwise.
 func (p *PullRequest) GetMergeCommitSHA() string {
 	if p == nil || p.MergeCommitSHA == nil {
diff --git a/github/github.go b/github/github.go
index 8c707be3..b53e33d3 100644
--- a/github/github.go
+++ b/github/github.go
@@ -856,14 +856,21 @@ func (t *UnauthenticatedRateLimitedTransport) RoundTrip(req *http.Request) (*htt
 	// To set extra querystring params, we must make a copy of the Request so
 	// that we don't modify the Request we were given. This is required by the
 	// specification of http.RoundTripper.
-	req = cloneRequest(req)
-	q := req.URL.Query()
+	//
+	// Since we are going to modify only req.URL here, we only need a deep copy
+	// of req.URL.
+	req2 := new(http.Request)
+	*req2 = *req
+	req2.URL = new(url.URL)
+	*req2.URL = *req.URL
+
+	q := req2.URL.Query()
 	q.Set("client_id", t.ClientID)
 	q.Set("client_secret", t.ClientSecret)
-	req.URL.RawQuery = q.Encode()
+	req2.URL.RawQuery = q.Encode()
 
 	// Make the HTTP request.
-	return t.transport().RoundTrip(req)
+	return t.transport().RoundTrip(req2)
 }
 
 // Client returns an *http.Client that makes requests which are subject to the
@@ -895,12 +902,24 @@ type BasicAuthTransport struct {
 
 // RoundTrip implements the RoundTripper interface.
 func (t *BasicAuthTransport) RoundTrip(req *http.Request) (*http.Response, error) {
-	req = cloneRequest(req) // per RoundTrip contract
-	req.SetBasicAuth(t.Username, t.Password)
+	// To set extra headers, we must make a copy of the Request so
+	// that we don't modify the Request we were given. This is required by the
+	// specification of http.RoundTripper.
+	//
+	// Since we are going to modify only req.Header here, we only need a deep copy
+	// of req.Header.
+	req2 := new(http.Request)
+	*req2 = *req
+	req2.Header = make(http.Header, len(req.Header))
+	for k, s := range req.Header {
+		req2.Header[k] = append([]string(nil), s...)
+	}
+
+	req2.SetBasicAuth(t.Username, t.Password)
 	if t.OTP != "" {
-		req.Header.Set(headerOTP, t.OTP)
+		req2.Header.Set(headerOTP, t.OTP)
 	}
-	return t.transport().RoundTrip(req)
+	return t.transport().RoundTrip(req2)
 }
 
 // Client returns an *http.Client that makes requests that are authenticated
@@ -916,20 +935,6 @@ func (t *BasicAuthTransport) transport() http.RoundTripper {
 	return http.DefaultTransport
 }
 
-// cloneRequest returns a clone of the provided *http.Request. The clone is a
-// shallow copy of the struct and its Header map.
-func cloneRequest(r *http.Request) *http.Request {
-	// shallow copy of the struct
-	r2 := new(http.Request)
-	*r2 = *r
-	// deep copy of the Header
-	r2.Header = make(http.Header, len(r.Header))
-	for k, s := range r.Header {
-		r2.Header[k] = append([]string(nil), s...)
-	}
-	return r2
-}
-
 // formatRateReset formats d to look like "[rate reset in 2s]" or
 // "[rate reset in 87m02s]" for the positive durations. And like "[rate limit was reset 87m02s ago]"
 // for the negative cases.
diff --git a/github/orgs_outside_collaborators.go b/github/orgs_outside_collaborators.go
index e34f8653..85ffd05f 100644
--- a/github/orgs_outside_collaborators.go
+++ b/github/orgs_outside_collaborators.go
@@ -48,3 +48,34 @@ func (s *OrganizationsService) ListOutsideCollaborators(ctx context.Context, org
 
 	return members, resp, nil
 }
+
+// RemoveOutsideCollaborator removes a user from the list of outside collaborators;
+// consequently, removing them from all the organization's repositories.
+//
+// GitHub API docs: https://developer.github.com/v3/orgs/outside_collaborators/#remove-outside-collaborator
+func (s *OrganizationsService) RemoveOutsideCollaborator(ctx context.Context, org string, user string) (*Response, error) {
+	u := fmt.Sprintf("orgs/%v/outside_collaborators/%v", org, user)
+	req, err := s.client.NewRequest("DELETE", u, nil)
+	if err != nil {
+		return nil, err
+	}
+
+	return s.client.Do(ctx, req, nil)
+}
+
+// ConvertMemberToOutsideCollaborator reduces the permission level of a member of the
+// organization to that of an outside collaborator. Therefore, they will only
+// have access to the repositories that their current team membership allows.
+// Responses for converting a non-member or the last owner to an outside collaborator
+// are listed in GitHub API docs.
+//
+// GitHub API docs: https://developer.github.com/v3/orgs/outside_collaborators/#convert-member-to-outside-collaborator
+func (s *OrganizationsService) ConvertMemberToOutsideCollaborator(ctx context.Context, org string, user string) (*Response, error) {
+	u := fmt.Sprintf("orgs/%v/outside_collaborators/%v", org, user)
+	req, err := s.client.NewRequest("PUT", u, nil)
+	if err != nil {
+		return nil, err
+	}
+
+	return s.client.Do(ctx, req, nil)
+}
diff --git a/github/pulls.go b/github/pulls.go
index 5b6b5ee8..c10e2feb 100644
--- a/github/pulls.go
+++ b/github/pulls.go
@@ -32,6 +32,7 @@ type PullRequest struct {
 	User                *User      `json:"user,omitempty"`
 	Merged              *bool      `json:"merged,omitempty"`
 	Mergeable           *bool      `json:"mergeable,omitempty"`
+	MergeableState      *string    `json:"mergeable_state,omitempty"`
 	MergedBy            *User      `json:"merged_by,omitempty"`
 	MergeCommitSHA      *string    `json:"merge_commit_sha,omitempty"`
 	Comments            *int       `json:"comments,omitempty"`


Test patch:
diff --git a/github/orgs_outside_collaborators_test.go b/github/orgs_outside_collaborators_test.go
index 816db715..b4c4421b 100644
--- a/github/orgs_outside_collaborators_test.go
+++ b/github/orgs_outside_collaborators_test.go
@@ -48,3 +48,87 @@ func TestOrganizationsService_ListOutsideCollaborators_invalidOrg(t *testing.T)
 	_, _, err := client.Organizations.ListOutsideCollaborators(context.Background(), "%", nil)
 	testURLParseError(t, err)
 }
+
+func TestOrganizationsService_RemoveOutsideCollaborator(t *testing.T) {
+	client, mux, _, teardown := setup()
+	defer teardown()
+
+	handler := func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, "DELETE")
+	}
+	mux.HandleFunc("/orgs/o/outside_collaborators/u", handler)
+
+	_, err := client.Organizations.RemoveOutsideCollaborator(context.Background(), "o", "u")
+	if err != nil {
+		t.Errorf("Organizations.RemoveOutsideCollaborator returned error: %v", err)
+	}
+}
+
+func TestOrganizationsService_RemoveOutsideCollaborator_NonMember(t *testing.T) {
+	client, mux, _, teardown := setup()
+	defer teardown()
+
+	handler := func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, "DELETE")
+		w.WriteHeader(http.StatusNotFound)
+	}
+	mux.HandleFunc("/orgs/o/outside_collaborators/u", handler)
+
+	_, err := client.Organizations.RemoveOutsideCollaborator(context.Background(), "o", "u")
+	if err, ok := err.(*ErrorResponse); !ok {
+		t.Errorf("Organizations.RemoveOutsideCollaborator did not return an error")
+	} else if err.Response.StatusCode != http.StatusNotFound {
+		t.Errorf("Organizations.RemoveOutsideCollaborator did not return 404 status code")
+	}
+}
+
+func TestOrganizationsService_RemoveOutsideCollaborator_Member(t *testing.T) {
+	client, mux, _, teardown := setup()
+	defer teardown()
+
+	handler := func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, "DELETE")
+		w.WriteHeader(http.StatusUnprocessableEntity)
+	}
+	mux.HandleFunc("/orgs/o/outside_collaborators/u", handler)
+
+	_, err := client.Organizations.RemoveOutsideCollaborator(context.Background(), "o", "u")
+	if err, ok := err.(*ErrorResponse); !ok {
+		t.Errorf("Organizations.RemoveOutsideCollaborator did not return an error")
+	} else if err.Response.StatusCode != http.StatusUnprocessableEntity {
+		t.Errorf("Organizations.RemoveOutsideCollaborator did not return 422 status code")
+	}
+}
+
+func TestOrganizationsService_ConvertMemberToOutsideCollaborator(t *testing.T) {
+	client, mux, _, teardown := setup()
+	defer teardown()
+
+	handler := func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, "PUT")
+	}
+	mux.HandleFunc("/orgs/o/outside_collaborators/u", handler)
+
+	_, err := client.Organizations.ConvertMemberToOutsideCollaborator(context.Background(), "o", "u")
+	if err != nil {
+		t.Errorf("Organizations.ConvertMemberToOutsideCollaborator returned error: %v", err)
+	}
+}
+
+func TestOrganizationsService_ConvertMemberToOutsideCollaborator_NonMemberOrLastOwner(t *testing.T) {
+	client, mux, _, teardown := setup()
+	defer teardown()
+
+	handler := func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, "PUT")
+		w.WriteHeader(http.StatusForbidden)
+	}
+	mux.HandleFunc("/orgs/o/outside_collaborators/u", handler)
+
+	_, err := client.Organizations.ConvertMemberToOutsideCollaborator(context.Background(), "o", "u")
+	if err, ok := err.(*ErrorResponse); !ok {
+		t.Errorf("Organizations.ConvertMemberToOutsideCollaborator did not return an error")
+	} else if err.Response.StatusCode != http.StatusForbidden {
+		t.Errorf("Organizations.ConvertMemberToOutsideCollaborator did not return 403 status code")
+	}
+}