PRE-3552: Add UHF refund - #316
Conversation
c0963a9 to
e681f24
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
e681f24 to
109875c
Compare
hdelaforce-payplug
left a comment
hdelaforce-payplug
left a comment
There was a problem hiding this comment.
Revue automatisée (bugs de correction + nettoyage) — voir les commentaires en ligne. Les points 1 à 3 (webhook remboursement) et 4 (verrous) méritent une attention particulière avant merge.
| * check-then-act on here (full refunds don't create one, mirroring process()'s legacy | ||
| * behavior), so the lock is the only guard available for this path. | ||
| */ | ||
| private function processHostedFields(PaymentInterface $payment): void |
There was a problem hiding this comment.
Simplification possible : processHostedFields() et processHostedFieldsWithAmount() (ligne 227) partagent ~50 lignes quasi identiques (acquisition/relâchement du verrou, bloc try/catch/UpdateHandlingException, log d'erreur "no operationIds", ajout à $details['refunds']). Un helper commun paramétré par la clé de verrou et les valeurs qui diffèrent réduirait la duplication et le risque de divergence future.
| { | ||
| $this->prepare($payment); | ||
|
|
||
| if (self::isHostedFields($payment)) { |
There was a problem hiding this comment.
Remarque d'architecture : le dispatch isHostedFields() est répété inline dans process(), processWithAmount() et prepare(), alors que le plugin a déjà un mécanisme dédié pour ce type de dispatch (DelegatesToHostedFieldsCommandProviderTrait, utilisé par les command providers Capture/Notify/Status). Centraliser ce check éviterait qu'il soit oublié pour un futur ajout (comme cela a failli arriver dans prepare()).
10c8280 to
16f998a
Compare
| * RefundHistory/refundId to check-then-act on here (full refunds don't create one, mirroring | ||
| * process()'s legacy behavior), so the lock is the only guard available for this path. | ||
| */ | ||
| private function processHostedFields(PaymentInterface $payment): void |
There was a problem hiding this comment.
Possible lost-update race between refund creation and webhook confirmation on Payment::details
This method reads $details = $payment->getDetails() before the (network) call to createRefund(), then writes it back afterwards from that same pre-call snapshot — no re-read before the write. Meanwhile HostedFieldsWebhookNotificationHandler::markMatchedRefundAsFailed() does its own independent read-modify-write of the same details['refunds'] array, and it runs unlocked (before applyLocked() is even called). The two paths also use disjoint lock namespaces (payplug_upc_refund_<paymentId> here vs payplug_upc_treat_<operationId> there), so they never mutually exclude each other.
Failure scenario: an admin triggers a full refund while an async webhook confirming an earlier refund's failure arrives around the same time (independent HTTP requests — plausible, since PayPlug can deliver a webhook within milliseconds of the synchronous API response).
processHostedFields()reads$details(nofailedflag yet on the earlier refund).- Concurrently, the webhook handler reads $details fresh, sets the earlier entry
failed => true, and callssetDetails(). processHostedFields()'screateRefund()call returns; it computessumRecordedRefunds()off its stale $details (still missing thefailedflag) and callssetDetails()with that stale array.- Whichever
setDetails()/flush happens last wins — thefailed => trueflag can be silently dropped.
Consequence: a refund PayPlug confirmed never actually completed goes back to counting as refunded money in sumRecordedRefunds(), so a later full refund under-refunds the customer by that amount (or, in the reverse ordering, a legitimate refund record is lost entirely).
Confidence: medium — depends on whether Sylius's Payment entity has Doctrine optimistic locking (@ORM\Version); if not (the Sylius core default), this is a silent lost-update, not just theoretical.
16f998a to
0815a5a
Compare
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try the Wiz Code extension for VS Code, JetBrains, or Visual Studio. |
3 participants
Description
Cette PR ajoute le remboursement pour les paiements Hosted Fields (UHF) côté back-office Sylius, avec confirmation asynchrone via webhook, et inclut l'adaptation nécessaire suite à la fusion de
payplug/unified-plugin-core's PRE-3589 surdevelop(qui embarque aussi le refactoring PRE-3590 "HF aliasing").Remboursement (synchrone + partiel/total) :
RefundPaymentProcessorroute les paiements Hosted Fields vers UPC'screateRefund()via un nouvel adaptateurUpc/UnifiedApiRefundCreator(implémentantRefundCreatorInterface), au lieu du SDK legacyorderIddu remboursement (numéro de commande, ou l'id du paiement à défaut) et lesubmerchantExternalId/accountId(compte/sous-marchand) sont résolus depuis la config du payment method spécifique au paiement remboursé (Upc/PaymentReferenceHelper::resolveGatewayCredentials()), avec un garde-fou local (LogicException) si l'un des deux n'est pas configuré — évite un aller-retour réseau pour un 400 dont le message ne remonte pas, et évite qu'un remboursement soit routé vers le mauvais compte si le marchand a plusieurs payment methods Hosted Fields configurésprocessHostedFields()) enregistre désormais aussi l'id d'opération du remboursement dansPayment::details['refunds']— nécessaire pour que le webhook de confirmation puisse retrouver le paiementmontant d'origine - somme des remboursements déjà confirmés), pas le montant total d'origine —UnifiedApiPaymentService::createRefund()rembourse le solde restant quand$amountest omis, pas le totalILock, clé = id du paiement) : les deux méthodes se bloquent mutuellement en cas de déclenchement concurrent, faute d'idempotency key côté API UPC pour ce fluxNotification webhook du remboursement :
HostedFieldsWebhookNotificationHandlerdistingue maintenant une confirmation de remboursement (via les ids qu'on a nous-mêmes enregistrés dansPayment::details['refunds']) d'une notification de paiement classique, et appliquePaymentOutcome::REFUNDEDau lieu dePAID— sans ce fix, une confirmation de remboursement aurait été mal interprétéeexecCodeindique un échec n'est jamais forcée enREFUNDED(l'argent n'a pas bougé) ni transmise telle quelle à la state machine du paiement (FAILEDy signifie "ce paiement a échoué", pas "ce remboursement a échoué") — seuls le log et le suivi d'idempotence sont appliqués ; l'entrée correspondante dansPayment::details['refunds']est marquéefailed => truepour ne plus compter dans un calcul de solde restant ultérieurAdaptation à PRE-3590 (UPC) :
payplug/unified-plugin-corepointe sur^1.1.0(version taguée incluant PRE-3589, publiée depuis) au lieu dedev-developUnifiedApiHostedPaymentServicea été supprimée par PRE-3590 (fusionnée dansUnifiedApiPaymentService::createPayment()) etHostedPaymentOutputrenommée enPaymentOutput—Upc/UnifiedApiHostedPaymentCreator,Upc/HostedPaymentCreatorInterface, etCaptureHostedPaymentRequestHandleradaptés en conséquenceHostedFieldDtoa un nouveau paramètre (recurringMode, pour l'aliasing PRE-3590) inséré avantbrowser/customerdans son constructeur — la construction dansCaptureHostedPaymentRequestHandlerest passée en arguments nommés pour éviter ce type de désalignement silencieux à l'avenirNettoyage : extraction d'un helper partagé
Upc/PaymentReferenceHelper(resolveOrderId(),idToString(),resolveGatewayCredentials()), qui remplace les copies précédemment dupliquées dansRefundPaymentProcessor,HostedFieldsWebhookNotificationHandler,CaptureHostedPaymentRequestHandleretUnifiedApiRefundCreator.Related Issue
Ticket: PRE-3552
Type of Change
✅ Quality Checklist
Local Environment & Hooks
make install).(PRE|SMP)-XXXX: descriptionpattern.(feature|fix|hotfix|refactor)/(PRE|SMP)-XXXX...or(release|patch)/x.y.z.Testing & Code Quality
plugin-dockerized-sylius).src/ortests/— n/a pour ce repo (cible PHP ^8.2), etpayplug/unified-plugin-coreest maintenant consommé via une version taguée stable plutôt qu'une branche de dev.CI/CD Deployment Context
compatibilitymatrix (PHP 7.1 / 7.4 / 8.0 / 8.1 / 8.2) and thequalityjob.Notes for Reviewer
resolveGatewayCredentials/resolveOrderId/idToString) désormais centralisée dansUpc/PaymentReferenceHelper. Le seul point non retenu (dispatchisHostedFields()répété) a été jugé peu pertinent : le traitDelegatesToHostedFieldsCommandProviderTraitexistant est conçu pour un autre usage (délégation entre command providers) et ne s'applique pas tel quel ici.payplug/unified-plugin-corepointe maintenant sur^1.1.0(version stable taguée) — plus besoin de suivredev-develop.plugin-dockerized-sylius:debug:autowiring/debug:container/cache:clearconfirment que le nouveau paramètreILockdeRefundPaymentProcessorse résout sans ambiguïté.CaptureHostedPaymentRequestHandler::buildHostedFieldDto()pour le changement d'arguments nommés — comportement identique, juste plus sûr face à un futur changement de signature deHostedFieldDto.