Feature/pre 3623 set uhf front - #120
hdelaforce-payplug merged 1 commit into
Conversation
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try the Wiz Code extension for VS Code, JetBrains, or Visual Studio. |
3e6f91e to
fc04749
Compare
8ee677c to
9b5b0ff
Compare
jhoaraupp
left a comment
There was a problem hiding this comment.
Revue automatisée — 10 points remontés en commentaires inline (voir onglet Files changed). Résumé : 1 point process/sécurité sur le fichier CLAUDE.md ajouté (affirmation vérifiée fausse), 1 High, 4 Medium, 2 Low, 2 Nit.
Un point additionnel n'a pas pu être ancré en inline (GitHub n'autorise pas de commentaire de review sur un fichier vide sans hunk) :
[LOW] Fichier de test vide ajouté — tests/utilities/services/Routes/getSourceUrlTest.php est ajouté vide (0 octet). Oubli d'ajout de contenu pour un test de getSourceUrl()/getHostedFieldsUrl() ?
9b5b0ff to
8a51c31
Compare
adumont-payplug
left a comment
There was a problem hiding this comment.
Revue : UHF front PrestaShop vs. implémentation Sylius + doc technique
Revue croisée avec la PR Sylius payplug/SyliusPayPlugPlugin#320 et la page [TECHNICAL] Unify Hosted Fields Implementation.
Revue faite sur 8a51c315.
Deux bonnes nouvelles d'abord
La note « Known issue » de CLAUDE.md sur la réintroduction de bugs est périmée : OrderAction::createAction(), ValidationAction et PayPlugNotifications::getMissingResourceStatusCode() sont aujourd'hui strictement identiques à origin/master sur cette branche, et auto-tag-rc.yml aussi. Les correctifs PRE-3557 et PRE-3580 sont bien là. Le paragraphe est à supprimer de CLAUDE.md.
Et deux choix de conception sont meilleurs que la référence Sylius : la map d'identifiants par devise ({"usd": …, "gbp": …}) là où Sylius n'a qu'un hfIdentifier unique, et le garde anti-double-soumission avec générations d'essai, que Sylius n'a pas du tout. Dommage que le premier soit transmis au mauvais endroit et que l'UX de validation par champ soit inerte (cf. commentaires) — les deux sont à deux doigts d'être de vrais apports.
État vis-à-vis de PRE-3623
| # | Attendu | État |
|---|---|---|
| 1 | PaymentOption visible aux côtés de l'option Retail API |
❌ Écart assumé |
| 2 | Champs hébergés dans un template Smarty dédié | |
| 3 | Tokenisation opérationnelle (hfToken + selectedBrand) | |
| 4 | Erreurs de tokenisation avec message explicite au client | ❌ Non rempli |
| 5 | Case « enregistrer ma carte » affichée, valeur transmise | |
| 6 | hfToken + selectedBrand + save card transmis au contrôleur UHF | ✅ Rempli |
| 7 | Flux Retail API existant non modifié | ❌ Écart assumé |
| 8 | Détection selectedBrand ; marque non CB/Visa/MC → message explicite |
Un seul critère est rempli sans réserve. Rien d'irrattrapable : les deux « non remplis » se règlent avec un changement de variable et quatre entrées de traduction. Les deux écarts relèvent en revanche d'une décision produit — commentée sur PRE-3623.
Points non rattachables à une ligne du diff
1. module_files.csv n'a pas été mis à jour. controllers/front/uhf.php, views/templates/hook/checkout/payment/hosted_fields.tpl et dev/css/less/component/payplug/embedded.less en sont absents, et l'ancien dev/css/less/component/payplug/integratedPayment.less (renommé) y figure toujours l.72. FilesHelper::clean() supprime les fichiers non listés sur les boutiques en production — le contrôleur et le template disparaîtraient à la mise à jour.
2. tests/utilities/services/Routes/getSourceUrlTest.php est committé vide (0 octet). Soit on l'écrit, soit on le retire. La couverture réelle de getHostedFieldsUrl() (lecture du .env, fallback) n'existe pas.
3. tests/.phpunit.result.cache est listé dans .gitignore (l.28) mais reste suivi et modifié par la PR. Un git rm --cached tests/.phpunit.result.cache réglerait le va-et-vient.
4. payplug/unified-plugin-core est en ^0.0.7 alors que Sylius requiert ^1.1.0. La 0.0.7 installée ne contient que Auth/ (OAuth2Client, TokenManager), 7 contrats et 4 modèles — ni HostedFieldDto, ni ExecCodeMapper, ni client Unified API paiement/remboursement. Par ailleurs aucun des 7 contrats n'a d'implémentation dans ce module (UPC n'y sert que pour PhoneHelper / AmountHelper), et l'authentification est toujours sur Payplug\Authentication + Merchant::generateJWT() alors que Sylius est passé en OAuth2/PKCE via UPC (PRE-3563). Ce n'est pas le périmètre de cette PR, mais c'est un prérequis du ticket de suivi qu'il vaut mieux chiffrer maintenant.
5. Une remarque sur la forme des tests. La suite passe, mais elle valide les affectations Smarty, pas le DOM rendu — c'est exactement pourquoi le bug de sélecteur et les clés de traduction manquantes ont survécu à la revue annoncée dans la description. Un test qui vérifie que les noms de classes du .tpl et les sélecteurs du sous-module JS concordent attraperait toute cette famille de régressions.
Sur le périmètre
Le report de la création du paiement au ticket suivant est clair et assumé. Une conséquence mérite d'être notée dès maintenant : le JS ne sait traiter qu'une seule forme de réponse, resp.return_url → window.location.href. Or la doc §4.2 dit de celle-ci qu'elle « n'arrive que si displayMode=raw — jamais envoyé par ce plugin ». La forme réellement attendue pour un paiement 3DS est redirectHtml, un formulaire auto-soumis à écrire dans la page (CaptureHttpResponseProvider côté Sylius). Le seul chemin de succès implémenté est donc celui qui n'arrivera pas.
Je demande des changements sur les points bloquants (sélecteur JS, clés de traduction, companyId, module_files.csv) ; le reste peut se discuter.
8a51c31 to
3a829f6
Compare
jhoaraupp
left a comment
There was a problem hiding this comment.
Review complète #2 — après prise en compte des retours
J'ai vérifié un par un les 22 points remontés lors du premier passage (10 par moi, 12 par @adumont-payplug) directement sur le code actuel de la branche. Détail dans les réponses postées sur chaque fil concerné.
✅ Résolu et vérifié en code (18 fils, réponse + resolve postés)
- Fichier
CLAUDE.md: affirmation fausse retirée. - Traductions
payment.error/payment.error.unsupported_brandajoutées (toutes locales). - Traductions
es/ptcomplétées (plus de clé brute affichée) — reste un point mineur non bloquant : plusieurs valeurs es/pt reprennent le texte anglais par défaut plutôt qu'une vraie traduction (hosted_fields_59cfdfd0..., plusieursMandatory field., etc.). uhf.php: vérification d'appartenance du panier ($id_cart !== $this->context->cart->id) + vérificationfeature_hosted_fields/isHostedFieldsIdentifierConfigured()ajoutées.dev/js/front.js:props.rootcorrigé (__moduleName__HostedFieldsau lieu de__moduleName__IntegratedPayment) — l'UI d'erreur/validation cible maintenant le bon DOM.dev/js/front.js: montage des iframes désormais différé jusqu'à sélection réelle de l'option de paiement (mêmes principes que Sylius).dev/js/front.js:cardholderNametransmis au serveur et loggé côtéuhf.php(n'est plus jeté).dev/js/front.js: commentaire trompeur suridentifiercorrigé ; variablerootinutilisée et propselectedBrandmorte supprimées.Routes::getHostedFieldsUrl(): ne devine plus d'URL de prod — renvoie une chaîne vide + log siHOSTED_FIELDS_URLn'est pas configuré ; le template gère aussi l'échec de chargement du script côté JS.tests/.phpunit.result.cache: supprimé du diff.CurrencyAdapter::hasOtherCurrency(): docblock corrigé.
🟡 Laissé ouvert — décisions produit actées, pas des bugs de code (3 fils)
- Architecture
PaymentOption(remplacestandardplutôt qu'additionnelle) et absence d'opt-in BO explicite (routage par devise plutôt que parembedded_mode) : @hdelaforce-payplug a confirmé que ce sont des choix assumés, validés en amont avec le PO. Je laisse la clôture finale de ces deux fils aux personnes concernées.
🟠 Laissé ouvert — point technique non tranchable depuis le code (1 fil)
- Source de
companyId(Configuration::oauth_company_idvscompany_refviagetAccount()) : désaccord technique non résolu par un changement de code ; je n'ai pas accès à la doc technique UHF citée pour trancher. À confirmer côté plateforme avant merge, vu le risque décrit (champs qui ne s'affichent pas sicompanyIdest vide).
🟠 Partiellement traité (1 fil, laissé ouvert)
- Identifiant par devise /
localenon transmis au SDK JS (dev/js/front.js, ligne ~1727) : le commentaire trompeur est corrigé, mais les deux points de fond restent : (1) pas de chemin vers le payload serveur pour l'identifiant par devise — a priori dans le périmètre du ticket de suivi puisqueuhf.phpne crée toujours pas de ressource de paiement ; (2)localetoujours absent de l'initdalenys.hostedFields(), donc les textes internes du SDK resteront dans sa langue par défaut pour un client non-FR (mineur, non bloquant).
❌ Non corrigé
tests/utilities/services/Routes/getSourceUrlTest.phpreste un fichier vide (0 octet). Ce point n'a pas pu être ancré en commentaire inline la première fois (pas de hunk sur un fichier vide) et semble être passé sous le radar — toujours à traiter (contenu de test manquant pourgetSourceUrl()/getHostedFieldsUrl(), ou fichier à supprimer si non voulu).
Bilan : très bon taux de correction (18/22 points objectifs corrigés et vérifiés), y compris plusieurs points bloquants réels (mauvais ciblage DOM, iframes en conteneur masqué, nom du porteur jeté, URL de prod inventée). Les points restants sont soit des désaccords/décisions produit hors du champ d'un correctif de code, soit un oubli mineur (fichier de test vide).
3a829f6 to
63ecacc
Compare
63ecacc to
0abcfd2
Compare
1d5ea5f
into
feature/PRE-3441-add-unified-hosted-fields
Description
Ajoute les champs hébergés UHF (Unified Hosted Fields) au checkout : pour un panier dont la devise n'est pas l'EUR, l'option de paiement
standard(Retail API) s'affiche en mode Hosted Fields au lieu du modeintegrated— exactement comme le modeintegratedexistant est sélectionné pour les paniers EUR via le réglage BOembedded_mode.Architecture
PrestashopAdapter17::displayPaymentOption()route désormais entre deux modes de rendu de l'optionstandard, selon la devise du panier :setIntegratedPaymentOption()(existant, désormais explicitement restreint à l'EUR — un panier non-EUR récupérait auparavant ce mode alors qu'il n'a jamais été prévu/testé pour ça) etsetHostedFieldsPaymentOption()(nouveau, miroir du premier), tous deux réutilisant un lien confidentialité et des traductions de placeholders partagés via deux méthodes privées communes.views/templates/hook/checkout/payment/hosted_fields.tpl: champs hébergés (carte/expiration/CVV via le SDK JS Payplug), nom du porteur, case "enregistrer ma carte".hosted_fieldsdansdev/js/front.js: initialisation du SDKdalenys.hostedFields, validation par champ (messages distincts "champ obligatoire" / "format invalide"), tokenisation, rejet des marques non supportées (CB/Visa/Mastercard uniquement — liste centralisée dansPrestashopAdapter17::HOSTED_FIELDS_ACCEPTED_BRANDS, partagée avec le contrôleur), garde anti-double-soumission avec identifiant de tentative + timeout de sécurité.controllers/front/uhf.php: reçoithfToken/selectedBrand/save_card/id_cart, revalide côté serveur (marque supportée,id_cartprésent et valide, panier pas déjà transformé en commande), accuse réception. La création de la ressource de paiement est hors périmètre, reportée au ticket suivant ("Branchement paiement / 3DS sur UPC").Routes::getHostedFieldsUrl(): URL du SDK JS, même principe quegetOneyLoaderUrl()(variable d'envHOSTED_FIELDS_URL, fallback codé en dur).StandardPaymentMethod) quand la boutique a plusieurs devises actives (EUR + autre) : explique que le mode "Intégré" ne s'applique désormais qu'aux commandes EUR.integratedPayment.less→embedded.less(stylise désormaisintegratedethosted_fields) + styles propres aux champs hébergés.Revue de code
Une revue complète a été menée sur l'ensemble du diff (scan ligne par ligne, comportements supprimés, traçage inter-fichiers, réutilisation/duplication, efficacité, altitude architecturale, conventions). Les 10 points relevés ont été corrigés et vérifiés :
companyIdcodé en dur au lieu de la valeur serveuruhf.phpacceptant unid_cartmanquant/invalidegetPaylaterSectionTestgetAvailablePaymentMethod()— test restauréDeux fichiers annexes (
composer.json,.github/workflows/auto-tag-rc.yml) présentaient un léger écart avecmaster(formatage parasite / retour à la ligne manquant) — vérifiés et corrigés.État actuel : suite complète verte (2717 tests, 0 échec), lint PHP/JS propre sur tous les fichiers modifiés.
Related Issue
Ticket: PRE-3623
Type of Change
[x] ✨ New feature
Notes for Reviewer
Le changement de comportement sur
integrated(désormais restreint aux paniers EUR) est intentionnel — voir le commentaire dansPrestashopAdapter17::displayPaymentOption().