feat(core-flows, fulfillment): Add create return specific method and add more tests #7357
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
|
… into feat/create-return-part2
| @@ -37,6 +37,7 @@ export const joinerConfig: ModuleJoinerConfig = { | |||
| name: ["return_reason", "return_reasons"], | |||
| args: { | |||
| entity: ReturnReason.name, | |||
| methodSuffix: pluralize(ReturnReason.name), | |||
There was a problem hiding this comment.
@carlos-r-l-rodrigues do you think it would be wise by default to do that? if the entity if provided and it is not the first ailas then we do that under the hood? convention being the main entity is always the first
There was a problem hiding this comment.
Yeah, it is alright to do that.
Maybe we could even infer the methodSuffix in the remote query from the property entity. and if it is different we can specify it.
But not in this PR.
|
@olivermrbl I have this CLI pipeline failing, I believe that you said something about it being fixed at some point is that correct? |
| data: FulfillmentTypes.CreateFulfillmentDTO, | ||
| @MedusaContext() sharedContext: Context = {} | ||
| ): Promise<FulfillmentTypes.FulfillmentDTO> { | ||
| const { order, ...fulfillmentDataToCreate } = data |
There was a problem hiding this comment.
Why do we have an order passed to the fulfillment module? I checked the typings and it seems it's an empty object now, but I guess it's better to remove it altogether?
There was a problem hiding this comment.
well I am not sure why we have that, it is also not used but it is required in the types and in the workflow for the fulfillment. I didn't want to do that in that pr until we know why we have that as I dont recall the need for it but I can still rm it everywhere once we are aware of why we added it in the first place. Also we have a link between order and fulfillment, so could it be for the provider?
cc @olivermrbl
There was a problem hiding this comment.
v1 uses the order to pass some info to the providers.
we could rename that to something else in the future though
There was a problem hiding this comment.
Yeah, what @carlos-r-l-rodrigues said. This is just a naming issue. The order can hold details that are required for the provider to create the fulfillment
There was a problem hiding this comment.
Ok got it 👍 Since a fulfillment is not necessarily tied to an order, let's try to rename it to something more meaningful (In payments we have data, which is very generic but it might be OK since each provider might require something different)
There was a problem hiding this comment.
I can change that to data in my follow up PR where I already removed order_id from types.
There was a problem hiding this comment.
I believe The fulfillment already have a field data which is what is returned by the provider, we night need to think about something else that seems more meaningful ? extra_data?
There was a problem hiding this comment.
Or something more specific like provider_data?
|
@olivermrbl can I merge even with this CI failing? |
|
Yes, sure. This is related to the starter, so should be fine. |
What