MONEI / Shopify-api-node

Node Shopify connector sponsored by MONEI
https://monei.com/shopify-payment-gateway/
MIT License
946 stars 277 forks source link

[api] Add createV2 action to Fulfillment resource #419

Closed felipebrahm closed 3 years ago

felipebrahm commented 3 years ago

This PR adds method shopify.fulfillment.createV2(params) (without specifying order id) which calls POST /admin/api/[api-version]/fulfillments.json. This is the new and recommended way of creating new fulfillments without specifying an order id.

I decided to call it createV2 because that's how Shopify calls the equivalent graphql mutation.

More info: https://shopify.dev/tutorials/manage-fulfillments-with-fulfillment-and-fulfillmentorder-resources REST API Docs: https://shopify.dev/docs/admin-api/rest/reference/shipping-and-fulfillment/fulfillment#createV2-2020-10 Graphql Docs: https://shopify.dev/docs/admin-api/graphql/reference/mutation/fulfillmentcreatev2

Fixes #418

coveralls commented 3 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling b02fd65e1a96a5207e253be214b0f9c46680e721 on felipebrahm:add-fulfillment-createv2 into 6428f553388eecc33a42d548a44abe59b14ce0c5 on MONEI:master.

lpinca commented 3 years ago

Can you please also update the the TypeScript type definitions?

Also, why do you say it's the recommended way? From my understanding it's only a different API not meant to replace /admin/api/orders/{order_id}/fulfillments.json.

Can we find a better name? I don't like createV2. How about make or generate or another synonym?

lpinca commented 3 years ago

I also wonder if this should live on the FulfillmentOrder resource instead of Fulfillment.

Edit: it's probably better to keep it on Fulfillment.

felipebrahm commented 3 years ago

Also, why do you say it's the recommended way? From my understanding it's only a different API not meant to replace /admin/api/orders/{order_id}/fulfillments.json.

I was quoting the article that I linked in the pull request description (Manage fulfillments with Fulfillment and FulfillmentOrder resources).

Screen Shot 2020-10-15 at 4 40 28 PM

Can we find a better name? I don't like createV2. How about make or generate or another synonym?

The first option I thought about was to simply use create for both endpoints, like this:

Fulfillment.prototype.create = function create(parentId, params) {
  if (typeof parentId === 'object') {
    params = parentId;
    const url = base.buildUrl.call(this);
    return this.shopify.request(url, 'POST', this.key, params);
  } else {
    const url = this.buildUrl(parentId);
    return this.shopify.request(url, 'POST', this.key, params);
  }
};

That way you could invoke the function with either shopify.fulfillment.create(orderId, params) or shopify.fulfillment.create(params), but I don't love this approach either because the attributes for the params object are completely different so it could be confusing.

I think make and generate are just synonyms of create, so calling it that might just cause confusion. I named it createV2 because that's how Shopify calls this function (mutation) in their GraphQL API, so it would be easier to understand which underlying API endpoint it was calling. I agree with you that createV2 might not be the best name, but I couldn't really think of anything better. Thoughts?

lpinca commented 3 years ago

Ok, thank you for the explanation.