fix(mollie-payments): wrap addPayment in transaction#33
Conversation
martijnvdbrug
left a comment
There was a problem hiding this comment.
Minor optional change requested, but approved if you choose to ignore it :-)
Good catch. I have yet to test it out though...
|
Thanks Martijn! Tested in my Vendure project w/ our test suite and are passing now after this change. Before they were failing, mentioning that the fn wasn't wrapped in a transaction |
grolmus
left a comment
There was a problem hiding this comment.
Reviewed locally + ran build & lint, both green.
The wrap pattern is correct and matches the canonical Vendure idiom mirroring core #4689. withTransaction correctly joins an existing tx on _ctx (webhook path via @Transaction() on MollieController) and opens a new one when _ctx has none (free-order path in createPaymentIntent where a fresh admin RequestContext is built). Applying the wrap at addPayment rather than higher up in handleMolliePaymentStatus is the right call — it also protects the free-order path and any future direct callers.
A few non-blocking suggestions for a follow-up:
-
Param naming:
_ctxis a touch confusing — the leading underscore conventionally signals an unused parameter, but here it's passed towithTransaction. Something likeouterCtxwould read better. Minor. -
Comment clarity at the top of
addPayment: the current note mentions the free-order admin ctx specifically, butaddPaymentis a public method and any future caller without an outer tx would hit the same issue. Could be reworded to lead with the general invariant — "transitionToStateandaddPaymentToOrdermust run in a single tx so save/hooks/save commit or roll back together;withTransactionjoins an existing tx on the passed ctx, or opens a new one (e.g. for the fresh admin ctx increatePaymentIntentfree-order branch)." -
E2E regression: core #4689 added
order-process-rollback.e2e-spec.tsas a regression guard. A sibling spec inpackages/mollie-plugin/e2e/that registers a throwing hook and asserts no half-transitioned state afteraddPaymentfailure would protect against future refactors. Worth tracking as a follow-up issue.
LGTM, approving.
This PR includes:
With the recent release of Vendure, it requires the
transitionOrderToStateandaddPaymentToOrderfns to be wrapped in a Transaction.That wasn't handled inside the
addPaymenthelper function in the Mollie service.cc @martijnvdbrug would be nice to have your input on this too, please 🙏
Related Vendure Core PR: vendurehq/vendure#4689
Need help on this PR? Tag
@codesmithwith what you need.