bigcommerce-api-php icon indicating copy to clipboard operation
bigcommerce-api-php copied to clipboard

Small fixes to Sku and Shipment classes

Open bc-jz opened this issue 10 years ago • 9 comments
trafficstars

Made a couple of small fixes to the PHP API Library:

  1. 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 } ] }

  1. 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

bc-jz avatar Jul 08 '15 21:07 bc-jz

Tests are failing.

gabelimon avatar Jul 08 '15 21:07 gabelimon

@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.

bc-jz avatar Jul 08 '15 22:07 bc-jz

Added a new commit to remove the current tests that are no longer needed after the previous commits and remove the SkuOption class.

bc-jz avatar Jul 09 '15 01:07 bc-jz

I'm not sure how much sense things makes so I'll defer. Ping @philipmuir

gabelimon avatar Jul 09 '15 03:07 gabelimon

@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.

bc-jz avatar Jul 15 '15 22:07 bc-jz

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.

ransomcarroll avatar Aug 26 '15 23:08 ransomcarroll

@aleachjr Can we get this PR merged? It relates to two other PRs put in for the same problem in the library.

bc-jz avatar Sep 29 '15 21:09 bc-jz

@bc-jz if you rebase this I'll see about getting it merged.

PascalZajac avatar Jun 02 '16 07:06 PascalZajac

@bc-jz Is this still a thing? We should close out old PRs unless we intend to merge them.

mattolson avatar Dec 06 '19 23:12 mattolson