User pagination and fields, docblocks, formatting, test improvements #261

joshcanhelp · 2018-06-08T21:42:15Z

Add fields and pagination parameters to Users::getAll() method
Docblocks and method parameter comments
Re-write User tests to match Connections and Clients

lbalmaceda

I think I saw this on other entities, but it's missing on this one. Maybe I'm confusing the repo. But I think it's missing the "scopes" required in the token for each call on the doc block.

lbalmaceda · 2018-06-11T18:07:56Z

src/API/Management/Users.php

+     * @param string $user_id - User ID to update.
+     * @param array $data - User data to update:
+     *      - Only certain fields can be updated; see the @link below for allowed fields.
+     *      - "user_metadata" and "app_metadata" fields are merged, not replaced.


this is true for this fields but also for the rest of the fields under User. I do like the clarification though.

In the API docs:

The properties of the new object will replace the old ones ... The metadata fields are an exception to this rule

Am I not capturing that correctly or are the docs wrong?

Ah. Users behave differently, ignore my first comment.

lbalmaceda · 2018-06-11T18:10:05Z

src/API/Management/Users.php

+     * Create a new User.
+     *
+     * @param array $data - User create data:
+     *      - "connection" name field is required and limited to sms, email, & DB connections.


instead of "DB" I'd use "sms, email and auth0 (Username and Password) strategies". I don't think you are allowed to create "ad" db users with this method

I'm a little hesitant to mix strategies in here. This field is looking for a connection name, not a strategy.

Should we clarify what kind of user can be created here?

This field is looking for a connection name, not a strategy.

true, but those connections must belong to "one of this strategies: sms, email, auth0"

lbalmaceda · 2018-06-11T18:10:44Z

src/API/Management/Users.php

+     * @param array $data - User create data:
+     *      - "connection" name field is required and limited to sms, email, & DB connections.
+     *      - "phone_number" field is required by sms connections.
+     *      - "email" field is required by email and DB connections.


depending on the connection settings, "username" could be required for sign-ups as well.

lbalmaceda · 2018-06-11T19:09:29Z

src/API/Management/Users.php

+
+            // Passwords are required for DB connections.
+            if ('email' !== $data['connection'] && empty($data['password'])) {
+                throw new \Exception('Missing required "password" field for DB connection.');


Maybe instead of 'db' use the name of the connection you are being passed.

lbalmaceda · 2018-06-11T19:29:27Z

src/API/Management/Users.php

+     *
+     * @link https://auth0.com/docs/users/search/v3#migrate-from-search-engine-v2-to-v3
+     * @link https://auth0.com/docs/api/management/v2#!/Users/get_users
+     * @link https://auth0.com/docs/users/search/best-practices


I see many links to sections under /docs/users/search. Why not just point to that page? Anchors to sections may break if a char on the title is changed.

First link is an anchor, the rest are direct. I wouldn't imagine that the migration guide link will change as it's in a number of places.

I'd just point to https://auth0.com/docs/users/search/v3 since that's the main page for the search API. Also, in that article there's a link to v2 syntax. So just keep:

https://auth0.com/docs/api/management/v2#!/Users/get_users (API)

https://auth0.com/docs/users/search/v3 (User's Search)

lbalmaceda · 2018-06-11T20:23:55Z

tests/API/BasicCrudTest.php

+     *
+     * @return bool
+     */
+    protected function errorHasString(\Exception $e, $str)


why \ precedes Exception always ?

\ indicates global namespce

lbalmaceda · 2018-06-11T20:25:23Z

tests/API/Management/UsersTest.php

+     *
+     * @var integer
+     */
+    protected $rand;


This seems common across entities tests. What about moving this up to the BasicCrudTest class?

Good catch ... that was kind of silly :)

lbalmaceda · 2018-06-11T20:29:03Z

tests/API/Management/UsersTest.php

+    protected function getAllEntities()
+    {
+        $fields = array_keys($this->getCreateBody());
+        $fields[] = $this->id_name;


what's this line doing? is like no index is passed to the array? 😕

$fields is an array of fields to get, created initially as the keys from the createBody array. This adds the id field to that array so we get the id as well.

but since "arrays in php are not arrays but dictionaries", am I right to assume this will end up adding a key named user_id (specified here https://github.com/auth0/auth0-PHP/pull/261/files/b018f664c75fbfaca8c8febb9148eacb2002d58f#diff-f6b74f1841de4422909e8c5982be5c87R19) with a null value?

No, this is just a plain array, numeric keys:

[ 'connection', 'email', 'password', 'picture', 'email_verified', 'user_metadata', 'user_id' ]

lbalmaceda · 2018-06-11T20:29:56Z

tests/API/Management/UsersTest.php

-        $this->domain = $env['DOMAIN'];
+        // Get the second page of Users with 1 per page (second result).
+        $paged_results = $this->api->getAll([
+            'sort' => 'created_at:1'


do the rest of the entities do the same sorting?

No. I added this because we're likely to get back a lot of results and this will ensure the pagination check works regardless of the number. Other endpoints don't appear to support sorting.

lbalmaceda · 2018-06-11T20:32:46Z

tests/API/Management/UsersTest.php

+        try {
+            $this->api->create([]);
+        } catch (\Exception $e) {
+            $caught_connection_error = $this->errorHasString($e, 'connection');


a bit vague. I don't remember the full error message but what about searching for something like "missing param 'connection'" at least? Same for the checks below

joshcanhelp · 2018-06-12T18:37:57Z

@lbalmaceda - Also added scopes here, good suggestion ... I'll make a note to add that to other docblocks

lbalmaceda

mostly docs changes

lbalmaceda · 2018-06-12T20:18:53Z

src/API/Management/Users.php

+{
+    /**
+     * Get a single User by ID.
+     * Requires scopes: read:users, read:user_idp_tokens.


Suggestion -> Enclose values in quotes. (applies for all docblocks)

IMHO required -> explain when the read:user_idp_tokens scope would be required. It's not that to call this endpoint you must provide it, only if you want to obtain the access_tokens for each logged in identity provider.

lbalmaceda · 2018-06-12T20:21:43Z

src/API/Management/Users.php

+    /**
+     * Update a User.
+     * Requires scope: update:users.
+     * May require scope: update:users_app_metadata


ahá!! again, when would this scope be required? Don't say it's implicit on the scope value. You can also rephrase it in a similar way to this:

Requires scope 'update:users'. If the values being changed include the 'user_metadata' or 'app_metadata' fields, additionally the scope must include 'update:users_app_metadata'.

lbalmaceda · 2018-06-12T20:23:04Z

src/API/Management/Users.php

+     *      - "phone_number" field is required by sms connections.
+     *      - "email" field is required by email and DB connections.
+     *      - "password" field is required by DB connections.
+     *      - Depending on the connection user, other fields may be required.


i think you meant user->used ??
Also, I don't think we enforce other fields apart from username. (We don't provide a way to customize this either AFAIK, maybe good to look at "custom db sign up scripts"). If it's that one alone, why not just name it?

lbalmaceda · 2018-06-12T20:27:25Z

src/API/Management/Users.php

-        $client = $this->apiClient->get()
-            ->users();
+        // Fields to include/exclude.
+        if (!isset($params['fields']) && null !== $fields) {


no debugging is needed as long as it's well documented. Being one case or the other. Both are corrects. I like more the one I suggested in my previous comment since the previous signature didn't have anything but the params dictionary, so anything added later (talking about order) would replace them, but I accept yours as well 👍 Choose whatever that sticks to the other entities' methods behavior.

lbalmaceda · 2018-06-12T20:31:35Z

src/API/Management/Users.php

+            if (is_array($params['fields'])) {
+                $params['fields'] = implode(',', $params['fields']);
+            }
+            if (null !== $include_fields) {


here you're not checking that include_fields is first set on params, so you're overriding it with the explicit parameter.

lbalmaceda · 2018-06-12T20:34:30Z

src/API/Management/Users.php

+     * Requires scope: update:users.
+     *
+     * @param string $user_id - User ID of the primary identity
+     * @param array $data - Secondary identity to link; either link_with JWT or provider, connection_id, and user_id.


What about adding something like "see ways of calling this endpoint in the @link below", like you did for users search v3 above. Or at least add a bulleted list below the data param to differentiate both payloads. (I still find it confusing)

i.e.

- Sending the JWT as the link_with parameter - Sending the provider, connection_id and user_id values.

lbalmaceda · 2018-06-12T20:39:38Z

tests/API/Management/UsersTest.php

+     * Should the results returned by getAll be searched for the created user?
+     *
+     * @var bool
+     */
    protected $findCreatedItem = false;


also, doesn't this apply for all the entities with a CREATE method? Should it be generalized like the rand in the super class?

No, it's an option of sorts

lbalmaceda

👨‍🎓

github-actions · 2022-08-24T00:20:29Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

User pagination and fields, docblocks, formatting test improvements

b018f66

joshcanhelp added CH: Added labels Jun 8, 2018

joshcanhelp added this to the v5-Next milestone Jun 8, 2018

joshcanhelp mentioned this pull request Jun 11, 2018

Fix PHPUnit test. #262

Closed

lbalmaceda suggested changes Jun 11, 2018

View reviewed changes

joshcanhelp added 3 commits June 11, 2018 14:20

PR feedback tweaks

1cb264b

adding scopes

92ba067

PR feedback tweaks

76830ee

lbalmaceda reviewed Jun 12, 2018

View reviewed changes

lbalmaceda approved these changes Jun 12, 2018

View reviewed changes

docblock changes and don't override 'include_fields'

6af9428

joshcanhelp force-pushed the add-users-pagination-tests branch from 816576d to 6af9428 Compare June 12, 2018 21:18

joshcanhelp removed the Ready to Review label Jun 13, 2018

joshcanhelp merged commit 11b9e3f into master Jun 13, 2018

joshcanhelp deleted the add-users-pagination-tests branch June 13, 2018 04:34

github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2022

User pagination and fields, docblocks, formatting, test improvements #261

User pagination and fields, docblocks, formatting, test improvements #261

Conversation

joshcanhelp commented Jun 8, 2018

lbalmaceda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshcanhelp commented Jun 12, 2018

lbalmaceda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 24, 2022

lbalmaceda Jun 11, 2018 •

edited

Loading