Conversation
…rms-on-prestashop-plugin Shabib/cet 926 eom style order terms on prestashop plugin
…load-in-prestashop-plugin KNA-3211/feat: Enabling invoice upload and minor improvements
…back) across order payload lines
…lment transitions, and sync ES translations
…presta-shop-and-improvements KNA-3366/feat: Basic tests and general order logic improvements
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Two Payment module for PrestaShop by implementing a provider-first checkout flow, which prioritizes Two's verification before local order creation, thereby eliminating phantom orders. It introduces critical security and data integrity measures, including cart snapshot validation and idempotency keys, alongside comprehensive improvements to tax and discount calculations for precise payload accuracy. The update also expands payment options with End-of-Month terms, refines user experience through better validation and error messaging, and integrates extensive AI development context and testing tools to ensure a robust and maintainable codebase. Highlights
Changelog
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request merges a significant number of changes from staging to main, introducing major architectural improvements and numerous fixes. Key enhancements include a more robust provider-first checkout flow, idempotency keys for order creation, and cart snapshot validation to prevent inconsistencies. The changes also address security vulnerabilities by escaping output and preventing auth header leakage. Overall, the code quality is high. I have identified one potential issue in the legacy order cancellation logic that could lead to data inconsistency between PrestaShop and the payment provider.
| $orderpaymentdata = $this->module->getTwoOrderPaymentData($id_order); | ||
| if ($orderpaymentdata && isset($orderpaymentdata['two_order_id'])) { | ||
| $two_order_id = $orderpaymentdata['two_order_id']; | ||
|
|
||
| $cancel_response = $this->module->setTwoPaymentRequest('/v1/order/' . $two_order_id . '/cancel', [], 'POST'); | ||
| $cancel_http_status = isset($cancel_response['http_status']) ? (int)$cancel_response['http_status'] : 0; | ||
|
|
||
| $response = $this->module->setTwoPaymentRequest('/v1/order/' . $two_order_id, [], 'GET'); | ||
| $response_http_status = isset($response['http_status']) ? (int)$response['http_status'] : 0; | ||
| $provider_cancelled = $this->module->isTwoOrderCancelledResponse($response, $response_http_status); | ||
|
|
||
| if ($provider_cancelled) { | ||
| $resolved_terms = $this->module->resolveTwoPaymentTermsFromOrderResponse( | ||
| $response, | ||
| isset($orderpaymentdata['two_day_on_invoice']) ? (string)$orderpaymentdata['two_day_on_invoice'] : (string)$this->module->getSelectedPaymentTerm(), | ||
| isset($orderpaymentdata['two_payment_term_type']) ? $orderpaymentdata['two_payment_term_type'] : Configuration::get('PS_TWO_PAYMENT_TERM_TYPE') | ||
| ); | ||
| $payment_data = array( | ||
| 'two_order_id' => $response['id'], | ||
| 'two_order_reference' => $response['merchant_reference'], | ||
| 'two_order_state' => $response['state'], | ||
| 'two_order_status' => $response['status'], | ||
| 'two_day_on_invoice' => $resolved_terms['two_day_on_invoice'], | ||
| 'two_invoice_url' => $response['invoice_url'], | ||
| 'two_payment_term_type' => $resolved_terms['two_payment_term_type'], | ||
| ); | ||
| $this->module->setTwoOrderPaymentData($order->id, $payment_data); | ||
| } else { | ||
| PrestaShopLogger::addLog( | ||
| 'TwoPayment: Legacy cancel callback could not confirm CANCELLED provider state for order ' . (int)$order->id . | ||
| ', Two order ' . $two_order_id . | ||
| ', cancel_http=' . $cancel_http_status . | ||
| ', fetch_http=' . $response_http_status . | ||
| ', provider_state=' . (isset($response['state']) ? (string)$response['state'] : 'unknown'), | ||
| 2 | ||
| ); | ||
| $message = sprintf($this->module->l('Could not update status to cancelled, please check with Two admin for id %s'), $two_order_id); | ||
| $this->errors[] = $message; | ||
| $this->redirectWithNotifications('index.php?controller=order'); | ||
| } | ||
| } | ||
|
|
||
| $this->module->restoreDuplicateCart($order->id, $order->id_customer); | ||
| $this->module->changeOrderStatus($order->id, $this->getCancelledStatus()); | ||
|
|
||
| $message = $this->module->l('Your order is cancelled.'); | ||
| $this->errors[] = $message; | ||
| $this->redirectWithNotifications('index.php?controller=order'); | ||
| } |
There was a problem hiding this comment.
In handleLegacyOrderCancel, if $orderpaymentdata is not found for the given id_order, the logic proceeds to line 184 and cancels the PrestaShop order locally without attempting to cancel the corresponding order on the Two provider's side. This can lead to data inconsistency between the two systems.
An order paid with Two should have this data. Its absence is an exceptional case. Instead of cancelling the local order, it would be safer to log a critical error and inform the user that the cancellation could not be fully processed.
Consider moving the local cancellation logic (lines 184-189) inside the if ($provider_cancelled) block to ensure it only happens after a confirmed provider-side cancellation. You should also add an else block to handle the case where $orderpaymentdata is missing.
1 participant
No description provided.