feat(core-flows, medusa): add shipping methods to cart API #7150
Conversation
🦋 Changeset detectedLatest commit: cdc07c1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
| const optionIds = transform({ input }, (data) => { | ||
| return (data.input.options ?? []).map((i) => i.id) | ||
| }) | ||
|
|
||
| validateCartShippingOptionsStep({ |
There was a problem hiding this comment.
question: I wonder if we should just remove the method, if it's no longer applicable? We've talked about doing that previously.
That would be a slightly better DX than telling users to handle this themselves in the client.
There was a problem hiding this comment.
Maybe it could be a config passed to the step?
There was a problem hiding this comment.
I created a new step called refreshCartShippingMethodsStep to do this, feels cleaner that way as the inputs are slightly different:
refreshCartShippingMethodsStep({ cart }) - removes shipping methods if no longer applicable
validateCartShippingOptionsStep({ cart, option_ids }) - throws error
should we merge them?
There was a problem hiding this comment.
I am fine to keep the two separated. In that case, I would probably unify the validation logic in a helper we can use in both methods, as this is almost identical right?
Update: I looked at the commit, and it looks like there are slight differences, so I am not sure if this is possible. I'll let you decide on how to proceed.
There was a problem hiding this comment.
I had a similar concern, maybe refresh can just return if a refresh was performed or not, and that can serve for the cases when we do want to validate? Just something that seemed confusing, but if you can't think of anything better than this is fine I guess
There was a problem hiding this comment.
The inputs are different and the outcome is different. They serve 2 different purposes even though they're doing similar things. If you both feel strongly enough, I'll merge them in.
There was a problem hiding this comment.
I am fine keeping it as is, so we can get it merged.
packages/core-flows/src/definition/cart/workflows/add-shipping-method-to-cart.ts
Show resolved
Hide resolved
packages/core-flows/src/definition/cart/steps/refresh-cart-shipping-methods.ts
Outdated
Show resolved
Hide resolved
| shippingMethodsToDelete.push(...diff) | ||
| } | ||
|
|
||
| const validShippingOptions = |
There was a problem hiding this comment.
I am a bit confused by this workflow as it lists shipping options twice and deletes, maybe we can make the code more obvious why we are doing this?
There was a problem hiding this comment.
I think this might be outdated, take a look again 🤞🏻
There was a problem hiding this comment.
@riqwan yup it was outdated, looks good 👍
| @@ -22,6 +23,19 @@ import { prepareConfirmInventoryInput } from "../utils/prepare-confirm-inventory | |||
| import { prepareLineItemData } from "../utils/prepare-line-item-data" | |||
| import { refreshPaymentCollectionForCartStep } from "./refresh-payment-collection" | |||
|
|
|||
| const cartFields = [ | |||
There was a problem hiding this comment.
This is a lot of data, do we really need everything? I'm afraid this might have a toll on performance.
There was a problem hiding this comment.
These actions are performed on a cart context, depending on what the rules are, they could be any of these. We can optimize this by whitelisting what we allow as "rules", but that can come later.
packages/core-flows/src/definition/cart/steps/validate-cart-shipping-options.ts
Show resolved
Hide resolved
| const optionIds = transform({ input }, (data) => { | ||
| return (data.input.options ?? []).map((i) => i.id) | ||
| }) | ||
|
|
||
| validateCartShippingOptionsStep({ |
There was a problem hiding this comment.
I had a similar concern, maybe refresh can just return if a refresh was performed or not, and that can serve for the cases when we do want to validate? Just something that seemed confusing, but if you can't think of anything better than this is fine I guess
| .filter((sm) => invalidShippingOptionIds.includes(sm.shipping_option_id!)) | ||
| .map((sm) => sm.id) | ||
|
|
||
| await cartModule.softDeleteShippingMethods(shippingMethodsToDelete) |
There was a problem hiding this comment.
Question: Do we want to do a soft or hard delete in this case, if a shipping method doesn't exist anymore?
There was a problem hiding this comment.
We can only soft delete here as we have to revert this if any other steps fail
what:
RESOLVES CORE-2007