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

feat: add product and productVariant related GraphQL mutations #5562

Merged

Conversation

@kieckhafer
Copy link
Member

commented Sep 16, 2019

Resolves parts of #5529
Impact: minor
Type: feature|refactor

Issue

Our product (not catalog) admin server code is still fully meteor, including a few mutations which use getGraphQLContextInMeteorMethod, which we are deprecating.

Solution

  1. Add four new GraphQL mutations for product admin:
  • createProduct
mutation {
  createProduct(input: { shopId: "{shopId}" }){
    product {
      _id
    }
  }
}
  • createProductVariant
mutation {
  createProductVariant(input: { productId: "{parentProductId}", shopId: "shopId" }){
    variant {
      _id
    }
  }
}
  • cloneProducts
mutation {
  cloneProducts(input: { productIds: ["{id1}"," {id2}"], shopId: "shopId" }){
    products {
      _id
    }
  }
}
  • cloneProductVariants
mutation {
  cloneProductVariants(input: { variantIds: ["{id1}"," {id2}"], shopId: "shopId" }){
    variants {
      _id
    }
  }
}
  1. Remove existing products/createProduct, products/createVariant, products/cloneVariant code, and all related code which is no longer needed / moved in the new mutations

  2. Update client code to call new GraphQL mutations instead of meteor methods.

  • This was a much bigger task than was expected. A lot of our client components use UNSAFE_... and other React lifecycle methods, and will need to be refactored completely in the near future, so I did my best to keep them as close to how they are as possible as to not need a complete refactor at the moment.

One regression / change here is that when you create a new product / variant, you are no longer redirect to that new variant page. The reason is when we use a GraphQL mutation, the new product / variant is created almost immediately, however, it takes 1+ seconds for the Meteor subscriptions to update and include the new product. Because of this, when redirecting, you'd land on the page of a variant that isn't yet in the Meteor subscription, and there would be an error. Because of the need to refactor the components soon anyway, I think this is a decent trade off, as opposed to delaying this PR and refactoring all of these components now. I've left a comment in the code suggesting we re-add this feature when these components are updated.

Breaking changes

Any custom plugins which use any of the above listed meteor methods will need to be updated to use the GraphQL mutation instead

Testing

  1. Create a product using the mutation, and also in the UI (Create Product button above Product table)
  2. Create a variant using the mutation, and also in the UI (+ button on product page)
  3. Create an option using the mutation, and also in the UI (+ button on product variant page in options area)
  4. Clone a product using the mutation, and also in the UI (check one or more products on the product table, and use the dropdown to clone, and also inside an individual product page, use the ... menu)
  5. Clone a variant using the mutation, and also in the UI (inside an individual product variant, use the ... menu))
  6. See all of the above work

You can see that all the new resolvers, shop, tags, 'slug' and variants on type Product, and options and shop on ProductVariant, work with this mutation:

mutation {
  cloneProducts(input: { productIds: ["{productId}"], shopId: "{shopId}" }){
    products {
      _id,
     slug
      shop {
				_id
        name
      }
      tagIds,
      tags {
        edges {
          node {
            displayTitle
            heroMediaUrl
            position
            slug
          }
        }
      },
      variants {
        _id
        shop {
           _id
          }
        options {
          _id
         shop {
           _id
          }
        }
      }
    }
  }
}
Signed-off-by: Erik Kieckhafer <ek@ato.la>
@kieckhafer kieckhafer changed the base branch from master to develop Sep 16, 2019
kieckhafer added 7 commits Sep 16, 2019
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
export default async function createProductVariant(_, { input }, context) {
const {
clientMutationId = null,
parentId

This comment has been minimized.

Copy link
@kieckhafer

kieckhafer Sep 17, 2019

Author Member

@aldeed should we pass shopId here as well? I know we usually want to pass shopId, however shopId is available on the parent product, which is found prior to anywhere that would use it: https://github.com/reactioncommerce/reaction/pull/5562/files#diff-23f5be7d77820413c96a2327c790435bR27

This comment has been minimized.

Copy link
@aldeed

aldeed Sep 18, 2019

Member

We've been trying to always ask for shopId even if we can get it because in many cases it allows the auth check to happen before any db queries, which is faster, cheaper, and can be handy if we move auth to a separate service.

This comment has been minimized.

Copy link
@aldeed

aldeed Sep 18, 2019

Member

But when checking permission based on the passed in shopId, you always have to remember to include shopId in the database find as well. Otherwise they could pass in any shop they have access to while passing in an entity ID that belongs to a different shop.

kieckhafer added 7 commits Sep 17, 2019
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
const count = await Products.find({ _id: { $in: variantIds }, type: "variant", shopId }).count();
if (count !== variantIds.length) throw new ReactionError("not-found", "One or more variants do not exist");

const newVariantIds = await Promise.all(variantIds.map(async (variantId) => {

This comment has been minimized.

Copy link
@kieckhafer

kieckhafer Sep 17, 2019

Author Member

@aldeed I'm doing a lot of awaiting promises and in here, this works, but not sure if it's the best way to do all of this.

Signed-off-by: Erik Kieckhafer <ek@ato.la>
delete clonedVariantObject.createdAt;

// Apply custom transformations from plugins.
for (const customFunc of context.getFunctionsOfType("mutateNewVariantBeforeCreate")) {

This comment has been minimized.

Copy link
@kieckhafer

kieckhafer Sep 17, 2019

Author Member

I copied the clone functions as close as I could from the Meteor methods. I noticed that this getFunctionsOfType loop was in the variant clone method, but not in the product clone method, even the variant section of that method. Does it need to be added there?

This comment has been minimized.

Copy link
@aldeed

aldeed Sep 18, 2019

Member

That might be an oversight. It seems like it should be in both, or possibly in neither since they are cloned from variants that were already mutated. It depends on what those functions do. Possibly there should be separate mutateNewVariantBeforeCreate and mutateNewVariantAfterClone for the most specificity.

kieckhafer added 10 commits Sep 17, 2019
…r-createProductBasedGraphQLMethods
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
…eProduct mutation

Signed-off-by: Erik Kieckhafer <ek@ato.la>
… table

Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
kieckhafer added 5 commits Sep 19, 2019
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
@aldeed aldeed referenced this pull request Sep 23, 2019
29 of 45 tasks complete
kieckhafer added 5 commits Sep 23, 2019
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
…r-createProductBasedGraphQLMethods
@kieckhafer kieckhafer marked this pull request as ready for review Sep 25, 2019
kieckhafer added 2 commits Sep 25, 2019
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
@kieckhafer

This comment has been minimized.

Copy link
Member Author

commented Sep 25, 2019

@aldeed @mikemurray integration tests are added, this is ready for an official review.

kieckhafer added 3 commits Sep 25, 2019
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
kieckhafer added 3 commits Sep 25, 2019
…r-createProductBasedGraphQLMethods
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Copy link
Member

left a comment

👍 Queries and UI related to those same queries all work.

aldeed added 2 commits Sep 30, 2019
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
@aldeed
aldeed approved these changes Sep 30, 2019
Copy link
Member

left a comment

I pushed a couple small commits on the new GraphQL schema. Looks good to merge if tests still pass.

@kieckhafer kieckhafer merged commit 185e3ed into develop Sep 30, 2019
5 checks passed
5 checks passed
DCO DCO
Details
WIP Ready for review
Details
build_and_test Workflow: build_and_test
Details
license/snyk - package.json (reactioncommerce) No new issues
Details
security/snyk - package.json (reactioncommerce) No new issues
Details
@kieckhafer kieckhafer deleted the refactor-kieckhafer-createProductBasedGraphQLMethods branch Sep 30, 2019
@kieckhafer kieckhafer referenced this pull request Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.