Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bc-jz
Copy link
Contributor

@bc-jz bc-jz commented Jul 8, 2015

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 added 2 commits July 8, 2015 15:20
… 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
@bc-jz bc-jz changed the title Small fixes Small fixes to Sku and Shipment classes Jul 8, 2015
@gabelimon
Copy link

Tests are failing.

@bc-jz
Copy link
Contributor Author

bc-jz commented Jul 8, 2015

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

…uOption.php and SkuOptionTest.php as these don't match with API v2 schema.
@bc-jz
Copy link
Contributor Author

bc-jz commented Jul 9, 2015

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

@gabelimon
Copy link

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

@bc-jz
Copy link
Contributor Author

bc-jz commented Jul 15, 2015

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

@ransomcarroll
Copy link

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.

@bc-jz bc-jz mentioned this pull request Sep 3, 2015
@bc-jz
Copy link
Contributor Author

bc-jz commented Sep 29, 2015

@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()
Copy link
Contributor

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)

@PascalZajac
Copy link
Contributor

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

@mattolson
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants