BIG-21201 fix Sku options

[zvuki]

related issue #92

cc @bc-joshroe @bc-sandeep @bc-bfenton

[zvuki]

I may need to increment a version for bigcommerce-api-php, do I?

[bc-bfenton]

if you want it to be deployable in the app, yes. I'm also not sure you have the ability to tag new versions? I think only maybe Anthony & Matt can.

Also, Travis is unhappy, and I don't think this is the right fix for this bug. You're removing functionality as well as code that will cause an error if executed, rather than fixing bug in a way that keeps the functionality

[zvuki]

@bc-bfenton
I am removing the code that adds product_id to the option level
$option->product_id = $this->product_id
but it doesn't belong to this level, check this out https://developer.bigcommerce.com/api/stores/v2/products/skus#get-a-product-sku

I'll remove the failing test as well, as it's checking the removed code functionality.

[bc-bfenton]

So what happens if we call $sku->option? do we get a list of SkuOption resources?

[zvuki]

@bc-bfenton It returns an Array of stdClass Objects

[bc-bfenton]

I think if we can pull it off it should be an array of \Resource\SkuOptions objects, but if not then stdClass is better than broken

[zvuki]

I deleted \Resource\SkuOptions as there is no such documented resource

cc @lord2800

[sandeepgraju]

Can you add the functional test you updated in SkuGetApiTest , with additional assert

[zvuki]

@bc-sandeep this must go to a separate PR, I thought you were writing this test. so after this PR is merged you'll be able to do composer install -o and check that the bug is gone

[lord2800]

Agree re: SkuOptions. I wasn't able to find that endpoint in our docs nor our code anywhere. Additionally, it's a class that would have exactly 2 public properties and no create/delete/update/get functions, so I don't see a point in it existing. Happy for others to prove me wrong though.

[bc-bfenton]

I'm good with that. Also we'll additionally have to tag a new release of the API client before composer install -o will get the new code. we might also need to update composer.json too, since it specifies that we're using "bigcommerce/api": "3.0.0-beta.10"

[sandeepgraju]

Considering this is going to master for all existing customers . Would that be okI would rather see both both dev code and test code going together.

Can i merge the updated test with additional asserts into your branch.
Should be super quick and would make sure it works atleast for basic vanilla case

[sandeepgraju]

Spoke offline. As the functional test I mentioned is in different repo, the one added in functional suite won't pass unless the steps done by Andrei are mentioned in above.

So I will merge the tests in bc repo once after this merge happens. Looks good from my side to merge 💚

[zvuki]

Hi @aleachjr! Could you merge this PR and set a version on it? :)

[bc-jz]

These are the same changes made in this PR:
#146

which also includes small fixes to the Shipments class.