[ECP-8668] Refactor the logic of getting capture mode of payment methods#3286
Conversation
…alidations from PaymentMethodInstance
…alidations from PaymentMethodInstance
…alidations from PaymentMethodInstance
…alidations from PaymentMethodInstance
[ECP-9676] Increasing the accuracy of the TxVariant class by adding
* [ECP-9900] Refactor response handlers to get ccType consistently * [ECP-9900] Use ccType while generating wallet card tokens * [ECP-9900] Fix unit tests * [ECP-9900] Run main workflows for feature branches always
…tion for capture_mode (#3234) * [ECP-8668] Deprecate PaymentMethodUtil class * [ECP-8668] Refactor isAutoCapture method * [ECP-8668] Move the config values to an array * [ECP-8668] Update Adyen card variants XML file * [ECP-8668] Rename TxVariantInterpreter to TxVariant * [ECP-8668] Consume newly built TxVariant class * [ECP-8668] Remove deprecated PaymentMethodUtil * [ECP-8668] Update the import statements * [ECP-8668] Wallets check the method instance together with card portion
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
candemiralp
changed the title
candemiralp
added
the
Breaking change
|
|
/gemini review |
![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 refactors the payment method variant handling and capture logic by introducing a TxVariant model and factory, replacing the static PaymentMethodUtil helper. The configuration schema and XML have been updated to include card_type and manual_capture flags for payment types. Review feedback highlights a logic error in manual capture detection for wallets, potential TypeError risks in the factory and response handlers due to missing validation of input data, and redundant object instantiation. Additionally, a typo in the Maestro UK label was identified, and the loss of diagnostic logging during the refactor was noted as a concern for production troubleshooting.
| $supportsManualCapture = filter_var( | ||
| $cardVariants[$txVariant]['manual_capture'] ?? false, | ||
| FILTER_VALIDATE_BOOLEAN | ||
| ) && $this->supportsManualCapture($adyenTxVariant->getMethodInstance()); |
There was a problem hiding this comment.
The logic for checking manual capture support on wallet variants is incorrect. It currently uses the full $txVariant (e.g., mc_googlepay) to look up configuration in $cardVariants, but $cardVariants only contains base scheme keys (e.g., mc). It should use the extracted card part from the TxVariant object instead.
$supportsManualCapture = filter_var(
$cardVariants[$adyenTxVariant->getCard()]['manual_capture'] ?? false,
FILTER_VALIDATE_BOOLEAN
) && $this->supportsManualCapture($adyenTxVariant->getMethodInstance());| if ($isWalletPaymentMethod) { | ||
| // Extract the scheme card brand from the wallet payment response | ||
| $txVariant = $this->txVariantFactory->create([ | ||
| 'txVariant' => $response['paymentMethod']['brand'] |
There was a problem hiding this comment.
Accessing $response['paymentMethod']['brand'] without checking if the key exists can lead to PHP notices and a subsequent TypeError when passing null to the TxVariantFactory::create method (which expects a string). Ensure the brand is present and not empty before attempting to create the variant.
$brand = $response['paymentMethod']['brand'] ?? null;
if ($brand) {
$txVariant = $this->txVariantFactory->create([
'txVariant' => $brand
]);
$ccType = $txVariant->getCard();
}|
|
||
| // Based on the CA config, card type might contain wallet type or not (ie: `mc` or `mc_googlepay`) | ||
| if (empty($cardVariants[$ccType])) { | ||
| $validatedTxVariant = $this->txVariantFactory->create(['txVariant' => $ccType]); |
There was a problem hiding this comment.
If $ccType is null or empty, passing it to TxVariantFactory::create will cause a TypeError because the TxVariant constructor expects a string. Add a check to ensure $ccType is a valid string before calling the factory.
if (!empty($ccType) && empty($cardVariants[$ccType])) {
$validatedTxVariant = $this->txVariantFactory->create(['txVariant' => $ccType]);
$ccType = $validatedTxVariant->getCard();
}| * @param string $notificationPaymentMethod `paymentMethod` provided on the webhook of the given order | ||
| * @return bool | ||
| */ | ||
| public function isAutoCapture(Order $order, string $notificationPaymentMethod): bool |
There was a problem hiding this comment.
The refactored isAutoCapture method has removed all the diagnostic logging that was present in the previous implementation. While the new logic is cleaner, losing these logs can make troubleshooting webhook processing significantly harder in production environments. Consider re-introducing relevant log messages to indicate why a specific capture mode was chosen.
| if ($this->txVariantSupportsManualCapture($notificationPaymentMethod)) { | ||
| $captureModePos = $this->configHelper->getAdyenPosCloudConfigData( | ||
| 'capture_mode_pos', | ||
| $order->getStoreId() | ||
| ); | ||
|
|
||
| /* | ||
| * if you are using authcap the payment method is manual. | ||
| * There will be a capture send to indicate if payment is successful | ||
| */ | ||
| if ($notificationPaymentMethod == "sepadirectdebit") { | ||
| if ($sepaFlow == "authcap") { | ||
| $this->adyenLogger->addAdyenNotification( | ||
| 'Manual Capture is applied for sepa because it is in authcap flow', | ||
| array_merge( | ||
| $this->adyenLogger->getOrderContext($order), | ||
| ['pspReference' => $order->getPayment()->getData('adyen_psp_reference')] | ||
| ) | ||
| ); | ||
| return false; | ||
| // Use order payment method to evaluate the capture mode instead of notification payment method for POS | ||
| if ($order->getPayment()->getMethod() === AdyenPosCloudConfigProvider::CODE && | ||
| $captureModePos === CaptureMode::CAPTURE_MODE_MANUAL) { | ||
| // Evaluate capture mode of POS payments based on the order's `paymentMethod` field and configuration | ||
| $isAutoCapture = false; | ||
| } else { | ||
| // Evaluate capture mode of ECOM payments based on the webhook's `paymentMethod` field | ||
|
|
||
| $sepaFlow = $this->configHelper->getSepaFlow($order->getStoreId()); | ||
| $isPaypalManualCapture = $this->configHelper->isPaypalManualCapture($order->getStoreId()); | ||
| $autoCaptureOpenInvoice = $this->configHelper->getAutoCaptureOpenInvoice($order->getStoreId()); | ||
| $captureMode = $this->configHelper->getCaptureMode($order->getStoreId()); | ||
|
|
||
| /* | ||
| * `adyenTxVariant` can be `null` if a card variant is provided. In this case, skip payment | ||
| * method specific checks and use the global capture mode setting. | ||
| */ | ||
| $adyenTxVariant = $this->txVariantFactory->create(['txVariant' => $notificationPaymentMethod]); |
There was a problem hiding this comment.
There is a redundant instantiation of the TxVariant object. It is first created inside txVariantSupportsManualCapture and then created again on line 775. For better efficiency, consider creating the TxVariant object once at the beginning of isAutoCapture and passing it to txVariantSupportsManualCapture.
| * @param array $data | ||
| * @return TxVariant|null | ||
| */ | ||
| public function create(array $data = []): ?TxVariant |
There was a problem hiding this comment.
The create method should validate that the txVariant key exists in the $data array and is a string before attempting to instantiate the object. This prevents potential TypeError exceptions during object creation.
public function create(array $data = []): ?TxVariant
{
if (!isset($data['txVariant']) || !is_string($data['txVariant'])) {
return null;
}
try {
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the manual capture detection logic by migrating the supported payment methods list from a hardcoded utility to the adyen_payment.xml configuration. It introduces a TxVariant model and factory to resolve payment method instances and card types dynamically. Feedback highlights the need to restore diagnostic logging for observability, suggests optimizing redundant TxVariant object creation, and identifies a potential configuration regression regarding Bancontact's manual capture support.
| public function isAutoCapture(Order $order, string $notificationPaymentMethod): bool | ||
| { | ||
| // TODO::Add a validation checking `$notificationPaymentMethod` belongs to the correct order (webhook) or not. | ||
|
|
||
| $payment = $order->getPayment(); | ||
| $paymentMethodInstance = $payment->getMethodInstance(); | ||
|
|
||
| // validate if payment methods allows manual capture | ||
| if (PaymentMethodUtil::isManualCaptureSupported($notificationPaymentMethod)) { | ||
| $captureMode = trim( | ||
| (string) $this->configHelper->getConfigData( | ||
| 'capture_mode', | ||
| 'adyen_abstract', | ||
| $order->getStoreId() | ||
| ) | ||
| ); | ||
| $sepaFlow = trim( | ||
| (string) $this->configHelper->getConfigData( | ||
| 'sepa_flow', | ||
| 'adyen_abstract', | ||
| $order->getStoreId() | ||
| ) | ||
| ); | ||
| $paymentCode = $order->getPayment()->getMethod(); | ||
| $autoCaptureOpenInvoice = $this->configHelper->getAutoCaptureOpenInvoice($order->getStoreId()); | ||
| $manualCapturePayPal = trim( | ||
| (string) $this->configHelper->getConfigData( | ||
| 'paypal_capture_mode', | ||
| 'adyen_abstract', | ||
| $order->getStoreId() | ||
| ) | ||
| $isAutoCapture = true; | ||
|
|
||
| /* | ||
| * First, validate the incoming tx_variant supports manual capture on the tx_variant level. | ||
| * Then check configuration and payment method specific settings. | ||
| */ | ||
| if ($this->txVariantSupportsManualCapture($notificationPaymentMethod)) { | ||
| $captureModePos = $this->configHelper->getAdyenPosCloudConfigData( | ||
| 'capture_mode_pos', | ||
| $order->getStoreId() | ||
| ); | ||
|
|
||
| /* | ||
| * if you are using authcap the payment method is manual. | ||
| * There will be a capture send to indicate if payment is successful | ||
| */ | ||
| if ($notificationPaymentMethod == "sepadirectdebit") { | ||
| if ($sepaFlow == "authcap") { | ||
| $this->adyenLogger->addAdyenNotification( | ||
| 'Manual Capture is applied for sepa because it is in authcap flow', | ||
| array_merge( | ||
| $this->adyenLogger->getOrderContext($order), | ||
| ['pspReference' => $order->getPayment()->getData('adyen_psp_reference')] | ||
| ) | ||
| ); | ||
| return false; | ||
| // Use order payment method to evaluate the capture mode instead of notification payment method for POS | ||
| if ($order->getPayment()->getMethod() === AdyenPosCloudConfigProvider::CODE && | ||
| $captureModePos === CaptureMode::CAPTURE_MODE_MANUAL) { | ||
| // Evaluate capture mode of POS payments based on the order's `paymentMethod` field and configuration | ||
| $isAutoCapture = false; | ||
| } else { | ||
| // Evaluate capture mode of ECOM payments based on the webhook's `paymentMethod` field | ||
|
|
||
| $sepaFlow = $this->configHelper->getSepaFlow($order->getStoreId()); | ||
| $isPaypalManualCapture = $this->configHelper->isPaypalManualCapture($order->getStoreId()); | ||
| $autoCaptureOpenInvoice = $this->configHelper->getAutoCaptureOpenInvoice($order->getStoreId()); | ||
| $captureMode = $this->configHelper->getCaptureMode($order->getStoreId()); | ||
|
|
||
| /* | ||
| * `adyenTxVariant` can be `null` if a card variant is provided. In this case, skip payment | ||
| * method specific checks and use the global capture mode setting. | ||
| */ | ||
| $adyenTxVariant = $this->txVariantFactory->create(['txVariant' => $notificationPaymentMethod]); | ||
|
|
||
| if (isset($adyenTxVariant)) { | ||
| $webhookMethodInstance = $adyenTxVariant->getMethodInstance(); | ||
| $webhookMethodCode = $webhookMethodInstance->getCode(); | ||
|
|
||
| if (($webhookMethodCode === self::ADYEN_SEPADIRECTDEBIT && $sepaFlow === SepaFlow::SEPA_FLOW_AUTHCAP) || | ||
| ($webhookMethodCode === self::ADYEN_PAYPAL && $isPaypalManualCapture) || | ||
| ($this->isOpenInvoice($webhookMethodInstance) && !$autoCaptureOpenInvoice) || | ||
| ($captureMode === CaptureMode::CAPTURE_MODE_MANUAL) | ||
| ) { | ||
| $isAutoCapture = false; | ||
| } | ||
| } else { | ||
| // payment method ideal, cash adyen_boleto has direct capture | ||
| $this->adyenLogger->addAdyenNotification( | ||
| 'This payment method does not allow manual capture.(2) paymentCode:' . | ||
| $paymentCode . ' paymentMethod:' . $notificationPaymentMethod . ' sepaFLow:' . $sepaFlow, | ||
| array_merge( | ||
| $this->adyenLogger->getOrderContext($order), | ||
| ['pspReference' => $order->getPayment()->getData('adyen_psp_reference')] | ||
| ) | ||
| ); | ||
| return true; | ||
| if ($captureMode === CaptureMode::CAPTURE_MODE_MANUAL) { | ||
| $isAutoCapture = false; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if ($paymentCode == "adyen_pos_cloud") { | ||
| $captureModePos = $this->configHelper->getAdyenPosCloudConfigData( | ||
| 'capture_mode_pos', | ||
| $order->getStoreId() | ||
| ); | ||
| if (strcmp((string) $captureModePos, 'auto') === 0) { | ||
| $this->adyenLogger->addAdyenNotification( | ||
| 'This payment method is POS Cloud and configured to be working as auto capture ', | ||
| array_merge( | ||
| $this->adyenLogger->getOrderContext($order), | ||
| ['pspReference' => $order->getPayment()->getData('adyen_psp_reference')] | ||
| ) | ||
| ); | ||
| return true; | ||
| } elseif (strcmp((string) $captureModePos, 'manual') === 0) { | ||
| $this->adyenLogger->addAdyenNotification( | ||
| 'This payment method is POS Cloud and configured to be working as manual capture ', | ||
| array_merge( | ||
| $this->adyenLogger->getOrderContext($order), | ||
| ['pspReference' => $order->getPayment()->getData('adyen_psp_reference')] | ||
| ) | ||
| ); | ||
| return false; | ||
| } | ||
| } | ||
| return $isAutoCapture; | ||
| } |
There was a problem hiding this comment.
The refactored isAutoCapture method has removed all the diagnostic logging that was present in the previous implementation. These logs are crucial for troubleshooting payment issues, as they explain exactly why a specific capture mode (auto vs manual) was selected for a given order and webhook notification. Please restore the logging statements to maintain observability.
| if ($this->txVariantSupportsManualCapture($notificationPaymentMethod)) { | ||
| $captureModePos = $this->configHelper->getAdyenPosCloudConfigData( | ||
| 'capture_mode_pos', | ||
| $order->getStoreId() | ||
| ); | ||
|
|
||
| /* | ||
| * if you are using authcap the payment method is manual. | ||
| * There will be a capture send to indicate if payment is successful | ||
| */ | ||
| if ($notificationPaymentMethod == "sepadirectdebit") { | ||
| if ($sepaFlow == "authcap") { | ||
| $this->adyenLogger->addAdyenNotification( | ||
| 'Manual Capture is applied for sepa because it is in authcap flow', | ||
| array_merge( | ||
| $this->adyenLogger->getOrderContext($order), | ||
| ['pspReference' => $order->getPayment()->getData('adyen_psp_reference')] | ||
| ) | ||
| ); | ||
| return false; | ||
| // Use order payment method to evaluate the capture mode instead of notification payment method for POS | ||
| if ($order->getPayment()->getMethod() === AdyenPosCloudConfigProvider::CODE && | ||
| $captureModePos === CaptureMode::CAPTURE_MODE_MANUAL) { | ||
| // Evaluate capture mode of POS payments based on the order's `paymentMethod` field and configuration | ||
| $isAutoCapture = false; | ||
| } else { | ||
| // Evaluate capture mode of ECOM payments based on the webhook's `paymentMethod` field | ||
|
|
||
| $sepaFlow = $this->configHelper->getSepaFlow($order->getStoreId()); | ||
| $isPaypalManualCapture = $this->configHelper->isPaypalManualCapture($order->getStoreId()); | ||
| $autoCaptureOpenInvoice = $this->configHelper->getAutoCaptureOpenInvoice($order->getStoreId()); | ||
| $captureMode = $this->configHelper->getCaptureMode($order->getStoreId()); | ||
|
|
||
| /* | ||
| * `adyenTxVariant` can be `null` if a card variant is provided. In this case, skip payment | ||
| * method specific checks and use the global capture mode setting. | ||
| */ | ||
| $adyenTxVariant = $this->txVariantFactory->create(['txVariant' => $notificationPaymentMethod]); |
There was a problem hiding this comment.
The TxVariant object for the notificationPaymentMethod is being created twice: once inside txVariantSupportsManualCapture (line 814) and again in isAutoCapture (line 775). This is redundant and inefficient. Consider refactoring txVariantSupportsManualCapture to return the TxVariant object (or null if it cannot be resolved) so it can be reused throughout the isAutoCapture logic.
| <label>Bancontact</label> | ||
| <code_alt>bcmc</code_alt> | ||
| <card_type>scheme</card_type> | ||
| <manual_capture>0</manual_capture> |
There was a problem hiding this comment.
The manual_capture support for Bancontact (BCMC) has been set to 0 (false). In the previous implementation (Helper/Util/PaymentMethodUtil.php), bcmc was included in the MANUAL_CAPTURE_SUPPORTED_PAYMENT_METHODS list. This change in configuration will result in Bancontact payments always being treated as auto-capture, which might be a regression if manual capture was intended to be supported for this method.
shubhamk67
force-pushed
the
feature/TxVariant-ManualCaptureHandling
branch
from
fea941a to
2fd2269
Compare
5 participants
Description
This PR implements the functionality to leverage payment method configuration objects under config.xml to identify whether manual capture is supported or not. Also, etc/adyen_payment.xml document is being used to identify the mode of scheme payment methods.
Besides these changes, Helper/Util/PaymentMethodUtil class has been deleted in favor of using structured configuration fields of the payment methods.