opsway / zohobooks-api

ZohoBooks PHP library
MIT License
6 stars 10 forks source link

Added `page_context` in order to use zoho pagination #19

Closed ppicatto closed 7 years ago

ppicatto commented 7 years ago
Q A
Branch master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

See https://www.zoho.com/books/api/v3/#pagination

phansys commented 7 years ago

@shandyDev, is there something preventing this PR to be merged?

mrVrAlex commented 7 years ago

Sorry, I was on vacation.

So, firstly this PR will BC with current API (changes output array structure). Second, I see zoho updated documentation and I little confused - https://www.zoho.com/books/api/v3/#pagination respose structure this:

 {
     "code": 0,
     "message": "success",
     "contacts": [
             {...},
             {...}
     ],
     "page_context": {
         "page": 2,
         "per_page": 25,
         "has_more_page": false
     }
 }

BUT there https://www.zoho.com/books/api/v3/#Contacts_List_contacts

{
    "code": 0,
    "message": "success",
    "contacts": [
        {
            "contacts": [
                {
                    "contact_id": 460000000026049,
                    ....
                }
            ],
            "page_context": {
                "page": 1,
                "per_page": 200,
                "has_more_page": false,
                "applied_filter": "Status.All",
                "sort_column": "contact_name",
                "sort_order": "D"
            }
        },
        {...},
        {...}
    ]
}

page_context key not root anymore, and response have nested "contacts" key.

So if this real works, maybe we should reflect new changes in getList, get methods?

Also my another proposition we can to avoid BC here if create ItemList class (with ArrayAccess, IteratorAgregate interfaces) and will return it in getList method: return new ItemList ($response[static::API_KEY.'s']); So if current code use result array as foreach($list as $item) - this will work, if code use result as $list[0]['some_key_in_item'] - this also will work. BUT ItemList object also will provide 2 additional methods: getItems() - return array items (as now) + getPaginator() - with data array in "page_context".

@ppicatto @phansys So, what you think?

ppicatto commented 7 years ago

Hi @shandyDev thanks for the answer. I have the following consideration about the points that you mark on your response: 1) The api documentation it's wrong because it returns just one contacts key as you can see in the following json that was retreived by calling https://books.zoho.com/api/v3/contacts endpoint:

{
  "code": 0,
  "message": "success",
  "contacts": [
    {
      "contact_id": "645664000000064001",
      "contact_name": ", ",
      "customer_name": ", ",
      "vendor_name": ", ",
      "company_name": "My Company",
      "contact_type": "customer",
      "status": "active",
      "source": "api",
      "is_linked_with_zohocrm": false,
      "payment_terms": 0,
      "payment_terms_label": "Due On Receipt",
      "currency_id": "645664000000058022",
      "currency_code": "ARS",
      "outstanding_receivable_amount": 0,
      "outstanding_payable_amount": 0,
      "unused_credits_receivable_amount": 0,
      "unused_credits_payable_amount": 0,
      "first_name": "",
      "last_name": "",
      "email": "johndoe@example.com",
      "phone": "",
      "mobile": "",
      "created_time": "2017-05-11T09:17:58-0300",
      "last_modified_time": "2017-05-11T09:18:00-0300",
      "custom_fields": [],
      "ach_supported": false,
      "has_attachment": false
    }
  ],
  "page_context": {
    "page": 1,
    "per_page": 200,
    "has_more_page": false,
    "report_name": "Contacts",
    "applied_filter": "Status.All",
    "custom_fields": [],
    "sort_column": "contact_name",
    "sort_order": "A"
  }
}

2) I think that your proposition about create an ItemList class it's a good idea so I'll try implement it in this PR

mrVrAlex commented 7 years ago

Now we have one moment: foreach($list as $item) - will works as usual, but $list[0]['some_key_in_item'] - will not work, only if change code as $list['items'][0]['some_key_in_item']. I think we should implement both versions like:

public function offsetGet($offset)
{
    if (is_numeric($offset) && isset($this->items[$offset])) {
        return $this->items[$offset];
    }
    return $this->offsetExists($offset) ? $this->{$offset} : [];
}
//.... and same logic in other offsetMethods...

So access directly by numeric key $list[0] will return entity in list (like long method $list['items'][0]) but access to page_access also will work via $list['pageContext']. This behavior will consistent with foreach:

foreach ($list as $key => $item) {
  if ($list[$key] !== $item) {
    echo "This should not happen."; // Now it happened.
  }
}

Also I added in master branch some unit tests on this behavior. This protect us for BC changes in future.

codecov[bot] commented 7 years ago

Codecov Report

Merging #19 into master will increase coverage by 3.55%. The diff coverage is 34.61%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #19      +/-   ##
===========================================
+ Coverage      3.27%   6.82%   +3.55%     
- Complexity       95     112      +17     
===========================================
  Files            12      12              
  Lines           183     205      +22     
===========================================
+ Hits              6      14       +8     
- Misses          177     191      +14
Impacted Files Coverage Δ Complexity Δ
src/Api/BaseApi.php 30% <100%> (ø) 6 <0> (ø) :arrow_down:
src/Api/ItemList.php 32% <32%> (ø) 18 <18> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 45d331f...cabe9bd. Read the comment docs.

mrVrAlex commented 7 years ago

So we have green status. I merged it & released new version.