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

Correctly set payment information when using paypal #12401

Merged
merged 10 commits into from
Jan 3, 2018

Conversation

therool
Copy link
Contributor

@therool therool commented Nov 22, 2017

Description

Correctly set payment method information when using PayPal Express so the checkout agreements data is correctly added to the request and validated correctly.
Edited the Magento_Paypal /js/action/set-payment-method to use the Magento_Checkout/js/action/set-payment-information so the additional payment/checkout data is correctly added.

Fixed Issues (if relevant)

  1. Magento 2.2 Paypal Can't Accept Checkout Agreements Before Routing to PayPal #11885: Paypal Can't Accept Checkout Agreements Before Routing to PayPal

Manual testing scenarios

  1. Create required terms and conditions
  2. Enable paypal express checkout
  3. Checkout with paypal express in the frontend - does not work because terms and coniditons data is not correctly added to the set payment method information request

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)

@magento-engcom-team magento-engcom-team added Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Nov 22, 2017
@joni-jones joni-jones self-requested a review November 22, 2017 16:09
@@ -10,42 +10,12 @@ define([
'mage/storage',
'Magento_Checkout/js/model/error-processor',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, remove unused dependencies

$data = new DataObject(
[
PaymentInterface::KEY_ADDITIONAL_DATA => [
Express\Checkout::PAYMENT_INFO_TRANSPORT_BILLING_AGREEMENT => $transportValue,
Express\Checkout::PAYMENT_INFO_TRANSPORT_PAYER_ID => $transportValue,
Express\Checkout::PAYMENT_INFO_TRANSPORT_TOKEN => $transportValue
Express\Checkout::PAYMENT_INFO_TRANSPORT_TOKEN => $transportValue,
\Magento\Framework\Api\ExtensibleDataInterface::EXTENSION_ATTRIBUTES_KEY => $extensionAttributeMock
Copy link
Contributor

@joni-jones joni-jones Nov 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes do not cover fixed bug. Please, cover them by the unit test.

@ishakhsuvarov
Copy link
Contributor

Hi @therool
Could you please check failing JS Unit so we could proceed with the merge?
Thank you

@therool
Copy link
Contributor Author

therool commented Nov 30, 2017

@ishakhsuvarov I will, I just will fix the commits, I pushed something unnecessary

@therool
Copy link
Contributor Author

therool commented Nov 30, 2017

@ishakhsuvarov fixed. I will try to add one more JS unit test later set-payment-information-mixin.js.test under dev/tests/js/jasmine/tests/app/code/Magento/CheckoutAgreements/frontend/js/model/set-payment-information-mixin.js.test which will test that the agreement ids are added as extension attributes to the payment data when the Magento_Checkout/js/action/set-payment-information is being called.

@ishakhsuvarov
Copy link
Contributor

@therool Thanks. Should we wait for the test then?
Please let us know when we can continue with review & testing.

@ishakhsuvarov
Copy link
Contributor

Hi @therool
Do you still want to continue work on this PR? Any help needed from us?

@therool
Copy link
Contributor Author

therool commented Dec 14, 2017

@ishakhsuvarov I will continue to work on this as everything is done I just want to add jasmine test for the checkout agreement mixins whcih is not necessary.

Maybe you know how to correctly test mixins with jasmine and squire.js as for mixins I need the squire to include the logic from https://github.com/magento/magento2/blob/2.2-develop/lib/web/mage/requirejs/mixins.js#L5 but I can't include "mage/requirejs/mixins" in the dependencies as that breaks all others tests. The test would look something like this:

/**
 * Copyright © Magento, Inc. All rights reserved.
 * See COPYING.txt for license details.
 */

define([
    'squire'
], function (Squire) {
    'use strict';

    describe('Magento_CheckoutAgreements/js/model/place-order-mixin', function () {
        var injector = new Squire(),
            mocks = {
                'Magento_CheckoutAgreements/js/model/agreements-assigner': jasmine.createSpy('agreements-assigner'),
		'Magento_Checkout/js/action/place-order': jasmine.createSpy('placeOrder'),
            },
            placeOrderAction;

        beforeAll(function (done) {
            injector.mock(mocks);
            injector.require(['mixins!Magento_Checkout/js/action/place-order'], function (placeOrder) {
                placeOrderAction = placeOrder;
                done();
            });
        });

        it('Magento_CheckoutAgreements/js/model/agreements-assigner is called', function () {
            var messageContainer = jasmine.createSpy('messageContainer'),
                paymentData = {};
            placeOrderAction(paymentData, messageContainer);
            expect(mocks['Magento_CheckoutAgreements/js/model/agreements-assigner']).toHaveBeenCalled();
            expect(mocks['Magento_Checkout/js/action/place-order']).toHaveBeenCalled();
        });
    });
});

And this is the one I currently have written but It does not work correctly because I can't get the 'mixins': require('mixins') working.

/**
 * Copyright © Magento, Inc. All rights reserved.
 * See COPYING.txt for license details.
 */

define([
    'squire',
    'jquery',
    'mage/requirejs/mixins'
], function (Squire, $) {
    'use strict';

    describe('Magento_CheckoutAgreements/js/model/set-payment-information-mixin', function () {
        var injector = new Squire(),
            mocks = {
                'Magento_Checkout/js/model/quote': jasmine.createSpyObj('quote', ['getQuoteId', 'billingAddress']),
                'Magento_Checkout/js/model/url-builder': jasmine.createSpyObj('urlBuilder', ['createUrl']),
                'mage/storage': jasmine.createSpyObj('storage', ['post']),
                'Magento_Checkout/js/model/error-processor': jasmine.createSpy('errorProcessor'),
                'Magento_Customer/js/model/customer': jasmine.createSpyObj('customer', ['isLoggedIn']),
                'Magento_Checkout/js/action/get-totals': jasmine.createSpy('getTotalsAction'),
                'Magento_Checkout/js/model/full-screen-loader': jasmine.createSpyObj('fullScreenLoader', ['startLoader']),
                'Magento_CheckoutAgreements/js/model/agreements-assigner': jasmine.createSpy('agreements-assigner'),
                'mixins': require('mixins')
            },
            setPaymentInformationAction;

        beforeAll(function (done) {
            window.checkoutConfig = {
                checkoutAgreements: {
                    isEnabled: true
                }
            };
            injector.mock(mocks);
            injector.require(['mixins!Magento_Checkout/js/action/set-payment-information'], function (setPaymentInformation) {
                setPaymentInformationAction = setPaymentInformation;
                done();
            });
        });

        it('Magento_CheckoutAgreements/js/model/agreements-assigner is called', function () {
            mocks['mage/storage'].post.and.callFake(function () {
                return $.Deferred();
            });
            var messageContainer = jasmine.createSpy('messageContainer'),
                paymentData = {};
            setPaymentInformationAction(messageContainer, paymentData);
            expect(mocks['Magento_CheckoutAgreements/js/model/agreements-assigner']).toHaveBeenCalled();
            expect(mocks['mage/storage'].post).toHaveBeenCalled();
        });
    });
});

@omiroshnichenko
Copy link
Contributor

Hi, @therool. Here is a sample how you can test mixin:

define(['squire'], function (Squire) {
    'use strict';

    var injector1 = new Squire(),
        injector2 = new Squire(),
        mixin, paymentInformation, mock1, mock2;

    beforeEach(function (done) {
        mock1 = {
        };
        mock2 = {
            'Magento_Checkout/js/model/quote': jasmine.createSpyObj(
                'quote',
                ['totals', 'getQuoteId', 'billingAddress']
            ),
            'Magento_Checkout/js/action/get-totals': jasmine.createSpy()
        };
        window.checkoutConfig = {
            checkoutAgreements: {
                isEnabled: 1
            }
        };
        injector1.mock(mock1);
        injector2.mock(mock2);
        injector1.require(['Magento_CheckoutAgreements/js/model/set-payment-information-mixin'], function (Mixin) {
            mixin = Mixin;
            injector2.require(['Magento_Checkout/js/action/set-payment-information'], function (PaymentInformation) {
                paymentInformation = PaymentInformation;
                done();
            });
        });
    });

    describe('Magento_CheckoutAgreements/js/model/agreement-validator', function () {
        describe('"validate" method', function () {
            it('Check with non existing checkboxes', function () {
                mixin(paymentInformation)('', {});
                expect(true).toBe(true);
            });
        });
    });
});

@therool
Copy link
Contributor Author

therool commented Dec 18, 2017

Hi @omiroshnichenko, thanks for the sample, I added mixin tests so everything should be fine now.

@AppSimone
Copy link

any news about dank00 patch, I've tried to install but it fails.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed in 2.2.x The issue has been fixed in 2.2 release line Progress: accept Release Line: 2.2 Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants