#569:Partial Fix: Fix for Users Blocking #600

varadarajana · 2017-03-25T19:28:49Z

Please find the 2 files containing the partial fixes for blocking users. I will create one more for organization.

dmitshur · 2017-03-25T21:23:33Z

github/orgs_members_test.go

 	invitations, _, err := client.Organizations.ListPendingOrgInvitations(context.Background(), 1, opt)
+=======
+	invitations, _, err := client.Organizations.ListPendingOrgInvitations(1, opt)
+>>>>>>> 1bd4328a520dfc6d58f92e0f441f3f56718b725d


This unresolved merge conflict shouldn't be here.

@shurcooL this was part of messed up fork of mine. I have corrected it and now the latest check in has the right changes.

dmitshur

Some initial comments based on what I'm seeing so far. It looks good at a high level, but there are small things to improve.

dmitshur · 2017-03-25T21:25:54Z

github/users_blocking.go

+	"fmt"
+)
+
+// ListBlockedUsers lists all the blocked users by the authenticated user


Sentences end with a period.

dmitshur · 2017-03-25T21:28:12Z

github/users_blocking.go

+// CheckIfUserIsBlocked allows the authenticated user to check if the other user is blocked
+//
+// GitHub API docs: https://developer.github.com/v3/users/blocking/#check-whether-youve-blocked-a-user
+func (s *UsersService) CheckIfUserIsBlocked(ctx context.Context, user string) (bool, *Response, error) {


I'm thinking we can shorten/simplify the method name to something like IsBlocked. @gmlewis, what do you think about the naming here?

Some precedent:

https://godoc.org/github.com/google/go-github/github#UsersService.IsFollowing

dmitshur · 2017-03-25T21:28:59Z

github/users_blocking.go

+	if err != nil {
+		return nil, nil, err
+	}
+


According to the the GitHub API docs:

I think you're missing that preview header.

@shurcooL Thank you, I missed the preview header, now it is added. Can you please review?

dmitshur · 2017-03-27T17:29:15Z

github/users_blocking.go

+// CheckIfUserIsBlocked allows the authenticated user to check if the other user is blocked.
+//
+// GitHub API docs: https://developer.github.com/v3/users/blocking/#check-whether-youve-blocked-a-user
+func (s *UsersService) CheckIfUserIsBlocked(ctx context.Context, user string) (bool, *Response, error) {


@varadarajana, did you see my comment at #600 (comment)?

@shurcooL I will change the method name and submit, I was waiting since you had asked this question to @gmlewis

Fair enough. Let's go with IsBlocked for now, I feel pretty good about it. If there are concerns or improvement suggestions, we can still change it.

@shurcooL I have corrected it to IsBlocked.

dmitshur · 2017-03-29T23:48:18Z

Not seeing any high level issues. Just need to review it at a lower level.

gmlewis · 2017-03-30T00:01:46Z

github/users_blocking.go

@@ -0,0 +1,91 @@
+// Copyright 2013 The go-github AUTHORS. All rights reserved.


@gmlewis this is fixed.

gmlewis · 2017-03-30T00:16:10Z

github/users_blocking.go

+//
+// GitHub API docs: https://developer.github.com/v3/users/blocking/#check-whether-youve-blocked-a-user
+func (s *UsersService) IsBlocked(ctx context.Context, user string) (bool, *Response, error) {
+	u := fmt.Sprintf("users/blocks/%v", user)


According to the docs, this URL is: "user/blocks/%v"

@gmlewis This is fixed.

gmlewis · 2017-03-30T00:16:59Z

github/users_blocking.go

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


"user/blocks/%v"

@gmlewis this is fixed

gmlewis · 2017-03-30T00:17:58Z

github/users_blocking.go

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


"user/blocks/%v"

@gmlewis this is fixed

gmlewis · 2017-03-30T00:18:07Z

github/users_blocking_test.go

@@ -0,0 +1,90 @@
+// Copyright 2014 The go-github AUTHORS. All rights reserved.


@gmlewis this is fixed

gmlewis · 2017-03-30T00:18:59Z

github/users_blocking_test.go

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


"user/blocks/u"

@gmlewis this is fixed

gmlewis · 2017-03-30T00:19:06Z

github/users_blocking_test.go

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


"user/blocks/u"

@gmlewis this is fixed

varadarajana · 2017-04-01T01:22:11Z

@gmlewis can you please review these changes?

gmlewis

LGTM, but I will be unable to merge until Monday.
If @shurcooL has time to review, he is welcome to merge at his discretion.

dmitshur

Minor documentation improvement suggestions. I can apply them before merging.

Reviewed at a lower level and did not spot any other issues.

LGTM.

dmitshur · 2017-04-08T19:27:30Z

github/users_blocking.go

+	return blockedUsers, resp, nil
+}
+
+// IsBlocked allows the authenticated user to check if the other user is blocked.


Instead of saying "MethodName allows the authenticated user to do something", we can be more direct and say "MethodName does something for authenticated user."

I would rephrase this as:

// IsBlocked reports whether specified user is blocked by the authenticated user.

dmitshur · 2017-04-08T19:28:12Z

github/users_blocking.go

+	return isBlocked, resp, err
+}
+
+// BlockUser allows authenticated User to block another User.


// BlockUser blocks specified user for the authenticated user.

dmitshur · 2017-04-08T19:28:32Z

github/users_blocking.go

+	return s.client.Do(ctx, req, nil)
+}
+
+// UnblockUser allows authenticated User to unblock BlockedUser.


// UnblockUser unblocks specified user for the authenticated user.

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

googlebot · 2017-04-08T19:41:18Z

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-08T19:50:59Z

Thanks @varadarajana!

…ersService. (google#600) This change adds support for user blocking preview API to UsersService. 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/users/blocking/. Helps google#569.

varadarajana added 15 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

dmitshur reviewed Mar 25, 2017

View reviewed changes

varadarajana added 2 commits March 26, 2017 08:54

google#569: merge issue

546b2aa

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

11768d6

dmitshur reviewed Mar 27, 2017

View reviewed changes

varadarajana added 2 commits March 29, 2017 22:24

Merge remote-tracking branch 'upstream/master'

b5b6801

google#569 : changed method name to IsBlocking

6868470

dmitshur requested a review from gmlewis March 29, 2017 23:46

gmlewis requested changes Mar 30, 2017

View reviewed changes

google#567 : Code review comments for GIT API URL

5cd446d

gmlewis approved these changes Apr 1, 2017

View reviewed changes

dmitshur approved these changes Apr 8, 2017

View reviewed changes

Rephrase documentation to be more direct.

f2a91e1

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

dmitshur merged commit d23570d into google:master Apr 8, 2017

		@@ -0,0 +1,91 @@
		// Copyright 2013 The go-github AUTHORS. All rights reserved.

		@@ -0,0 +1,90 @@
		// Copyright 2014 The go-github AUTHORS. All rights reserved.

#569:Partial Fix: Fix for Users Blocking #600

#569:Partial Fix: Fix for Users Blocking #600

Uh oh!

Conversation

varadarajana commented Mar 25, 2017

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!

Choose a reason for hiding this comment

Uh oh!

dmitshur Mar 25, 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!

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 Mar 28, 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!

dmitshur commented Mar 29, 2017

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!

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!

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!

varadarajana commented Apr 1, 2017

Uh oh!

gmlewis left a comment

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!

dmitshur Apr 8, 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!

dmitshur Mar 25, 2017 •

edited

Loading

dmitshur Mar 28, 2017 •

edited

Loading

dmitshur left a comment •

edited

Loading

dmitshur Apr 8, 2017 •

edited

Loading