Presteamshop v2 #74
Presteamshop v2 #74
Conversation
- Se ajustan los primeros cambios, falta aún probar bien que la migración quedara bien y posterior, seguir cambiando y modificando código para dejar todo más limpio y entendible.
- Se crean nuevas clases donde se van a manejar las tablas y migrar el codigo que se tenia de los antiguos Helpers, y asi tener más control y claridad de lo que se lee y guarda.
- Se crea fichero para iniciar con la construccion de las pruebas unitarias del codigo reestructurado de la creacion/actualizacion de pedido. Se usara Mockery. - Se crean las entidades de doctrine que se usarán para manipular las tablas, que tambien pueden ser reestructuradas.
- Se migra a servicio el crear payment en monei. - Se acomoda la entidad de MoPayment para guardar los payments. - Se elimina PsCurl, ya con el SDK se maneja todo. - Se pasa logica del modulo directo al SRC sin el PsMonei. - Se pausa Tests. -
- se modifican entidades para manejo en cascada, falta seguir modificando. - se empieza hacer parte de reestructura del backoffice.
- se termina de acomodar bien las entidades. - se deja funcionando el historial de estados y los refund en el detalle del pedido del admin. - falta empezar a depurar ficheros y cambiar nombres de tablas de mo a monei2
- Se deja funcional el módulo con el cambio de estructura. Aún falta mejorar código, optimizar funciones, y hacer más pruebas. - Se eliminan fichero sin usar.
- se valida que este logueado y la opcion de token de tarjeta activa para poder mostrar el check de guardar tarjeta. - se implementa el poder guardar una cuenta de test y producción a la vez.
- se hace el proceso de migrar los datos de las tablas antiguas a las nuevas, conservando las tablas antiguas por si sucede algún error. - se cambia el nombre de las tablas de mo a monei2.
- se quita de la tabla customer_cards las propiedades threeds y threeds_version que no son necesarios. - se migra la logica de obtener los pagos al servicio PaymentOptionService. - pediente de hacer flag para saber si un payment ya tiene pedido.
- se agrega tabla "monei2_order_payment" para guardar un flag del payment y el order, pero solo se guarda como log, porque no me sirve para detener logica, pues tendria que cambiar mucho.
- Add customer login and guest status checks before retrieving cards - Ensure customer object is properly loaded before processing payment option
- min version php and phpunit changed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/Service/Payment/PaymentOptionService.php (4)
40-64: Initialize$paymentOptionsto prevent null return value.The method declares a return type of
?arraybut the contract suggests it should always return an array. The$paymentOptionsproperty is never initialized, so if no payment options are added by the individual methods, the method will returnnullinstead of an empty array.Apply this fix to ensure consistent return type:
public function getPaymentOptions(): ?array { $this->paymentMethodsAllowed = $this->moneiService->getPaymentMethodsAllowed(); + $this->paymentOptions = []; // ensure non-null
79-103: Fix parameter count mismatch and logic error in payment method validation.The method has two critical issues:
- Parameter mismatch: Declared with one parameter but called with two arguments (lines 187, 198), causing fatal "Too many arguments" error
- Logic error:
isset($this->paymentMethodsAllowed)is alwaystrueafter line 42 assignment, even for empty arraysApply this fix:
-private function isPaymentMethodAllowed($paymentMethod) +private function isPaymentMethodAllowed(string $paymentMethod): bool { if ($this->currencyIsoCode !== 'EUR') { return false; } - if (!isset($this->paymentMethodsAllowed)) { + if (empty($this->paymentMethodsAllowed)) { return true; } if (!in_array($paymentMethod, $this->paymentMethodsAllowed, true)) { return false; } return true; }
185-193: Remove extra argument from Apple Pay payment method check.After fixing the method signature above, this call must be updated to use only one argument. Move the browser check outside the helper method.
-if ($this->configuration->get('MONEI_ALLOW_APPLE') && $this->isPaymentMethodAllowed('applePay', $this->isSafariBrowser())) { +if ($this->configuration->get('MONEI_ALLOW_APPLE') && $this->isPaymentMethodAllowed('applePay') && $this->isSafariBrowser()) {
196-204: Remove extra argument from Google Pay payment method check.Similar to Apple Pay, this call passes an unsupported second argument that will cause a fatal error.
-if ($this->configuration->get('MONEI_ALLOW_GOOGLE') && $this->isPaymentMethodAllowed('googlePay', !$this->isSafariBrowser())) { +if ($this->configuration->get('MONEI_ALLOW_GOOGLE') && $this->isPaymentMethodAllowed('googlePay') && !$this->isSafariBrowser()) {
🧹 Nitpick comments (2)
src/Service/Payment/PaymentOptionService.php (1)
105-138: Consider extracting redirect URL generation logic.The card payment option method handles both redirect and onsite payment flows with significant conditional complexity. The redirect URL generation could be extracted into a helper method to improve readability.
Consider extracting the redirect URL logic:
+private function buildRedirectUrl(string $method, array $extraParams = []): string +{ + $params = array_merge([ + 'method' => $method, + 'transaction_id' => $this->getTransactionId(), + ], $extraParams); + + return $this->context->link->getModuleLink('monei', 'redirect', $params); +} private function getCardPaymentOption() { // ... existing code ... if ($this->configuration->get('MONEI_CARD_WITH_REDIRECT')) { if ($this->configuration->get('MONEI_TOKENIZE')) { - $redirectUrl = $this->context->link->getModuleLink('monei', 'redirect', [ - 'method' => 'card', - 'transaction_id' => $this->getTransactionId(), - ]); + $redirectUrl = $this->buildRedirectUrl('card'); $smarty->assign('link_create_payment', $redirectUrl);src/Service/Monei/MoneiService.php (1)
282-390: Consider breaking down the complex payment creation method.The
createMoneiPaymentmethod is quite lengthy (108 lines) and handles multiple responsibilities: validation, data preparation, request building, API call, and persistence. Consider extracting some logic into helper methods for better maintainability.Consider extracting these operations into separate methods:
- Payment request validation and preparation
- Customer and address data setup
- Payment token handling logic
+private function validatePaymentCreationInputs(Cart $cart, Currency $currency, Customer $customer): void +{ + if (!$cart) { + throw new MoneiException('The cart could not be loaded correctly'); + } + if (!Validate::isLoadedObject($currency)) { + throw new MoneiException('The currency could not be loaded correctly', MoneiException::CURRENCY_NOT_FOUND); + } + if (!Validate::isLoadedObject($customer)) { + throw new MoneiException('The customer could not be loaded correctly', MoneiException::CUSTOMER_NOT_FOUND); + } +} +private function setupPaymentToken(CreatePaymentRequest $request, bool $tokenizeCard, int $cardTokenId, int $customerId): void +{ + if ($tokenizeCard) { + $request->setGeneratePaymentToken(true); + } elseif ($cardTokenId) { + $monei2CustomerCard = $this->moneiCustomerCardRepository->findOneBy([ + 'id' => $cardTokenId, + 'id_customer' => $customerId, + ]); + + if ($monei2CustomerCard) { + $request->setPaymentToken($monei2CustomerCard->getTokenized()); + $request->setGeneratePaymentToken(false); + } + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (12)
views/img/payments/amex.svgis excluded by!**/*.svgviews/img/payments/apple-pay.svgis excluded by!**/*.svgviews/img/payments/bancontact.svgis excluded by!**/*.svgviews/img/payments/bizum.svgis excluded by!**/*.svgviews/img/payments/cards.svgis excluded by!**/*.svgviews/img/payments/giropay.svgis excluded by!**/*.svgviews/img/payments/google-pay.svgis excluded by!**/*.svgviews/img/payments/klarna.svgis excluded by!**/*.svgviews/img/payments/mbway.svgis excluded by!**/*.svgviews/img/payments/multibanco.svgis excluded by!**/*.svgviews/img/payments/paypal.svgis excluded by!**/*.svgviews/img/payments/sofort.svgis excluded by!**/*.svg
📒 Files selected for processing (4)
src/Service/Monei/MoneiService.php(1 hunks)src/Service/Payment/PaymentOptionService.php(1 hunks)views/templates/front/additional_info.tpl(1 hunks)views/templates/hook/displayPaymentByBinaries.tpl(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- views/templates/front/additional_info.tpl
🚧 Files skipped from review as they are similar to previous changes (1)
- views/templates/hook/displayPaymentByBinaries.tpl
🔇 Additional comments (2)
src/Service/Monei/MoneiService.php (2)
54-67: LGTM! Good error handling for client initialization.The method properly validates API key availability and throws appropriate exceptions. The conditional logic for production vs test mode is clear and correctly implemented.
208-217: LGTM! The findBy issue has been correctly addressed.The method now properly uses
findBy()to retrieve multiple refund entities and correctly iterates over them to calculate the total. This addresses the previous issue wherefindOneBy()was incorrectly used.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/Service/Monei/MoneiService.php (1)
246-247: ISO 8601 date strings need conversion to timestamps.The
setDateAdd()andsetDateUpd()methods expect integer timestamps, butgetCreatedAt()andgetUpdatedAt()return ISO 8601 date strings. This type mismatch needs to be resolved.Apply this fix:
- $monei2PaymentEntity->setDateAdd($moneiPayment->getCreatedAt()); - $monei2PaymentEntity->setDateUpd($moneiPayment->getUpdatedAt()); + $monei2PaymentEntity->setDateAdd(strtotime($moneiPayment->getCreatedAt())); + $monei2PaymentEntity->setDateUpd(strtotime($moneiPayment->getUpdatedAt()));
🧹 Nitpick comments (1)
src/Service/Monei/MoneiService.php (1)
394-401: Consider improving error handling approach.Returning
falseon exceptions may mask important error details from the caller. Consider either letting exceptions bubble up or returning a more informative error object.Alternative approach:
} catch (Exception $ex) { PrestaShopLogger::addLog( 'MONEI - Exception - MoneiService.php - createMoneiPayment: ' . $ex->getMessage() . ' - ' . $ex->getFile(), PrestaShopLogger::LOG_SEVERITY_LEVEL_ERROR ); - return false; + throw new MoneiException('Payment creation failed: ' . $ex->getMessage(), MoneiException::PAYMENT_REQUEST_NOT_VALID, $ex); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Exception/MoneiException.php(1 hunks)src/Service/Monei/MoneiService.php(1 hunks)views/css/front/checkout_page.css(1 hunks)views/templates/front/onsite_card.tpl(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- views/css/front/checkout_page.css
🚧 Files skipped from review as they are similar to previous changes (1)
- views/templates/front/onsite_card.tpl
🔇 Additional comments (6)
src/Exception/MoneiException.php (1)
6-26: Well-designed exception class with comprehensive error codes.The
MoneiExceptionclass provides a clean, organized approach to error handling with meaningful constant names and sequential numbering. The error codes cover all the major error scenarios for the Monei payment integration.src/Service/Monei/MoneiService.php (5)
42-67: Solid dependency injection and client initialization.The constructor properly injects required dependencies, and
getMoneiClient()correctly handles production/test mode switching with proper validation.
69-92: Excellent error handling with graceful degradation.The method properly validates account ID, includes comprehensive error handling, and returns an empty array on failure rather than breaking the flow.
216-229: Correctly fixed the collection iteration issue.The method now properly uses
findOneBy()to get a single payment entity and then callsgetRefundList()on that entity to iterate through associated refunds. This resolves the previous issue wherefindOneBy()was incorrectly treated as returning an array.
116-135: Excellent improvements addressing previous review feedback.The method now includes proper validation for empty cart details and currency ID, uses clearer variable naming (
$precisionMultiplier), and throws appropriate exceptions with specific error codes.
404-438: Comprehensive refund validation successfully implemented.The method now includes all the suggested improvements: amount validation, refundable balance checking, and proper error handling with meaningful exception messages and codes.
|
@coderabbitai full review |
There was a problem hiding this comment.
Pull Request Overview
This PR upgrades the Monei PrestaShop module to v2 by fully embracing a service-driven architecture, streamlining controllers, and adding testing/configuration support.
- Added PHPUnit configuration and PSR-4 autoload setup for testing and code quality
- Refactored front and admin controllers to use centralized services (
service.monei,service.order) - Introduced comprehensive DI service and hook YAML files for module wiring
Reviewed Changes
Copilot reviewed 124 out of 124 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| sql/index.php | Removed an extraneous blank line |
| phpunit.xml | Added PHPUnit configuration with strict coverage rules |
| files/index.php | Removed an extraneous blank line |
| controllers/front/validation.php | Switched JSON parsing to json_decode and updated exception logging |
| controllers/front/redirect.php | Simplified redirect logic and moved payment creation into service |
| controllers/front/customerCards.php | New controller for listing and deleting saved customer cards |
| controllers/front/createPayment.php | Refactored payment creation to use DI service |
| controllers/front/confirmation.php | Updated confirmation flow to call the order service |
| controllers/front/cards.php | Removed legacy cards controller |
| controllers/admin/AdminMoneiController.php | Refactored refund AJAX action to service calls with JSON utility |
| config/hooks.yml | Introduced global hooks service defaults |
| config/front/services.yml | Added front-end service container imports |
| config/front/hooks.yml | Defined front-end hook defaults |
| config/common.yml | Declared core services, repositories, and service definitions |
| config/admin/services.yml | Added admin-side service container imports |
| config/admin/hooks.yml | Defined admin-side hook defaults |
| composer.json | Bumped module to type “prestashop-module”, updated autoload, deps |
| build/package.json | Bumped npm package version to 2.0.0 |
| .php-cs-fixer.php | Added PHP-CS-Fixer configuration |
Comments suppressed due to low confidence (3)
controllers/front/redirect.php:4
- The import path for
MoneiException(PsMonei\Exception\MoneiException) differs from other controllers (e.g.,use PsMonei\MoneiException). Normalize the namespace across files to avoid class resolution issues.
use PsMonei\Exception\MoneiException;
controllers/admin/AdminMoneiController.php:13
- The new
ajaxRenderJsonmethod anddisplayAjaxRefundaction lack dedicated unit or integration tests. Add tests to cover both successful and failure JSON responses.
protected function ajaxRenderJson($content)
config/front/services.yml:4
- [nitpick] Importing two
hooks.ymlfiles (global and front-specific) with very similar names can be confusing. Consider renaming or consolidating hook config files for clarity.
- - { resource: hooks.yml }
There was a problem hiding this comment.
Actionable comments posted: 19
♻️ Duplicate comments (39)
phpunit.xml (1)
1-3: Updated PHPUnit XML schema to version 9.5.
The schema location now correctly referenceshttps://schema.phpunit.de/9.5/phpunit.xsd, matching PHPUnit 9.5.config/front/services.yml (1)
3-4: Remove redundant hooks.yml import.This appears to be the same issue mentioned in previous reviews. The file imports hooks configuration from both a relative path (
../hooks.yml) and a local path (hooks.yml), which could cause redundancy and confusion if both files exist.Please verify the file structure to determine if both hooks.yml files exist and contain different configurations:
#!/bin/bash # Description: Check for existence and differences between hooks.yml files # Check if both hooks.yml files exist echo "Checking for hooks.yml files:" fd -t f "hooks.yml" config/ # If both exist, compare their contents if [ -f "config/hooks.yml" ] && [ -f "config/front/hooks.yml" ]; then echo "Both files exist. Comparing contents:" diff config/hooks.yml config/front/hooks.yml || echo "Files are different" elif [ -f "config/hooks.yml" ] && [ ! -f "config/front/hooks.yml" ]; then echo "Only config/hooks.yml exists - line 4 import is redundant" elif [ ! -f "config/hooks.yml" ] && [ -f "config/front/hooks.yml" ]; then echo "Only config/front/hooks.yml exists - line 3 import will fail" else echo "Neither hooks.yml file exists" ficonfig/admin/services.yml (1)
3-4: Remove redundant hooks.yml import.Same redundancy issue as in the front services configuration. The file imports hooks configuration from both a relative path (
../hooks.yml) and a local path (hooks.yml).This has the same pattern as
config/front/services.yml. Please verify if both import paths are necessary or if one should be removed to avoid configuration conflicts.composer.json (1)
25-25: Invalid version constraint for monei/monei-php-sdk.The version constraint ^2.6 for monei/monei-php-sdk will not resolve as the latest available version is v0.1.10.
src/Repository/MoneiHistoryRepository.php (1)
8-47: Extract common save/remove logic into a shared base or trait.There are four repository classes duplicating the same save/remove methods with identical exception handling. This issue was already identified in previous reviews and should be addressed by creating an
AbstractMoneiRepositorybase class.src/Repository/MoneiCustomerCardRepository.php (3)
4-8: PreferServiceEntityRepositoryover legacyEntityRepository.
23-25: Preserve exception chain when re-throwing.
41-43: Preserve exception chain when re-throwing.Apply the same exception chaining pattern to the
remove()method as suggested for thesave()method.-} catch (ORMException $e) { - throw new MoneiException('Error removing monei customer card: ' . $e->getMessage()); +} catch (ORMException $e) { + throw new MoneiException( + 'Error removing Monei customer card: ' . $e->getMessage(), + 0, + $e + ); }src/Repository/MoneiPaymentRepository.php (1)
25-27: Preserve exception chain when re-throwing.views/templates/hook/displayAdminOrder.tpl (1)
141-144: Fix table column ordering issue in the refunds table.There's an issue with the table columns in the refunds section. The "Live" column's closing tag appears before the "Employee" column data, causing potential display issues. The "Employee" data and the "Live" header are misaligned.
- <td>{if isset($paymentRefund.paymentHistory.responseDecoded.livemode) and $paymentRefund.paymentHistory.responseDecoded.livemode}Yes{else}No{/if} - <td>{$paymentRefund.id_employee|escape:'html':'UTF-8'}</td> + <td>{if isset($paymentRefund.paymentHistory.responseDecoded.livemode) and $paymentRefund.paymentHistory.responseDecoded.livemode}Yes{else}No{/if}</td> + <td>{$paymentRefund.id_employee|escape:'html':'UTF-8'}</td>controllers/admin/AdminMoneiController.php (2)
13-17: Header must be sent before body
header('Content-Type: application/json');must be called before any output.
$this->ajaxRender()immediately echoes and terminates — therefore the header should be set insideajaxRender()or you should callheader()first and thenajaxRender.
42-47: HTTP status code is always 200 – JS cannot distinguish errorBecause the catch block still returns 200, the frontend
$.ajax({ … success: … })branch is triggered even on failure.Either:
} catch (Exception $e) { + http_response_code($e->getCode() ?: 500); $this->ajaxRenderJson([ 'code' => $e->getCode(), 'message' => $e->getMessage(), ]);or keep 200 but add
success: json.code === 200logic in JS.
Current mismatch causes false success pop-ups.upgrade/upgrade-2.0.0.php (2)
14-21: Risk of data loss during migrationThe current implementation truncates tables before migration, which can lead to permanent data loss if the upgrade fails or is interrupted.
54-62: Double json_decode is fragile and may corrupt dataviews/js/admin/admin.js (3)
106-131: Add error handling for AJAX request
137-138: Add additional checks for required global variables
149-156: Add error handling for JSON parsingcontrollers/front/redirect.php (2)
58-60: Avoid leaking internal error messages to the customer-visible cookie
51-56: Guard against null when chaining getNextAction()src/Entity/Monei2Refund.php (3)
20-20:inversedBy="refund"is likely incorrect - should be pluralThe inverse side in
Monei2Paymentlikely contains a collection of refunds (OneToMany), not a single refund. This will cause a Doctrine mapping exception.
110-113: Division by null will cause TypeErrorWhen
$this->amountisnulland$inDecimalistrue, the division operation will fail. Add null check before division.
127-130: Missing null check will cause fatal error
$this->date_add->format()will crash whendate_addis null. Add null check before calling format().src/Entity/Monei2CustomerCard.php (1)
13-16:@ORM\GeneratedValueis invalid for string columnsDoctrine's auto-generation strategies only work with integer or GUID types. Remove the annotation for manual ID assignment.
src/Entity/Monei2History.php (4)
41-42: Use TEXT type for potentially large JSON dataVARCHAR(4000) can cause silent truncation of UTF-8 data. Use TEXT type for unbounded JSON storage.
119-122: Missing null check will cause fatal error
getDateAddFormatted()callsformat()without checking ifdate_addis null, which will crash.
66-71: Bidirectional relation not synchronizedThe
setPayment()method should keep both sides of the relationship in sync by calling the inverse side's add method.Consider adding:
public function setPayment(Monei2Payment $payment) { $this->payment = $payment; + // Keep bidirectional relation in sync + // $payment->addHistory($this); return $this; }
136-144: Include primary key in array representationsBoth
toArray()andtoArrayLegacy()omit the primary keyid, which is often needed for admin grids and API responses.Add the ID field:
public function toArray() { return [ + 'id' => $this->getId(), 'status' => $this->getStatus(), 'statusCode' => $this->getStatusCode(), 'response' => $this->getResponse(), 'dateAdd' => $this->getDateAddFormatted(), ]; }Also applies to: 146-154
controllers/front/customerCards.php (2)
59-65: Set proper Content-Type header for JSON responsesThe AJAX response lacks Content-Type header, which may cause clients to misinterpret the response.
Add the header:
private function ajaxResponse(bool $success, string $errorMessage = ''): void { + header('Content-Type: application/json'); die(json_encode([ 'success' => $success, 'error' => $success ? null : $errorMessage ])); }
40-57:⚠️ Potential issueAdd CSRF token validation for security
The AJAX delete endpoint lacks CSRF protection, allowing malicious sites to trigger card deletions.
Add token validation:
public function displayAjaxDeleteCustomerCard() { + // Validate CSRF token + if (!Tools::getValue('token') || Tools::getValue('token') !== Tools::getToken(false)) { + $this->ajaxResponse(false, 'Invalid security token'); + return; + } + try { $customerCardId = (int) Tools::getValue('customerCardId');src/Service/Order/OrderService.php (2)
38-42:⚠️ Potential issueUndefined variable
$resultin log statement.The variable
$resultis not defined. Use$orderPaymentExistsinstead to correctly reference the existing order payment data.Apply this diff to fix the undefined variable:
- PrestaShopLogger::addLog('MONEI - createOrUpdateOrder - Order: (' . $result['id_order'] . ') already exists. Payment ID: ' . $moneiPaymentId . ' Date: ' . $result['date_add'], PrestaShopLogger::LOG_SEVERITY_LEVEL_WARNING); + PrestaShopLogger::addLog('MONEI - createOrUpdateOrder - Order: (' . $orderPaymentExists['id_order'] . ') already exists. Payment ID: ' . $moneiPaymentId . ' Date: ' . $orderPaymentExists['date_add'], PrestaShopLogger::LOG_SEVERITY_LEVEL_WARNING);
58-63:⚠️ Potential issuePotential null dereference when payment failed.
If the payment status is
FAILED,$ordermay remainnull, but the code still attempts to access$order->id.Apply this diff to add a null check:
- $this->moneiService->saveMoneiPayment($moneiPayment, $order->id); + if ($order !== null) { + $this->moneiService->saveMoneiPayment($moneiPayment, $order->id); + }src/Service/Payment/PaymentOptionService.php (4)
40-64:⚠️ Potential issueInitialize
$paymentOptionsto ensure non-null return.The property
$this->paymentOptionsis not initialized, causing the method to returnnullinstead of an empty array when no payment options are added.Initialize the property at the start of the method:
public function getPaymentOptions(): ?array { $this->paymentMethodsAllowed = $this->moneiService->getPaymentMethodsAllowed(); + $this->paymentOptions = [];
79-103:⚠️ Potential issueFix logic bug in payment method validation.
The
isset()check is always true after assignment. Useempty()to properly check if the array has no elements.Apply this diff to fix the logic:
- if (!isset($this->paymentMethodsAllowed)) { + if (empty($this->paymentMethodsAllowed)) { // If the payment methods are not found, we allow the payment method return true; }
185-194:⚠️ Potential issueFix method call with incorrect number of arguments.
The
isPaymentMethodAllowedmethod only accepts one parameter, but two arguments are being passed.Apply this diff to fix the method call:
- if ($this->configuration->get('MONEI_ALLOW_APPLE') && $this->isPaymentMethodAllowed('applePay', $this->isSafariBrowser())) { + if ($this->configuration->get('MONEI_ALLOW_APPLE') && $this->isPaymentMethodAllowed('applePay') && $this->isSafariBrowser()) {
196-205:⚠️ Potential issueFix method call with incorrect number of arguments.
The
isPaymentMethodAllowedmethod only accepts one parameter, but two arguments are being passed.Apply this diff to fix the method call:
- if ($this->configuration->get('MONEI_ALLOW_GOOGLE') && $this->isPaymentMethodAllowed('googlePay', !$this->isSafariBrowser())) { + if ($this->configuration->get('MONEI_ALLOW_GOOGLE') && $this->isPaymentMethodAllowed('googlePay') && !$this->isSafariBrowser()) {sql/install.php (2)
9-11: 🛠️ Refactor suggestionMake
refunded_amountNOT NULL with default 0.Allowing NULL in
refunded_amountcomplicates arithmetic operations. Use NOT NULL with default 0 for predictable behavior.Apply this diff to improve the schema:
- `refunded_amount` INT(11) DEFAULT NULL, + `refunded_amount` INT(11) NOT NULL DEFAULT 0,
51-63: 🛠️ Refactor suggestionAdd foreign key constraints for referential integrity.
The table references other tables but lacks foreign key constraints, which could lead to orphan records.
Add foreign key constraints after table creation. Run this script to verify if the referenced tables exist:
#!/bin/bash # Description: Check if the referenced tables exist in the schema # Search for the payment and history table definitions echo "Checking for monei2_payment table definition:" rg -A 5 "CREATE TABLE.*monei2_payment" echo -e "\nChecking for monei2_history table definition:" rg -A 5 "CREATE TABLE.*monei2_history" echo -e "\nChecking for employee table references in PrestaShop:" fd -e php | xargs rg "CREATE TABLE.*employee" | head -5src/Entity/Monei2Payment.php (1)
1-309: Well-structured Doctrine entity with proper null handling!The entity implementation follows Doctrine best practices and all previously identified issues have been properly addressed:
- Nullable fields are correctly annotated
- Date formatting methods include null checks
- The
isRefundable()method properly handles null refunded amounts- Timestamp setters correctly convert to DateTime objects
src/Service/Monei/MoneiService.php (1)
246-247: Convert ISO 8601 date strings to integer timestamps before calling the settersThe methods
setDateAdd()andsetDateUpd()onMonei2Paymentare declared to accept?inttimestamps, butPayment::getCreatedAt()/getUpdatedAt()return ISO 8601 strings—not\DateTimeobjects or integers. Passing raw strings will either be coerced incorrectly (yielding only the leading numeric portion) or trigger a type error under strict mode.Apply this fix:
- $monei2PaymentEntity->setDateAdd($moneiPayment->getCreatedAt()); - $monei2PaymentEntity->setDateUpd($moneiPayment->getUpdatedAt()); + $monei2PaymentEntity->setDateAdd(strtotime($moneiPayment->getCreatedAt())); + $monei2PaymentEntity->setDateUpd(strtotime($moneiPayment->getUpdatedAt()));
🧹 Nitpick comments (10)
config/admin/hooks.yml (1)
1-3: Set public visibility for admin hook services.
Declaring all services as public by default simplifies discovery in the admin context. Consider restricting visibility or explicitly marking only those services needed externally as public to follow the principle of least privilege.config/hooks.yml (1)
1-3: Global hook services marked public.
This default enables all hook-related services to be public. To reduce duplication and potential drift, consolidate common hook defaults into a single shared file and import it into bothadminandfrontcontexts.config/front/hooks.yml (1)
1-3: Set public visibility for front hook services.
The public default ensures accessibility in the front-office. For improved maintainability and security, consider marking only the necessary services as public and leaving others private.controllers/front/validation.php (1)
26-26: Consider documenting the sleep delay rationale.The 3-second sleep delay helps prevent race conditions but could impact performance. Consider adding a comment explaining this is intentional to prevent duplicate order creation.
- sleep(3); // Adding a delay to prevent processing too quickly and potentially generating duplicate orders. + // Adding a 3-second delay to prevent race conditions that could generate duplicate orders + // when multiple webhooks are processed simultaneously for the same payment + sleep(3);views/js/front/customerCards.js (2)
15-15: Make DOM traversal more robust.The current DOM traversal using
this.parentElement.parentElementis fragile and could break if the HTML structure changes. Consider using a more specific selector or data attribute to identify the card container.- ajaxDeleteTokenizedCard(this.dataset.customerCardId, this.parentElement.parentElement); + const cardContainer = this.closest('[data-card-container]') || this.closest('.customer-card-item'); + ajaxDeleteTokenizedCard(this.dataset.customerCardId, cardContainer);
60-66: Improve error handling in catch block.The catch block shows a generic error message regardless of the actual error. Consider logging the error for debugging purposes.
.catch(error => { + console.error('Error deleting customer card:', error); Swal.fire({ title: 'Error', - text: 'An unexpected error occurred.', + text: MoneiVars.errorRemovingCard || 'An unexpected error occurred.', icon: 'error' }); });src/Repository/MoneiPaymentRepository.php (1)
14-16: Fix inconsistent return type documentation.The DocBlock indicates
@return voidbut the method actually returnsMonei2Payment. Update the documentation to match the implementation./** * @param Monei2Payment $monei2Payment * @param bool $flush * - * @return void + * @return Monei2Payment */src/Exception/MoneiException.php (1)
6-27: Consider adding static factory methods for common exceptionsTo improve developer experience and ensure consistent error messages, consider adding static factory methods.
Example implementation:
class MoneiException extends Exception { const CART_NOT_FOUND = 1; // ... other constants ... + + private static $errorMessages = [ + self::CART_NOT_FOUND => 'Cart not found', + self::CART_AMOUNT_EMPTY => 'Cart amount is empty', + // ... other messages ... + ]; + + public static function cartNotFound($cartId = null) + { + $message = self::$errorMessages[self::CART_NOT_FOUND]; + if ($cartId !== null) { + $message .= " (ID: $cartId)"; + } + return new self($message, self::CART_NOT_FOUND); + } + + public static function invalidRefundAmount($amount, $available) + { + $message = "Refund amount ($amount) exceeds available balance ($available)"; + return new self($message, self::REFUND_AMOUNT_EXCEEDS_AVAILABLE_REFUNDABLE_BALANCE); + } }views/js/admin/admin.js (1)
1-3: Remove redundant 'use strict' directiveThe 'use strict' directive is unnecessary in JavaScript modules as they automatically run in strict mode.
(function ($) { - 'use strict'; - // Configuración de constantes🧰 Tools
🪛 Biome (1.9.4)
[error] 2-2: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
controllers/front/redirect.php (1)
33-35: Consider logging the crypto hash failureWhen crypto hash validation fails, it could indicate a security issue or tampering attempt. Consider logging this event for security monitoring.
if (!$check_encrypt) { + PrestaShopLogger::addLog( + 'MONEI: Invalid crypto hash for cart ' . $cart->id . ' from IP ' . Tools::getRemoteAddr(), + 3, // Error level + null, + 'Cart', + (int)$cart->id + ); throw new MoneiException('Invalid crypto hash', MoneiException::INVALID_CRYPTO_HASH); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (14)
composer.lockis excluded by!**/*.lockviews/img/payments/amex.svgis excluded by!**/*.svgviews/img/payments/apple-pay.svgis excluded by!**/*.svgviews/img/payments/bancontact.svgis excluded by!**/*.svgviews/img/payments/bizum.svgis excluded by!**/*.svgviews/img/payments/cards.svgis excluded by!**/*.svgviews/img/payments/giropay.svgis excluded by!**/*.svgviews/img/payments/google-pay.svgis excluded by!**/*.svgviews/img/payments/klarna.svgis excluded by!**/*.svgviews/img/payments/mbway.svgis excluded by!**/*.svgviews/img/payments/multibanco.svgis excluded by!**/*.svgviews/img/payments/paypal.svgis excluded by!**/*.svgviews/img/payments/sofort.svgis excluded by!**/*.svgviews/js/sweetalert.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (107)
.php-cs-fixer.php(1 hunks)build/package.json(2 hunks)composer.json(1 hunks)config/admin/hooks.yml(1 hunks)config/admin/services.yml(1 hunks)config/common.yml(1 hunks)config/front/hooks.yml(1 hunks)config/front/services.yml(1 hunks)config/hooks.yml(1 hunks)controllers/admin/AdminMoneiController.php(1 hunks)controllers/front/cards.php(0 hunks)controllers/front/confirmation.php(2 hunks)controllers/front/createPayment.php(2 hunks)controllers/front/customerCards.php(1 hunks)controllers/front/redirect.php(2 hunks)controllers/front/validation.php(3 hunks)files/index.php(0 hunks)index.php(1 hunks)phpunit.xml(1 hunks)sql/index.php(0 hunks)sql/install.php(1 hunks)sql/uninstall.php(1 hunks)src/Entity/Monei2CustomerCard.php(1 hunks)src/Entity/Monei2History.php(1 hunks)src/Entity/Monei2Payment.php(1 hunks)src/Entity/Monei2Refund.php(1 hunks)src/Exception/MoneiException.php(1 hunks)src/Exception/OrderException.php(1 hunks)src/Monei/Api/AppleApi.php(0 hunks)src/Monei/Api/PaymentMethodsApi.php(0 hunks)src/Monei/Api/PaymentsApi.php(0 hunks)src/Monei/ApiException.php(0 hunks)src/Monei/Configuration.php(0 hunks)src/Monei/CoreClasses/Monei.php(0 hunks)src/Monei/CoreClasses/MoneiCard.php(0 hunks)src/Monei/CoreHelpers/PsCartHelper.php(0 hunks)src/Monei/CoreHelpers/PsOrderHelper.php(0 hunks)src/Monei/CoreHelpers/PsTools.php(0 hunks)src/Monei/Model/ModelInterface.php(0 hunks)src/Monei/Model/MoneiAccount.php(0 hunks)src/Monei/Model/MoneiAddress.php(0 hunks)src/Monei/Model/MoneiBillingDetails.php(0 hunks)src/Monei/Model/MoneiCard.php(0 hunks)src/Monei/Model/MoneiCustomer.php(0 hunks)src/Monei/Model/MoneiNextAction.php(0 hunks)src/Monei/Model/MoneiPayment.php(0 hunks)src/Monei/Model/MoneiPaymentMethod.php(0 hunks)src/Monei/Model/MoneiPaymentMethods.php(0 hunks)src/Monei/Model/MoneiPaymentStatus.php(0 hunks)src/Monei/Model/MoneiRefundPayment.php(0 hunks)src/Monei/Model/MoneiRefundReason.php(0 hunks)src/Monei/Model/MoneiShippingDetails.php(0 hunks)src/Monei/Model/MoneiShop.php(0 hunks)src/Monei/Model/MoneiTraceDetails.php(0 hunks)src/Monei/MoneiClient.php(0 hunks)src/Monei/MoneiException.php(0 hunks)src/Monei/Traits/Mappeable.php(0 hunks)src/Monei/Traits/ModelHelpers.php(0 hunks)src/Monei/Traits/ValidationHelpers.php(0 hunks)src/PsCurl/ArrayUtil.php(0 hunks)src/PsCurl/CaseInsensitiveArray.php(0 hunks)src/PsCurl/Decoder.php(0 hunks)src/PsCurl/Encoder.php(0 hunks)src/PsCurl/LICENSE(0 hunks)src/PsCurl/PsCurl.php(0 hunks)src/PsCurl/README.md(0 hunks)src/PsCurl/StringUtil.php(0 hunks)src/PsCurl/Url.php(0 hunks)src/PsCurl/index.php(0 hunks)src/Repository/MoneiCustomerCardRepository.php(1 hunks)src/Repository/MoneiHistoryRepository.php(1 hunks)src/Repository/MoneiPaymentRepository.php(1 hunks)src/Repository/MoneiRefundRepository.php(1 hunks)src/Service/Monei/MoneiService.php(1 hunks)src/Service/Order/OrderService.php(1 hunks)src/Service/Payment/PaymentOptionService.php(1 hunks)src/index.php(0 hunks)translations/en.php(1 hunks)translations/es.php(5 hunks)upgrade/index.php(0 hunks)upgrade/upgrade-2.0.0.php(1 hunks)views/css/checkout_page.css(0 hunks)views/css/front/checkout_page.css(1 hunks)views/css/index.php(0 hunks)views/img/index.php(0 hunks)views/index.php(0 hunks)views/js/admin/admin.js(1 hunks)views/js/cards.js(0 hunks)views/js/front/customerCards.js(1 hunks)views/js/index.php(0 hunks)views/js/moneiback.js(0 hunks)views/templates/admin/index.php(0 hunks)views/templates/admin/order17.tpl(0 hunks)views/templates/admin/order177.tpl(0 hunks)views/templates/admin/scripts/admin_scripts.tpl(0 hunks)views/templates/admin/scripts/index.php(0 hunks)views/templates/front/additional_info.tpl(1 hunks)views/templates/front/cards.tpl(0 hunks)views/templates/front/index.php(0 hunks)views/templates/front/onsite_card.tpl(1 hunks)views/templates/hook/_partials/customerCards.tpl(1 hunks)views/templates/hook/displayAdminOrder.tpl(1 hunks)views/templates/hook/displayCustomerAccount.tpl(2 hunks)views/templates/hook/displayPaymentByBinaries.tpl(4 hunks)views/templates/hook/displayPaymentReturn.tpl(1 hunks)views/templates/hook/index.php(0 hunks)views/templates/index.php(0 hunks)
💤 Files with no reviewable changes (62)
- upgrade/index.php
- views/templates/hook/index.php
- views/js/index.php
- views/css/index.php
- views/index.php
- sql/index.php
- views/templates/front/index.php
- views/img/index.php
- src/index.php
- files/index.php
- views/templates/admin/index.php
- views/templates/admin/scripts/index.php
- views/templates/index.php
- src/PsCurl/index.php
- src/PsCurl/LICENSE
- src/Monei/CoreHelpers/PsTools.php
- views/templates/admin/scripts/admin_scripts.tpl
- src/Monei/MoneiException.php
- src/Monei/Model/MoneiBillingDetails.php
- src/PsCurl/Encoder.php
- src/PsCurl/README.md
- src/Monei/Model/MoneiShop.php
- src/Monei/Model/MoneiRefundReason.php
- src/Monei/Model/ModelInterface.php
- src/Monei/Model/MoneiAccount.php
- src/Monei/Traits/ValidationHelpers.php
- views/js/cards.js
- src/Monei/Model/MoneiPaymentMethods.php
- views/templates/front/cards.tpl
- src/Monei/ApiException.php
- src/Monei/Model/MoneiPaymentMethod.php
- views/js/moneiback.js
- src/Monei/Configuration.php
- src/Monei/CoreClasses/Monei.php
- src/PsCurl/Decoder.php
- src/PsCurl/ArrayUtil.php
- src/Monei/Model/MoneiShippingDetails.php
- views/css/checkout_page.css
- src/Monei/Model/MoneiCustomer.php
- src/PsCurl/StringUtil.php
- src/PsCurl/CaseInsensitiveArray.php
- src/Monei/Api/PaymentMethodsApi.php
- controllers/front/cards.php
- src/Monei/Model/MoneiNextAction.php
- src/Monei/Model/MoneiPaymentStatus.php
- src/Monei/CoreHelpers/PsCartHelper.php
- src/Monei/Model/MoneiRefundPayment.php
- src/Monei/CoreClasses/MoneiCard.php
- src/Monei/Model/MoneiAddress.php
- src/Monei/Model/MoneiTraceDetails.php
- src/Monei/MoneiClient.php
- views/templates/admin/order17.tpl
- src/Monei/Traits/Mappeable.php
- src/Monei/Api/AppleApi.php
- views/templates/admin/order177.tpl
- src/PsCurl/PsCurl.php
- src/PsCurl/Url.php
- src/Monei/Traits/ModelHelpers.php
- src/Monei/Model/MoneiCard.php
- src/Monei/Api/PaymentsApi.php
- src/Monei/CoreHelpers/PsOrderHelper.php
- src/Monei/Model/MoneiPayment.php
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/Repository/MoneiCustomerCardRepository.php (5)
src/Entity/Monei2CustomerCard.php (1)
Monei2CustomerCard(10-187)src/Exception/MoneiException.php (1)
MoneiException(6-27)src/Repository/MoneiHistoryRepository.php (2)
save(16-28)remove(36-46)src/Repository/MoneiPaymentRepository.php (2)
save(16-28)remove(36-46)src/Repository/MoneiRefundRepository.php (2)
save(16-28)remove(36-46)
controllers/admin/AdminMoneiController.php (3)
monei.php (1)
getService(104-124)src/Service/Monei/MoneiService.php (1)
createRefund(404-438)src/Service/Order/OrderService.php (1)
updateOrderStateAfterRefund(174-187)
src/Repository/MoneiPaymentRepository.php (5)
src/Entity/Monei2Payment.php (1)
Monei2Payment(11-309)src/Exception/MoneiException.php (1)
MoneiException(6-27)src/Repository/MoneiCustomerCardRepository.php (2)
save(16-26)remove(34-44)src/Repository/MoneiHistoryRepository.php (2)
save(16-28)remove(36-46)src/Repository/MoneiRefundRepository.php (2)
save(16-28)remove(36-46)
🪛 Biome (1.9.4)
views/js/admin/admin.js
[error] 2-2: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (42)
index.php (1)
11-11: LGTM! Good formatting standardization.Adding the newline after the
exit;statement improves consistency across the codebase and aligns with the formatting cleanup being applied to other index.php files throughout the project.build/package.json (2)
3-3: Ensure version bump propagation across module metadata.
Please verify that the version "2.0.0" is also updated in other relevant files (e.g.,monei.php,composer.json, or the module manifest) to keep metadata consistent.
27-27: Removed legacy client from release-it bumper output.
The removal of../src/Monei/MoneiClient.phpaligns with the deletion of legacy API client classes.phpunit.xml (3)
4-10: Strict testing flags enabled.
Enforcing strict mode on globals backup, coverage annotations, test output, and TODO tests will improve test reliability and maintainability.
11-13: Test suite configuration is correct.
Including all*Test.phpfiles under thetestsdirectory aligns with the new directory structure and naming conventions.
15-19: Code coverage whitelist setup is appropriate.
WhitelistingsrcwithprocessUncoveredFilesFromWhitelist="true"ensures that newly introduced service and entity classes are measured for coverage.config/front/services.yml (1)
6-8: Service configuration looks good.The default service configuration with public visibility is appropriate for a Symfony/PrestaShop module context.
views/templates/hook/displayCustomerAccount.tpl (2)
1-1: Improved semantic variable naming.The condition change from
$is_warehouse == 0to$isWarehouseInstalled(falsey check) makes the code more readable and semantically clear.
3-3: Consistent controller update across both branches.The controller name change from 'cards' to 'customerCards' is applied consistently in both conditional branches, aligning with the new controller structure.
Also applies to: 12-12
views/templates/hook/displayPaymentReturn.tpl (1)
1-3: Well-implemented payment notification template.The template follows best practices:
- Proper Bootstrap alert styling with semantic HTML
- Correct localization using the module context
- Good use of sprintf for dynamic value injection
- Accessibility-friendly with
role="alert"config/admin/services.yml (1)
6-8: Service configuration looks good.The default service configuration with public visibility is appropriate for the admin context.
views/templates/front/additional_info.tpl (1)
1-9: LGTM! Excellent semantic HTML improvement.The change from a simple
<div>to a<section>with a<p>element provides better semantic structure while preserving all the original content, links, and styling classes. This enhances accessibility and follows HTML5 best practices.sql/uninstall.php (1)
4-8:✅ Verification successful
Verify table names match the installation script.
The updated table names with the
monei2prefix align with the schema migration described in the summary. Ensure these table names exactly match those created in the correspondingsql/install.phpfile.
🏁 Script executed:
#!/bin/bash # Description: Verify that uninstall table names match install script # Expected: Same table names in both install and uninstall scripts echo "=== Tables in uninstall script ===" rg "DROP TABLE.*monei2_" sql/uninstall.php echo -e "\n=== Tables in install script ===" rg "CREATE TABLE.*monei2_" sql/install.php || echo "install.php not found in changed files"Length of output: 996
Tables Align with Installation Script
Verified that all
monei2_table names insql/uninstall.phpexactly match those insql/install.php. No further changes are needed.views/templates/front/onsite_card.tpl (2)
1-1: Good addition of container wrapper.The
payment-form-moneiwrapper div provides better structure for styling and JavaScript targeting.
9-16:✅ Verification successful
Verify the tokenization logic change is intentional.
The tokenization checkbox display logic has changed from showing when
$isCustomerLoggedto requiring both$isCustomerLogged && $tokenize. This is more restrictive and will hide the checkbox in some scenarios where it was previously shown.Ensure this behavioral change aligns with the new
PaymentOptionServicelogic mentioned in the summary.
🏁 Script executed:
#!/bin/bash # Description: Check how $tokenize variable is set in controllers # Expected: Find where $tokenize is assigned to understand the new logic echo "=== Searching for tokenize variable assignment ===" rg -A 3 -B 3 "tokenize.*=" --type php echo -e "\n=== Searching for tokenize in payment controllers ===" rg -A 5 -B 5 "tokenize" controllers/front/ --type php || echo "No front controllers found"Length of output: 5760
Tokenization checkbox gated by configuration
The checkbox is now deliberately shown only when the customer is logged in and the
MONEI_TOKENIZEfeature flag is enabled. We’ve confirmed that:
- In
src/Service/Payment/PaymentOptionService.php,is what drives the new'tokenize' => (bool) $this->configuration->get('MONEI_TOKENIZE'),$tokenizevariable in Smarty.- The
{if $isCustomerLogged && $tokenize}condition inviews/templates/front/onsite_card.tpltherefore aligns exactly with this configuration-driven logic.No code changes are required. Please ensure the default value of
MONEI_TOKENIZEis set totrue(if you want existing merchants to see the option by default) in your module’s installation script or configuration UI.controllers/front/createPayment.php (2)
2-4: Good addition of environment check.Adding the
_PS_VERSION_check is a security best practice that prevents direct script execution outside of the PrestaShop context.
15-17:✅ Verification successful
Excellent refactoring to service-oriented architecture.
The transition from direct module method calls to service locator pattern improves modularity and testability. This aligns well with the broader architectural changes in the PR.
Let me verify that the service is properly registered and the method exists:
🏁 Script executed:
#!/bin/bash # Description: Verify the service registration and method existence # Check if the service.monei is registered in YAML configuration files echo "Checking service registration..." fd -e yml -e yaml . config/ --exec grep -l "service.monei" {} \; # Verify the MoneiService class has the createMoneiPayment method echo -e "\nChecking MoneiService class for createMoneiPayment method..." ast-grep --pattern 'class MoneiService { $$$ createMoneiPayment($_) { $$$ } $$$ }'Length of output: 394
To ensure the
MoneiServiceand itscreateMoneiPaymentmethod truly exist, let’s locate the class file and scan for the method definition:
🏁 Script executed:
#!/bin/bash # Locate the MoneiService implementation echo "Finding MoneiService.php file..." fd MoneiService.php # Verify the class declaration echo -e "\nSearching for class MoneiService definition..." rg -n "class\s\+MoneiService" # Verify the method signature echo -e "\nSearching for createMoneiPayment method definition..." rg -n "createMoneiPayment"Length of output: 1200
Service registration and method existence verified
I confirmed that:
service.moneiis registered in config/common.ymlMoneiServiceexists at src/Service/Monei/MoneiService.php- It declares
public function createMoneiPayment(Cart $cart, bool $tokenizeCard = false, int $cardTokenId = 0)Everything looks correctly wired—no further action needed here.
views/templates/hook/displayPaymentByBinaries.tpl (3)
12-12: Consistent function naming with backend services.The rename from
createPaymenttocreateMoneiPaymentaligns well with the backend service method naming convention, maintaining consistency across the codebase.Also applies to: 47-47
147-147: Improved button styling for better user experience.Adding the
w-100class for full width andmt-3for top margin improves the visual consistency of the payment button.
331-331: Enhanced selector compatibility.Expanding the selector to include
.payment__optionimproves theme compatibility by accommodating different class naming conventions that may exist across various PrestaShop themes.composer.json (3)
3-3: Correct module type declaration.Setting the type as
prestashop-moduleproperly identifies this package for PrestaShop's module system.
10-17: Well-structured autoload configuration.The PSR-4 autoloading with the consolidated
PsMoneinamespace and explicit classmap formonei.phpprovides a clean and organized structure for the refactored codebase.
26-26:❓ Verification inconclusive
Verify PrestaShop service container dependency version.
The addition of
prestashop/module-lib-service-containeris essential for the new service-oriented architecture. Please ensure version ^2.0 is compatible with your target PrestaShop versions.
🌐 Web query:
What is the latest version of prestashop/module-lib-service-container package and what PrestaShop versions does it support?💡 Result:
The latest version of the
prestashop/module-lib-service-containerpackage is v2.0, released on June 20, 2022. (packagist.org)This package is compatible with PrestaShop versions 1.7 and later. For PrestaShop 1.6, additional Symfony dependencies are required:
composer require symfony/config:^3.4 symfony/dependency-injection:^3.4 symfony/expression-language:^3.4 symfony/yaml:^3.4These dependencies are necessary because PrestaShop 1.6 does not include Symfony components by default. (packagist.org)
For PrestaShop 1.7 and newer versions, the package can be installed directly without additional dependencies, as PrestaShop 1.7+ includes Symfony components. (packagist.org)
Citations:
- 1: https://packagist.org/packages/prestashop/module-lib-service-container?utm_source=openai
- 2: https://packagist.org/packages/prestashop/module-lib-service-container?utm_source=openai
- 3: https://packagist.org/packages/prestashop/module-lib-service-container?utm_source=openai
Verify
prestashop/module-lib-service-container(^2.0) compatibility with your target PrestaShop versionsThe package’s latest release is v2.0 (June 20, 2022) and it natively supports PrestaShop 1.7+. If your module’s minimum version is 1.7 or newer, no additional steps are needed.
If you must support PrestaShop 1.6, you’ll need to pull in Symfony 3.4 components manually:composer require \ symfony/config:^3.4 \ symfony/dependency-injection:^3.4 \ symfony/expression-language:^3.4 \ symfony/yaml:^3.4• File: composer.json (line 26)
• Action: Confirm your minimum PrestaShop version and, if 1.6 support is required, add the above dependencies.controllers/front/confirmation.php (4)
6-6: Consistent model import updates.The import change from
MoneiPaymentStatustoPaymentStatusand the corresponding constant update align with the new domain model structure.Also applies to: 18-18
21-21: Proper use of PrestaShop's native logging constants.Switching from module-specific logging constants to PrestaShop's built-in
PrestaShopLoggerconstants improves consistency with the framework's logging standards.Also applies to: 31-31
24-24: Service-oriented order management.The transition to using
$this->module->getService('service.order')->createOrUpdateOrder()maintains consistency with the new service architecture pattern across the module.
28-28: Appropriate exception handling scope for payment processing.Broadening the exception handling to catch all
Exceptiontypes is appropriate here, as any exception during payment confirmation should prevent order processing and redirect to the error page.views/templates/hook/_partials/customerCards.tpl (1)
1-65: Well-implemented customer cards template with proper security and accessibility.The template demonstrates excellent practices:
- All dynamic data is properly escaped for HTML output
- Responsive table design with Bootstrap classes
- Good accessibility with aria-labels and screen reader support
- Clean handling of empty states
- Proper conditional rendering for different icon sets
controllers/front/validation.php (1)
28-72: Excellent migration to service-based architecture.The refactoring to use dependency injection and service classes (
$this->module->getService('service.order')) is a significant improvement over the legacy approach. The exception handling and logging are well-implemented.src/Repository/MoneiCustomerCardRepository.php (1)
46-57: LGTM! Well-implemented active cards query.The
getActiveCustomerCardsmethod correctly filters cards by customer ID and expiration timestamp. The logic properly excludes expired cards by comparing the expiration timestamp with the current timestamp.views/css/front/checkout_page.css (1)
1-88: Well-structured CSS with good practices.The CSS code follows good practices with:
- Proper spacing and layout rules
- Modern CSS features (flexbox, transitions)
- Consistent naming conventions
- Responsive design considerations
- Good visual hierarchy and accessibility (focus states)
The Monei-specific styling maintains consistency with the overall design system.
views/templates/hook/displayAdminOrder.tpl (1)
1-1:✅ Verification successful
Verify the SweetAlert2 script source for security.
The template includes an external script from a variable source. Ensure that
$sweetalert2points to a trusted CDN or local asset to prevent potential security vulnerabilities.
🏁 Script executed:
#!/bin/bash # Description: Check where the $sweetalert2 variable is defined and what URL it contains # Search for sweetalert2 variable assignment in PHP files rg -A 2 "sweetalert2.*=" --type php # Search for any SweetAlert2 references in configuration or module files fd -e php -e tpl | xargs rg -i "sweetalert"Length of output: 801
SweetAlert2 source confirmed as trusted.
The
$sweetalert2variable is defined inmonei.phpashttps://cdn.jsdelivr.net/npm/sweetalert2@11, which is a reputable CDN hosting the official SweetAlert2 release. No further changes are needed..php-cs-fixer.php (1)
1-63: Well-configured code style enforcement.The PHP-CS-Fixer configuration is comprehensive and follows best practices:
- Appropriate directory exclusions
- Sensible rule customizations based on Symfony preset
- Caching enabled for performance
- Clear formatting preferences that will maintain code consistency
src/Exception/MoneiException.php (1)
6-27: Well-structured exception class with comprehensive error codesThe exception class follows best practices with clearly defined error constants. Good job on maintaining a clean, focused exception hierarchy.
views/js/admin/admin.js (1)
4-171: Well-structured admin interface implementationExcellent code organization with clear separation of concerns through dedicated handlers, configuration object, and utility functions. The modular approach makes the code maintainable and testable.
controllers/front/redirect.php (1)
37-48: Good refactoring to service-based architectureExcellent use of dependency injection and service delegation. The separation of concerns between payment creation (
MoneiService) and order management (OrderService) improves maintainability and testability.controllers/front/customerCards.php (1)
34-38: Missing active card filteringThe AI summary mentions using
getActiveCustomerCardsto filter non-expired cards, but the implementation retrieves all cards without filtering.Verify if card filtering is needed:
#!/bin/bash # Check if MoneiCustomerCardRepository has a method for active cards ast-grep --pattern 'class MoneiCustomerCardRepository { $$$ getActiveCustomerCards($_) { $$$ } $$$ }'Likely an incorrect or invalid review comment.
src/Service/Order/OrderService.php (3)
95-110: Good implementation of state transition validation.The method properly enforces valid state transitions, preventing invalid order state changes.
174-187: Correct implementation of refund state management.The method properly determines whether to set the order as partially or fully refunded based on the refund amount.
198-213: Proper order creation with correct amount conversion.The method correctly converts the amount from cents to the base currency unit and creates the order with appropriate parameters.
translations/es.php (1)
5-147: Translation updates look good!The Spanish translations are grammatically correct and consistent with the module's terminology. They properly support the new features including refunds, API configuration, customer card management, and admin displays.
src/Service/Monei/MoneiService.php (1)
35-439: Excellent service architecture with comprehensive improvements!The service demonstrates well-structured code with:
- Proper dependency injection and separation of concerns
- Comprehensive validation and error handling
- Domain-specific exceptions with meaningful error codes
- All previously identified issues addressed except for the DateTime conversion
Great work on implementing the suggested improvements for refund validation, cart amount calculation clarity, and exception constant definitions!
- se estaba tomando mal el valor del pago con tarjeta. - se mejora visual del form de tarjeta en el onepagecheckoutps.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/Service/Order/OrderService.php (2)
62-75:⚠️ Potential issueNull-dereference path when payment is FAILED
For failed payments
$ordercan benull, yet:
$this->moneiService->saveMoneiPayment($moneiPayment, $order->id);- SQL insert uses
$order->idhandlePostOrderCreation()always dereferences$orderAll three will trigger fatal errors.
Quick fix:
- $this->moneiService->saveMoneiPayment($moneiPayment, $order->id); + if ($order !== null) { + $this->moneiService->saveMoneiPayment($moneiPayment, $order->id); + } ... - if ($connection->execute($sql)) { + if ($order !== null && $connection->execute($sql)) { ... - $this->handlePostOrderCreation($redirectToConfirmationPage, $cart, $customer, $order); + if ($order !== null) { + $this->handlePostOrderCreation($redirectToConfirmationPage, $cart, $customer, $order); + }Alternatively, bail out early on
$failed === true.
40-42:⚠️ Potential issueUndefined variable
$result→ use$orderPaymentExists
$resultis never defined and will raise a notice. Replace with the already-fetched$orderPaymentExists.- PrestaShopLogger::addLog('MONEI - createOrUpdateOrder - Order: (' . $result['id_order'] . ') already exists. Payment ID: ' . $moneiPaymentId . ' Date: ' . $result['date_add'], PrestaShopLogger::LOG_SEVERITY_LEVEL_WARNING); + PrestaShopLogger::addLog( + 'MONEI - createOrUpdateOrder - Order: (' . $orderPaymentExists['id_order'] . ') already exists. Payment ID: ' . $moneiPaymentId . ' Date: ' . $orderPaymentExists['date_add'], + PrestaShopLogger::LOG_SEVERITY_LEVEL_WARNING + );src/Service/Monei/MoneiService.php (1)
246-247: Convert ISO 8601 date strings to timestamps.Based on previous review verification,
getCreatedAt()andgetUpdatedAt()return ISO 8601 date strings, butsetDateAdd()andsetDateUpd()expect integer timestamps.- $monei2PaymentEntity->setDateAdd($moneiPayment->getCreatedAt()); - $monei2PaymentEntity->setDateUpd($moneiPayment->getUpdatedAt()); + $monei2PaymentEntity->setDateAdd(strtotime($moneiPayment->getCreatedAt())); + $monei2PaymentEntity->setDateUpd(strtotime($moneiPayment->getUpdatedAt()));
🧹 Nitpick comments (2)
src/Exception/OrderException.php (1)
7-13: Add PHPDoc + clarify numeric rangeThe constants are now in a distinct
200+range – nice fix.
Consider adding a short class-level PHPDoc explaining the domain (“order-related errors, range 200-2xx”) so that future contributors keep ranges non-overlapping.src/Repository/MoneiPaymentRepository.php (1)
6-28: Consistent with base repository, but consider modern Doctrine base classExtending
EntityRepositoryworks, yet Doctrine now recommendsServiceEntityRepositoryto leverage typed constructors & autowiring. If you ever revisit DI config, migrating would remove the need for manual repository registration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/Entity/Monei2CustomerCard.php(1 hunks)src/Entity/Monei2Refund.php(1 hunks)src/Exception/MoneiException.php(1 hunks)src/Exception/OrderException.php(1 hunks)src/Repository/AbstractMoneiRepository.php(1 hunks)src/Repository/MoneiCustomerCardRepository.php(1 hunks)src/Repository/MoneiHistoryRepository.php(1 hunks)src/Repository/MoneiPaymentRepository.php(1 hunks)src/Repository/MoneiRefundRepository.php(1 hunks)src/Service/Monei/MoneiService.php(1 hunks)src/Service/Order/OrderService.php(1 hunks)views/css/front/checkout_page.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- views/css/front/checkout_page.css
- src/Repository/MoneiCustomerCardRepository.php
- src/Exception/MoneiException.php
- src/Entity/Monei2Refund.php
- src/Repository/MoneiHistoryRepository.php
- src/Entity/Monei2CustomerCard.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Repository/AbstractMoneiRepository.php (5)
src/Exception/MoneiException.php (1)
MoneiException(6-27)src/Repository/MoneiCustomerCardRepository.php (1)
remove(25-28)src/Repository/MoneiHistoryRepository.php (1)
remove(25-28)src/Repository/MoneiRefundRepository.php (1)
remove(25-28)src/Repository/MoneiPaymentRepository.php (1)
remove(25-28)
🔇 Additional comments (8)
src/Repository/MoneiRefundRepository.php (1)
6-28: Repository looks goodDocblocks now match the actual return types and the class cleanly delegates to
AbstractMoneiRepository.src/Service/Monei/MoneiService.php (7)
35-52: Well-structured dependency injection setup.The constructor properly injects all required dependencies and follows good service architecture patterns.
216-229: Refund calculation correctly implemented.The method properly uses the payment's refund list and calculates the total refunded amount. This addresses the previous findOneBy vs findBy issue.
294-402: Comprehensive payment creation with proper validation.The method includes thorough validation, proper error handling, and follows the payment creation workflow correctly. The integration of tokenization features is well-implemented.
404-438: Well-implemented refund validation and processing.The refund method includes proper amount validation, balance checking, and error handling as recommended in previous reviews. The implementation correctly prevents over-refunding.
119-420:✅ Verification successful
Verify all exception constants are defined.
Multiple exception constants are referenced throughout the service but may not be defined in the MoneiException class, which could cause runtime errors.
🏁 Script executed:
#!/bin/bash # Check all exception constants used in the service echo "Checking exception constants used in MoneiService..." rg "MoneiException::" src/Service/Monei/MoneiService.php | sed 's/.*MoneiException::\([A-Z_]*\).*/\1/' | sort -u > /tmp/used_constants.txt echo "Exception constants used:" cat /tmp/used_constants.txt echo -e "\nChecking which constants are defined in MoneiException class..." ast-grep --pattern $'class MoneiException { $$$ const $_$$$ $$$ }' | rg "const" || echo "No constants found with ast-grep, trying alternative approach..." # Alternative approach using rg to find constant definitions rg "const [A-Z_]+" src/Exception/MoneiException.php -o | sed 's/const //' || echo "MoneiException file not found or no constants defined"Length of output: 1595
All exception constants referenced in MoneiService are defined in MoneiException
I’ve verified that eachMoneiException::…used insrc/Service/Monei/MoneiService.phphas a matchingconstinsrc/Exception/MoneiException.php. No missing constants remain.
168-168:✅ Verification successful
Verify country exception constant exists.
The code uses
MoneiException::COUNTRY_NOT_FOUNDwhich may not be defined. Either add this constant to MoneiException or use an existing one.
🏁 Script executed:
#!/bin/bash # Check if COUNTRY_NOT_FOUND constant exists in MoneiException ast-grep --pattern $'class MoneiException { $$$ COUNTRY_NOT_FOUND$$$ $$$ }'Length of output: 82
🏁 Script executed:
#!/bin/bash # Search for any definition or usage of COUNTRY_NOT_FOUND rg -n "COUNTRY_NOT_FOUND" -g "*.php" # Inspect the MoneiException class to list its existing constants rg -n "class MoneiException" -g "*.php" -A10Length of output: 1080
MoneiException::COUNTRY_NOT_FOUND is already defined
The constantCOUNTRY_NOT_FOUND(value118) exists insrc/Exception/MoneiException.php, so no changes are needed.
63-63:❓ Verification inconclusive
Verify exception constant exists.
The code references
MoneiException::MONEI_CLIENT_NOT_INITIALIZEDwhich may not be defined in the MoneiException class.
🏁 Script executed:
#!/bin/bash # Check if the exception constant is defined in MoneiException class ast-grep --pattern $'class MoneiException { $$$ MONEI_CLIENT_NOT_INITIALIZED$$$ $$$ }'Length of output: 93
🏁 Script executed:
#!/bin/bash # Locate the MoneiException class and list its constants file=$(rg -l "class\s\+MoneiException" || true) if [ -z "$file" ]; then echo "MoneiException class file not found." exit 0 fi echo "Found MoneiException in: $file" echo "Defined constants:" rg -n "const\s" "$file"Length of output: 158
Verify presence of exception constant
The code throws:
throw new MoneiException( 'Monei client not initialized.', MoneiException::MONEI_CLIENT_NOT_INITIALIZED );—but I wasn’t able to locate the
MoneiExceptionclass or itsMONEI_CLIENT_NOT_INITIALIZEDconstant in the repo. Please confirm that:
MoneiExceptionis defined (e.g. insrc/Service/Monei/MoneiException.phpor the vendor package).- It declares
const MONEI_CLIENT_NOT_INITIALIZED.Without this, referencing the constant will cause a fatal error.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Exception/MoneiException.php (1)
6-29: Add PHPDoc documentation to improve maintainability.The exception class is well-structured and follows PHP best practices. Consider adding PHPDoc comments to document the class purpose and organize the error codes by category for better maintainability.
+/** + * Custom exception class for Monei payment integration errors + * + * Defines error codes for various payment-related scenarios including + * cart validation, customer data, API communication, and entity persistence. + */ class MoneiException extends Exception { + // Cart-related errors (100-101) const CART_NOT_FOUND = 100; const CART_AMOUNT_EMPTY = 101; + + // Customer and address validation errors (102-104) const CURRENCY_NOT_FOUND = 102; const CUSTOMER_NOT_FOUND = 103; const ADDRESS_NOT_FOUND = 104; + + // API configuration errors (105-106) const MONEI_API_KEY_IS_EMPTY = 105; const MONEI_CLIENT_NOT_INITIALIZED = 106; + + // Payment and order errors (107-108) const PAYMENT_REQUEST_NOT_VALID = 107; const ORDER_NOT_FOUND = 108; + + // Account configuration errors (109-110) const MONEI_ACCOUNT_ID_IS_EMPTY = 109; const MONEI_ACCOUNT_INFORMATION_NOT_FOUND = 110; + + // Response validation errors (111-112) const INVALID_JSON_RESPONSE = 111; const INVALID_CRYPTO_HASH = 112; + + // Refund-related errors (113-115) const REFUND_AMOUNT_MUST_BE_GREATER_THAN_ZERO = 113; const REFUND_AMOUNT_EXCEEDS_AVAILABLE_REFUNDABLE_BALANCE = 114; const REFUND_CREATION_FAILED = 115; + + // Additional validation errors (116-118) const CART_SUMMARY_DETAILS_EMPTY = 116; const INVALID_CURRENCY_ID_PROVIDED = 117; const COUNTRY_NOT_FOUND = 118; + + // Entity persistence errors (119-120) const SAVE_ENTITY_ERROR = 119; const REMOVE_ENTITY_ERROR = 120; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Exception/MoneiException.php(1 hunks)src/Repository/AbstractMoneiRepository.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Repository/AbstractMoneiRepository.php
🔇 Additional comments (1)
src/Exception/MoneiException.php (1)
1-29: LGTM! Well-structured exception class with comprehensive error codes.The implementation follows PHP best practices with proper namespace usage, clear constant naming conventions, and logical error code organization. The sequential numbering (100-120) and descriptive constant names make the error codes easy to understand and maintain. This provides a solid foundation for the new service-oriented architecture.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Service/Order/OrderService.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cursor BugBot
🔇 Additional comments (7)
src/Service/Order/OrderService.php (7)
1-30: Well-structured class with proper dependency injection.The class follows good practices with clear namespace, proper PrestaShop version check, and dependency injection pattern. The imports are comprehensive and include necessary PrestaShop classes and custom exceptions.
86-97: Good status mapping with proper fallback handling.The status mapping logic is well-implemented with proper use of configuration values and a sensible fallback to failed status. The mapping of both
REFUNDEDandPARTIALLY_REFUNDEDto the same configuration key is noted - ensure this aligns with your business requirements.
99-114: Excellent state transition validation.The state transition logic properly enforces business rules and prevents invalid order state changes. The use of configuration values makes it flexible and maintainable.
116-134: Solid validation methods with proper error handling.Both validation methods follow consistent patterns with appropriate use of PrestaShop's validation utilities and meaningful custom exceptions. The error codes will be helpful for debugging and error handling.
136-176: Comprehensive existing order handling with proper validations.The logic properly validates existing orders, ensures they're from the correct payment module, and applies state transitions with appropriate validation. The comprehensive logging will be valuable for troubleshooting.
178-200: Well-implemented refund handling and payment updates.The refund logic properly distinguishes between partial and full refunds, respects configuration settings, and safely updates order payment transaction IDs with proper validation.
202-233: Proper order creation using PrestaShop conventions.The new order creation correctly uses PrestaShop's
validateOrdermethod with all necessary parameters. The amount conversion (÷100) correctly handles Monei's cent-based amounts. The post-order handling appropriately covers both redirect and API response scenarios.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
CLAUDE.md (2)
26-30: Call out the missing test & JS-lint pipelines more explicitlyYou mention “No linting command” and “No test suite implemented”. Consider adding TODO bullets or a “Roadmap” section with actionable next steps (e.g. “Add ESLint with Airbnb preset”, “Add PHPUnit functional tests”) so contributors know these are open tasks.
41-43: Minor punctuation / list formatting nitThe nested list items under “Key Directories” start with an en-dash and a backtick‐enclosed path (
- \Repository/``), causing the grammar checker warning. Drop the leading dash inside sub-items to avoid the duplicate punctuation:- `/src`: Business logic organized by type - `Entity/`: … - `Repository/`: …
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CLAUDE.md(1 hunks)translations/en.php(1 hunks)views/templates/hook/displayAdminOrder.tpl(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- translations/en.php
🚧 Files skipped from review as they are similar to previous changes (1)
- views/templates/hook/displayAdminOrder.tpl
🧰 Additional context used
🪛 LanguageTool
CLAUDE.md
[uncategorized] ~41-~41: Loose punctuation mark.
Context: ...ei2CustomerCard, etc.) - Repository/: Data access layer - Service/: Core ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~43-~43: Loose punctuation mark.
Context: ...enizationService, etc.) - Exception/: Custom exceptions - /controllers: Pre...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor BugBot
🔇 Additional comments (1)
CLAUDE.md (1)
68-71: Verify the existence ofMonei::getService()helperThe snippet shows:
$paymentService = Monei::getService('monei2.payment_service');If the module does not define a static
getService()wrapper around PrestaShop’s DI container, this example will mislead readers. Double-check that such a method exists (or document the proper alternative, e.g.ServiceLocator::get()).
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…lations in Monei module
There was a problem hiding this comment.
Bug: JSON Validation Fails on Falsy Values
The JSON validation logic is flawed. After switching to json_decode(), the check if (!$json_array) incorrectly flags valid JSON payloads that decode to falsy values (e.g., null, false, 0, "", or []) as invalid. This incorrectly throws an "Invalid JSON" exception. It should instead check json_last_error() !== JSON_ERROR_NONE to properly detect parsing failures.
controllers/front/validation.php#L43-L47
MONEI-PrestaShop/controllers/front/validation.php
Lines 43 to 47 in af851d7
Bug: Cart Amount Calculation Precision Error
The getCartAmount method incorrectly calculates the precision multiplier as $decimals * 2, leading to amounts being rounded to 4 decimal places. Additionally, the final conversion to cents using (int) number_format() is fragile and prone to floating-point precision errors.
src/Service/Monei/MoneiService.php#L131-L134
MONEI-PrestaShop/src/Service/Monei/MoneiService.php
Lines 131 to 134 in af851d7
Bug: Order ID Generation and Extraction Failures
The createMoneiOrderId method can generate duplicate order IDs due to using time() % 1000 as a suffix, leading to collisions for orders created within the same second, especially in high-traffic scenarios. Additionally, the extractCartIdFromMoneiOrderId method fails to handle cases where the 'm' separator is absent, causing substr() to receive an invalid length and resulting in incorrect cart ID extraction.
src/Service/Monei/MoneiService.php#L106-L116
MONEI-PrestaShop/src/Service/Monei/MoneiService.php
Lines 106 to 116 in af851d7
Was this report helpful? Give feedback by reacting with 👍 or 👎
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
translations/es.php (1)
14-14: Consider extracting the validation message format.The JSON validation error message uses a placeholder for the component name. Consider if this pattern should be standardized across all JSON validation messages for consistency.
The translation "no tiene un formato JSON válido" could be made more specific by following the pattern used in lines 149-150 where the error details are included.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
translations/en.php(1 hunks)translations/es.php(4 hunks)views/templates/admin/configure.tpl(2 hunks)views/templates/hook/displayAdminOrder.tpl(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- translations/en.php
- views/templates/admin/configure.tpl
- views/templates/hook/displayAdminOrder.tpl
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor BugBot
🔇 Additional comments (7)
translations/es.php (7)
5-6: New customer-focused translations look good.The translations for "Refund completed" and "My credit cards" are accurate and align with the new customer card management functionality mentioned in the AI summary.
17-20: Test environment translations are well-implemented.The addition of production/test environment toggles and separate test API credentials translations provides proper localization for the enhanced testing capabilities. The Spanish translations are accurate and consistent with the existing terminology.
Also applies to: 25-28
30-30: Enhanced tokenization description is more user-friendly.The updated description for credit card tokenization ("Permite al cliente guardar la información tokenizada de sus tarjetas de crédito para pagos posteriores") is more detailed and user-friendly than what would have been there before.
110-133: Admin order management translations are comprehensive.The extensive set of translations for the admin order display functionality is well-structured and provides good localization for the new admin interface. The translations cover payments, refunds, status management, and employee tracking appropriately.
138-144: Customer cards interface translations are complete.The customer card management translations provide a complete interface for users to manage their saved cards, including table headers, empty state messages, and deletion confirmations.
148-162: JSON validation messages are thorough and professional.The comprehensive set of JSON validation error messages demonstrates excellent attention to error handling and user experience. These translations cover all JSON parsing errors and provide specific validation messages for Card Input, Bizum, and Payment Request style configurations.
91-93: Verify refund confirmation translations in the UI
I couldn’t find any direct references to these keys in the PHP controllers or template files—only their definitions in translations/en.php and translations/es.php. Please confirm that:
- The refund confirmation modal (in your refund controller or .tpl/.twig template) is using
monei_ccaf4ee393d094ecde7f21b15fdf8f1f(“Are you sure you want to refund this order?”)monei_c3904b9e7f6a2d9a816f93fd8fe0af80(“Yes, make the refund”)- The wording still matches the actual refund workflow in the new service architecture.
Refactored payment method eligibility logic to ensure Apple Pay is only available on Safari and Google Pay is not shown on Safari. Also set a custom user agent for MONEI API requests and updated Apple Pay domain registration to apply in all environments.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Service/Monei/MoneiService.php(1 hunks)src/Service/Payment/PaymentOptionService.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Service/Payment/PaymentOptionService.php
🔇 Additional comments (14)
src/Service/Monei/MoneiService.php (14)
35-52: Well-structured service class with proper dependency injectionThe class constructor properly implements dependency injection with all required repositories. The service architecture follows good separation of concerns.
54-67: Clean client initialization with proper error handlingThe MoneiClient initialization correctly handles environment-specific API keys and includes proper validation. The exception handling is appropriate.
69-93: Robust payment methods retrieval with fallback handlingThe method properly handles API failures by logging warnings and returning an empty array as fallback. The user agent setting is a good practice for API identification.
95-115: Utility methods are well-implementedThe order ID generation with timestamp suffix and extraction logic are clean and functional. The padding ensures consistent format.
117-136: Cart amount calculation correctly excludes duplicate shipping costsBased on the learning from previous reviews, the implementation correctly uses
total_price_without_taxwhich already includes shipping costs, avoiding double-counting.
138-157: Customer data extraction handles validation and sanitization properlyThe method validates both customer and address objects, and includes email sanitization by removing colons. The conditional return for MoneiCustomer object is well-implemented.
160-191: Address data extraction with comprehensive validationThe method properly validates address, country, and state objects with appropriate error handling. The conditional return for billing details object maintains flexibility.
193-214: Payment methods configuration mapping is clean and maintainableThe array-based mapping between configuration keys and payment method names is clear and easy to maintain. The loop logic is straightforward.
216-229: Refund calculation properly uses entity relationshipsThe method correctly uses the payment entity's refund list relationship instead of the problematic
findOneByapproach from previous reviews. This addresses the earlier concern about incorrect repository usage.
231-267: Payment persistence logic is comprehensive and well-structuredThe method handles both new and existing payment entities, properly sets all required fields, and includes history tracking. The refund handling within the same transaction is appropriate.
269-292: Token management includes proper duplicate preventionThe method checks for existing tokens before creating new ones, preventing duplicates. The conditional logic for card payment and token existence is well-implemented.
294-402: Payment creation method is comprehensive with proper error handlingThe method includes extensive validation, proper URL generation for callbacks, and handles both tokenized and regular payments. The try-catch block with logging is appropriate for debugging.
404-439: Refund creation includes proper validation and error handlingThe method addresses previous feedback by including amount validation and checking against available refundable balance. The error handling with specific exception messages is well-implemented.
63-63: No action needed: Exception constants are already definedAll exception codes used in src/Service/Monei/MoneiService.php (e.g. MONEI_CLIENT_NOT_INITIALIZED, CART_AMOUNT_EMPTY, REFUND_CREATION_FAILED, etc.) are declared in src/Exception/MoneiException.php. You can ignore this comment.
Likely an incorrect or invalid review comment.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Removals