PayPal support

Changed version and I will be working on a patch soon.

Implemented this. See https://github.com/rgpublic/commerce_braintree/tree/master

@rgpublic
Thanks! Please post a patch here.

.

Attached is a patch against the latest 8.x-1.x version. It's almost a direct copy of @rgpublic's work in comment #2 (with some small styling fixes etc).

Not sure if you need any tests? Doesn't look like this module has any tests at all .... :)

Ah, really great! I just couldnt find the time to move this forward or create an appropriate patch in recent months because I'm stuck in the midst of a bad overdue project. I already had somewhat of a bad conscience for not attending to this any sooner. Thank you so much @Sophie.SK for taking the time!! Well done! Thanks also for the styling fixes. I'm not really good at that. Shame on me :-/

I think the only thing I've added since then is in commerce_braintree.form.js in "Drupal.commerceBraintree.prototype.bootstrap" a line to remove the disabled attribute like so:

  Drupal.commerceBraintree.prototype.bootstrap = function () {

    var options = this.getOptions(this.settings.integration);
    this.$submit.removeAttr('disabled');

    braintree.setup(this.settings.clientToken, this.settings.integration, options);
    if (this.settings.integration == 'paypal') {
      this.bootstrapPaypal();
    }

  }

This make it possible to switch from PayPal back to credit card. Otherwise the continue button could obviously still be disabled.

Let's try that again with a patch that doesn't have console logs in it 0:-)

Thanks for the heads up @rgpublic! I've added that line in now. Attaching an updated patch and an interdiff.

Whoops. I hate JavaScript. Missed some self references... new patch and interdiff attached.

Tested the latest patch #9 and it's working. Is there a way to pre-fill or hide the customer profile fields when someone want to pay with paypal?

Another small patch adding a missing self reference.

Oops! uploaded wrong patch.

This will need to be reworked for #2815309: Migrate to JavaScript SDK 3.x, to use the PayPal Checkout integration option present in v3.

Planning to work on the issue in about 2 weeks, but don't let that stop you from rerolling.

Started work on a patch (attached). Mostly the JS file needed rewriting.

I've hit a wall, though. Braintree seems to think I no longer have an attached PayPal account, but I do :( Can't do any further testing but hopefully it's a good starting point for you Bojanz.

Here is another patch which is working but I needed someone else to review it just to confirm that I am on the right path.

I did split the js stuff in 3 files:

1. js/commerce_braintree.js: main file calling HostedFields or Paypal when needed
2. js/hosted-fields.js: contain Drupal.commerceBraintreeHostedFields constructor
3. js/paypal.js: contain Drupal.commerceBraintreePaypal constructor

Also I did refactor the commerce_braintree.libraries.js to include checkout.js from https://github.com/paypal/paypal-checkout and separated paypal and braintree related js requirements into their own library. With this in place I was able to <?php $element['#attached']['library'][] = 'commerce_braintree/paypal'; ?> when building paypal form and <?php $element['#attached']['library'][] = 'commerce_braintree/hosted-fields'; ?> when building hosted fields form.

Change status to need review for now so that people can review.

Opps dind change the status!:-)

Alas, while I was able to apply the patch cleanly, it didn't work :(

When I chose "New paypal account" on checkout, the spinny Ajax loading wheel came up but then disappeared with no other changes. When I clicked on "Continue to review", I got:

Fatal error: Unsupported operand types in /var/www/html/docroot/core/lib/Drupal/Core/Asset/JsCollectionRenderer.php on line 95

No errors in console or in the log. Boo! Unfortunately I don't have the time right now to try and fix this problem.. but if it's working for you, it must be okay?! Wonder why it didn't work for me.

For the record, we apply the patch using Composer. Not sure if that makes a difference.

@Sophie.Sk did you link your Braintree sandbox account with your PayPal sandbox account? Since SDK v3 it is required for testing to work see https://developers.braintreepayments.com/guides/paypal/testing-go-live/p.... As for the error I really don't have any idea why it's there since I didn't experience the same problem.

Also, I am running d8.3.6, commerce latest dev as of yesterday and Commerce Braintree latest dev. Can you, please provide version a of d8 and commerce you are
running so that I can see if can reproduce the error. I will be working a little bit on this during the weekend.

Thanks

@nikathone Oh, I wonder if that's it. This is a client-created sandbox account (they don't want us using our own), so I'm not totally sure what they have and haven't set up. i'll get in touch and ask them to set that up, thanks for the pointers.

We're using 8.3.5 right now due to upgrade next week, and the stable release candidate of Commerce, so maybe a little bit behind you. I'll let you know if we have any luck with the PayPal linking.

Thanks for the speedy feedback.

Chiming in that I ran into.

Error: Unsupported operand types in Drupal\Core\Asset\JsCollectionRenderer->render() (line 95 of /var/platform/web/core/lib/Drupal/Core/Asset/JsCollectionRenderer.php) #0 
                                        /var/platform/web

I have

drupal/commerce                       2.x-dev ba9cf6c
drupal/commerce_braintree             1.x-dev 2bb9213
drupal/core                           8.3.2

Attached is why

+++ b/commerce_braintree.libraries.yml
@@ -7,18 +7,68 @@ braintree:
+      attributes:
+        data-version-4

This is why. needs to be

      attributes:
        - data-version-4

After the fix of "Can't switch gateways from Braintree" I was able to use some js from it to make PayPal work again. Right now I am not even sure if should keep the splitted js files approach but here is the patch attached.

This adds only a few changes compared to #23, like a unique orderId for the sale() SDK call and some sanity checks.

@czigor can you please provide an interdiff. Thanks

Why was it put to needs work? Especially if #24 only adds a few items to #23.

I'm attaching an interdiff for review.

I think I see some fixes in there. Specifically for triggering the form submit. However not fully explained.

1. +++ b/composer.json
   @@ -1,11 +1,11 @@
   +	"name" : "drupal/commerce_braintree",
   +	"type" : "drupal-module",
   +	"description" : "Provides Commerce integration for Braintree Payments.",
   +	"homepage" : "http://drupal.org/project/commerce_braintree",
   +	"license" : "GPL-2.0+",
   +	"require" : {
   +		"drupal/commerce" : "~2.0",
   +		"braintree/braintree_php" : "~3.25"
   +	}
   +}
   \ No newline at end of file
   

   We need to revert the changes to the `composer.json`

2. +++ b/js/paypal.js
   @@ -41,8 +39,8 @@
   +                // May be there is a better way to display email. In the old ¶
   

   Trailing whitespace

3. +++ b/src/Plugin/Commerce/PaymentGateway/HostedFields.php
   @@ -160,7 +160,8 @@ class HostedFields extends OnsitePaymentGatewayBase implements HostedFieldsInter
   +      // orderId must be unique.
   +      'orderId' => $payment->getOrderId() . '-' . $this->time->getCurrentTime(),
   

   Is this an issue due to sandbox and production order IDs conflicting? Maybe we should link to the documentation that explains this. Does the order ID show in the backend?

4. +++ b/src/Plugin/Commerce/PaymentGateway/HostedFields.php
   @@ -181,11 +182,13 @@ class HostedFields extends OnsitePaymentGatewayBase implements HostedFieldsInter
   +    if ($result->success) {
   +      $next_state = $capture ? 'completed' : 'authorization';
   +      $payment->setState($next_state);
   +      $payment->setRemoteId($result->transaction->id);
   +      // @todo Find out how long an authorization is valid, set its expiration.
   +      $payment->save();
   +    }
   

   I don't think this is needed? If it wasn't successful it would have thrown an exception, correct?

Follow up on interdiff, again. The new patch in #24 ensures the form submit is triggered with `tokenized: true` and displays error messages.

1. +++ b/js/paypal.js
   @@ -51,6 +49,7 @@
                    // Submit 'payload.nonce' to the server.
                    $('.braintree-nonce', $form).val(payload.nonce);
                    $submit.prop('disabled', false);
   +                $form.trigger('submit', { 'tokenized' : true });
   

   Ensures that the submit marked as tokenized is sent.

2. +++ b/js/paypal.js
   @@ -60,9 +59,9 @@
   -            // var message = that.errorMsg(error);
   +            var message = that.errorMsg(error);
   ...
   -            // $form.prepend(Drupal.theme('commerceBraintreeError', error));
   +            $form.prepend(Drupal.theme('commerceBraintreeError', error));
   

   Allows errors to render.

1. Reverted the composer.json changes.
2. Fixed trailing whitespace.
3. The orderId issue appears when trying to do a paypal checkout and the payment fails for some reason. When we retry payment, paypal receives a sale() request with the same orderId and throws an exception. See https://developers.braintreepayments.com/reference/request/transaction/s... "On PayPal transactions, this field maps to the PayPal invoice number. PayPal invoice numbers must be unique in your PayPal business account." This is the case e.g. when the merchant paypal account is not configured to accept payments in a different currency and we still try to do so.
4. We definitely need a better error handling. Not all braintree error responses trigger throwing an exception.

An additional change in #24 compared to #23 was setting the paypal payment method's expiration to 0, since commerce 8.x-2.x-dev already supports non-expiring payment methods.

The "needs work" status has been set because we still need:
- Fix the "Continue to review" button on the order_information checkout page when braintree with paypal has been selected.
- Better error handling at least in createPayment() and refundPayment().

Additionally, I'm not sure if it's ok for paypal to extend OnsitePaymentGatewayBase instead of OffsitePaymentGatewayBase.

Discussed the onsite vs offsite issue with Bojan, came to the conclusion that this is an onsite gateway. The reason is that although we have an iframe paypal button, we play the nonce-token dance that is characteristic to onsite gateways.

3. The orderId issue appears when trying to do a paypal checkout and the payment fails for some reason. When we retry payment, paypal receives a sale() request with the same orderId and throws an exception. See https://developers.braintreepayments.com/reference/request/transaction/s... "On PayPal transactions, this field maps to the PayPal invoice number. PayPal invoice numbers must be unique in your PayPal business account." This is the case e.g. when the merchant paypal account is not configured to accept payments in a different currency and we still try to do so.

So the invoice number will be an order ID and a random timestamp :/? I wonder if there's a way to reconcile this. How can we reproduce this flow? Cancel payment after decline then try to use PayPal again?

Better error handling at least in createPayment() and refundPayment().

Braintree has documented sandbox testing. Which areas should we add test coverage for? We have kernel tests covering some of them.

Maybe the easiest way to see that a unique orderId is required is trying to add an additional payment on the drupal Payments UI. (See screenshot, where I set the orderId in createPayment() back to $payment->getOrderId().)

Two changes:

1. Added an if ($result->success) check after each payment gateway method to prevent creating/updating payment transactions in commerce when the remote transaction failed.
2. The "Continue to review" button is now hidden via css when the new braintree-paypal payment method is selected.

+++ b/src/Plugin/Commerce/PaymentGateway/HostedFields.php
@@ -181,11 +182,13 @@ class HostedFields extends OnsitePaymentGatewayBase implements HostedFieldsInter
+    if ($result->success) {

@@ -206,9 +209,11 @@ class HostedFields extends OnsitePaymentGatewayBase implements HostedFieldsInter
+    if ($result->success) {

@@ -226,8 +231,10 @@ class HostedFields extends OnsitePaymentGatewayBase implements HostedFieldsInter
+    if ($result->success) {

@@ -249,26 +256,36 @@ class HostedFields extends OnsitePaymentGatewayBase implements HostedFieldsInter
+    if ($result->success) {

All of this should be handled in \Drupal\commerce_braintree\ErrorHelper::handleErrors already, which is called after a response is returned.

Fixed concerns in #35.

The patch is working for me now! There are a few changes from the previous SDK (can't select previously used accounts, it now asks me to login with a sandbox account), but I was able to proceed through checkout and buy the product.

A couple of UI/UX things though:
* The PayPal button should be under the address form
* The address form should have some kind of header to indicate that it is a billing address - it looks out of place right now
* There is no way of manually moving on to the next step, it's all automated redirects. I had a problem first time around where I filled in dummy info for the address, logged in with paypal, tried to change the address to something correct, but got redirected halfway through. A manual button would be good.

Attaching a screenshot to demonstrate.

Thanks for all your hard work on this :)

Updated patch to address the button placement. No interdiff, added the following to buildPayPalForm

    // Put the PayPal button below the billing address.
    $element['#weight'] = 50;

There is no way of manually moving on to the next step, it's all automated redirects. I had a problem first time around where I filled in dummy info for the address, logged in with paypal, tried to change the address to something correct, but got redirected halfway through. A manual button would be good.

I agree. We should persist the button and force manual movement to next form. I vote we remove commerce_braintree.paypal.form.css. We can easily trigger an event or introduce a namespaced callback to be overridden in onAuthorize to allow auto-redirect.

The address form should have some kind of header to indicate that it is a billing address - it looks out of place right now

This is something core Drupal Commerce should probably provide whenever it's a PayPal payment method.

Putting needs work because:

• Remove auto-submit when PayPal information gathered. Just show their PayPal email
• In #37 it sounds like vault got broken?

Removing auto-submission. We disable the main "Continue to review" button until the customer clicks the Paypal button and gets a nonce.

I no longer get redirected automatically and I can select previously-selected PayPal accounts now. Brilliant :)

Marking as RTBC. There are still some improvements to be made in the UX but we have a lot of templating/design work to do for this client so may be able to feed back on what we've done.

Thanks for the hard work!

Thanks everyone for working on this!