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: hooks in the inventory package to use Hooks.Events #3887

Merged

Conversation

5 participants
@kieckhafer
Copy link
Member

kieckhafer commented Mar 2, 2018

Resolves #3865
Impact: breaking
Type: refactor

Issue

Hooks inside the inventory package should be updated to use Hooks.Events

Solution

Convert old hooks (X.after.update, X.before.update...) into Hooks.Events for easier visibility as to what is going on when actions are taken.

  • Products.after.insert is now Hooks.Events.add("afterInsertCatalogProduct")
  • Products.after.remove is now Hooks.Events.add("afterRemoveCatalogProduct")
  • Products.after.update is now Hooks.Events.add("afterUpdateCatalogProduct")
  • Orders.after.insert is now Hooks.Events.add("afterOrderInsert")
  • Orders.after.update is now Hooks.Events.add("onOrderShipmentShipped")
  • Added Hooks.Events.add("afterAddItemsToCart")
  • Added Hooks.Events.add("afterModifyQuantityInCart")

Breaking changes

There are no longer any "magical" background hooks to update inventory on collection insert, remove, or update. All updates are now made via Hooks.Events.

Testing

  1. Archive / Delete a variant from a product
  2. See that product inventory updates in relation to the removed variant

  1. Add a new product or variant
  2. See that product inventory updates in relation to new product or variant

@kieckhafer kieckhafer changed the title [WIP] Ref 3865 kieckhafer inventory collection hooks [WIP] ref: refactor hooks in the inventory package to use Hooks.Events Mar 2, 2018

@kieckhafer kieckhafer changed the title [WIP] ref: refactor hooks in the inventory package to use Hooks.Events [WIP] refactor: hooks in the inventory package to use Hooks.Events Mar 2, 2018

kieckhafer added some commits Mar 2, 2018

Merge branch '3634-willopez-remove-product-collection-hooks' into ref…
…-3865-kieckhafer-inventoryCollectionHooks
Merge branch '3634-willopez-remove-product-collection-hooks' into ref…
…-3865-kieckhafer-inventoryCollectionHooks
@mikemurray
Copy link
Member

mikemurray left a comment

@kieckhafer Seems there are some failing tests. When you have a chance can you take a look?

willopez added some commits Mar 16, 2018

Don't run search mongo hook if conditions are not met
The hook afterUpdateCatalogProduct should not execute the callback in the search mongo package if options are not provided.

@aldeed aldeed added this to the Release 2.0 milestone Mar 16, 2018

willopez and others added some commits Mar 16, 2018

Refactor: Return constant in hooks
As a best practice in reaction hooks the constant should always be returned; this will prevent unexpected undefined errors from occurring when a hook has multiple callbacks.
fix: parse field to number
Field may be a string so it's parsed to an integer.
fix: incorrect inventory adjustment due to a stale document
To ensure the product document isn't stale, fetch a fresh copy before running inventory adjustments.
refactor: fetch a fresh product for search document
To ensure the product document isn't stale, fetch a fresh copy before processing it for search.
@mikemurray
Copy link
Member

mikemurray left a comment

@kieckhafer When I add an item to the cart and checkout it's marked as inventory status sold.

When I cancel the order and restock items, it doesn't return the item to inventory status new.

kieckhafer added some commits Mar 19, 2018

@kieckhafer

This comment has been minimized.

Copy link
Member Author

kieckhafer commented Mar 19, 2018

@willopez @mikemurray Tests are fixed. Inventory issue seems to be an existing issue that was reproducible on the release-1-10-0 branch, not introduced in this PR. I opened a ticket for it: #4039

@spencern spencern modified the milestones: Release 1.10, Release 1.11.0 Mar 20, 2018

@willopez
Copy link
Member

willopez left a comment

👍 Inventory works as expected, there is a know issue for which an issue has been created.

@mikemurray
Copy link
Member

mikemurray left a comment

👍 Looks good. Tested with @willopez adding, removing and adjusting inventory all seem to work.

Fixed a lint issue with a long if statement. And canceling an order is does not return items to status "new" but that is captured in #4039

@mikemurray mikemurray merged commit 4f79a56 into release-1.10.0 Mar 20, 2018

8 checks passed

WIP ready for review
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: docker-build Your tests passed on CircleCI!
Details
ci/circleci: docker-push Your tests passed on CircleCI!
Details
ci/circleci: dockerfile-lint Your tests passed on CircleCI!
Details
ci/circleci: eslint Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
security/snyk No dependency changes
Details

@mikemurray mikemurray deleted the ref-3865-kieckhafer-inventoryCollectionHooks branch Mar 20, 2018

@spencern spencern referenced this pull request Mar 23, 2018

Merged

Release 1.10.0 #4013

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.