Repository: xanzy/go-gitlab
Pull request #384 (2018-04-27 14:28:14)

Fix block/unblock
While doing some testing with error handling coming back from the block/unblock actions we discovered some issues with how errors were being handled in the gitlab library. The provided tests do not pass with the code in master and do with the changes,

svanharmelen: Thanks @joerocklin! I have one small comment, the rest looks great!

Patch:
diff --git a/users.go b/users.go
index 6a2f715..b6e68b6 100644
--- a/users.go
+++ b/users.go
@@ -22,6 +22,13 @@ import (
 	"time"
 )
 
+// List a couple of standard errors.
+var (
+	ErrUserBlockPrevented   = errors.New("Cannot block a user that is already blocked by LDAP synchronization")
+	ErrUserNotFound         = errors.New("User does not exist")
+	ErrUserUnblockPrevented = errors.New("Cannot unblock a user that is blocked by LDAP synchronization")
+)
+
 // UsersService handles communication with the user related methods of
 // the GitLab API.
 //
@@ -425,7 +432,7 @@ func (s *UsersService) BlockUser(user int, options ...OptionFunc) error {
 	}
 
 	resp, err := s.client.Do(req, nil)
-	if err != nil {
+	if err != nil && resp == nil {
 		return err
 	}
 
@@ -433,9 +440,9 @@ func (s *UsersService) BlockUser(user int, options ...OptionFunc) error {
 	case 201:
 		return nil
 	case 403:
-		return errors.New("Cannot block a user that is already blocked by LDAP synchronization")
+		return ErrUserBlockPrevented
 	case 404:
-		return errors.New("User does not exist")
+		return ErrUserNotFound
 	default:
 		return fmt.Errorf("Received unexpected result code: %d", resp.StatusCode)
 	}
@@ -453,7 +460,7 @@ func (s *UsersService) UnblockUser(user int, options ...OptionFunc) error {
 	}
 
 	resp, err := s.client.Do(req, nil)
-	if err != nil {
+	if err != nil && resp == nil {
 		return err
 	}
 
@@ -461,9 +468,9 @@ func (s *UsersService) UnblockUser(user int, options ...OptionFunc) error {
 	case 201:
 		return nil
 	case 403:
-		return errors.New("Cannot unblock a user that is blocked by LDAP synchronization")
+		return ErrUserUnblockPrevented
 	case 404:
-		return errors.New("User does not exist")
+		return ErrUserNotFound
 	default:
 		return fmt.Errorf("Received unexpected result code: %d", resp.StatusCode)
 	}


Test patch:
diff --git a/users_test.go b/users_test.go
new file mode 100644
index 0000000..d777394
--- /dev/null
+++ b/users_test.go
@@ -0,0 +1,165 @@
+package gitlab
+
+import (
+	"fmt"
+	"net/http"
+	"reflect"
+	"testing"
+)
+
+func TestBlockUser(t *testing.T) {
+	mux, server, client := setup()
+	defer teardown(server)
+
+	path := fmt.Sprintf("/%susers/1/block", apiVersionPath)
+	mux.HandleFunc(path, func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, "POST")
+		w.WriteHeader(http.StatusCreated)
+	})
+
+	err := client.Users.BlockUser(1)
+	if err != nil {
+		t.Errorf("Users.BlockUser returned error: %v", err)
+	}
+}
+
+func TestBlockUser_UserNotFound(t *testing.T) {
+	mux, server, client := setup()
+	defer teardown(server)
+
+	path := fmt.Sprintf("/%susers/1/block", apiVersionPath)
+	mux.HandleFunc(path, func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, "POST")
+		w.WriteHeader(http.StatusNotFound)
+	})
+
+	err := client.Users.BlockUser(1)
+	if err != ErrUserNotFound {
+		t.Errorf("Users.BlockUser error.\nExpected: %+v\n     Got: %+v", ErrUserNotFound, err)
+	}
+}
+
+func TestBlockUser_BlockPrevented(t *testing.T) {
+	mux, server, client := setup()
+	defer teardown(server)
+
+	path := fmt.Sprintf("/%susers/1/block", apiVersionPath)
+	mux.HandleFunc(path, func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, "POST")
+		w.WriteHeader(http.StatusForbidden)
+	})
+
+	err := client.Users.BlockUser(1)
+	if err != ErrUserBlockPrevented {
+		t.Errorf("Users.BlockUser error.\nExpected: %+v\n     Got: %+v", ErrUserBlockPrevented, err)
+	}
+}
+
+func TestBlockUser_UnknownError(t *testing.T) {
+	mux, server, client := setup()
+	defer teardown(server)
+
+	path := fmt.Sprintf("/%susers/1/block", apiVersionPath)
+	mux.HandleFunc(path, func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, "POST")
+		w.WriteHeader(http.StatusTeapot)
+	})
+
+	want := fmt.Errorf("Received unexpected result code: %d", http.StatusTeapot)
+
+	err := client.Users.BlockUser(1)
+	if !reflect.DeepEqual(err, want) {
+		t.Errorf("Users.BlockUser error.\nExpected: %+v\n     Got: %+v", want, err)
+	}
+}
+
+func TestBlockUser_BadResponseFromNet(t *testing.T) {
+	client := NewClient(nil, "")
+	client.SetBaseURL("")
+
+	want := fmt.Sprintf("Post /%susers/1/block: unsupported protocol scheme \"\"", apiVersionPath)
+
+	err := client.Users.BlockUser(1)
+	if err.Error() != want {
+		t.Errorf("Users.BlockUser error.\nExpected: %+v\n     Got: %+v", want, err)
+	}
+}
+
+//  ------------------------  Unblock user -------------------------
+func TestUnblockUser(t *testing.T) {
+	mux, server, client := setup()
+	defer teardown(server)
+
+	path := fmt.Sprintf("/%susers/1/unblock", apiVersionPath)
+	mux.HandleFunc(path, func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, "POST")
+		w.WriteHeader(http.StatusCreated)
+	})
+
+	err := client.Users.UnblockUser(1)
+	if err != nil {
+		t.Errorf("Users.UnblockUser returned error: %v", err)
+	}
+}
+
+func TestUnblockUser_UserNotFound(t *testing.T) {
+	mux, server, client := setup()
+	defer teardown(server)
+
+	path := fmt.Sprintf("/%susers/1/unblock", apiVersionPath)
+	mux.HandleFunc(path, func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, "POST")
+		w.WriteHeader(http.StatusNotFound)
+	})
+
+	err := client.Users.UnblockUser(1)
+	if err != ErrUserNotFound {
+		t.Errorf("Users.UnblockUser error.\nExpected: %+v\n     Got: %+v", ErrUserNotFound, err)
+	}
+}
+
+func TestUnblockUser_UnblockPrevented(t *testing.T) {
+	mux, server, client := setup()
+	defer teardown(server)
+
+	path := fmt.Sprintf("/%susers/1/unblock", apiVersionPath)
+	mux.HandleFunc(path, func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, "POST")
+		w.WriteHeader(http.StatusForbidden)
+	})
+
+	err := client.Users.UnblockUser(1)
+	if err != ErrUserUnblockPrevented {
+		t.Errorf("Users.UnblockUser error.\nExpected: %+v\n     Got: %+v", ErrUserUnblockPrevented, err)
+	}
+}
+
+func TestUnblockUser_UnknownError(t *testing.T) {
+	mux, server, client := setup()
+	defer teardown(server)
+
+	path := fmt.Sprintf("/%susers/1/unblock", apiVersionPath)
+	mux.HandleFunc(path, func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, "POST")
+		w.WriteHeader(http.StatusTeapot)
+	})
+
+	want := fmt.Errorf("Received unexpected result code: %d", http.StatusTeapot)
+
+	err := client.Users.UnblockUser(1)
+	if !reflect.DeepEqual(err, want) {
+		t.Errorf("Users.UnblockUser error.\nExpected: %+v\n     Got: %+v", want, err)
+	}
+}
+
+func TestUnblockUser_BadResponseFromNet(t *testing.T) {
+	client := NewClient(nil, "")
+	client.SetBaseURL("")
+
+	want := fmt.Sprintf("Post /%susers/1/unblock: unsupported protocol scheme \"\"", apiVersionPath)
+
+	err := client.Users.UnblockUser(1)
+	if err.Error() != want {
+		t.Errorf("Users.UnblockUser error.\nExpected: %+v\n     Got: %+v", want, err)
+	}
+}