Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to lock (toggle) users. #98

Merged
merged 3 commits into from
Feb 22, 2023
Merged

Allow to lock (toggle) users. #98

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a018b22
Allow to lock (toggle) users.
ndeet Feb 21, 2023
1f7edfa
Adding missing parameter to example.
ndeet Feb 21, 2023
79dd759
Rename toggleUser().
ndeet Feb 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions examples/user_usage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,21 +49,30 @@ public function createUser()
}
}

public function deleteUser()
public function deleteUser($userId)
{
$userId = '';

try {
$client = new User($this->host, $this->apiKey);
var_dump($client->deleteUser($userId));
} catch (\Throwable $e) {
echo "Error: " . $e->getMessage();
}
}

public function toggleUser($userId, $toggle)
{
try {
$client = new User($this->host, $this->apiKey);
var_dump($client->toggleUser($userId, $toggle));
} catch (\Throwable $e) {
echo "Error: " . $e->getMessage();
}
}
}

$users = new Users();
//$users->getCurrentUserInformation();
//$users->deleteCurrentUserProfile();
//$users->createUser();
//$users->deleteUser();
//$users->toggleUser("test@example.com", true);
22 changes: 22 additions & 0 deletions src/Client/User.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,4 +81,26 @@ public function deleteUser(string $userId): bool
throw $this->getExceptionByStatusCode($method, $url, $response);
}
}

public function toggleUser(string $userId, bool $locked): bool
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe setIsLocked() ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tracking here #100

{
$url = $this->getApiUrl() . 'users/' . urlencode($userId) . '/lock';
$headers = $this->getRequestHeaders();
$method = 'POST';

$body = json_encode(
[
'locked' => $locked,
],
JSON_THROW_ON_ERROR
);

$response = $this->getHttpClient()->request($method, $url, $headers, $body);

if ($response->getStatus() === 200) {
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why return true if you're never going to return false? Just return void and let us be happy there was no exception.

Copy link
Collaborator Author

@ndeet ndeet Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on sticking with existing code style. Would be better to refactor in next major version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tracking it here #99

} else {
throw $this->getExceptionByStatusCode($method, $url, $response);
}
}
}