-
Notifications
You must be signed in to change notification settings - Fork 185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Small fixes to Sku and Shipment classes #146
base: master
Are you sure you want to change the base?
Conversation
… on a Sku object is an array of option objects, it is not a reference to a separate resource as this method expects.
…address' and 'shipping_address' - this makes the PHP library match the API requirements
Tests are failing. |
It looks like a failure in the test: 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. |
…uOption.php and SkuOptionTest.php as these don't match with API v2 schema.
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.
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. |
@@ -19,17 +19,6 @@ class Sku extends Resource | |||
'product_id', | |||
); | |||
|
|||
public function options() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd look at keeping this method for backwards compaitability (assuming it returns the same objects), with the implementation @ransomcarroll suggested here: #146 (comment)
@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. |
Made a couple of small fixes to the PHP API Library:
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
}
]
}
Read-only fields can be seen here:
https://developer.bigcommerce.com/api/stores/v2/orders/shipments