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

Inventory Rewrite - new simple-inventory plugin #5164

Merged
merged 58 commits into from
May 13, 2019

Conversation

aldeed
Copy link
Contributor

@aldeed aldeed commented May 3, 2019

Resolves #4215
Resolves #4931
Resolves #4932
Resolves #5107
Resolves #5123
Impact: breaking
Type: feature|performance

Changes

Overview

  • Inventory code that was mixed up with Catalog and Product code is now properly isolated in inventory plugins.
  • The "Inventory" collection is no longer created or available on context.collections. It was no longer used. It is not dropped, though, in case anyone is using it.
  • Previously some inventory information was stored in the Catalog collection, on the variants and options objects. Now all of these fields are removed from the database except three top-level product bools that must be in the collection data for sorting and filtering purposes. This doesn't affect GraphQL, though, because those fields are still set at read time.
  • The whole idea of auto-publishing inventory is mostly gone due to the above change, so the AUTO_PUBLISH_INVENTORY_FIELDS environment variable no longer affects anything.
  • buildOrderItem, part of the process of placing an order, is updated to query inventory using the new API
  • In the operator UI, the product table column for inventory flags is removed, and the variant column table for quantity is removed. These could be added back in the future if they are necessary, but it was extra complexity for the purposes of this initial rewrite.

Core Inventory Plugin

There was already a core inventory plugin, but much of the inventory code was still scattered in other plugins. Now all inventory code is in the core inventory plugin, and plugins do not import or reference inventory code outside of the standard ways (context).

Here's what the core inventory plugin now does:

  • Sets product.isSoldOut, product.isLowQuantity, and product.isBackorder booleans on a CatalogItem record when it is published. Then keeps these values correct whenever inventory information changes for a variant. We must store these fields in the DB rather than adding them at read time because we allow sorting and/or filtering by them.
  • Adds inventory-related fields to CatalogProductVariant and CartItem at read time by registering transform functions that the catalog plugin calls.
  • Adds context.queries.inventoryForProductConfigurations, and a convenience function for a single configuration, context.queries.inventoryForProductConfiguration, as the standard way for other plugins to retrieve inventory info.
    • These functions delegate to one or more other inventory plugins, such as the included simple-inventory plugin, for the actual data.
  • Registers isSoldOut, isLowQuantity, and isBackorder as supported boolean filters for the catalogItems query.

Simple Inventory Plugin

As mentioned above, the core inventory plugin requires one or more inventory plugins to do the actual inventory tracking for product configurations (variants).

In general, an inventory plugin needs to do the following:

  • Register an inventoryForProductConfigurations type function that returns current inventory in stock and other related data for a list of product configurations.
  • Optionally provide a way of editing the inventory in stock in the operator UI, by registering client components
  • Optionally track order status changes to keep track of "reserved" inventory, which is inventory that is still technically in stock but should not be considered available to sell.

A new included simple-inventory plugin does all of the above in a manner similar to the way inventory worked before this PR. The UI has been improved a bit but is still in the same place. One major difference is that the actual inventory data is now stored in a new SimpleInventory collection rather than on Products, which makes it much easier to read and write. A migration populates SimpleInventory collection from data currently on Products, so the change should be transparent.

One UI improvement is regarding the "reserved" quantity for a variant. The UI now shows in stock, reserved, and available numbers, and there is a button that allows you to force the "reserved" number to update if it becomes incorrect. This plugin considers inventory to be reserved when it's part of an order that has been placed but is not yet approved.

How to sync inventory quantities from an external system

Option 1 If your external inventory system can be queried in real time, the simplest integration is to remove the simple-inventory plugin and add your own custom inventory plugin that registers an inventoryForProductConfigurations type function that returns current inventory in stock and other related data for a list of product configurations by calling through to your inventory management system.

Option 2 If your external inventory system produces periodic exports but cannot be queried in real time, then you may want to keep the included simple-inventory plugin and create a script that reads the exported files and updates the SimpleInventory collection as necessary. If you do this in a custom plugin, then it's best to call context.mutations.updateSimpleInventory with context.isInternalCall set to true, and let that take care of properly updating the collection. If you can't do it in a custom plugin, then your external script should be written to look similar to the updateSimpleInventory mutation function in the simple-inventory plugin.

Breaking changes

The GraphQL API is mostly unchanged. Inventory fields for CatalogProductVariant and CartItem are as they were. But internally, everything is different. So custom plugins that manage or use inventory are likely to need a rewrite.

The example web storefront will break unless you have the changes in reactioncommerce/example-storefront#527.

Testing

This change touches a lot of areas, and although most have decent test coverage, we should carefully test everything that might be affected.

  • Make sure there are no errors logged when migration 63 runs. Make sure inventory data looks the same on sellable variants before and after pulling this code and running the migration.
  • Verify you can publish a product to the catalog with no errors
  • Test all fields on the Inventory card of a sellable variant's edit page
  • Manually set the inventoryReserved number to an incorrect number in the SimpleInventory collection and verify that it is fixed when clicking the "Recalculate reserved quantity" button
  • On the storefront product grid, verify that the badge is correct.
    • Find a product and adjust its variant quantities such that you would expect "Sold Out" to appear for the product. Without needing to publish, you should be able to refresh the storefront page and see the "Sold Out" badge.
    • Do the same for "Low Quantity"
    • Do the same for "Backordered"
    • Do the same for no badge.
  • With inventory management turned OFF for a variant, verify that you can add any quantity of that variant to your cart, and successfully place the order.
  • With inventory management turned ON for a variant and "Allow backorder" turned OFF, verify that you cannot add more than the available quantity to your cart. There may be some edge case ways to add them to your cart but verify that placing the order fails.
  • With inventory management turned ON for a variant and "Allow backorder" turned ON, verify that you can add more than the available quantity to your cart, and placing the order succeeds.
  • Verify that filtering out sold out products when doing the catalogItems GraphQL query works.
  • Reserved quantity testing:
    • After you place an order for a variant, if you look at the Inventory card for that variant in the operator UI, you should see the ordered quantity listed as reserved.
    • If you approve the fulfillment group, the reserved quantity should decrease and the in stock quantity should decrease.
    • If you cancel an item before approving, the reserved quantity should decrease and the in stock quantity should stay the same.
    • If you cancel an item after approving and choose to "return to stock", the reserved quantity should stay the same and the in stock quantity should increase.
    • If you cancel an item after approving and choose NOT to "return to stock", the reserved quantity should stay the same and the in stock quantity should stay the same.

Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
to inventory core plugin

Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
to inventory plugin

Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
…ory plugin

Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
when ordering

Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
and related server changes

Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
@aldeed aldeed self-assigned this May 3, 2019
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
aldeed added 5 commits May 2, 2019 22:08
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
@aldeed
Copy link
Contributor Author

aldeed commented May 10, 2019

Remaining code comments have been addressed

@kkuzemka
Copy link

@kieckhafer - can you take another look at this in hopes that we can try and get it merged and over to SDI today?

Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
@kieckhafer
Copy link
Member

@kkuzemka will re-test first thing this morning.

Copy link
Member

@kieckhafer kieckhafer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting this error inside migration 18. Ran it a few times and confirmed it happens every time. Migrations stop at 18 because of this.

Error while migrating TypeError: doc.layout is not iterable
      at imports/plugins/core/versions/server/migrations/18_use_react_for_header_and_footer_layout.js:42:5
      at Array.forEach (<anonymous>)
      at Object.up (imports/plugins/core/versions/server/migrations/18_use_react_for_header_and_footer_layout.js:28:5)
      at migrate (packages/percolate_migrations.js:233:25)
      at Object.Migrations._migrateTo (packages/percolate_migrations.js:253:7)
      at Object.Migrations.migrateTo (packages/percolate_migrations.js:167:10)
      at appEvents.on (imports/plugins/core/versions/server/startup.js:44:9)
      at Promise.asyncApply (imports/node-app/core/util/appEvents.js:24:11)
      at /home/node/.meteor/packages/promise/.0.11.2.1gwulfb.vjny++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/meteor-promise/fiber_pool.js:43:40

When I ran from an existing install, which was already up to version 62, migration 63 ran OK.

Copy link
Member

@kieckhafer kieckhafer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another migration error... If i manually set Migrations to 19, to allow them to continue to run, migration 22 has an issue:

Error while migrating TypeError: Cannot read property 'registry' of undefined
      at Object.up (imports/plugins/core/versions/server/migrations/22_register_verify_account.js:9:5)
      at migrate (packages/percolate_migrations.js:233:25)
      at Object.Migrations._migrateTo (packages/percolate_migrations.js:253:7)
      at Object.Migrations.migrateTo (packages/percolate_migrations.js:167:10)
      at appEvents.on (imports/plugins/core/versions/server/startup.js:44:9)
      at Promise.asyncApply (imports/node-app/core/util/appEvents.js:24:11)

Copy link
Member

@kieckhafer kieckhafer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside form those two migrations, after I get them all to run (by manually pushing it past the broken ones in the database), I get missing index errors on a lot of indexes. Here is one example:

 MongoError: index not found with name [c2_billing.$.paymentMethod.items.$.productId]
      at /home/node/.meteor/packages/npm-mongo/.3.1.1.v3rpzk.m5kk8++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/mongodb-core/lib/connection/pool.js:581:63
      at authenticateStragglers (/home/node/.meteor/packages/npm-mongo/.3.1.1.v3rpzk.m5kk8++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/mongodb-core/lib/connection/pool.js:504:16)
      at Connection.messageHandler (/home/node/.meteor/packages/npm-mongo/.3.1.1.v3rpzk.m5kk8++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/mongodb-core/lib/connection/pool.js:540:5)
      at emitMessageHandler (/home/node/.meteor/packages/npm-mongo/.3.1.1.v3rpzk.m5kk8++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/mongodb-core/lib/connection/connection.js:310:10)
      at Socket.<anonymous> (/home/node/.meteor/packages/npm-mongo/.3.1.1.v3rpzk.m5kk8++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/mongodb-core/lib/connection/connection.js:453:17)
      at emitOne (events.js:116:13)
      at Socket.emit (events.js:211:7)
      at addChunk (_stream_readable.js:263:12)
      at readableAddChunk (_stream_readable.js:250:11)
      at Socket.Readable.push (_stream_readable.js:208:10)
      at TCP.onread (net.js:597:20)

I get over 70 errors for missing indexes

Copy link
Member

@kieckhafer kieckhafer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get a white screen, with the following error when trying to go to localhost:3000:

TypeError: Cannot read property 'sort' of undefined
    at ReactionLayout (router.js:436)
    at Function.Router.initPackageRoutes (router.js:548)
    at initBrowserRouter (browserRouter.js:167)
    at startup.js:40
    at Computation._compute (tracker.js:308)
    at Computation._recompute (tracker.js:324)
    at Object.Tracker._runFlush (tracker.js:495)
    at onGlobalMessage (meteor.js?hash=33066830ab46d87e2b249d2780805545e40ce9ba:504)

That line is

const sortedLayout = shop.layout.sort((prev, next) => prev.priority - next.priority);

so considering the migrations that were broken had to do with layout, my guess is this is related.

aldeed and others added 2 commits May 10, 2019 12:48
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Copy link
Member

@kieckhafer kieckhafer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After you place an order for a variant, if you look at the Inventory card for that variant in the operator UI, you should see the ordered quantity listed as reserved.
If you approve the fulfillment group, the reserved quantity should decrease and the in stock quantity should decrease.

When I approve a fulfillment group, the reserved quantity decreased, but the in-stock quantity stayed the same.

If you cancel an item before approving, the reserved quantity should decrease and the in stock quantity should stay the same.

This is happening, but it's happening whether I click return to stock or not... should this be the case? Should it only happen if I return to stock?

If you cancel an item after approving and choose to "return to stock", the reserved quantity should stay the same and the in stock quantity should increase.

I cannot test this, as mentioned in the first point, the quantity did not decrease when I approved.

If you cancel an item after approving and choose NOT to "return to stock", the reserved quantity should stay the same and the in stock quantity should stay the same.

I cannot test this, as mentioned in the first point, the quantity did not decrease when I approved.

aldeed and others added 6 commits May 10, 2019 16:35
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
@kieckhafer kieckhafer dismissed their stale review May 10, 2019 23:19

Fixed existing issues

@kieckhafer
Copy link
Member

@aldeed All above mentioned issues related to the Orders UI seem to be fixed. Still seeing the issues we discussed with startup and packages loading.

Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
@aldeed
Copy link
Contributor Author

aldeed commented May 13, 2019

I fixed the issue with Package registry being blank on first run with blank DB. Migration 3 was clearing it so I commented out migration 3. 😅 I doubt anyone is upgrading from that far back, and it isn’t needed for a fresh install, so it seems like a safe way to fix.

Copy link
Member

@kieckhafer kieckhafer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 With the startup changes, I think this is ready to go.

@kieckhafer kieckhafer merged commit 74f8945 into develop May 13, 2019
@kieckhafer kieckhafer deleted the feat-aldeed-isolate-inventory branch May 13, 2019 18:54
@jeffcorpuz jeffcorpuz mentioned this pull request Jul 2, 2019
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.

4 participants