Allow to lock (toggle) users. #98
Conversation
src/Client/User.php
Outdated
| @@ -81,4 +81,26 @@ public function deleteUser(string $userId): bool | |||
| throw $this->getExceptionByStatusCode($method, $url, $response); | |||
| } | |||
| } | |||
|
|
|||
| public function toggleUser(string $userId, bool $locked): bool | |||
There was a problem hiding this comment.
I don't like the name of this method. Why not setUserLock() or something like that? It's not really a toggle if you're gonna pass the value. A toggle is off to on or on to off, not setting a value.
There was a problem hiding this comment.
Or maybe setIsLocked() ?
There was a problem hiding this comment.
Yeah I basically tried to use what is in the API docs to avoid confusion. But yes we decided to do an opinionated library so it is ok to name things differently I guess. Will change it.
I will do setUserLock as we have User in all the other functions already. In hindsight we might should have left that repeating of the ClassName out on functions. Also the classes maybe should have been called UserClient and UserResult to avoid the need for aliasing classes when both are used. But yeah not too bad either way.
There was a problem hiding this comment.
These are all good ideas. We can do this, but would need to release a new major version as they are breaking changes. I am for doing this as existing users can just stay on the current version.
| $response = $this->getHttpClient()->request($method, $url, $headers, $body); | ||
|
|
||
| if ($response->getStatus() === 200) { | ||
| return true; |
There was a problem hiding this comment.
Why return true if you're never going to return false? Just return void and let us be happy there was no exception.
There was a problem hiding this comment.
If you search the code we have that in many places like this. The benefit over void is that you can rely on it if it was successful? e.g. if (someFunction()) {} ? A void return type would return null and you can't be sure it worked? On the other hand you have it in a try catch block anyway, so yeah not sure. Maybe keep it for now as of consistency with the other places we use it like this and refactor on a major release?
There was a problem hiding this comment.
Agreed on sticking with existing code style. Would be better to refactor in next major version.
There was a problem hiding this comment.
tracking it here #99
No description provided.