Since Adyen Web v6 the countryCode is a mandatory configuration property#39
Conversation
Summary of ChangesHello @remcotolsma, 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 addresses a breaking change introduced in Adyen Web v6, where the Highlights
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
|
There was a problem hiding this comment.
Code Review
This pull request correctly identifies that the countryCode is a mandatory property for Adyen Web v6 and adds it to the checkout configuration. My review focuses on ensuring this change is robust and handles cases where a country code might not be immediately available from the payment data, preventing potential runtime errors.
src/Gateway.php
Outdated
| * | ||
| * @link https://docs.adyen.com/online-payments/upgrade-your-integration/migrate-to-web-v6 | ||
| */ | ||
| 'countryCode' => Util::get_country_code( $payment ), |
There was a problem hiding this comment.
The countryCode is a mandatory property, but Util::get_country_code() can return null if the payment has no billing address or country. This will likely cause payment failures.
To make this more robust, you should handle the null case. The suggestion below adds a fallback to the shipping address and throws an exception if no country code can be found, ensuring a valid country code is always sent to Adyen.
( function () use ( $payment ) {
$country_code = Util::get_country_code( $payment );
if ( null === $country_code ) {
$shipping_address = $payment->get_shipping_address();
if ( null !== $shipping_address ) {
$country_code = $shipping_address->get_country_code();
}
}
if ( null === $country_code ) {
throw new \Exception( 'A country code is required for Adyen payments, but none could be determined from the payment details.' );
}
return $country_code;
} )()|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly adds the mandatory countryCode configuration property for Adyen Web v6. The changes are well-implemented across the configuration, gateway, and integration layers. The logic to fall back from the payment's country code to the one in the settings is a good approach. I have one suggestion to improve the user experience and code consistency when configuring the new setting.
| $fields[] = [ | ||
| 'section' => 'general', | ||
| 'meta_key' => '_pronamic_gateway_adyen_country_code', | ||
| 'title' => \__( 'Country code', 'pronamic_ideal' ), | ||
| 'type' => 'text', | ||
| 'classes' => [ 'regular-text', 'code' ], | ||
| 'tooltip' => __( 'Country code used when it cannot be derived from the customer details.', 'pronamic_ideal' ), | ||
| 'required' => true, | ||
| 'default' => 'NL', | ||
| ]; |
There was a problem hiding this comment.
This new settings field block can be improved in a couple of ways:
- Default value: Providing a default country code (
'NL') might lead to misconfigurations for users not based in the Netherlands. Since this field is mandatory (requiredistrue), it's better to let the user explicitly provide the value. This ensures they are aware of the setting and provide a value that is correct for their situation. Other required fields in this configuration don't have a default value, so removing this would also make it consistent. - Translation function call: For consistency with other translation function calls in this file (and to ensure the global WordPress function is called from within the namespace),
__(...)for thetooltipshould be\__(...).
$fields[] = [
'section' => 'general',
'meta_key' => '_pronamic_gateway_adyen_country_code',
'title' => \__( 'Country code', 'pronamic_ideal' ),
'type' => 'text',
'classes' => [ 'regular-text', 'code' ],
'tooltip' => \__( 'Country code used when it cannot be derived from the customer details.', 'pronamic_ideal' ),
'required' => true,
];|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly adds the mandatory country_code property to the Adyen gateway configuration to comply with Adyen Web v6. The changes include adding the property to the config object, creating a setting in the admin UI, and passing it to the Adyen Drop-in component. My review includes a few suggestions for improvement. I've pointed out a potential issue where the new country_code logic is not applied to backend API requests, which could lead to payment failures. I've also recommended adding a check to prevent sending an empty country code to Adyen. Additionally, I've suggested improving the new admin setting for better usability and data validation, and a minor correction to a docblock for accuracy.
| /** | ||
| * Country code. | ||
| * | ||
| * Since Adyen Web v6 the `countryCode` is a mandatory configuration property. | ||
| * | ||
| * @link https://docs.adyen.com/online-payments/upgrade-your-integration/migrate-to-web-v6 | ||
| */ | ||
| 'countryCode' => Util::get_country_code( $payment ) ?? $this->config->country_code, |
There was a problem hiding this comment.
This correctly adds the countryCode for the frontend checkout configuration. However, there are two related issues that should be addressed:
-
Inconsistent logic for backend requests: The
countryCodeis also a required parameter for backend API calls to/sessions(used by Drop-in) and/payments(used by API-only integrations). ThePaymentRequestHelper::complement()method, which prepares these requests, is not updated in this PR. It currently only attempts to get the country code from the payment details (Util::get_country_code( $payment )) and does not have the fallback to the gateway configuration. This will cause payments to fail when the country code isn't available on the payment object. You should updatePaymentRequestHelper::complement()to use the same logic as here:Util::get_country_code( $payment ) ?? $config->country_code. -
Risk of empty
countryCode: SincecountryCodeis a mandatory field for Adyen, sending an empty string will likely result in an error. For existing configurations that haven't been updated with a country code,$this->config->country_codecould be an empty string. IfUtil::get_country_code( $payment )also returnsnull, an empty string will be sent to Adyen. It would be safer to add a check to ensure that a valid country code is available before proceeding with the payment, and throw an exception if it's missing.
Addressing these points will make the integration more robust.
| /** | ||
| * Country code. | ||
| * | ||
| * @var string |
| $fields[] = [ | ||
| 'section' => 'general', | ||
| 'meta_key' => '_pronamic_gateway_adyen_country_code', | ||
| 'title' => \__( 'Country code', 'pronamic_ideal' ), | ||
| 'type' => 'text', | ||
| 'classes' => [ 'regular-text', 'code' ], | ||
| 'tooltip' => \__( 'Country code used when it cannot be derived from the customer details.', 'pronamic_ideal' ), | ||
| 'required' => true, | ||
| ]; |
There was a problem hiding this comment.
To improve user experience and prevent invalid input, consider changing this field's type from text to a select dropdown populated with a list of countries. This would ensure that users select a valid ISO 3166-1 alpha-2 country code.
Also, the tooltip could be more specific by mentioning the expected format, for example: 'Two-letter ISO 3166-1 alpha-2 country code (e.g., NL, US) used when it cannot be derived from the customer details.'
No description provided.