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: 4741 catalog variant inventory flags always false #4742

Merged

Conversation

mikemurray
Copy link
Member

@mikemurray mikemurray commented Oct 16, 2018

Resolves #4741
Impact: major
Type: bugfix

Issue

isSoldOut and isLowQuantity for variants and options in the catalog were always false, and not reflective of their actual status.

Solution

Fix the transform that creates the catalog variants by processing the correct inventory flags just like it's done for the top-level product

Breaking changes

Added a new, final param to xformVariant with the processed inventory flags

Testing

Try to set the quantities and warn at thresholds to various variants and options and check the DB or GQL for the expected results.

For example:

  • If you set a variant quantity to 0, it should be sold out if it has no options
  • If you set quantities of the options of a variant to 0, each option should be sold out as well as the top-level variant

Play around with different combinations of sold out, low inventory and back ordered

Below are some variations


  1. With a product that only has top-level variants
  2. Set the quantity to 0 and publish
  3. Query in GQL or Mongo for that catalog item variant
  4. See that isSoldOut is true

  1. With a product that only has top level variants
  2. Set the low quantity threshold to 1000 and publish
  3. Query in GQL or Mongo for that catalog item variant
  4. See that isLowQuantity is true

  1. With a product that has variant options
  2. Set each of the options quantity to 0 and publish
  3. Query in GQL or Mongo for that catalog item variant that contains those options
  4. See that isSoldOut is true
  5. See that isSoldOut is true for the option itself

  1. With a product that has variant options
  2. Set the low quantity threshold to 1000 on the top-level variant and publish
  3. Query in GQL or Mongo for that catalog item variant that contains those options
  4. See that isLowQuantity is true for the option itself

  1. Checkout with a product that has a quantity of 1
  2. Approving or canceling the order in the order panel should update inventory status accordingly

  1. Checkout with a product that has a quantity of 11 and a warn at at 10
  2. Approving or canceling the order in the order panel should update inventory status accordingly

Test the Migrations

  1. Start reaction with this branch
  2. mongorestore --drop the reaction-2.0.0-rc.1 mongo dump
  3. Run a graph QL query for a product that has legacy variant inventory flags, or check the catalog collection.
{
  catalogItemProduct(slugOrId: "converse-all-stars-black") {
    product {
      variants {
        title
        isSoldOut
        isLowQuantity
        inventoryPolicy
        inventoryManagement
        options {
          title
          isSoldOut
          isLowQuantity
        }
      }
    }
  }
}
  1. See the variant and variant options isSoldOut are false. This is incorrect.
  2. Stop and start reaction again
  3. Wait for migration 42 to run
  4. Re-run the query and see the variant and variant options isSoldOut are now true

@mikemurray mikemurray changed the title [WIP] fix: 4741 catalog variant inventory flags always false fix: 4741 catalog variant inventory flags always false Oct 18, 2018
@mikemurray mikemurray changed the base branch from release-2.0.0-rc.5 to release-2.0.0-rc.6 October 18, 2018 16:24
@aldeed
Copy link
Contributor

aldeed commented Oct 18, 2018

@mikemurray A bigger picture question. Why store these in the catalog at all given how dynamic they are and the fact that the source-of-truth inventory values are stored on Product and are not published? Can we just look them up in the CatalogProduct.variants resolver? (/imports/plugins/core/catalog/server/no-meteor/resolvers/CatalogProduct/index.js)

Copy link
Contributor

@nnnnat nnnnat left a comment

Choose a reason for hiding this comment

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

@mikemurray I tested several variant/option combinations and I feel like the isSoldout, isLowQuantity logic is correct and working as expected.

One combination that I felt a little unclear about is when a variant has a warning threshold of 1000 and multiple options. If option "A" has a quantity of 1 while option "B" has a quantity of 2000 both option "A" and the variant will have isLowQuantity: true. Is that the intended outcome in that situation? It makes sense for the variant to have isLowQuanitity: true but I could also see it being false just want to verify.

@mikemurray
Copy link
Member Author

mikemurray commented Oct 19, 2018

@aldeed I agree it feels weird for the inventory values to be published to the catalog, but the only time they are/should be published is when there's a change. For example, a product is only sold out when it reaches 0 on all variants/options.

Making a resolver is a good idea, but we'll have to make a request to the products collection on every request for a product and its variants, which defeats the purpose of the catalog collection being a sort of cache for product data.

@mikemurray
Copy link
Member Author

@aldeed To add... If we stored the inventory quantities in a better place, perhaps in a separate collection away from the products, then that could help with performance if we are requesting inventory status for products on a grid.

Doing an extra fetch for variants for the PDP page is less of an issue.

@mikemurray mikemurray changed the title fix: 4741 catalog variant inventory flags always false [WIP] fix: 4741 catalog variant inventory flags always false Oct 22, 2018
@mikemurray
Copy link
Member Author

The function updateCatalogProductInventoryStatus in /imports/plugins/core/catalog/server/no-meteor/utils/updateCatalogProductInventoryStatus.js needs to be updated to also include the variant and variant options when updating the inventory status.

@mikemurray mikemurray changed the base branch from release-2.0.0-rc.6 to release-2.0.0-rc.7 November 12, 2018 21:59
@mikemurray
Copy link
Member Author

@nnnnat Pushed some changes updateCatalogProductInventoryStatus so it now also updates the variants and options of the catalog item.

Additional testing would include:

  • Approving or canceling an order in the order panel should update inventory status properly

Should be ready for your review.

@mikemurray mikemurray changed the title [WIP] fix: 4741 catalog variant inventory flags always false fix: 4741 catalog variant inventory flags always false Nov 13, 2018
Copy link
Contributor

@nnnnat nnnnat left a comment

Choose a reason for hiding this comment

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

👍 was able to test and everything looks good data wise. @mikemurray if you can resolve the conflicts I'll merge.

I believe one thing that may need addressing is the customer PDP is still using Product collection and not Catalog, but I think that should be another issue/PR.

@aldeed
Copy link
Contributor

aldeed commented Nov 26, 2018

I believe one thing that may need addressing is the customer PDP is still using Product collection and not Catalog

@nnnnat There is code in the server publication function that publishes Catalog collection to Product client collection, so it's actually catalog data even though it appears to be product data. This is not how it should be long term, but I guess was the quickest solution at the time. If we update the grid to use GraphQL only, that will be the best proper solution.

This PR has some failing tests that will need to be fixed before it can be merged.

@mikemurray
Copy link
Member Author

@nnnnat All tests are passing now.

Copy link
Contributor

@nnnnat nnnnat left a comment

Choose a reason for hiding this comment

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

Reapproved! Thanks @mikemurray

@nnnnat nnnnat merged commit 4e59e02 into release-2.0.0-rc.7 Nov 26, 2018
@nnnnat nnnnat deleted the fix-4741-mikemurray-catalog-variant-inventory branch November 26, 2018 22:52
@spencern spencern mentioned this pull request Jan 8, 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.

None yet

3 participants