Clients endpoint pagination and improvements #256

joshcanhelp · 2018-05-31T17:24:41Z

Add pagination to getAll method via page and per_page parameters.
Add application type to getAll method via app_type parameter and app_type constants.
Change apiClient fluent methods to explicit.
Add validation for required 'name' parameter in create method.
Add docblocks throughout Clients.
Add additional testing for Clients and BasicCrudTest.

Add pagination to getAll method via page and per_page parameters. Add application type to getAll method via app_type parameter and app_type constants. Change apiClient fluent methods to explicit. Add validation for required 'name' parameter in create method. Add docblocks throughout Clients. Add additional testing for Clients and BasicCrudTest.

lbalmaceda · 2018-06-01T18:25:45Z

src/API/Management/Clients.php

                $fields = implode(',', $fields);
            }
            $request->withParam('fields', $fields);
        }

-        if ($include_fields !== null) 
-        {
+        if (null !== $include_fields) {


any reason to invert the null conditions?

Yoda conditionals, they are.

lbalmaceda · 2018-06-01T18:31:51Z

src/API/Management/Clients.php

     */
-    public function getAll($fields = null, $include_fields = null) 
+    public function getAll($fields = null, $include_fields = null, $app_type = null, $page = 0, $per_page = null)


so you're changing the existing method's signature. I guess because you're setting default values on the side of each parameter, calling e.g. getAll("name,address", true) will continue to work, not introducing breaking changes on this change right?
Separate from that, would it make sense to have a dedicated parameter+object for "client properties" so you keep the parameter count low even when new parameters are introduced to the endpoint? You'd need to keep fields and include_fields since those where there before. i.e. would you keep adding parameters to this "getAll" signature after they are added on the server side?

Also, note is missing "is_global", "is_first_party", "include_totals". This function is already executing the request (call) so the user is not able to add those if he/she wants.

lbalmaceda · 2018-06-01T18:59:32Z

src/API/Management/Clients.php

-     * @param null|string|array $include_fields
+     * Allowed app types for self::getAll().
+     */
+    const APP_TYPE_NATIVE = 'native';


IMHO since "app_type" is an optional parameter you can request to filter the result and not something you're required to specify to just "get a client" (in opposition to the email_templates endpoints where this was the ID), this is only helpful for people who want to use this param as a filter. Which, is probably a low number of people. It will do no harm to keep it, and TBH I don't think there will be any new app type nor that the types change in the future. But consider removing them and just explaining in the javadocs what are the allowed values.

lbalmaceda · 2018-06-01T19:14:30Z

src/API/Management/Clients.php

-        {
-            if (is_array($fields)) 
-            {
+        if (!empty($fields)) {


empty worked for null values too, right?

lbalmaceda · 2018-06-01T19:45:00Z

tests/API/BasicCrudTest.php

+     *
+     * @return mixed
+     */
+    protected function getAll($crud_api, $created_entity)


why is crud_api a parameter, shouldn't that one be common to all the test file?

Will rename the getAll() for the test class.

lbalmaceda · 2018-06-01T19:58:31Z

tests/API/BasicCrudTest.php

+
+        // Test a generic "get entity" method.
+        $got_entity = $crud_api->get($created_entity_id);
+        $this->afterCreate($got_entity);


why are you calling "after create" after a "get one"?

Will add a compare method to check against created entity.

lbalmaceda · 2018-06-01T20:00:26Z

tests/API/BasicCrudTest.php

+        $this->afterCreate($got_entity);
+
+        // Test a generic "get all entities" method for this API client.
+        $all_entities = $this->getAll($crud_api, $created_entity);


why is this call passing the crud api instead of calling getAll over that api in the first place? like you do on the rest of the methods..

Will set $crud_api as a property of the class.

lbalmaceda · 2018-06-01T20:10:01Z

tests/API/Management/ClientsTest.php

+    protected function getCreateBody()
+    {
+        return [
+            'name' => $this->create_client_name,


why not just put all this body data on a JSON file and read it or even a single parameter instead of having many of them defined above? it looks much cleaner IMO.

lbalmaceda · 2018-06-01T20:12:36Z

tests/API/Management/ClientsTest.php

+        $fields[] = $this->id_name;
+
+        // Check that pagination works.
+        $all_results = $client->getAll($fields, true, $this->create_app_type, 1, 1);


this is getting page 1 (of 0 based) and 1 result per page. If a single client exists, that one would belong to page 0 right? So this is returning the second client. Is that assumption OK ?

lbalmaceda · 2018-06-01T20:15:26Z

tests/API/Management/ClientsTest.php

-        return ['name' => $client_name, 'sso' => false];
+        // If we want to check for the created result, we need all Clients.
+        if ($this->findCreatedItem) {
+            $all_results = $client->getAll($fields, true, $this->create_app_type, 0, 100);


I find a waste of resources to make 2 consecutive calls to get them. Just get them all on the first call and that's it.. If you need to test that the page params are being set correctly, just check that on the ongoing request body.

lbalmaceda

LGTM

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

src/API/Helpers/RequestBuilder.php

@@ -286,6 +286,17 @@ public function withParams($params) {
        return $this;
    }

+    /**
+     * @param array $params


isn't this a dict?

lbalmaceda · 2018-06-04T20:13:09Z

src/API/Helpers/RequestBuilder.php

@@ -286,6 +286,20 @@ public function withParams($params) {
        return $this;
    }

+    /**
+     * Add URL parameters using $key => $value array.


isn't this a dict?

@lbalmaceda - Yes, just being explicit. There's another function that looks for an array of arrays with key and value keys

lbalmaceda · 2018-06-04T20:34:46Z

src/API/Management/Clients.php

-        $request = $this->apiClient->get()
-            ->clients();
+        // Set additional parameters first so they are over-written by function parameters.
+        $params = $add_params;


what if $add_params is null? this would make following interactions with this element fail

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

src/API/Management/Clients.php

    {
-        $request = $this->apiClient->get()
-            ->clients();
+        // Set additional parameters first so they are over-written by function parameters.


this is fine, but would be nice to get it clearer on the docblock for this add_params param. e.g. "the values specified here are overwritten by the previous params" or something like that

lbalmaceda · 2018-06-04T20:38:07Z

src/API/Management/Clients.php

     */
-    public function getAll($fields = null, $include_fields = null) 
+    public function getAll($fields = null, $include_fields = null, $page = 0, $per_page = null, $add_params = [])


I guess you kept pagination params separated so they are used and not ignored, right?

lbalmaceda · 2018-06-04T20:52:09Z

tests/API/BasicCrudTest.php

+    {
+        // Test a generic "create entity" method for this API client.
+        $created_entity = $this->api->create($this->getCreateBody());
+        $created_entity_id = $this->getId($created_entity);


since this line is used not in the line right next below this one, i'll swap this line and the one below.

lbalmaceda · 2018-06-04T20:57:32Z

tests/API/Management/ClientsTest.php

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


what's this line doing?

Makes sure we have the ID as well so the findCreatedItem loop will work properly.

lbalmaceda · 2018-06-04T21:03:21Z

tests/API/Management/ClientsTest.php

+        $this->assertLessThan($many_results_per_page, count($many_results));
+
+        // Make sure our paged result above appears in the right place.
+        $this->assertEquals($paged_results[0][$this->id_name], $many_results[$page_num][$this->id_name]);


at a first glance it looked wrong to use $page_num but it's fine. Please add another comment line saying something like "because we previously requested 1 per page, $page_num represents the expected item in the $paged_results"

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

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.

joshcanhelp added CH: Added labels May 31, 2018

joshcanhelp mentioned this pull request May 31, 2018

Clients endpoint pagination and improvements #255

Closed

joshcanhelp force-pushed the add-clients-pagination-apptype-tests branch 2 times, most recently from cee43fb to 0d6c50a Compare May 31, 2018 17:53

joshcanhelp force-pushed the add-clients-pagination-apptype-tests branch from 0d6c50a to 7c7f0c2 Compare May 31, 2018 19:44

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

joshcanhelp mentioned this pull request Jun 1, 2018

Allow fetching all connections for accounts that have a lot of them #247

Closed

lbalmaceda reviewed Jun 1, 2018

View reviewed changes

PR review feedback part 1

16c2dbd

joshcanhelp force-pushed the add-clients-pagination-apptype-tests branch from c6877a6 to 5f1d7bc Compare June 4, 2018 15:19

better tests based on PR feedback

82e7b10

joshcanhelp force-pushed the add-clients-pagination-apptype-tests branch from 5f1d7bc to 82e7b10 Compare June 4, 2018 16:24

lbalmaceda approved these changes Jun 4, 2018

View reviewed changes

Minor tweaks from PR review

d36da38

joshcanhelp removed the Ready to Review label Jun 4, 2018

joshcanhelp merged commit e5ba87d into master Jun 4, 2018

joshcanhelp deleted the add-clients-pagination-apptype-tests branch June 4, 2018 22:12

joshcanhelp mentioned this pull request Jun 4, 2018

Pagination, additional parameters, and tests for the Connections endpoint #258

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clients endpoint pagination and improvements #256

Clients endpoint pagination and improvements #256

joshcanhelp commented May 31, 2018

lbalmaceda Jun 1, 2018

joshcanhelp Jun 1, 2018

lbalmaceda Jun 1, 2018 •

edited

Loading

lbalmaceda Jun 1, 2018

lbalmaceda Jun 1, 2018

lbalmaceda Jun 1, 2018

joshcanhelp Jun 1, 2018

lbalmaceda Jun 1, 2018

joshcanhelp Jun 1, 2018

lbalmaceda Jun 1, 2018

joshcanhelp Jun 1, 2018

lbalmaceda Jun 1, 2018

lbalmaceda Jun 1, 2018

lbalmaceda Jun 1, 2018

lbalmaceda left a comment

lbalmaceda Jun 4, 2018

lbalmaceda Jun 4, 2018

joshcanhelp Jun 4, 2018

lbalmaceda Jun 4, 2018

lbalmaceda Jun 4, 2018

lbalmaceda Jun 4, 2018

joshcanhelp Jun 4, 2018

lbalmaceda Jun 4, 2018

lbalmaceda Jun 4, 2018

joshcanhelp Jun 4, 2018

lbalmaceda Jun 4, 2018

github-actions bot commented Aug 24, 2022

Clients endpoint pagination and improvements #256

Clients endpoint pagination and improvements #256

Conversation

joshcanhelp commented May 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda Jun 1, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 24, 2022

lbalmaceda Jun 1, 2018 •

edited

Loading