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

(fix): Determine denormalized product inventory status during Shopify import #3366

Merged

Conversation

spencern
Copy link
Contributor

@spencern spencern commented Dec 4, 2017

Fixes an unreported issue where denormalized product inventory status was not set during import of products from Shopify.

This PR resolves this by setting isSoldOut, isLowQuantity, and isBackorder flags correctly on the top level product during import.

@impactmass
Copy link
Contributor

impactmass commented Dec 5, 2017

Looks good generally, but I have some questions:

  • Looks like there's no equivalent isLowQuantity value from the Shopify data?
  • How does this change affect anyone that already used the current version to import products?

And for testing:

  • I just imported products and checked that the new values are set on the top level product.

@impactmass
Copy link
Contributor

@spencern looks like you missed the comment above

@spencern
Copy link
Contributor Author

spencern commented Dec 11, 2017

@impactmass
arg. I thought I responded to this already, but probably have a draft mode somewhere.

isLowQuantity does not have an equivalent on Shopify, so we just set it to false.

Re: this change affecting anyone that already used the current version to import products, that's a great question. I'm pretty sure that top-level field gets updated every time inventory gets changed, but it might be worth including a migration here that loops through all products and updates them anyway? I could write the migration to check for a shopifyId first and then to check for the presence of one of these fields isLowQuantity etc that would be absent otherwise so that it only happens on Shopify products that have not been updated

@impactmass
Copy link
Contributor

I think if we can confirm that the top-level field gets updated every time inventory gets changed, this should be good to approve then.

… the grid

This is how it currently works in Reaction when creating a product and I think was the intention with the grid.
@spencern
Copy link
Contributor Author

@impactmass Those do get updated as part of this denormalize function that gets called when variants are updated. See catalog.js https://github.com/reactioncommerce/reaction/blob/master/server/methods/catalog.js#L188

It's then called on L564 by updateVariant, and on L965 by updateProductField.

@spencern
Copy link
Contributor Author

I've added one more update in here to fix the way variant images are imported. To this point only the first image imported was added to the grid via toGrid: 1 but after experimenting with how Reaction assigns product media to the grid when creating a product and variants, I've updated the Shopify importer to now add the first image for each variant to the grid as well. This should resolve the situation we've seen in #3368 where grid item layouts do not show additional product media that has been imported from Shopify.

It will not affect existing images from Shopify that have been imported.

I'm not certain if we want to add a migration here, as it's possible that people have built their apps to account for the way that the media was imported originally, but a migration could be added to set the first image of each variant to have toGrid: 1

I lean towards this being a fix and not adding a migration, but could understand an argument either way. @impactmass what do you think?

@spencern
Copy link
Contributor Author

@impactmass ready for another review here.

@impactmass
Copy link
Contributor

Yes, doing a migration in this case can cause a break in the way someone else already built on earlier imports. So, I tend towards the way it's setup here.

@spencern spencern changed the base branch from master to release-1.7.0 December 13, 2017 21:32
@spencern spencern merged commit 792ca09 into release-1.7.0 Dec 13, 2017
@spencern spencern deleted the fix-spencer-shopify-import-denormalized-inventory branch December 13, 2017 21:33
This was referenced Dec 14, 2017
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.

None yet

2 participants