bigcommerce-api-php
bigcommerce-api-php copied to clipboard
Small fixes to Sku and Shipment classes
Made a couple of small fixes to the PHP API Library:
- Sku class had an options() method that tried to pull a list of options from a separate resource - this is not needed as a sku object has data to access in the 'options' property. Leaving this method creates a conflict trying to access that array. Here is an example of a SKU object:
{ "id": 1, "product_id": 5, "sku": "MB-1", "cost_price": "0.0000", "upc": "", "inventory_level": 0, "inventory_warning_level": 0, "bin_picking_number": "", "options": [ { "product_option_id": 15, "option_value_id": 17 }, { "product_option_id": 16, "option_value_id": 28 } ] }
- The Shipment class had incorrect ignore values. Namely it was set to ignore 'shipping_method' even though that is an updatable field.
Read-only fields can be seen here: https://developer.bigcommerce.com/api/stores/v2/orders/shipments
Tests are failing.
@gabelimon
It looks like a failure in the test:
https://github.com/bigcommerce/bigcommerce-api-php/blob/master/test/Unit/Api/Resources/SkuTest.php#L29-L47
I don't exactly understand the code but it appears it is testing to make sure that 'options' property which holds a 'resource' value on the SKU object is replaced by the results of a GET request to '/products/1/skus/1/options', which is what the options() method did. I have removed the options() method so this test appears to no longer be needed.
I can add another commit to remove the above test if you agree with me. Thinking more on this subject, the entire SkuOption class is no longer needed.
Added a new commit to remove the current tests that are no longer needed after the previous commits and remove the SkuOption class.
I'm not sure how much sense things makes so I'll defer. Ping @philipmuir
@philipmuir Can you confirm/deny that these changes make sense? I have been getting cases from Developers having issues accessing the 'options' property on a SKU object due to the existing options() method on the SKU class. Removing that method has fixed things for them and makes sense when you look at the sku object schema.
This is the function I used to correctly pull in options.
class Sku extends Resource
{
public function options()
{
return $this->fields->options;
}
}
It's as simple as that, there doesn't need to be an extra resource for these.
@aleachjr Can we get this PR merged? It relates to two other PRs put in for the same problem in the library.
@bc-jz if you rebase this I'll see about getting it merged.
@bc-jz Is this still a thing? We should close out old PRs unless we intend to merge them.