-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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: updates to inventory counts and statuses #4859
Conversation
@aldeed all comments addressed, this is ready for another look
// we can map over the unique productIds at the end, and update each one once | ||
orderItems.forEach(async (item) => { | ||
// Update supplied item inventory | ||
const updatedItem = await collections.Products.findOneAndUpdate( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you set the returnOriginal
option to false
, findOneAndUpdate
default behavior is to return the original, not the updated. In this case it doesn't cause an issue, but updatedItem
is an inaccurate variable name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the return is technically an object where value
is the document, either original or updated. So it would be better to do
const { value: originalProduct } =
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment for the other similar calls in this PR, but it doesn't seem to be anything more than a variable name issue in all places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't really matter, but do you think it might be best to have the updated document for use in the future, to expand on this, if needed: 09e2b4c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that's best, but the option is called returnOriginal
and needs to be set to false
: http://mongodb.github.io/node-mongodb-native/3.1/api/Collection.html#findOneAndUpdate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it...updated. Was running off of the MongoDB Shell docs: https://docs.mongodb.com/manual/reference/method/db.collection.findOneAndUpdate/
Resolves #4337
Impact: minor
Type: refactor
Issue
Some inventory statuses are confusing and cause issues when trying to easily determine a current status. Inventory also does not correctly update on variants, depending on if a variant has options or not.
Solution
This PR helps to clarify some statuses by adding new statuses, and defining older statuses better. It also fixes the issues concerning out dated inventory numbers on variants that have option children.
Add
canBackorder
boolean
. canBackorder represents the ability to backorder products:inventoryPolicy === false && inventoryManagement === true
. Added to Variants and Options in the Catalog Schema and the Product Schema.Better define (via comments)
isBackorder
as being the current state of backorder, not the ability to backorder (which iscanBackorder
). PreviouslyinventoryPolicy === false && inventoryManagement === true && currentQuantity === 0
, nowcanBackorder === true && inventoryAvailableToSell === 0
Depreciate
currentQuantity
from cart, and addinventoryAvailableToSell
andinventoryInStock
Add
inventoryAvailableToSell
inventory count to determine how many products are available for sale, i.e. not reserved.inventoryAvailableToSell
is updated when a customer completes their order. This field is added on Options, Variants, and Product levels, in both the Catalog and the Products collections. If avariant
orproduct
, this is not an input number, but rather a number calculated based of the summed up total of its children's inventory.Add
inventoryInStock
in the Catalog collection. This is an existing field in the Products collection, calledinventoryQuantity
. This is updated when an operator processes an order.Add calculations that automatically update the
inventoryAvailableToSell
andinventoryInStock
numbers on Products and Variants, based on the summed inventory count of it's children. These functions areupdateParentVariantsInventoryAvailableToSellQuantity
andupdateParentVariantsInventoryInStockQuantity
. For example,Variant A
has childrenOption A
andOption B
. In the past, we would stop updating inventory onVariant A
, and in the UI show a client side calculation of the sum of inventory ofOption A
andOption B
. This update calculates the sum ofOption A
andOption B
whenever they are updated, and saves the number as an actual inventory number onVariant A
in the database. This way we can trust the number we are provided by the data, instead of doing a client side calculation at runtime.Breaking changes
inventoryAvailableToSell
to all products / variants, to correctly calculate the numbers on parent products / variants, and to publish this data to already published Catalog items.currentQuantity
has been marked withdepreciated
in the cart. This isn't a breaking change at the moment, but lays the path to remove this field and replace withinventoryAvailableToSell
andinventoryInStock
in the future.Catalog.getVariantQuantity
andReactionProduct.getVariantQuantity
have been removed. Custom plugins using these methods will need to be updated. The same data returned by these methods is now on the object that was being passed into these methods as the fieldinventoryQuantity
orinventoryAvailableToSell
isBackorder
,isLowQuantity
, andisSoldOut
functions from thecatalog
plugin to the newinventory
plugin. Custom plugins using these methods will need to update their import path.Testing
canBackorder
,inventoryAvailableToSell
andinventoryInStock
are present on products, variants, and options on ourcatalogItemProduct
query.You can use this GQL query to only see the relevent info and get rid of the noise in GraphiQL:
Add various combinations of products with only variants, only options, etc, and see that the
inventoryAvailableToSell
is correctly decremented when an order is placed.Process the order, and see that the same / correct updates are made to
inventoryInStock
.** Inventory numbers will be different between the time an order is placed, and it is processed. They should be the same if there are no current orders waiting to be processed.
When an order is cancelled BEFORE it's approved, only the
inventoryAvailableToSell
number should be adjusted.When an order is cancelled AFTER it's been approved, both inventory counts should be adjusted, IF the user chooses "restock" on the cancellation.
As a customer, add
x
products to your cart and complete checkout. Go to PDP as admin, Adjust inventory via the operator UI, and see that the both inventory numbers adjust appropriately on all options, variants, and top level products.Hook up to the Cart endpoint via GraphiQL and see that
inventoryAvailableToSell
andinventoryInStock
have been added.Migration 51 has two parts: the first adds the new inventory field to the
Products
collection product, and the second publishes those changes to published catalog products. If you are running Reaction from scratch, this second part will not do anything on the first run, since the Basic Reaction Product isn't published to the catalog by default. The best way to test the second part is to run the app from scratch, go to the PDP and publish the product, then go into Mongo and deleteinventoryAvailableToSell
andinventoryInStock
fields from all products / variants / options inside theCatalog
collection. Then, inside theMigrations
collection, change the migration version to50
. Then, restart the core docker container.