Bug/Design gap
cancelFulfillment() in the Fulfillment module service calls its own
cancellation guard via a hardcoded self-reference to the base class
rather than a polymorphic this.constructor call:
|
FulfillmentModuleService.canCancelFulfillmentOrThrow(fulfillment) |
async cancelFulfillment(id: string, sharedContext = {}) {
const fulfillment = await this.fulfillmentService_.retrieve(id, {}, sharedContext)
FulfillmentModuleService.canCancelFulfillmentOrThrow(fulfillment) // <-- hardcoded
...
}
This breaks the standard JS/TS subclassing pattern: a consumer following
Medusa's own documented "extend a module" approach (subclass the core
module's service, register it via medusa-config.ts in place of the
built-in module) who overrides canCancelFulfillmentOrThrow on their
subclass finds it's silently ignored — the base class's own static
method still runs, because the call is bound to FulfillmentModuleService
by name, not this.constructor.
Why this matters
The guard (shipped_at/delivered_at → throw, unconditionally) makes
sense for physical fulfillment, but is a real obstacle for non-physical
fulfillment providers (e.g. digital products, tickets, licenses) where
"shipped"/"delivered" timestamps may get set (intentionally or by an
admin mis-click) without there being any real, physical, irreversible
world-state behind them. There's currently no supported extension point
to relax or customize this rule per fulfillment provider/type — the only
way to change it is to fully reimplement cancelFulfillment(), including
reaching into private (_-suffixed) fields (fulfillmentService_,
fulfillmentProviderService_, baseRepository_) that aren't part of any
documented/stable API.
Suggested fix
Minimal, behavior-preserving for the base class:
async cancelFulfillment(id: string, sharedContext = {}) {
const fulfillment = await this.fulfillmentService_.retrieve(id, {}, sharedContext)
;(this.constructor as typeof FulfillmentModuleService).canCancelFulfillmentOrThrow(fulfillment)
...
}
This alone would make the documented module-override pattern actually
work for this guard. A further improvement worth considering: let a
fulfillment provider declare something like handlesPhysicalGoods: false
(returned from getFulfillmentOptions() or a new provider capability),
and have the core guard consult it — so digital/non-physical providers
don't need a full module override at all.
Reproduction
- Create a fulfillment via a
manual-based provider for a
non-physical/digital order.
- Call the admin "mark as delivered" action on it (or hit
markFulfillmentAsDeliveredWorkflow directly).
- Attempt to cancel the fulfillment (or the order, which needs all
fulfillments canceled first) → MedusaError, "Fulfillment with id ... already delivered", with no supported way to relax this per
provider/type without a full module-service override touching
private fields.
Environment
Medusa v2.19.0
Bug/Design gap
cancelFulfillment()in the Fulfillment module service calls its owncancellation guard via a hardcoded self-reference to the base class
rather than a polymorphic
this.constructorcall:medusa/packages/modules/fulfillment/src/services/fulfillment-module-service.ts
Line 1900 in bda24b9
This breaks the standard JS/TS subclassing pattern: a consumer following
Medusa's own documented "extend a module" approach (subclass the core
module's service, register it via
medusa-config.tsin place of thebuilt-in module) who overrides
canCancelFulfillmentOrThrowon theirsubclass finds it's silently ignored — the base class's own static
method still runs, because the call is bound to
FulfillmentModuleServiceby name, not
this.constructor.Why this matters
The guard (
shipped_at/delivered_at→ throw, unconditionally) makessense for physical fulfillment, but is a real obstacle for non-physical
fulfillment providers (e.g. digital products, tickets, licenses) where
"shipped"/"delivered" timestamps may get set (intentionally or by an
admin mis-click) without there being any real, physical, irreversible
world-state behind them. There's currently no supported extension point
to relax or customize this rule per fulfillment provider/type — the only
way to change it is to fully reimplement
cancelFulfillment(), includingreaching into private (
_-suffixed) fields (fulfillmentService_,fulfillmentProviderService_,baseRepository_) that aren't part of anydocumented/stable API.
Suggested fix
Minimal, behavior-preserving for the base class:
This alone would make the documented module-override pattern actually
work for this guard. A further improvement worth considering: let a
fulfillment provider declare something like
handlesPhysicalGoods: false(returned from
getFulfillmentOptions()or a new provider capability),and have the core guard consult it — so digital/non-physical providers
don't need a full module override at all.
Reproduction
manual-based provider for anon-physical/digital order.
markFulfillmentAsDeliveredWorkflowdirectly).fulfillments canceled first) →
MedusaError,"Fulfillment with id ... already delivered", with no supported way to relax this perprovider/type without a full module-service override touching
private fields.
Environment
Medusa v2.19.0