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

refactor: Update `inventoryQuantity` field to be `inventoryInStock` #4930

Merged
merged 6 commits into from Jan 23, 2019

Conversation

2 participants
@kieckhafer
Copy link
Member

commented Jan 22, 2019

Impact: breaking|minor
Type: refactor

Issue

A recent update to how we calculate inventory added a new field, inventoryAvailalbeToSell. This left us with two inventory fields, inventoryAvailalbeToSell and inventoryQuantity, in the Products collection.

In the Catalog collection, we renamed inventoryQuantity to inventoryInStock, as it was a better description of what the field actually is.

We initially left the Products collection alone, but feel like we should keep these field names in sync to avoid confusion.

Solution

Rename the field inventoryQuantity to inventoryInStock in the Products collection.

Breaking changes

No breaking changes in Reaction code. If you use the inventoryQuantity field in any custom code, it will need to be updated.

Testing

  1. Create orders with various amounts of products, and see that inventory correctly increments / decrements
  2. See that Sold Out, Low Inventory badges work as they were

@kieckhafer kieckhafer requested a review from aldeed Jan 22, 2019

@@ -108,7 +108,7 @@ export async function xformProduct({ collections, product, shop, variants }) {
variantInventory = {
canBackorder: canBackorder(variantOptions),
inventoryAvailableToSell: variant.inventoryAvailableToSell || 0,
inventoryInStock: variant.inventoryQuantity || 0,
inventoryInStock: variant.inventoryInStock || 0,

This comment has been minimized.

Copy link
@aldeed

aldeed Jan 22, 2019

Member

This may not be the time to correct this, but inventoryInStock and inventoryAvailableToSell are optional in the catalog GraphQL schema, and it seems to me that null would mean something different from 0. If inventory isn't tracked in the product (null) but yet that becomes 0 in the catalog, that seems to be changing the meaning from "unknown" to "none".

So essentially I'm proposing that we remove the || 0 from all of these

Show resolved Hide resolved ...ons/server/migrations/56_change_inventoryQuantity_to_inventoryInStock.js

kieckhafer added some commits Jan 22, 2019

Update `inventoryQuantity` field to be `inventoryInStock`
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Add migration for inventoryQuantity to inventoryInStock update
Signed-off-by: Erik Kieckhafer <ek@ato.la>
refactor: remove double call to products DB
Signed-off-by: Erik Kieckhafer <ek@ato.la>
fix: missing bracket
Signed-off-by: Erik Kieckhafer <ek@ato.la>
refactor: remove || 0 to allow `null` as a value for inventory
Signed-off-by: Erik Kieckhafer <ek@ato.la>
refactor: reverse commit removing || 0
Signed-off-by: Erik Kieckhafer <ek@ato.la>

@kieckhafer kieckhafer force-pushed the refactor-kieckhafer-renameInventoryQuantity branch from bc1e3fd to e039447 Jan 22, 2019

@aldeed

aldeed approved these changes Jan 22, 2019

@kieckhafer

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2019

@aldeed can this be merged?

@aldeed aldeed merged commit 831c381 into develop Jan 23, 2019

3 checks passed

License Compliance All checks passed.
Details
WIP ready for review
Details
security/snyk - package.json (Reaction Commerce) No new issues
Details

@aldeed aldeed deleted the refactor-kieckhafer-renameInventoryQuantity branch Jan 23, 2019

@jeffcorpuz jeffcorpuz referenced this pull request Mar 1, 2019

Merged

Release v2.0.0 rc.10 #5016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.