Posted by rszrama on December 12, 2012 at 3:26pm
4 followers
Jump to:
| Project: | Commerce PayPal |
| Version: | 7.x-2.x-dev |
| Component: | PayPal EC |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
Issue Summary
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:
- #1855900: Move invocation of drupal_alter in commerce_checkout_pages() so that defaults are filled in first
- #1854216: Introduce a new callback in commerce_payment to inform offsite payment method implementations that caused a failed payment
| Attachment | Size |
|---|---|
| commerce_paypal-commerce_paypal_ec-1.patch | 37.1 KB |
Comments
#1
Assigning to Julien for the initial review and European testing.
#2
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)
#3
Just moving to the new branch.
#4
You were spot on on a lot of your feedback, Julien. I've touched up the PayPal module changes and made an intermediate commit.
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.
#5
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
#6
Changes made to the Express Checkout module:
user_access('access checkout')check, as it may be reasonable for a site to disable on-site checkout but still want Express Checkout.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
#7
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:
There are some @todo items in the code for each of these flows that need to be addressed:
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().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
#8
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
#9
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
#10
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
#11
Why did you use "L_PAYMENTREQUEST_n_NUMBERm" instead of "L_PAYMENTREQUEST_n_DESCm", that latest field seem more appropriate ?
#12
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.