We need to integrate PayPal Express Checkout and ensure it works at least in the US, UK, and France. There was a module for EC, but upon review, cvangysel decided it would be better to start from scratch and patch the PayPal module and Commerce itself to support a tighter integration with the checkout / payment APIs. Attached is his patch ready for review, though it won't be committed until we have the 2.x branch opened. : )

Testing this patch depends on the patches in:

CommentFileSizeAuthor
commerce_paypal-commerce_paypal_ec-1.patch37.1 KBrszrama

Comments

rszrama’s picture

rszrama commented
Assigned: Unassigned » JulienD

Assigning to Julien for the initial review and European testing.

JulienD’s picture

JulienD commented
Status: Active » Needs work

Hey there,

To make my review I've done a fresh install of drupal + drupal_commerce, I've installed the commerce_paypal module, I've applied the patch #1855900 and then applied Christophe's patch.

After the install I've browsed the website, activated the payment method, browsed the website again, I've set the configuration and final browsed the website. To be like a real e-commerce site I've installed an other payment method.

Bellow are some of my notes :

+++ b/commerce_paypal.module
@@ -406,3 +406,133 @@ function commerce_paypal_icons() {
+function commerce_paypal_request($payment_method, $nvp = array(), $order = NULL) {
...
+  drupal_alter('commerce_paypal_wpp_request', $nvp, $order);

The commerce_paypal_request function has been moved from wpp module to paypal module but the .api file has not been updated. This also need to be moved.

function commerce_paypal_ec_commerce_checkout_page_info_alter(&$checkout_pages) {
  if (commerce_paypal_ec_checkout()) {
    $checkout_pages['payment']['weight'] = -5;
  }
}

After testing the module, it appear that this new weight alter ALL the payment methods. The test just check if the payment is activated not if it's selected for this order. This is a blocking point.

+++ b/modules/ec/commerce_paypal_ec.module
@@ -0,0 +1,731 @@
+  if (commerce_paypal_ec_checkout()) {

This test is not enough sufficient, it will return true regardless the payment method chosen by the customer at the moment where the paypal EC method is activated.

+++ b/modules/ec/commerce_paypal_ec.module
@@ -0,0 +1,731 @@
+  if (commerce_paypal_ec_enabled() && strpos($form_id, 'views_form_commerce_cart_form_') === 0) {

It should be better to start the test with the check on the form_id than calling commerce_paypal_ec_enabled() to avoid useless database queries

+++ b/modules/ec/commerce_paypal_ec.module
@@ -0,0 +1,731 @@
+        '#type' => 'image_button',

I had big problems with the form and that button. Regardless on which button I click (checkout, remove product...) I'm always redirected on Paypal's website. The problem doesn't happens when the button is of the type submit.

+++ b/modules/ec/commerce_paypal_ec.module
@@ -0,0 +1,731 @@
+    '#options' => commerce_paypal_wpp_currencies(),

Here we have an hard dependency with the wpp module. Like the other functions moved from wpp module to paypal generic module we should do the same for this function.

+++ b/modules/ec/commerce_paypal_ec.module
@@ -0,0 +1,731 @@
+function commerce_paypal_ec_enabled($order = NULL) {
+  $rule = rules_config_load('commerce_payment_paypal_ec');
+
+  $enabled = !empty($rule) && $rule->active;
+  if (!empty($order) && $enabled) {
+    $enabled = !empty($order->data['payment_method']) && ($order->data['payment_method'] == 'paypal_ec|commerce_payment_paypal_ec');
+  }
+
+  return $enabled;
+}

This function should also check if the payment method is configured.

+++ b/modules/ec/commerce_paypal_ec.module
@@ -0,0 +1,731 @@
+  );
+  $items['express-checkout/%test'] = array(

%test, why this loader is called like that?

+++ b/modules/ec/commerce_paypal_ec.module
@@ -0,0 +1,731 @@
+function commerce_paypal_ec_submit_form($payment_method, $pane_values, $checkout_pane, $order) {
...
+function commerce_paypal_ec_submit_form_validate($payment_method, $pane_form, $pane_values, $order, $form_parents = array()) {
...
+function commerce_paypal_ec_submit_form_submit($payment_method, $pane_form, $pane_values, $order, $charge) {

Paypal EC is an off-site payment solution, I don't understand why those functions have been be implemented. Maybe I'm wrong but I thought they were only used for on-site payment ?

Paypal also provide the possibility to used Auth mode and not only Sales mode. The possibility for the merchant to choose has been implemented but not the capture action. According to the API it's also possible to implement the refund action. I had a look on the other commerce_paypal_ec module and this module do the capture and refund actions, it also provide a great features missing in this patch, translation! For me it's a big lack. If I'm selling products on a non English website I don't want my customer to be redirected on an English page. We have to let the merchant configure paypal's checkout page language on the payment settings page

Currently, I haven't succeeded to make this patch works and allowing me to use the EC payment method or any other together.

Here is a little to do list for my memory:
- correcting the cart form and make all buttons working as they are excepted
- allowing multiple payment methods to works together
- adding the capture mode
- adding the refund mode
- implemented the language configuration (paypal's logo + paypal form)

rszrama’s picture

rszrama commented
Version: 7.x-1.x-dev » 7.x-2.x-dev

Just moving to the new branch.

rszrama’s picture

rszrama commented

You were spot on on a lot of your feedback, Julien. I've touched up the PayPal module changes and made an intermediate commit.

  1. I renamed commerce_paypal_request() to commerce_paypal_api_request() and fleshed out the doc block.
  2. I renamed the alter to hook_commerce_paypal_api_request_alter() and made $payment_method the last parameter. This will be an important API change for any module that previously implemented this hook, as not just the name but also the arguments have changed. Moved the documentation to commerce_paypal.api.php.

Commit: http://drupalcode.org/project/commerce_paypal.git/commitdiff/389d676

I'll follow this with review of the EC submodule based on your feedback. It appears I solved the image button issue in Ubercart by using a separate form entirely with a small hack to determine if the image button was clicked. I'm sure image button support has changed somewhat in D7 so I won't need the same hack, but I'll see what I can do to keep using a separate form for the redirect it it makes sense.

rszrama’s picture

rszrama commented

Quick follow-up commit to create a common function for returning currency codes per JulienD's feedback to avoid the dependency from the EC submodule to the WPP submodule. Went ahead and moved the WPS currencies in there as well so we can keep track of differing currencies all in one place.

Commit: http://drupalcode.org/project/commerce_paypal.git/commitdiff/c5c9d33

rszrama’s picture

rszrama commented
Assigned: JulienD » Unassigned

Changes made to the Express Checkout module:

  1. We actually don't need to add an Express Checkout link to the cart block. The image form on the cart form is sufficient.
  2. I made a standalone form for redirecting to PayPal and attached it as a #suffix to the cart form.
  3. I removed the user_access('access checkout') check, as it may be reasonable for a site to disable on-site checkout but still want Express Checkout.
  4. I added a checkout page and checkout pane specific to this module, using hook_commerce_checkout_router() to route around it when necessary. The checkout pane will show a review message and allow the store admin to configure additional checkout panes to show on the confirmation screen, such as the shipping selection pane, to update the order before making the final payment request from PayPal EC. This allows us to implement EC as a forked checkout workflow that still works through the same list of pages by simply bypassing the first couple and then including necessary elements on the final page prior to Completion.
  5. I updated profile creation / updates to be type sensitive; it was using the shipping address to create a billing profile, but that should only apply to the shipping profile and the payer first / last name should only apply to the billing profile.

I suppose most of the module has changed, with some code being removed entirely. There are still a few @todo items, so feel free to pitch in with follow-up patches if you have the time / inclination. Right now, I have a blocking issue that I may have to contact PayPal about - basically if I use the Express Checkout flow (i.e. use the Express Checkout button on /cart), I can proceed through everything just fine. However, if I choose to go to PayPal from the payment method selection checkout pane, it sends me to PayPal just fine but refuses to send me back. The API requests and return URLs are identical, but for some reason PayPal just won't send me back - it dumps a generic error message with no further information. : (

(Oh, and it also means that #1855900: Move invocation of drupal_alter in commerce_checkout_pages() so that defaults are filled in first is no longer a requirement.)

Commit: http://drupalcode.org/project/commerce_paypal.git/commitdiff/c805b7c

rszrama’s picture

rszrama commented

I was able to solve that blocking issue this evening. What was happening was I used a redirect key to setup the initial transaction with Express Checkout, but if a customer used the "Mark" flow (i.e. chose to pay via PayPal after going through the on-site checkout form), the redirect key was being reset to a new value when the redirect form was built. This meant the back / return URLs were not recognizing the key, causing the payment module to kick the customer back to PayPal.

That was solved with a commit to Commerce itself: http://drupalcode.org/project/commerce.git/commitdiff/078686f

However, I did go ahead and push a commit to this project that created a new function called commerce_paypal_ec_set_express_checkout() for performing the initial API request to setup a new Express Checkout for an order. We use this now for both "flows" of Express Checkout:

  • "EC" flow (or normal Express Checkout) - where the customer clicks the "Checkout with PayPal" button on the cart form.
  • "Mark" flow - where the customer uses the on-site checkout form but chooses to pay at PayPal from the payment method selection checkout pane.

There are some @todo items in the code for each of these flows that need to be addressed:

  1. The API request function commerce_paypal_ec_set_express_checkout() indicates in two places where we need to better itemize the order information we're submitting to PayPal. Full itemization is a requirement of the integration, so these need to be addressed. We should also itemize order data in commerce_paypal_ec_do_payment().
  2. For the "EC" flow, the customer goes from the cart to PayPal to submit their shipping and billing information. Before finalizing payment, the customer is sent back to the site where we can get their shipping info to create a customer profile locally (we do this already) and then display a page for reviewing the order that includes taxes / shipping and such. Right now, we show a final review of the order, but because the order is no longer in a shopping cart status, line item prices won't have been recalculated to include taxes. We'll need to do this manually.
  3. Additionally, in commerce_paypal_ec.checkout_pane.inc you'll see the @todo notes indicating that we need to give administrators the ability to enable checkout panes on the special Express Checkout review pane. This will let us show the Shipping services pane on this page, for example, even if in the normal checkout workflow it appears on an earlier page.
  4. For the "Mark" flow, we never actually capture the payment with a DoPayment request. There's a note for this in the docblock of commerce_paypal_ec_redirect_form_submit().
  5. For the "Mark" flow, we need to improve our address options. SetExpressCheckout lets us pass in known addresses and also lets us tell PayPal whether or not to show things like shipping address prompts or to default them to the address we pass in instead of the address on file. We should take advantage of these options found at https://www.x.com/developers/paypal/documentation-tools/api/setexpressch....

There are other @todos dealing with localization, multiple Express Checkout configurations, HTTP vs. HTTPS, etc. If the items above are addressed, those @todos are next on the list. : )

Current commit: http://drupalcode.org/project/commerce_paypal.git/commitdiff/76651d1

rszrama’s picture

rszrama commented

This commit takes care of #7.4 but necessitated moving the code from the redirect form submit callback to the validate callback. Additionally, I fixed a bug with customer profile updating where we were getting duplicate customer profiles because the entity context for the profile had not been set (as on the order edit form). Testing this turned up a new to-do item added to the list in #7.

I also found and fixed a bug in our payment transaction creation process - it wasn't properly setting the remote ID and status values based on the response to the DoPayment API request.

Commit: http://drupalcode.org/project/commerce_paypal.git/commitdiff/75a9587

rszrama’s picture

rszrama commented

I just added support for Express Checkout Account Optional. We can allow merchants to use EC without requiring a PayPal account, and I've given them the option to specify the landing page (PayPal login vs. billing form) if they want it to be optional.

Commit: http://drupalcode.org/project/commerce_paypal.git/commitdiff/12b3d14

rszrama’s picture

rszrama commented

I've spawned child issues from the bulleted list above:

I haven't created child issues for all of the @todo items in the code comments yet, though. So if these are emptied out, grep the source for @todo and feel free to post patches to this issue or create a separate issue for them.

Additionally, I added line item labels to our API requests: http://drupalcode.org/project/commerce_paypal.git/commitdiff/57b11b8

JulienD’s picture

JulienD commented

Additionally, I added line item labels to our API requests: http://drupalcode.org/project/commerce_paypal.git/commitdiff/57b11b8

Why did you use "L_PAYMENTREQUEST_n_NUMBERm" instead of "L_PAYMENTREQUEST_n_DESCm", that latest field seem more appropriate ?

rszrama’s picture

rszrama commented

In the case of product line items, the label is the SKU, so it matches with PayPal's concept of "NUMBER". That said, we should probably add a check on the line item type. If it's a product line item type, we can use NUMBER, but for any other line item type we can just ignore it. We wouldn't really have a DESC for non-product line item types anyways.

rszrama’s picture

rszrama commented
Status: Needs work » Fixed

This should have been closed out when the beta was released. My comment in #12 has already been addressed in the code, and merchants are using EC in production nowadays. Three cheers for features!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Adding patch dependencies.