Add validation and tests for deprecation of product and variant inventory-related fields #655

djhoughton · 2019-12-20T22:05:05Z

fixes #632

The following fields on Product (total_inventory) and Variant (inventory_quantity_adjustment, inventory_quantity, old_inventory_quantity) were deprecated earlier this year and were removed in the 2019-10 api version.

This PR updates the code and adds tests to ensure the behaviour is correct.

Reviewer's note: In test/test_helper.rb I changed the code to return the current API version if none was specified. The old code returned the Jan 2019 version. I believe this new behaviour is correct but would like confirmation.

swalkinshaw · 2020-01-14T15:24:04Z

lib/shopify_api/resources/product.rb

@@ -16,6 +16,15 @@ def price_range
      end
    end

+    def serializable_hash(options = {})
+      super.except("total_inventory")


I'm confused why this is needed? For versions without the field, it won't be here anyway. And for versions with it, this is a breaking change since it's done unconditionally.

If someone were still on 2019-07 for example, this would be a breaking change.

Same question applies for the variant change below.

Right, I see what you mean. Honestly, I can't remember why we added that (it was before Christmas) but looking at it now and based on your arguments above I agree that we can remove it. 👍

@eljojo Do you remember the logic behind the changes in serializable_hash?

I think in these scenarios we can actually strip them out from serializable_hash in versions 2019-10 and later; if we already know we're going to fail those requests without clients making those changes, we should at least make it easier for them by omitting them in the scenario where they copy the response directly into the request.

swalkinshaw · 2020-01-14T19:09:07Z

lib/shopify_api/resources/product.rb

@@ -16,6 +16,11 @@ def price_range
      end
    end

+    def total_inventory=(new_value)
+      raise(ShopifyAPI::ValidationException, 'deprecated behaviour') unless Base.api_version < ApiVersion.find_version('2019-10')


Maybe the exception messages should point to https://help.shopify.com/en/api/guides/inventory-migration-guide?

spadae22 · 2020-03-17T21:17:37Z

Are they going to merge this, issue still exists?

djhoughton · 2020-03-18T12:48:45Z

Thanks for the ping! We're still working out a few things with the deprecation and we'll update this PR once it is sorted. Stay tuned...

Saibotx · 2020-03-18T18:57:07Z

Hello - do we have an ETA on this being merged?

As of now, any shopify product / variant edit / create / deletes are broken right now. I'm on standby waiting on this fix.

dnswus · 2020-03-18T18:59:16Z

I'm doing the following temp fix (#632 (comment)) on my side before this is merged:

variant.attributes.delete(:inventory_quantity)
variant.attributes.delete(:old_inventory_quantity)

EmmaB · 2020-03-18T18:59:24Z

We had to do an urgent workaround.

djhoughton · 2020-03-18T19:08:40Z

I'm working on updating the current PR to remove those fields from the response for requests using the Oct 2019 API release and later.

slimdave · 2020-03-18T20:05:51Z

This change just moves the error to the gem, rather than relying on the response from Shopify, is that correct? Or have I misunderstood?

djhoughton · 2020-03-18T20:47:12Z

This change just moves the error to the gem, rather than relying on the response from Shopify, is that correct? Or have I misunderstood?

Yes, that's correct

…in 2019-10 and later

djhoughton · 2020-03-19T00:18:34Z

To be clear this PR makes 2 changes if a client is using the 2019-10 API version or later:
1). If the client tries to set a deprecated inventory-related field on Product or Variant, we now throw an exception in the gem rather than going to the server to get the failed request
2). We remove the deprecated fields from being serialized in case clients try to send them back to the server without removing them.

The deprecations are outlined here:
https://shopify.dev/docs/admin-api/rest/reference/products/product-variant
https://shopify.dev/tutorials/migrate-your-app-to-support-multiple-locations
https://shopify.dev/tutorials/manage-product-inventory-with-admin-api

@swalkinshaw Would appreciate your feedback again on this one to make sure we're aligned now that the changes have been shipped to Core.

spadae22 · 2020-03-19T18:17:43Z

Hi, when will this be merged?

spadae22 · 2020-03-20T17:12:07Z

HI,
The fields have been removed with 9.0.3, but I when I PUT this product file, I still get Bad Request? I am NOT updating inventory.

PUT https://chriscoffee-dev.myshopify.com/admin/api/2019-10/products/4340483522611.json

{"product":{"id":4340483522611,"title":"1.0mm #0 Steam Tip","body_html":"\u003cp\u003e8.8mm female thread\u003c/p\u003e","vendor":"Slayer Espresso","product_type":"Slayer","created_at":"2019-11-10T21:17:26-05:00","handle":"1-0mm-0-steam-tip","updated_at":"2020-02-17T00:15:03-05:00","published_at":"2019-11-10T21:17:25-05:00","template_suffix":null,"published_scope":"web","tags":"Accessories","admin_graphql_api_id":"gid://shopify/Product/4340483522611","variants":[{"id":31148589187123,"product_id":4340483522611,"title":"Default Title","price":"25.95","sku":"46000-50340","position":1,"inventory_policy":"deny","compare_at_price":null,"fulfillment_service":"manual","inventory_management":"shopify","option1":"Default Title","option2":null,"option3":null,"created_at":"2019-11-10T21:17:26-05:00","updated_at":"2020-02-11T14:02:55-05:00","taxable":true,"barcode":"false","grams":23,"image_id":null,"weight":0.0507,"weight_unit":"lb","inventory_item_id":32667358199859,"inventory_quantity":49,"old_inventory_quantity":49,"tax_code":"","requires_shipping":true,"admin_graphql_api_id":"gid://shopify/ProductVariant/31148589187123"}],"options":[{"id":5642415570995,"product_id":4340483522611,"name":"Title","position":1,"values":["Default Title"]}],"images":[{"id":13402636582963,"product_id":4340483522611,"position":1,"created_at":"2019-11-10T21:17:26-05:00","updated_at":"2019-11-10T21:17:26-05:00","alt":null,"width":1200,"height":1200,"src":"https://cdn.shopify.com/s/files/1/0251/6398/9043/products/46000-50340.jpg?v=1573438646","variant_ids":[],"admin_graphql_api_id":"gid://shopify/ProductImage/13402636582963"}],"image":{"id":13402636582963,"product_id":4340483522611,"position":1,"created_at":"2019-11-10T21:17:26-05:00","updated_at":"2019-11-10T21:17:26-05:00","alt":null,"width":1200,"height":1200,"src":"https://cdn.shopify.com/s/files/1/0251/6398/9043/products/46000-50340.jpg?v=1573438646","variant_ids":[],"admin_graphql_api_id":"gid://shopify/ProductImage/13402636582963"}}}

Response:BadRequest: Response(code=400, body="{"error":"Write requests to inventory_quantity and inventory_quantity_adjustment are no longer supported. Please use the Inventory Levels API."}", headers={'x-shopify-stage': 'production', 'x-stats-userid': '', 'cf-cache-status': 'DYNAMIC', 'x-shopify-shop-api-call-limit': '1/80', 'x-shopid': '25163989043', 'http_x_shopify_shop_api_call_limit': '1/80', 'x-request-id': 'e3878b1e-6905-4bab-9a43-139e44cce883', 'x-xss-protection': '1; mode=block; report=/xss-report?source%5Baction%5D=update&source%5Bapp%5D=Shopify&source%5Bcontroller%5D=admin%2Fproducts&source%5Bsection%5D=admin_api&source%5Buuid%5D=e3878b1e-6905-4bab-9a43-139e44cce883', 'x-content-type-options': 'nosniff', 'x-stats-apiclientid': '3250443', 'x-shardid': '50', 'transfer-encoding': 'chunked', 'referrer-policy': 'origin-when-cross-origin', 'x-sorting-hat-shopid': '25163989043', 'x-download-options': 'noopen', 'x-stats-apipermissionid': '198562512947', 'set-cookie': '__cfduid=d3f64e40387b1a49f6442ae64e2cb1e2f1584723654; expires=Sun, 19-Apr-20 17:00:54 GMT; path=/; domain=.myshopify.com; HttpOnly; SameSite=Lax', 'x-shopify-api-terms': 'By accessing or using the Shopify API you agree to the Shopify API License and Terms of Use at https://www.shopify.com/legal/api-terms', 'x-shopify-api-version': '2019-10', 'date': 'Fri, 20 Mar 2020 17:00:55 GMT', 'cf-ray': '5770fdfb4900eff5-EWR', 'expect-ct': 'max-age=604800, report-uri="https://report-uri.cloudflare.com/cdn-cgi/beacon/expect-ct"', 'alt-svc': 'h3-27=":443"; ma=86400, h3-25=":443"; ma=86400, h3-24=":443"; ma=86400, h3-23=":443"; ma=86400', 'content-security-policy': "default-src 'self' data: blob: 'unsafe-inline' 'unsafe-eval' https://* shopify-pos://; block-all-mixed-content; child-src 'self' https:// shopify-pos://; connect-src 'self' wss:// https://*; frame-ancestors 'none'; img-src 'self' data: blob: https:; script-src https://cdn.shopify.com https://cdn.shopify.cn https://checkout.shopifycs.com https://js-agent.newrelic.com https://bam.nr-data.net https://api.stripe.com https://mpsnare.iesnare.com https://appcenter.intuit.com https://www.paypal.com https://js.braintreegateway.com https://c.paypal.com https://maps.googleapis.com https://www.google-analytics.com https://v.shopify.com https://widget.intercom.io https://js.intercomcdn.com 'self' 'unsafe-inline' 'unsafe-eval'; upgrade-insecure-requests; report-uri /csp-report?source%5Baction%5D=update&source%5Bapp%5D=Shopify&source%5Bcontroller%5D=admin%2Fproducts&source%5Bsection%5D=admin_api&source%5Buuid%5D=e3878b1e-6905-4bab-9a43-139e44cce883", 'x-dc': 'gcp-us-central1,gcp-us-east1,gcp-us-east1', 'strict-transport-security': 'max-age=7889238', 'x-sorting-hat-podid': '50', 'server': 'cloudflare', 'connection': 'close', 'x-permitted-cross-domain-policies': 'none', 'x-frame-options': 'DENY', 'content-type': 'application/json; charset=utf-8'}, msg="Bad Request")

djhoughton · 2020-03-20T17:27:11Z

The deprecated parameters are still being sent in the request you pasted above:

        "inventory_quantity": 49,
        "old_inventory_quantity": 49,

spadae22 · 2020-03-20T18:27:22Z

Correct, I thought the update would remove or ignore them, I GET for same same product, these fields are included. I thought the point of the update was to fix the Bad Request issue. So I still have to strip them out?

spadae22 · 2020-03-20T18:31:46Z

To be clear this PR makes 2 changes if a client is using the 2019-10 API version or later:
1). If the client tries to set a deprecated inventory-related field on Product or Variant, we now throw an exception in the gem rather than going to the server to get the failed request
2). We remove the deprecated fields from being serialized in case clients try to send them back to the server without removing them.

I am not setting inventory 1., and 2. states the fields would be removed ?

djhoughton · 2020-03-20T18:51:00Z

Do you have some sample code for what you're doing? Also, which API version are you using?

spadae22 · 2020-03-20T18:56:34Z

I set the version 2019-10, as base url. I am just trying to unpublish a product from our ERP. Here is the code, it was working until Mach 16.

@api.multi
def shopify_unpublished(self):
instance = self.shopify_instance_id
instance.connect_in_shopify()
if self.shopify_tmpl_id:
new_product = shopify.Product.find(self.shopify_tmpl_id)
if new_product:
new_product.id = self.shopify_tmpl_id
new_product.published = 'false'
new_product.published_at = None
result = new_product.save()
if result:
result_dict = new_product.to_dict()
updated_at = result_dict.get('updated_at')
published_at = result_dict.get('published_at')
self.write({'updated_at': updated_at, 'published_at': False, 'website_published': False})
return True`

spadae22 · 2020-03-20T19:21:24Z

The real issue I have is I did refactor code for 2019-10, months ago, what changes occured on the March 16th, that broke this. I have many functions that I would have to update, we update stock using the correct method without issue. I feeling is the GET is pulling all fields, fine. But if I tried to set inventory the old way, then yes I should get a bad request, not when I am updating two fields. So if I change the 'title' it is going to throw the same Bad Request. These fields should be ignored by the server since they are not used.

spadae22 · 2020-03-20T19:24:33Z

This is the code we pass to update inventory, the fields,

"inventory_quantity": 49,
"old_inventory_quantity": 49,
are not being used>

quantity = product_obj.get_stock_ept(variant.product_id, location_id.warehouse_id.id,variant.fix_stock_type,variant.fix_stock_value, instance.stock_field.name) try: shopify.InventoryLevel.set(location_id.shopify_location_id, variant.inventory_item_id, int(quantity))

spadae22 · 2020-03-20T19:52:21Z

Same issue as here:#702

djhoughton · 2020-03-20T20:44:40Z

We're going to revert the changes introduced by this PR and work on a new fix for the issue.
Thanks for the comments and prompt feedback!

marschattha · 2020-04-02T11:28:30Z

lib/shopify_api/resources/variant.rb

+    end
+
+    def serializable_hash(options = {})
+      allow_inventory_params? ? super(options) : super(options).except('inventory_quantity', 'old_inventory_quantity')


Changing to

def serializable_hash(options = {}) allow_inventory_params? ? super(options) : super(options).tap do |s| (s['variant'] || s).except!('inventory_quantity', 'old_inventory_quantity') end end

Fixes this PR.

Larochelle · 2020-04-13T19:12:51Z

lib/shopify_api/resources/variant.rb

+    end
+
+    def serializable_hash(options = {})
+      allow_inventory_params? ? super(options) : super(options).except('inventory_quantity', 'old_inventory_quantity')


Is this accurate? Those fields were deprecated for writes, but reading variant.total_inventory should still be supported

djhoughton self-assigned this Dec 20, 2019

djhoughton force-pushed the inventory_param_deprecation branch from b178725 to 14a697c Compare January 14, 2020 14:34

djhoughton changed the title ~~[WIP] Add validation and tests for deprecation of product and variant inventory-related fields~~ Add validation and tests for deprecation of product and variant inventory-related fields Jan 14, 2020

djhoughton marked this pull request as ready for review January 14, 2020 14:38

djhoughton requested a review from a team as a code owner January 14, 2020 14:38

tanema approved these changes Jan 14, 2020

View reviewed changes

swalkinshaw reviewed Jan 14, 2020

View reviewed changes

swalkinshaw approved these changes Jan 14, 2020

View reviewed changes

djhoughton force-pushed the inventory_param_deprecation branch from 603f717 to cea7bb1 Compare January 14, 2020 22:07

tanema mentioned this pull request Mar 18, 2020

Edit product raise error: Bad Request (Write requests to inventory_quantity and inventory_quantity_adjustment are no longer supported. Please use the Inventory Levels API.) #702

Closed

jonathankwok approved these changes Mar 18, 2020

View reviewed changes

djhoughton force-pushed the inventory_param_deprecation branch from cea7bb1 to 0ed662d Compare March 18, 2020 21:22

jonathankwok self-requested a review March 18, 2020 21:47

Do not allow deprecated product and variant inventory-related fields …

4235a6f

…in 2019-10 and later

djhoughton force-pushed the inventory_param_deprecation branch from 82843fa to 4235a6f Compare March 19, 2020 00:10

swalkinshaw approved these changes Mar 19, 2020

View reviewed changes

djhoughton merged commit 31f01b1 into master Mar 19, 2020

djhoughton mentioned this pull request Mar 19, 2020

Package for release 9.0.3 #705

Merged

djhoughton requested a deployment to rubygems March 19, 2020 20:32 In progress

djhoughton requested a deployment to rubygems March 19, 2020 20:32 Abandoned

djhoughton had a problem deploying to rubygems March 19, 2020 20:32 Failure

djhoughton requested a deployment to rubygems March 19, 2020 20:45 In progress

djhoughton requested a deployment to rubygems March 19, 2020 20:45 Abandoned

djhoughton had a problem deploying to rubygems March 19, 2020 20:45 Failure

tanema requested a deployment to rubygems March 19, 2020 21:41 In progress

tanema had a problem deploying to rubygems March 19, 2020 21:41 Failure

tanema requested a deployment to rubygems March 19, 2020 21:42 In progress

tanema requested a deployment to rubygems March 19, 2020 21:42 Abandoned

tanema had a problem deploying to rubygems March 19, 2020 21:42 Failure

tanema requested a deployment to rubygems March 20, 2020 16:40 In progress

tanema had a problem deploying to rubygems March 20, 2020 16:40 Failure

tanema requested a deployment to rubygems March 20, 2020 16:50 In progress

tanema requested a deployment to rubygems March 20, 2020 16:50 Abandoned

tanema temporarily deployed to rubygems March 20, 2020 16:50 Inactive

djhoughton mentioned this pull request Mar 20, 2020

Revert "Do not allow deprecated product and variant inventory-related fields in 2019-10 and later" #708

Merged

marschattha reviewed Apr 2, 2020

View reviewed changes

Larochelle reviewed Apr 13, 2020

View reviewed changes

marschattha mentioned this pull request May 14, 2020

Inventory param patch fix #731

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add validation and tests for deprecation of product and variant inventory-related fields #655

Add validation and tests for deprecation of product and variant inventory-related fields #655

djhoughton commented Dec 20, 2019 •

edited by tanema

Loading

swalkinshaw Jan 14, 2020

djhoughton Jan 14, 2020

jonathankwok Mar 18, 2020

swalkinshaw Jan 14, 2020

spadae22 commented Mar 17, 2020

djhoughton commented Mar 18, 2020

Saibotx commented Mar 18, 2020

dnswus commented Mar 18, 2020

EmmaB commented Mar 18, 2020

djhoughton commented Mar 18, 2020

slimdave commented Mar 18, 2020

djhoughton commented Mar 18, 2020

djhoughton commented Mar 19, 2020

spadae22 commented Mar 19, 2020

spadae22 commented Mar 20, 2020 •

edited

Loading

djhoughton commented Mar 20, 2020

spadae22 commented Mar 20, 2020

spadae22 commented Mar 20, 2020

djhoughton commented Mar 20, 2020

spadae22 commented Mar 20, 2020 •

edited

Loading

spadae22 commented Mar 20, 2020

spadae22 commented Mar 20, 2020

spadae22 commented Mar 20, 2020

djhoughton commented Mar 20, 2020

marschattha Apr 2, 2020 •

edited

Loading

spadae22 Apr 3, 2020

Larochelle Apr 13, 2020

Add validation and tests for deprecation of product and variant inventory-related fields #655

Add validation and tests for deprecation of product and variant inventory-related fields #655

Conversation

djhoughton commented Dec 20, 2019 • edited by tanema Loading

swalkinshaw Jan 14, 2020

Choose a reason for hiding this comment

djhoughton Jan 14, 2020

Choose a reason for hiding this comment

jonathankwok Mar 18, 2020

Choose a reason for hiding this comment

swalkinshaw Jan 14, 2020

Choose a reason for hiding this comment

spadae22 commented Mar 17, 2020

djhoughton commented Mar 18, 2020

Saibotx commented Mar 18, 2020

dnswus commented Mar 18, 2020

EmmaB commented Mar 18, 2020

djhoughton commented Mar 18, 2020

slimdave commented Mar 18, 2020

djhoughton commented Mar 18, 2020

djhoughton commented Mar 19, 2020

spadae22 commented Mar 19, 2020

spadae22 commented Mar 20, 2020 • edited Loading

djhoughton commented Mar 20, 2020

spadae22 commented Mar 20, 2020

spadae22 commented Mar 20, 2020

djhoughton commented Mar 20, 2020

spadae22 commented Mar 20, 2020 • edited Loading

spadae22 commented Mar 20, 2020

spadae22 commented Mar 20, 2020

spadae22 commented Mar 20, 2020

djhoughton commented Mar 20, 2020

marschattha Apr 2, 2020 • edited Loading

Choose a reason for hiding this comment

spadae22 Apr 3, 2020

Choose a reason for hiding this comment

Larochelle Apr 13, 2020

Choose a reason for hiding this comment

djhoughton commented Dec 20, 2019 •

edited by tanema

Loading

spadae22 commented Mar 20, 2020 •

edited

Loading

spadae22 commented Mar 20, 2020 •

edited

Loading

marschattha Apr 2, 2020 •

edited

Loading