Skip to content

fix(fulfillment,core-flows): support polymorphic cancel guard and add address country code fallback - #16761

Open
vjymisal0 wants to merge 3 commits into
medusajs:developfrom
vjymisal0:fix/fulfillment-cancel-guard-and-order-address-workflow
Open

fix(fulfillment,core-flows): support polymorphic cancel guard and add address country code fallback#16761
vjymisal0 wants to merge 3 commits into
medusajs:developfrom
vjymisal0:fix/fulfillment-cancel-guard-and-order-address-workflow

Conversation

@vjymisal0

@vjymisal0 vjymisal0 commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

What

Fix three independent edge cases:

  • Dispatch fulfillment cancellation through the concrete service constructor so subclasses can override the guard.
  • Avoid rejecting new order addresses when no existing country code is present.
  • Accept top-level redisUrl options in the workflow-engine Redis loader.

Why

These cases caused incorrect validation failures, prevented polymorphic service behavior, or rejected a supported configuration shape.

How

The implementation changes the static dispatch and country-code guard conditions, and normalizes the Redis loader's accepted option shapes. Each behavior has a focused regression test.

Testing

  • Added/updated focused unit tests for all three changes.
  • Added a changeset for @medusajs/fulfillment, @medusajs/core-flows, and @medusajs/workflow-engine-redis.
  • CI currently reports environment/setup and package test failures; those failures are recorded in the PR checks.

Linked issues

Closes #16744
Closes #16636
Closes #16697

@vjymisal0
vjymisal0 requested a review from a team as a code owner September 8, 2026 14:30
@changeset-bot

changeset-bot Bot commented Sep 8, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: fb0b202

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 83 packages
Name Type
@medusajs/fulfillment Patch
@medusajs/core-flows Patch
@medusajs/workflow-engine-redis Patch
@medusajs/medusa Patch
@medusajs/test-utils Patch
integration-tests-http Patch
@medusajs/loyalty-plugin Patch
@medusajs/medusa-oas-cli Patch
@medusajs/analytics Patch
@medusajs/api-key Patch
@medusajs/auth Patch
@medusajs/caching Patch
@medusajs/cart Patch
@medusajs/currency Patch
@medusajs/customer Patch
@medusajs/file Patch
@medusajs/index Patch
@medusajs/inventory Patch
@medusajs/link-modules Patch
@medusajs/locking Patch
@medusajs/notification Patch
@medusajs/order Patch
@medusajs/payment Patch
@medusajs/pricing Patch
@medusajs/product Patch
@medusajs/promotion Patch
@medusajs/rbac Patch
@medusajs/region Patch
@medusajs/sales-channel Patch
@medusajs/search Patch
@medusajs/settings Patch
@medusajs/stock-location Patch
@medusajs/store Patch
@medusajs/tax Patch
@medusajs/translation Patch
@medusajs/user Patch
@medusajs/workflow-engine-inmemory Patch
@medusajs/search-postgres Patch
@medusajs/draft-order Patch
@medusajs/oas-github-ci Patch
@medusajs/cache-inmemory Patch
@medusajs/cache-redis Patch
@medusajs/event-bus-local Patch
@medusajs/event-bus-redis Patch
@medusajs/analytics-local Patch
@medusajs/analytics-posthog Patch
@medusajs/auth-emailpass Patch
@medusajs/auth-github Patch
@medusajs/auth-google Patch
@medusajs/auth-oidc Patch
@medusajs/caching-redis Patch
@medusajs/file-local Patch
@medusajs/file-s3 Patch
@medusajs/fulfillment-manual Patch
@medusajs/locking-postgres Patch
@medusajs/locking-redis Patch
@medusajs/notification-local Patch
@medusajs/notification-sendgrid Patch
@medusajs/payment-stripe Patch
@medusajs/framework Patch
@medusajs/instantsearch-adapter Patch
@medusajs/js-sdk Patch
@medusajs/modules-sdk Patch
@medusajs/orchestration Patch
@medusajs/query Patch
@medusajs/types Patch
@medusajs/utils Patch
@medusajs/workflows-sdk Patch
create-medusa-app Patch
@medusajs/http-types-generator Patch
@medusajs/cli Patch
@medusajs/deps Patch
@medusajs/eslint-plugin Patch
@medusajs/telemetry Patch
@medusajs/admin-bundler Patch
@medusajs/admin-sdk Patch
@medusajs/admin-shared Patch
@medusajs/admin-vite-plugin Patch
@medusajs/dashboard Patch
@medusajs/icons Patch
@medusajs/toolbox Patch
@medusajs/ui-preset Patch
@medusajs/ui Patch

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

@medusa-os-bot

medusa-os-bot Bot commented Sep 8, 2026

Copy link
Copy Markdown

Thanks for the contribution! We need more information before reviewing this further.

The author submits three targeted fixes: (1) using this.constructor for polymorphic static dispatch of canCancelFulfillmentOrThrow in the fulfillment module service so subclasses can override the guard, (2) adding a guard in updateOrderValidationStep so country-code conflict validation is only triggered when the order already has an existing country code in the relevant address, and (3) making the workflow-engine-redis loader accept a top-level redisUrl option in addition to the nested redis object format. Each fix includes a focused unit test. No security issues were found in the diff. However, the PR body does not follow the project's PR template — the required What, Why, How, and Testing sections are absent, replaced with a custom Summary/Files Changed format. A changeset is also missing for this source change under packages/. Heads up: several earlier open PRs reference the same underlying issues. PR #16745 (opened 2026-09-07) references issue #16744 (fulfillment polymorphism); PR #16637 (opened 2026-08-27) references issue #16636 (order address country code); PR #16699 (opened 2026-09-02) references issue #16697 (redis top-level options). If any of those earlier PRs are merged first, this PR may be closed as a duplicate.

Triggered by: new PR opened

@vjymisal0

Copy link
Copy Markdown
Contributor Author

I updated the PR description to include the required What, Why, How, and Testing sections. The implementation and focused tests were already present; the remaining red checks are CI environment/package failures shown in the checks.

@medusa-os-bot

medusa-os-bot Bot commented Sep 11, 2026

Copy link
Copy Markdown

Thanks for the contribution! A few items need to be addressed before this can move forward:

The author updated the PR body to include the required What/Why/How/Testing sections — that prior issue is resolved. Three outstanding concerns remain. The underlying logic changes are reasonable: adding a guard so the order-update validation only rejects a country-code change when the order already has a country code, switching to this.constructor for polymorphic static dispatch in the fulfillment service, and accepting top-level Redis options in the workflow-engine-redis loader. No security issues and no performance issues were found in the diff. However, two of the three added test files contain issues that would cause them to fail, and a changeset is still missing.

  • Missing changeset: source files under packages/ were changed but no changeset file was added. Run yarn changeset and select patch for @medusajs/fulfillment, @medusajs/core-flows, and @medusajs/workflow-engine-redis. This was raised in the previous review and remains unaddressed.
  • No linked issue: these are non-trivial behavioral fixes and the contribution guidelines require a linked issue via a closing keyword (e.g. Closes #N). The previous review noted that earlier PRs reference issues FulfillmentModuleService.cancelFulfillment calls the shipped/delivered guard via a hardcoded class reference, breaking subclass overrides #16744, updateOrderWorkflow: "Country code cannot be changed" when ADDING an address to an order that has none #16636, and [Bug]: [workflow-engine-redis] Options type declares top-level redisUrl but loader only reads nested options.redis #16697 for the three respective fixes — link the relevant ones.
  • packages/modules/fulfillment/src/services/tests/fulfillment-module-service.spec.ts: the test does not mock baseRepository_. After canCancelFulfillmentOrThrow (overridden as a jest.fn()) returns, the cancelFulfillment method reaches this.baseRepository_.serialize(fulfillment) at line 1922 of the service. Since baseRepository_ is undefined on the Object.create-constructed instance, this throws TypeError: Cannot read properties of undefined (reading 'serialize'), causing the test to fail. Mock baseRepository_ on the constructed instance (e.g. service.baseRepository_ = { serialize: jest.fn().mockResolvedValue(fulfillmentMock) }) or test canCancelFulfillmentOrThrow directly as a standalone static method.
  • packages/core/core-flows/src/order/workflows/tests/update-order.spec.ts: the test invokes (updateOrderValidationStep as any)._invoke(...). _invoke is not part of the documented public API of a step object returned by createStep. If this method does not exist or is renamed, the call would throw and the test would fail (or pass vacuously for wrong reasons). Use a supported testing approach instead — for example, extract the step's handler as a plain named function and import and call it directly in the test, rather than reaching into the step object's internals.

Triggered by: PR description updated

@vjymisal0

Copy link
Copy Markdown
Contributor Author

Addressed the latest bot review: added the changeset, linked the related issues in the PR body, mocked in the fulfillment test, and replaced the test with a direct exported validation handler.

@vjymisal0

Copy link
Copy Markdown
Contributor Author

Small correction: the shell ate the inline code names in my previous comment.

Addressed the latest bot review: added the changeset, linked the related issues in the PR body, mocked baseRepository_ in the fulfillment test, and replaced the _invoke test with a direct exported validation handler.

@medusa-os-bot

medusa-os-bot Bot commented Sep 12, 2026

Copy link
Copy Markdown

Thanks for the contribution! A few items need to be addressed before this can move forward:

The author has addressed all issues raised in the previous review: the PR template is complete with all required sections, linked issues are present (Closes #16744, #16636, #16697), a changeset file was added, the fulfillment test now mocks baseRepository_, and the order-update test was refactored to import and call updateOrderValidationHandler directly. The three underlying logic changes are sound: switching to this.constructor for polymorphic static dispatch of the cancel guard, gating the country-code conflict check on the order already having a country code, and accepting top-level Redis options via options?.redis ?? options ?? {}. No security or performance issues were found. Two items remain before the PR is ready.

  • packages/modules/fulfillment/src/services/tests/fulfillment-module-service.spec.ts: the baseRepository_ mock is missing a getFreshManager method. The @InjectManager() decorator (see packages/core/utils/src/modules-sdk/decorators/inject-manager.ts, line 54) calls this.baseRepository_.getFreshManager(originalContext) when baseRepository_ is present but no manager is in the shared context. The mock object is present, so the null-guard passes, but then getFreshManager is undefined on the mock and calling it throws TypeError: resourceWithManager.getFreshManager is not a function before execution ever reaches the static-dispatch assertion. Fix: add getFreshManager: jest.fn().mockReturnValue({}) to the baseRepository_ mock (the returned value is assigned to copiedContext.manager; any non-null object works to let the test proceed through the decorator and into the method body).
  • .changeset/fresh-pandas-wave.md: the changeset message does not follow the required format. The message reads as plain prose; it must start with the conventional prefix — e.g. fix(fulfillment, core-flows, workflow-engine-redis): support polymorphic cancel guard, order address country-code fallback, and top-level Redis loader options. See the existing .changeset/enable-manage-inventory-creates-inventory-item.md for the expected style.

Triggered by: PR description updated

@medusa-os-bot

medusa-os-bot Bot commented Sep 12, 2026

Copy link
Copy Markdown

Thanks for the contribution! A few items need to be addressed before this can move forward:

This is a re-review following the author's latest update. Three previously raised issues have been resolved: the PR template is complete with all required sections, linked issues are present (Closes #16744, #16636, #16697), and the order-update test was refactored to import and call the exported updateOrderValidationHandler directly rather than reaching into createStep internals. The three underlying logic changes remain sound: switching to this.constructor for polymorphic static dispatch of the cancel guard, gating the country-code conflict check on the order already having a country code, and accepting top-level Redis options via options?.redis ?? options ?? {}. No security or performance issues were found. Two items raised in the previous review are still unresolved in the current diff.

  • packages/modules/fulfillment/src/services/tests/fulfillment-module-service.spec.ts: the baseRepository_ mock ({ serialize: jest.fn().mockResolvedValue({ id: 'ful_123' }) }) is missing a getFreshManager method. The @InjectManager() decorator (packages/core/utils/src/modules-sdk/decorators/inject-manager.ts line 55) calls resourceWithManager.getFreshManager(originalContext) when baseRepository_ is set; since the mock lacks this method, the test throws TypeError: resourceWithManager.getFreshManager is not a function before reaching the static-dispatch assertion. Fix: add getFreshManager: jest.fn().mockReturnValue({}) to the baseRepository_ mock object.
  • .changeset/fresh-pandas-wave.md: the changeset message is plain prose and does not follow the required conventional prefix format. It must begin with fix(fulfillment, core-flows, workflow-engine-redis): <short description> (using the short package names). See other files under .changeset/ for the expected style.

Triggered by: new commit pushed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment