Skip to content

Conversation

vovayatsyuk
Copy link
Member

Description

This commit fixes the case when customer navigates back
to the shipping step and change address fields.

Manual testing scenarios

  1. Navigate to checkout, fill address and proceed to the Payment step.
  2. Select "PayPal (Braintree)" method.
  3. Go back to the shipping information step, change first and last names and proceed to the Payment step again.
  4. Click "Pay with PayPal" button.
  5. Popup will be opened with the first entered address instead of the latest one.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

This commit fixes the case when customer navigates back
to the shipping step and change address fields.
@VladimirZaets
Copy link
Contributor

Hi, @vovayatsyuk, I took your PR into processing, thank you for collaboration.

@VladimirZaets
Copy link
Contributor

Hi @vovayatsyuk, please fix failed tests. Thanks

@vovayatsyuk
Copy link
Member Author

Done.

@VladimirZaets
Copy link
Contributor

@vovayatsyuk thanks.

@VladimirZaets
Copy link
Contributor

@magento-engcom-team give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @VladimirZaets. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @VladimirZaets, here is your Magento instance.
Admin access: https://pr-15133.engcom.dev.magento.com/admin
Login: admin Password: 123123q
Please make sure you are PR author or assignee to access the instance.

@magento-engcom-team
Copy link
Contributor

Hi @vovayatsyuk. Thank you for your contribution.
Changes from your Pull Request will be available with the upcoming 2.2.5 release.

Please, consider porting this solution across release lines.
You may use Porting tool to port commits automatically.

vovayatsyuk pushed a commit to vovayatsyuk/magento2 that referenced this pull request May 24, 2018
magento-engcom-team added a commit that referenced this pull request Jul 26, 2018
…tree's 'Pay with PayPal' button #15133 #15482

 - Merge Pull Request #15482 from vovayatsyuk/magento2:2.3-fix-braintree-outdated-address
 - Merged commits:
   1. 2d9734d
magento-engcom-team pushed a commit that referenced this pull request Jul 26, 2018
magento-engcom-team added a commit that referenced this pull request Jul 26, 2018
Accepted Public Pull Requests:
 - #15482: [Forwardport] Fix outdated address data when using Braintree's "Pay with PayPal" button #15133 (by @vovayatsyuk)
 - #17067: Fixed invalid knockoutjs data binding for Braintree PayPal (by @joni-jones)
@bassplayer7
Copy link

Interestingly enough, this change appears to cause Chrome, and perhaps other browsers, to block this Braintree Paypal payment window as a popup. I'm curious if others are noticing the same behavior. I can post the mixin I wrote to patch it and provide more details if so.

@vovayatsyuk
Copy link
Member Author

@bassplayer7 true.

My patch made to call initAuthFlow as a callback of Braintree load event. Popup is blocked by the browsers because of that.

Previously initAuthFlow was called right after user click.

So what about your patch? How you fixed it?

@bassplayer7
Copy link

@vovayatsyuk, ultimately, I used the window hashchange to watch for the payment form coming up again. There may be a different even that can be subscribed to. This allowed me to have Braintree fully initialized when the button was clicked:

initialize: function() {
    this._super();

    $(window).hashchange(function() {
        if (window.location.hash.indexOf('payment') !== -1) {
            this.reInitPayPal.call(this, function() {})
        }
    }.bind(this));
}

This was my whole mixin:

define([
    'jquery',
    'Magento_Braintree/js/view/payment/adapter',
    'Magento_Checkout/js/model/payment/additional-validators',
    'jquery/jquery.hashchange'
], function($, Braintree, additionalValidators) {

    /**
     * Braintree must be started synchronously. The core now starts it via a callback that is passed to Braintree.
     * @see Magento_Braintree/js/view/payment/method-renderer/paypal::reInitPayPal()
     *
     * @link https://github.com/magento/magento2/pull/15133 (description)
     *
     * @type {{initialize: initialize, payWithPayPal: payWithPayPal}}
     */
    var mixin = {
        initialize: function() {
            this._super();

            $(window).hashchange(function() {
                if (window.location.hash.indexOf('payment') !== -1) {
                    this.reInitPayPal.call(this, function() {})
                }
            }.bind(this));
        },

        payWithPayPal: function () {
            if (!additionalValidators.validate()) {
                return;
            }

            try {
                Braintree.checkout.paypal.initAuthFlow();
            } catch (e) {
                this.messageContainer.addErrorMessage({
                    message: $t('Payment ' + this.getTitle() + ' can\'t be initialized.')
                });
            }
        }
    };

    return function(target) {
        return target.extend(mixin);
    }
})

@vovayatsyuk
Copy link
Member Author

I see.

Your solution is like to call reInit on "Next Step" click (after submitShippingInformation) instead of "Place Order".

Possible downside: when you will apply discount code, total will not updated in Braintree config.

p.s. I was looking for any other method except braintree.setup that could be used to update address and total in braintree object to get rid of PR callback.
Unfortunately, looks like it's the only way to do that. Whole braintree logic is built around 2 steps separated in time (you can't run them in chain without issue with popup): setup, initAuthFlow.

@bassplayer7
Copy link

@vovayatsyuk, that's a good point. I wonder if it would be feasible to use a Knockout observer for watching any changes to the payment information, and then reInit based on that.

@vovayatsyuk
Copy link
Member Author

Dirty and not 100% solution (popup still blocked if setup takes more than 1 second). Additionally, I don't know if IE allows popups from setTimeout.

diff --git a/app/code/Magento/Braintree/view/frontend/web/js/view/payment/method-renderer/paypal.js b/app/code/Magento/Braintree/view/frontend/web/js/view/payment/method-renderer/paypal.js
index 253f3530701..80a1ed73b25 100644
--- a/app/code/Magento/Braintree/view/frontend/web/js/view/payment/method-renderer/paypal.js
+++ b/app/code/Magento/Braintree/view/frontend/web/js/view/payment/method-renderer/paypal.js
@@ -53,6 +53,8 @@ define([
              * {Object}
              */
             clientConfig: {
+                ready: false,
+
                 dataCollector: {
                     paypal: true
                 },
@@ -63,6 +65,7 @@ define([
                  */
                 onReady: function (checkout) {
                     Braintree.checkout = checkout;
+                    this.clientConfig.ready = true;
                     this.additionalData['device_data'] = checkout.deviceData;
                     this.enableButton();
                     Braintree.onReady();
@@ -230,18 +233,25 @@ define([
             }
 
             this.disableButton();
+            this.clientConfig.ready = false;
             this.clientConfig.paypal.amount = this.grandTotalAmount;
             this.clientConfig.paypal.shippingAddressOverride = this.getShippingAddress();
 
             if (callback) {
-                this.clientConfig.onReady = wrapper.wrap(
-                    this.clientConfig.onReady,
-                    function (original, checkout) {
-                        this.clientConfig.onReady = original;
-                        original(checkout);
+                /**
+                 * Do not use onReady because Braintree's popup will be blocked
+                 * by the browser if called from callback.
+                 *
+                 * 999 - Is the the max wait time between `click` and `window.open`.
+                 *       When the time is longer - browser blocks popup.
+                 */
+                setTimeout(function cb() {
+                    if (this.clientConfig.ready) {
                         callback();
-                    }.bind(this)
-                );
+                    } else {
+                        setTimeout(cb.bind(this), 200);
+                    }
+                }.bind(this), 999);
             }
 
             Braintree.setConfig(this.clientConfig);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants