#569 part 2 : Add Organization Blocking Users API #620

varadarajana · 2017-04-20T18:06:32Z

This is continuation of #569. I have made a first cut for review here.

Say what the methods do more explicitly. This should be simpler and more readable.

googlebot · 2017-04-20T18:06:36Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

dmitshur · 2017-04-20T20:53:22Z

@varadarajana The commits in this PR are currently identical to the commits in PR #600, which we already merged (as d23570d).

varadarajana · 2017-04-21T05:25:33Z

@shurcooL sorry, I missed pushing my code. Now it is done. you can check commit 7377254.

dmitshur

This mostly looks really good. No major issues.

I left 2 inline comments. One you should address (about admin:org scope TODO comments). The other one, I'm waiting to hear back from GitHub support. If they take too long to respond, we can proceed merging this without waiting.

dmitshur · 2017-04-21T20:36:47Z

github/orgs_users_blocking.go

+// GitHub API docs: https://developer.github.com/v3/orgs/blocking/#unblock-a-user
+func (s *OrganizationsService) UnblockUser(ctx context.Context, org string, user string) (*Response, error) {
+	// TODO: add a check for scope to be admin:org
+	u := fmt.Sprintf("user/%v/blocks/%v", org, user)


Hmm. It's really surprising to see user/%v/blocks/%v here, instead of orgs/%v/blocks/%v.

However, I can see it matches with what the GitHub API says at https://developer.github.com/v3/orgs/blocking/#unblock-a-user.

But I suspect it's a mistake on their part. Look at how it's inconsistent compared to the user-specific blocks (on the right hand side):

This is worthy of sending them an email to clarify. I've done that just now, and I'll report back here when I hear back from them.

@shurcooL Thank you, I never realized the URL seems incorrect.

dmitshur · 2017-04-21T20:52:08Z

github/orgs_users_blocking.go

+//
+// GitHub API docs: https://developer.github.com/v3/orgs/blocking/#list-blocked-users
+func (s *OrganizationsService) ListBlockedUsers(ctx context.Context, org string, opt *ListOptions) ([]*User, *Response, error) {
+	// TODO: add a check for scope to be admin:org


You have this // TODO: add a check for scope to be admin:org in 8 places. You also asked:

I have a question. While i was reading the Blocking Users (Organizations) API, I find the following lines:

The token used to authenticate the call must have the admin:org scope in order to make any blocking calls for an organization. Otherwise, the response returns HTTP 404.

In authorizations.go we have all the scopes defines. I do not see any references to scope anywhere? Is there a central place where this scope validation is done?

Let me answer that here. That note is meant for users of GitHub API. When authenticating, they need to make sure the token they're using has that scope.

There's nothing we need to do for these endpoints inside go-github, that information is for users of go-github. Authentication is orthogonal matter, see https://github.com/google/go-github#authentication.

So, nothing needs to be done here, and you can safely remove these TODOs.

However, if you want, you can add that note to the documentation for these 4 endpoints. E.g.:

// ListBlockedUsers lists all the users blocked by an organization. // // The token used to authenticate the call must have the admin:org scope // in order to make any blocking calls for an organization. Otherwise, // the response returns HTTP 404. // // GitHub API docs: https://developer.github.com/v3/orgs/blocking/#list-blocked-users

We generally don't try to document this for all endpoints, people are expected to read GitHub API documentation to figure out what scopes are required. So I'm not sure if it's worth doing or not just here.

Thank you @shurcooL . I do not think there is a need to document this at end point since one needs to follow the API documentation. I have removed the TODO.

dmitshur

LGTM. One optional inline comment.

We don't necessarily need to block on a response from GitHub about the strange path. When they reply, if anything needs to be done, we can update it then.

Thanks @varadarajana.

You don't need to do it for this PR, but in the future, please try to have a cleaner PR containing only the relevant commits. You should rebase or cherry-pick your work onto latest master of go-github before sending a PR. That makes reviewing it easier. See https://github.com/servo/servo/wiki/Github-workflow and https://github.com/servo/servo/wiki/Beginner's-guide-to-rebasing-and-squashing for helpful resources on the topic.

dmitshur · 2017-04-22T15:47:05Z

github/orgs_users_blocking_test.go

+		w.WriteHeader(http.StatusNoContent)
+	})
+
+	_, err := client.Organizations.BlockUser(context.Background(), "o1", "u1")


Is there a reason this test uses "o1" and "u1", while the rest use "o" and "u"? It seems a little inconsistent.

@shurcooL I was in a debugging effort and trying out combinations of URL. that's where I out o1 and u1. For consistency as you have mentioned, I have replaced o1 and u1 with o and u.

dmitshur

About #620 (comment), I got a reply from GitHub support:

Thanks for reporting this, Dmitri -- that looks like a bug in the docs. It should be /orgs/:org/blocks/:user. I'll ask our documentation team to fix that up.

dmitshur · 2017-04-24T18:57:29Z

github/orgs_users_blocking.go

+//
+// GitHub API docs: https://developer.github.com/v3/orgs/blocking/#unblock-a-user
+func (s *OrganizationsService) UnblockUser(ctx context.Context, org string, user string) (*Response, error) {
+	u := fmt.Sprintf("user/%v/blocks/%v", org, user)


Therefore, this should be changed to "orgs/%v/blocks/%v".

@shurcooL I have made these changes in the latest checkin.

gmlewis

LGTM after @shurcooL's comments are addressed.
Thank you, @varadarajana and @shurcooL!

gmlewis · 2017-04-24T19:25:06Z

github/orgs_users_blocking_test.go

+	setup()
+	defer teardown()
+
+	mux.HandleFunc("/user/o/blocks/u", func(w http.ResponseWriter, r *http.Request) {


Obviously, this line will be changed too once @shurcooL's comments are addressed, to make the tests pass.

dmitshur

LGTM. Thanks @varadarajana!

This change adds support for user blocking preview API to OrganizationsService. This preview API was announced at https://developer.github.com/changes/2017-02-28-user-blocking-apis-and-webhook/. The endpoints are documented at https://developer.github.com/v3/orgs/blocking/. Follows d23570d. Helps google#569.

varadarajana and others added 23 commits January 21, 2017 11:51

google#514: added inviter to Invitation for orgs and teams invite

74b3db5

google#514: go fmt

4a4a270

google#514: removed prints

3f2f969

google#514: changed time data types and included them in test

1d7107a

google#514 Added Org List Pending Invitations

f5b2eeb

google#514 removed typos in error handling

112d718

google#514: Modified to pointer time.Time in Invitation

2a95ca6

google#514: Set up Naming conventions for multi words, pointer to bool

6305685

google#514: multi word variable format

1840a00

google#514 : used User instead of new struct

5c4cab8

google#514 : removed Inviter struct

e732a04

Merge remote-tracking branch 'upstream/master'

7e8a9ee

google#514: Added quotes to list values in comment

1bd4328

google#569:user block: first cut

7883eb5

Merge branch 'master' of https://github.com/varadarajana/go-github

8206fe8

google#569: merge issue

546b2aa

google#569 : Changes to media type for user blocking APIs

11768d6

Merge remote-tracking branch 'upstream/master'

b5b6801

google#569 : changed method name to IsBlocking

6868470

google#567 : Code review comments for GIT API URL

5cd446d

Rephrase documentation to be more direct.

f2a91e1

Say what the methods do more explicitly. This should be simpler and more readable.

Merge remote-tracking branch 'upstream/master'

a9fbc77

google#569:2 added organization blocking API

7377254

google#569:2 added organization blocking API

e97de33

dmitshur added the waiting for reply label Apr 21, 2017

dmitshur removed the waiting for reply label Apr 21, 2017

dmitshur reviewed Apr 21, 2017

View reviewed changes

varadarajana added 2 commits April 22, 2017 20:03

Merge remote-tracking branch 'upstream/master'

9896307

google#569:2 removed the TODOs related to scope

a8a85e4

dmitshur approved these changes Apr 22, 2017

View reviewed changes

dmitshur requested a review from gmlewis April 22, 2017 15:51

dmitshur requested changes Apr 24, 2017

View reviewed changes

dmitshur added the waiting for reply label Apr 24, 2017

gmlewis approved these changes Apr 24, 2017

View reviewed changes

google#569:2 Added the correct REST API URL after clarification from GIT

f09a46a

dmitshur approved these changes Apr 25, 2017

View reviewed changes

dmitshur removed the waiting for reply label Apr 25, 2017

dmitshur merged commit e8d4666 into google:master Apr 25, 2017

dmitshur mentioned this pull request Sep 28, 2017

Support Marketplace (preview) API. #720

Closed

#569 part 2 : Add Organization Blocking Users API #620

#569 part 2 : Add Organization Blocking Users API #620

Uh oh!

Conversation

varadarajana commented Apr 20, 2017

Uh oh!

googlebot commented Apr 20, 2017

Uh oh!

dmitshur commented Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

varadarajana commented Apr 21, 2017

Uh oh!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

dmitshur Apr 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dmitshur commented Apr 20, 2017 •

edited

Loading

dmitshur left a comment •

edited

Loading

dmitshur Apr 24, 2017 •

edited

Loading