feat(core-flows,medusa,types): cancel fulfillments API #7095
Conversation
🦋 Changeset detectedLatest commit: dd49bdc The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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
|
riqwan
force-pushed
the
feat/fulfillment-delete
branch
from
360fd64
to
31435a2
Compare
| MedusaResponse, | ||
| } from "../../../../types/routing" | ||
|
|
||
| export const DELETE = async ( |
There was a problem hiding this comment.
@olivermrbl since we're not exposing delete for fulfillment in the module, am i right in assuming that this is a cancel endpoint? If so, wouldn't it make more sense to call this POST /admin/fulfillments/:id/cancel?
There was a problem hiding this comment.
I think that makes sense, since we are not deleting anything here.
| MedusaResponse, | ||
| } from "../../../../types/routing" | ||
|
|
||
| export const DELETE = async ( |
There was a problem hiding this comment.
I think that makes sense, since we are not deleting anything here.
| ModuleRegistrationName.FULFILLMENT | ||
| ) | ||
|
|
||
| await fulfillmentModuleService.retrieveFulfillment(id) |
There was a problem hiding this comment.
Is this only for validation? If so, I'd suggest we do it in the workflow instead
There was a problem hiding this comment.
I have come across a similar case a few times, and I can't figure out what the right thing to do is, so maybe we should establish a convention.
Typically, our workflows expect a specific shape of data for bulk mutating a resource and therefore do not have validation built-in – as they would have in the case of a single mutation through retrieve. Given that our HTTP layer almost always operates on a single (root) resource, one could argue the validation is well-placed in the controller.
E.g. right now, sending a request to update a product with an ID that does not exist will perform a selector-based update. There is not a product to update, so the return value of the workflow in an empty array and the subsequent call to fetch the product fails.
There was a problem hiding this comment.
@olivermrbl you have a point due to the fact that we do a select-based mutation. My thought-process is that workflows should validate that what they are operating on is correct, since they can be executed from a non-http flow, but in this specific scenario I can see why it might make sense to do the validation here, cc @riqwan WDYT?
There was a problem hiding this comment.
I think its a moot point for this endpoint since the module internally calls retrieveFulfillment to validate its existence.
I agree that the validation generally being on the workflow makes total sense as its built to run independently, but running validations on the endpoint is also totally valid in cases where the endpoint needs to have agency over its job of rendering the response graciously and consistently.
There was a problem hiding this comment.
The only aspect to consider here is to not fire 3-4 DB requests for just checking if something exists, which is why I suggested moving the check elsewhere. Ideally the HTTP endpoint can "clean up" the response in a purely synchronous manner, but I agree that there might be times where an async call to some endpoint is needed for a better response.
Anyhow, it's not too important for now, but these are considerations we'll need to make as we make v2.0 more prod-ready
| ModuleRegistrationName.FULFILLMENT | ||
| ) | ||
|
|
||
| await promiseAll(ids.map((id) => service.cancelFulfillment(id))) |
There was a problem hiding this comment.
@olivermrbl this looks like a non reversible step. Should we remove bulk capabilities here if that is the case?
There was a problem hiding this comment.
I think that makes sense, nice catch. We should treat all steps that interface with 3rd party providers this way – as we did with payments already. So bulk operations would have to be handled "manually" by the consumer
what:
RESOLVES CORE-1972