Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ECP-9175] Options payment/*/active are not scopable #2596

Open
dimitriBouteille opened this issue Apr 25, 2024 · 5 comments
Open

[ECP-9175] Options payment/*/active are not scopable #2596

dimitriBouteille opened this issue Apr 25, 2024 · 5 comments
Assignees
Labels
Bug report Indicates that issue has been marked as a possible bug

Comments

@dimitriBouteille
Copy link
Contributor

Describe the bug

I think there is a problem with the Payment methods enabled option, in Magento this option is of type [store view]. When saving this option, the payment/*/active configurations are saved at the default level while this configuration is normally scopable.

Capture d’écran du 2024-04-25 11-00-55

When I look at the togglePaymentMethodsActivation function, no store is specified so it is the default store that is used.

public function togglePaymentMethodsActivation(?bool $isActive =null): array
{
$enabledPaymentMethods = [];
if (is_null($isActive)) {
$isActive = $this->configHelper->getIsPaymentMethodsActive();
}
foreach ($this->getAdyenPaymentMethods() as $paymentMethod) {
if (in_array($paymentMethod, self::EXCLUDED_PAYMENT_METHODS)) {
continue;
}
$value = $isActive ? '1': '0';
$field = 'active';
$this->configHelper->setConfigData($value, $field, $paymentMethod);
$enabledPaymentMethods[] = $paymentMethod;

Expected behavior

Choose from two options:

  • Make this configuration scopable
  • Make this configuration global

Versions

  • Magento: 2.4.5
  • Plugin version: 9.x
@dimitriBouteille dimitriBouteille added the Bug report Indicates that issue has been marked as a possible bug label Apr 25, 2024
@RokPopov RokPopov changed the title Options payment/*/active are not scopable [ECP-9175] Options payment/*/active are not scopable May 1, 2024
@RokPopov RokPopov self-assigned this May 1, 2024
@RokPopov
Copy link
Contributor

RokPopov commented May 1, 2024

Hi @dimitriBouteille, thank you for reporting this.

I have opened an internal ticket to address this issue. Once the PR introducing the fix is merged, this issue will be automatically closed.

Kind regards,
Rok

@Anve94
Copy link

Anve94 commented May 16, 2024

Hi @RokPopov, is there an ETA on this? Then I can determine whether I write my own fix for this or wait for the official update from Adyen.

@Anve94
Copy link

Anve94 commented May 16, 2024

As a follow-up, I do believe moving towards website/store support is needed. To clarify why, let's imagine the following scenario:

Merchant has 3 website scopes: United Kingdom, Germany and Austria. In Germany and Austria, Adyen is used. In United Kingdom, another local PSP is used.

If I were this merchant, in the old flow (before 9.x) I would just:

  • Configure the Germany website scope for a German Adyen merchant account. Add the key, merchant account, other credentials to that scope and enable adyen_cc and adyen_hpp in the scope.
  • Do the same for Austria
  • Use the other PSP for United Kingdom according to their own requirements

Pretty straight forward: just configure the scope where it's needed and you're good to go.

In the new flow, as it stands, I would need to do the following:

  • Enable payment methods in the default scope because otherwise the recurring setup script doesn't enable any payment methods on the default scope
  • Configure the merchant account and appropriate credentials in the Germany and Austria scope. Configuring the payment method active field is not needed since it is inherited from the default scope.
  • Disable payment methods in the United Kingdom scope, and remember that for every new store in the future it should be disabled in that scope.

It's mostly this last part that's a bit irksome. In general I think it would be a cleaner solution if the recurring setup script would iterate over all the scopes, look for the payment methods config in that scope, and mutate the payment methods in that scope. This would be a win-win for everyone:

  • Developers/merchants that predominantly use the default scope can just enable the payment methods configuration in the default scope. The recurring script will only check that scope and configure all the payment methods in the default scope through the script.
  • If developers/merchants don't want to rely on the configuration fallback to much, they can just disable the payment method config field in the default scope, and enable it only for the websites/stores where it is relevant. This also prevents the issue where irrelevant stores not using Adyen will need to be configured explicitly to turn it off again and prevent inheriting from default.

@Anve94
Copy link

Anve94 commented May 22, 2024

As another follow-up, it just so happens that the default store scope for the merchant I'm upgrading adyen does not use adyen in that store. As a result, I either need to enable the payment methods in a store where Adyen is not used (making the configuration inaccurate), or fix the recurring data script.

In my case, we have the following setup:

  • Default scope (ID 0)
    • Website one (ID 1)
      • Is the default website
      • Store one (ID 1)
        • Is the default store of the website
        • Does not use Adyen
    • Website two (ID 2)
      • Store two (ID 2)
        • Does use adyen
    • Website 3 through 19
      • Various localized stores, some using Adyen and some not

The recurring script, which is run during every setup:upgrade, uses the scope store with a scope code of NULL and relies on the Magento scope resolver to determine the scope to read the payment/adyen_abstract/payment_methods_enabled from. This is problematic, since Magento resolves it to 1 which does not have Adyen enabled.

In the scenario above, we also do not want to enable Adyen for store scope 1 because:

  1. It will cause inaccurate configuration to be shown in the admin. Adyen is not used for this store but needs to have this configuration enabled to prevent issues with the recurring script
  2. While other payment methods will still be rendered, Adyen tries to load payment methods which either loads payment methods from the default scope (which we don't want) or produce a knockout js error (if no merchant account is configured).

I think long-term it would be great if Adyen can take a look at this flow again and offer proper multi-store support. In the interim, since I don't know if Adyen wants to adopt multi-store support, I am taking the quick route here and will patch the recurring script to enforce loading the config value from the default scope. This way, we can enable adyen in the admin/default scope, but still have it turned off for store 1 in the scenario above. This is still not ideal, since we need to configure adyen for every store that exists. With proper multistore support, it would be easier if we can just configure adyen on only the stores where it's needed. Now, we still need to make sure to explicitly configure adyen and turn it off in every irrelevant store, to prevent it from falling back to default.

Patch:

Context: Adyen assumes the PSP is enabled across the board and is lacking multistore support. When the default store configured (e.g. store 1, not store 0) does not use Adyen,
the plugin will disable all payment methods during every setup:upgrade, e.g. during every production release.
Issue: Adyen reads the payment_methods_active configuration through both an afterSave and during a Recurring script. There is no multi-store support for the recurring script.
While one may think the default scope configuration (with ID of 0) is read, this is not the case since the Magento ScopeResolver resolves the scope ID to the default website/store scope configured.
Let's say we have a website / store combination and the website is considered the default website and the store is the default store of that website. The store has an ID of 1.
The recurring adyen scripts will resolve to the store ID of 1 to read the config and determine if adyen payment methods are enabled. This is fine if Adyen is used in that store, but not fine if another PSP is used.
Enabling the configuration for store with ID 1 will produce knockout exceptions in the checkout (since Adyen should not be used here), and disabling it in store ID 1 will disable all payment methods every setup upgrade
since adyen only reads the active payment methods config from this store.
Resolution: Apply patch to ensure the recurring datascript reads the payment_methods_active configuration from the default/admin scope instead of resolving to the default store/website configured which
may not even use Adyen. This patch is still not ideal, since we'd expect adyen to be configurable on a per-store / per-website basis, instead of only in the global default scope.
Source: https://github.com/Adyen/adyen-magento2/issues/2596
===================================================================
diff --git a/vendor/adyen/module-payment/Helper/PaymentMethods.php b/vendor/adyen/module-payment/Helper/PaymentMethods.php
--- a/vendor/adyen/module-payment/Helper/PaymentMethods.php
+++ b/vendor/adyen/module-payment/Helper/PaymentMethods.php	(date 1716370283308)
@@ -168,7 +168,7 @@
         $enabledPaymentMethods = [];

         if (is_null($isActive)) {
-            $isActive = $this->configHelper->getIsPaymentMethodsActive();
+            $isActive = $this->configHelper->getIsPaymentMethodsActive('0');
         }

         foreach ($this->getAdyenPaymentMethods() as $paymentMethod) {
@@ -190,6 +190,10 @@
         $quote = $this->getQuote();
         $store = $quote->getStore();

+        if (!$this->configHelper->getIsPaymentMethodsActive($store->getId())) {
+            return json_encode([]);
+        }
+
         $merchantAccount = $this->configHelper->getAdyenAbstractConfigData('merchant_account', $store->getId());
         if (!$merchantAccount) {
             return json_encode([]);

Edit: this patch is incomplete since Adyen will still load the payment methods based on the adyen_cc/active flags etc. from the default scope since the payment_methods_active is only used to generate all the other flags it seems... this is getting quite obnoxious to work with in multistore setups at this point.

Edit 2: updated the patch to do another check before the payment methods are retrieved from the adyen backoffice. If payment methods are disabled for Adyen, retrieving them is not needed. Do note that this will cause a knockout error during checkout when adyen components are being loaded, because the component itself cannot handle there being no paymentMethods response on the object. The same would happen if no merchant account was configured. The combination of the 2 changes appear to work for me (ignoring the knockout error when the payment response is empty...) when the default store does not use Adyen. Works in both store ID 1 and a store actually using Adyen.

@dimitriBouteille
Copy link
Contributor Author

@RokPopov

I think this issue is now fixed with this MR #2587, I'm wrong ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug report Indicates that issue has been marked as a possible bug
Projects
None yet
Development

No branches or pull requests

3 participants