magento / community-features

Magento Features Development is an Initiative to Allows Community Memebers Join to Development of Magento Features
46 stars 18 forks source link

Braintree environment specific settings #103

Open leoquijano opened 5 years ago

leoquijano commented 5 years ago

[Moved from Magento 2 main repo: magento/magento2/issues/19699]

Hello,

Summary (*)

When using the sensitive and environment specific settings feature of Magento 2.2 (https://devdocs.magento.com/guides/v2.2/extension-dev-guide/configuration/sensitive-and-environment-settings.html), Braintree correctly configures the payment/braintree/private_key as a sensitive setting.

Note: I'm using Magento 2.2.7.

See: app/code/Magento/Braintree/etc/di.xml

However, it doesn't configure environment specific settings, including:

See: https://articles.braintreepayments.com/control-panel/important-gateway-credentials The following additional settings might be helpful as well:

This means that if you run:

bin/magento app:config:dump

... while having Braintree configured, Magento will export the settings above as shared settings. This means that they will be shared between production and sandbox sites (or any other setups the developer may have), when using the pipeline deployment system.

Examples (*)

This is an issue when configuring different Braintree accounts across production and staging sites, since they will have different settings.

For example, if a production site has live Braintree public and private keys while a staging site has sandbox ones, developers won't be able share the app/etc/config.php settings between them. This is a problem since the purpose of a staging site is to replicate production as close as possible, modeling factor 3 in the 12-factor app design guidelines (as mentioned here).

Proposed solution

Add the settings mentioned above to the environment specific type pool.

In app/code/Magento/Braintree/etc/di.xml.

<type name="Magento\Config\Model\Config\TypePool">
    <arguments>
        <argument name="sensitive" xsi:type="array">
            <!-- ... -->
        </argument>
        <argument name="environment" xsi:type="array">
            <!-- ... -->
            <item name="payment/braintree/environment" xsi:type="string">1</item>
            <item name="payment/braintree/kount_id" xsi:type="string">1</item>
            <item name="payment/braintree/merchant_id" xsi:type="string">1</item>
            <item name="payment/braintree/merchant_account_id" xsi:type="string">1</item>
            <item name="payment/braintree/public_key" xsi:type="string">1</item>
            <item name="payment/braintree/private_key" xsi:type="string">1</item>
            <item name="payment/braintree/debug" xsi:type="string">1</item>
            <item name="payment/braintree_paypal/debug" xsi:type="string">1</item>
        </argument>
    </arguments>
</type>
joni-jones commented 5 years ago

Don't agree that a debug mode configuration should be environment settings. I don't see reasons why it should not be shared.

In my opinion it always should be enabled to catch details of failed transactions on production and staging servers.

leoquijano commented 5 years ago

It would not recommend having debug mode enabled by default in production servers, as a general policy. If an issue comes up, it can be switched on to debug it, and then switched off.

On the other hand, staging/dev servers would have it always enabled.

That is the rationale behind it being environment specific. Now, if someone wants to enable it on production servers at all times, it could be configured like that. But there's definitively an use case for the not shared scenario.

joni-jones commented 5 years ago

Without debug mode you would not have an ability to get failed transaction and enabling it after failure won't help, it's very rare case on production. It's a good practice to log all payment transactions. The debug mode for payment integrations only logs transactions into a separate file and nothing else.

leoquijano commented 5 years ago

I'd rather have it disabled, as a general policy. Depending on transaction volume and other concerns, a team can choose to not enable it.

In any case, it is a decision each developer/team would have to make, so that's why it's a good idea to have it be environment-specific.