Add SVEA deployment support to the WIX integration - #68
Conversation
…connect ISSUE: LIS-116
m1k3lm
left a comment
There was a problem hiding this comment.
Code review (automated, xhigh effort)
15 findings below, most severe first, posted inline. Verified against the PR head: full suite green (951 tests / 2723 assertions, including the 6 new PaymentMethodsCheckoutApiTest cases) and PHPStan level 6 clean — these are design/correctness issues the gates do not catch.
The four worth acting on before merge:
hasAvailablePaymentMethods()returns a truthy object on the error path, so a storefront guarding on it renders seQura on every failed call.- Dropping
$merchantIdfrom the publicOrderService::getAvailablePaymentMethodsInCategories()is a silent BC break for host integrations. - The new
$storeId-parameterised endpoint gives no actual store isolation, becauseSeQuraOrderRepositoryis not store-scoped. OrderNotFoundExceptiondegrades tostatusCode: 0/general.errors.unknowninstead of the 404 the service sets.
1 and 4 were confirmed by executing the error path, not inferred from reading.
Two non-code notes: the PR title ("Add SVEA deployment support to the WIX integration") does not match its contents (a checkout payment-methods endpoint), and the "Adjusted disconnect logic" bullet is unrelated scope inside a LIS-116 PR.
Unrelated to this PR but worth knowing: ./bin/phpcs cannot run as configured — .phpcs.xml.dist references SlevomatCodingStandard.Namespaces.FullyQualifiedGlobalFunctions and slevomat is absent from vendor/, so phpcs aborts with "Referenced sniff does not exist". The style gate is not actually running for anyone.
[Generated with Claude Code]
| * | ||
| * @return bool | ||
| */ | ||
| public function hasAvailablePaymentMethods(): bool |
There was a problem hiding this comment.
hasAvailablePaymentMethods() is truthy on the error path.
On a failed call the facade returns TranslatableErrorResponse (via ErrorHandlingAspect), and ErrorResponse::__call() swallows unknown methods and returns $this. Verified by executing it: get_class() is TranslatableErrorResponse and (bool) $response->hasAvailablePaymentMethods() is true.
Any storefront doing if ($response->hasAvailablePaymentMethods()) { render seQura } renders seQura payment methods on every failed call (unknown order ref, seQura down). The helper is only safe behind an isSuccessful() guard, which nothing enforces and no test covers.
| * @throws OrderNotFoundException | ||
| */ | ||
| public function getAvailablePaymentMethodsInCategories(string $orderRef, string $merchantId): array | ||
| public function getAvailablePaymentMethodsInCategories(string $orderRef): array |
There was a problem hiding this comment.
Silent BC break for host integrations.
This is a consumed library, and dropping $merchantId from a public method changes its contract. PHP does not error on extra args to userland functions, so an existing getAvailablePaymentMethodsInCategories($ref, $merchantId) call in WooCommerce/PrestaShop keeps compiling while the merchant id is silently ignored and resolved from local storage instead.
It also now calls getSeQuraOrder() and throws OrderNotFoundException for any order not persisted in the host DB — a case that previously worked precisely because the caller supplied the merchant. Consider keeping the parameter as an optional deprecation shim.
| * | ||
| * @return object | ||
| */ | ||
| public function paymentMethods(string $storeId): object |
There was a problem hiding this comment.
The new store-scoped endpoint provides no actual store isolation.
StoreContextAspect($storeId) only constrains store-scoped repositories, and SeQuraOrderRepository is not one: it has no StoreContext and getByOrderReference() filters on reference alone (src/BusinessLogic/DataAccess/Order/Repositories/SeQuraOrderRepository.php).
So in a multistore install, CheckoutAPI::get()->paymentMethods('storeA')->getPaymentMethodsInCategories(new PaymentMethodsInCategoriesRequest($refFromStoreB)) resolves store B's order and builds the authorized proxy from store B's merchant id.
This contradicts .claude/docs/codingStandard.md §8 ("Repositories are store-scoped: inject StoreContext, filter every query by storeId") and CLAUDE.md ("Anything reading/writing per-store config must respect the active store"). The gap is pre-existing, but this PR is what turns it into a storefront-facing, $storeId-parameterised endpoint.
| * @throws HttpRequestException | ||
| * @throws OrderNotFoundException | ||
| */ | ||
| public function getPaymentMethodsInCategories( |
There was a problem hiding this comment.
The endpoint's primary failure mode degrades to a generic unhandled error.
OrderNotFoundException extends Infrastructure\Exceptions\BaseException, so ErrorHandlingAspect misses its BaseTranslatableException catch and falls through to the generic Throwable catch. Verified output:
['statusCode' => 0, 'errorCode' => 'general.errors.unknown',
'errorMessage' => 'Unhandled error occurred: SeQura order with reference X is not found.']
The 404 that OrderService::getSeQuraOrder() deliberately sets is discarded, the storefront cannot distinguish "unknown order" from "seQura is down", and every stale checkout page hit is logged at ERROR as an unhandled error.
| 'title' => $category->getTitle(), | ||
| 'description' => $category->getDescription(), | ||
| 'icon' => $category->getIcon(), | ||
| 'methods' => array_map(static function (SeQuraPaymentMethod $paymentMethod) { |
There was a problem hiding this comment.
The public checkout payload delegates to the ORM persistence serializer.
SeQuraPaymentMethod::toArray() is what DataAccess/PaymentMethod/Entities/PaymentMethod::toArray() persists: it emits long_title, starts_at, ends_at, cost_description, min_amount.
The sibling CachedPaymentMethodsResponse::toArray() in this same Responses/ folder emits longTitle, startsAt, endsAt, costDescription, minAmount for the same model. Storefront JS consuming CheckoutAPI now gets two different shapes for one entity, and any future change to the entity storage format silently changes the public checkout API.
Map the fields explicitly here, matching CachedPaymentMethodsResponse.
|
|
||
| return $this->proxy->getAvailablePaymentMethodsInCategories( | ||
| new GetAvailablePaymentMethodsRequest($orderRef, $merchantId) | ||
| new GetAvailablePaymentMethodsRequest($orderRef, (string)$order->getMerchant()->getId()) |
There was a problem hiding this comment.
The (string) cast turns a missing merchant id into ''.
Merchant::getId() is untyped (@return int|string over an untyped property). For a stored order whose merchant record lost its id, (string) null yields '', which OrderProxy hands to authorizedProxyFactory->build(''); that fails deep in credentials lookup with a confusing CredentialsNotFoundException rather than a clear "order has no merchant".
Note getAvailablePaymentMethods() about 20 lines above does not cast at all — the same expression handled two different ways in adjacent methods. An explicit check/exception would be clearer than either.
| * | ||
| * @throws Exception | ||
| */ | ||
| public function testGetPaymentMethodsInCategoriesUsesMerchantOfStoredOrder(): void |
There was a problem hiding this comment.
Third copy of the same setup block.
This block and the two around it (~393 and ~522) repeat new OrderService(new MockOrderProxy(), new MockSeQuraOrderRepository(), $this->merchantOrderBuilder, TestServiceRegister::getService(OrderCreationInterface::class)) plus the same SeQuraOrder.json json_decode/file_get_contents/setReference/setSeQuraOrder sequence, with only the merchant id varying.
A private function storeOrderWithMerchant(string $merchantId): void plus a small service factory would collapse all three; as written they have to be edited in lockstep whenever the OrderService constructor changes.
| * | ||
| * @throws Exception | ||
| */ | ||
| public function testGetPaymentMethodsInCategoriesForUnknownOrder(): void |
There was a problem hiding this comment.
Setup asymmetry with the sibling test.
This test passes a fresh MockSeQuraOrderRepository inline while leaving $this->orderRepository pointing at the container's repository; the test directly above it assigns $this->orderRepository.
It passes today only because it never touches that property. The next person adding an assertion through $this->orderRepository would be inspecting a different object from the one the service under test uses, and would get a confusing false negative.
| /** | ||
| * @return SeQuraPaymentMethodCategory[] | ||
| */ | ||
| public function getPaymentMethodCategories(): array |
There was a problem hiding this comment.
getPaymentMethodCategories() has no caller.
Grep across src/ and tests/ finds only the declaration. CLAUDE.md working principle 2: "Write the minimum code that solves the problem — nothing speculative... no abstractions for single-use code." Drop it, or add the test that justifies it.
| * | ||
| * @return object | ||
| */ | ||
| public function paymentMethods(string $storeId): object |
There was a problem hiding this comment.
Naming: the generic name went to the narrower feature.
CheckoutAPI::paymentMethods() and CheckoutAPI::cachedPaymentMethods() now sit side by side, resolving PaymentMethodsCheckoutController and CachedPaymentMethodsController from the same CheckoutAPI\PaymentMethods namespace.
An integrator picking paymentMethods() by name has no way to know it requires an already-solicited order reference while the other takes a merchant id. Consider naming it for what it does (e.g. solicitedOrderPaymentMethods()), or adding it as a second method on the existing controller.
ISSUE: LIS-116
54ef0df to
4393880
Compare
ISSUE: LIS-116
What is the goal?
References
How is it being implemented?
How is it tested?