PRE-3670: Add authorization, capture and cancellation operation - #35
Conversation
75cbc25 to
745483b
Compare
adumont-payplug
left a comment
There was a problem hiding this comment.
Code review — capture/cancel/authorization lifecycle
Solid, well-tested work overall: capturePayment()/cancelPayment() genuinely reuse createRefund()'s pattern, file placement (AuthorizationType in DataValues/, CaptureOutput/CancellationOutput in Output/, 10 new exception marker classes) all match this repo's established conventions, and PHP 7.1 compatibility is intact throughout. Verified independently: make stan (level 8) clean, make cs-lint clean, make test 440/440 passing — no regression on the existing payment/refund/3DS flows.
Left 3 inline comments on specific findings (1 Important correctness risk in the error-classification keyword matching, 1 Important unverified assumption behind remainingCapturableAmount, 1 documentation-scope note on AC7/idempotency). Nothing blocking at the Critical level.
Acceptance criteria (PRE-3670), quick summary: 6/10 fully met (AC2, AC4, AC6, AC8, AC9, plus AC3 with a caveat noted inline), AC1/AC5/AC7 partially met (each is a reasonable, documented interpretation rather than a gap in effort — see inline comments), AC10 (changelog/version) is out of scope for this PR per the repo's existing tag-based release flow (no CHANGELOG.md exists in this repo).
Ready to merge with the two Important fixes below addressed (or at least explicitly acknowledged/tracked).
745483b to
629bf1e
Compare
jhoaraupp
left a comment
There was a problem hiding this comment.
Review — PRE-3670 capture / annulation / autorisation
Bonne base : capturePayment()/cancelPayment() reprennent bien le modèle de createRefund(), les fichiers sont au bon endroit (DataValues/, Output/, exceptions marqueurs), la compatibilité PHP 7.1 est respectée et les tests sont nombreux. La correction du premier retour (not capturable/not voidable testés avant already) est bien en place et testée.
Il me reste surtout deux sujets, détaillés en commentaires :
- Classification des erreurs (1 HIGH, 2 MEDIUM) : les mots-clés génériques (
expired,exceed,already+captur) passent avant le test execCode émetteur et matchent des tournures imprévues. Refus émetteur et cas ambigus sont donc mal classés (vérifié en rejouant la logique en local). - Montants restants (2 MEDIUM) :
remainingCapturableAmountignore l'autorisation partielle etremainingCancellableAmountignore les captures déjà faites. Ce sont des problèmes de formule, distincts du point sur le caractère cumulatif deamount.
Plus quelques points LOW/NIT (currency non validé quand un montant est fourni, execCodes en attente, @throws incomplets, champs d'autorisation acceptés avec capture=true).
Je recommande de traiter le HIGH et le point currency avant merge. Les autres peuvent faire l'objet d'un ticket de suivi.
…n gaps from review
Description
Expose l'autorisation, la capture et l'annulation dans UPC, pour compléter le cycle de vie du paiement déjà couvert (paiement, remboursement, 3DS).
Ajouts principaux
Ce qui n'est délibérément pas couvert par ce PR
IPaymentRepository/ILock(pas encore implémentés dans UPC), à charge du plugin CMS consommateur.Contracts/) pour capture/annulation : ces deux méthodes suivent le modèle à paramètres scalaires déjà établi parcreateRefund(), plutôt que d'introduire de nouvelles interfaces versionnées.CHANGELOG.md; le versioning se fait par tag Git au moment du cut d'une brancherelease/*, pas par PR de feature.Related Issue
Ticket: PRE-3670
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
make cs-fix).make stan— PHPStan level 8).make test).src/ortests/(no typed properties, arrowfunctions, constructor property promotion,
match,enum).CI/CD Deployment Context
compatibilitymatrix(PHP 7.1 / 7.4 / 8.0 / 8.1 / 8.2) and the
qualityjob.Notes for Reviewer
Revue complète postée en tant que PR review avec commentaires en ligne. Résumé :
make stan/make cs-lint/make test(440/440) passent tous sans régression sur les flux paiement/remboursement/3DS existants.UnifiedApiPaymentService::throwForMessageKeyword()— le test de mot-clé"already"+"captur"peut matcher par erreur à l'intérieur du mot "capturable" (pas seulement "captured"), risquant une exception mal classifiée sur un message du type "already voided and therefore not capturable".remainingCapturable/CancellableAmountdeCaptureOutput/CancellationOutputsuppose que le champ"amount"de l'API est cumulatif à travers des captures partielles successives — hypothèse non encore vérifiée sur une vraie réponse de succès (seul le chemin de rejet est testé en intégration).Mise à jour (490cd03)
Suite à la revue de @Jhoarrau, plusieurs corrections ont été apportées :
assertOperationSuccess()/throwForMessageKeyword()a été revu — le mot-clé "duplicate" et l'execCode "4XXX" (refus émetteur) sont désormais vérifiés avant tout autre mot-clé générique (évitant qu'un refus émetteur contenant "expired"/"exceed" soit mal classifié), et "already captured"/"already cancelled" matchent une phrase contiguë (regex) plutôt que deux sous-chaînes indépendamment co-occurrentes.currencyest désormais validé localement (InvalidCaptureRequestException/InvalidCancellationRequestException) dès qu'unamountpartiel est fourni àcapturePayment()/cancelPayment(), au lieu de remonter un 400 opaque de l'API.authorizationType/partialAuthorizationsont désormais rejetés parCommonFieldsDtoValidatorquandcapturevauttrue.@throwsdecapturePayment()/cancelPayment()couvrent maintenant les exceptions "croisées" (ex.cancelPayment()peut leverPaymentAlreadyCapturedException).remainingCapturable/CancellableAmount(cumulatif, et cohérence sous autorisation partielle / captures antérieures) reste non vérifiée contre une vraie réponse de succès — documentée explicitement dans le code et CLAUDE.md plutôt que corrigée. Idem pour l'ambiguïté dePartialCancellationNotAllowedException(contrat vs. montant invalide) et le cas non observé des execCodes "en attente" (0002/0003) sur une réponse 2xx.Toutes les discussions ouvertes par @Jhoarrau ont reçu une réponse indiquant leur résolution (ou, pour les deux points ci-dessus, pourquoi ils restent documentés plutôt que corrigés).