When there are shippable (tangible) items, Paypal should not change the workflow to Completed after a successful payment, but it does. I looked at the code and apparently the function product_is_shippable is called with txn_id and not with nid that it is expecting.

CommentFileSizeAuthor
#15 paypal_has_shippable_item.patch1.43 KBmfb
#12 shipping_check_paypal.module.patch.txt1.31 KBñull
#8 36083.patch762 bytesneclimdul
#3 paypal-module-localisation-country-shipping.patch3.14 KBñull
#1 paypal-module-localisation-country-shippable.patch6.58 KBñull

Comments

ñull’s picture

ñull commented
StatusFileSize
new paypal-module-localisation-country-shippable.patch6.58 KB

Somebody can correct me, but it seems that the code in Paypal that sets the workflow to completed when there are no shippable items, is obsolete. I just discovered that the store's cron function does the same job near line 59. Also I did not discover any similar code in the other payment processor modules. Consequently I removed the code that would do that in Paypal. Included a patch that fixes Paypal's country setting, a localization error and it removes the shippable code. Could somebody check this?

ñull’s picture

ñull commented
Status: Active » Needs review
ñull’s picture

ñull commented
StatusFileSize
new paypal-module-localisation-country-shipping.patch3.14 KB

Same patch applying to the new 4.6 backport from HEAD

matt westgate’s picture

matt westgate commented
Priority: Critical » Normal
Status: Needs review » Active

I committed the string translation fix.

Can you check that the country codes address.module uses are compliant with the country codes PayPal expects?

Regarding paypal module setting the workflow: this is for file downloads. The idea is if someone purchases downloads they want to access their items right away so PP will set the workflow status to complete so they can be accessed.

I'm open to other solutions here...

jeff veit’s picture

jeff veit commented

Matt said: "Can you check that the country codes address.module uses are compliant with the country codes PayPal expects?"

They are not.

Paypal expects ISO 3166 country codes: these are always in uppercase for a start whereas the country codes we are using are in lower case.

There are differences in specific country codes too. For instance the United Kingdom has the county code GB. E-commerce uses UK. I haven't been through the full list to find other differences.

The ISO country codes can be found here: https://www.paypal.com/en_GB/pdf/PP_WebsitePaymentsStandard_IntegrationG...

ñull’s picture

ñull commented

When we consider a change in address.module to ISO 3166 country codes, then we need to consider:

  • what country code is used by other payment processors?
  • the update script / sql should translate the country codes to the new standard.
thekenshow’s picture

thekenshow commented
Version: 4.6.x-1.x-dev » 4.7.x-1.x-dev
Component: paypal.module » paypal

I encountered this in 4.7 (shippable product workflow set to "complete" for paypal purchases). Digging in to paypal.module, I found the following problem on line 237:

    if (product_is_shippable($item->nid)) { /* $item hasn't been defined yet - should be $pp_item->nid. */
      $has_shippable_item = true;
    }

Changing this line resolved the problem.

neclimdul’s picture

neclimdul
He/Him
commented
Title: Shippable is not detected » Paypal finishes workflow even with shippable items.
Version: 4.7.x-1.x-dev » master
StatusFileSize
new 36083.patch762 bytes

Ok, based on the previous comments I have attached a patch. The ISO issue should be taken up seperately in my opinion. I'm marking this cvs since its a problem present in the cvs HEAD and we can backport it.

neclimdul’s picture

neclimdul
He/Him
commented
Status: Active » Fixed

Committed my patch as it is a needed fix. Going to mark this fixed.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)
ñull’s picture

ñull commented
Status: Closed (fixed) » Needs work

After my recent upgrade to 4.7, I am still seeing the same issue which brought great irritation to my client! Even though there are shippable items, still the workflow is changed to completed. The code that is supposed to check if there are shippable items is really faulty.

Then on second note I am wondering why the payment module should be responsible to change the workflow? A payment module should only change payment status and workflow issues should be left to other modules like store.module. If you compare with other payment modules you will see that not all implemented this check. For instance I don't see it in Worldpay and Authorize.net. Both only touch payment status.

My suggestion is like I originally proposed, to just take out the code that checks for shippable items and remove the code that changes the workflow. This function belongs to the store's cron job and in fact you can find it back there:

     /* If the transaction has no shippable items and the payment status is
           'complete' and the workflow is 'transaction received', set the
           workflow to 'complete' */
        if (!$has_shippable_item && $data->payment_status == 2 && $data->workflow == 1) {
          db_query("UPDATE {ec_transaction} SET workflow = '6' WHERE txnid = %d", $data->txnid);
        }

So please, could this be cleaned up somehow? Let this be a function of the store.module and delete it from all payment modules that mistakenly implemented it. Why do it in the different payment modules when it is already done "centrally" in the store module?

I noticed however that the store module only will do this if the admin activated the notification email. May be it would be good to make this check independent from the notification email.

ñull’s picture

ñull commented
Version: master » 4.7.x-1.x-dev
StatusFileSize
new shipping_check_paypal.module.patch.txt1.31 KB

Attached a patch for 4.7 that fixes the issue like described in the former comment

Christoph C. Cemper’s picture

Christoph C. Cemper commented

hmmm.. pretty interesting

So If I want an order to be NOT set to completed because a service still has to be performed
(like a script installation) then I need to set the product "Install script" to "shippable" ?

neclimdul’s picture

neclimdul
He/Him
commented

Ok, I'm leaving this as needs work. If the shipping check is failing then my guess would be we're having other problems related to this code that need to be resolved and just removing the check doesn't seem like the end all solution without further review.

Just for the sake of clarity, you aren't actually using 4.7.x-1.x-dev as shown by the bug? Also, workflow is something that has been of interest to us but something that hasn't been able to be properly developed further. There are some other things that need to mature to support a better setup for this and isn't really something I want to deal with in this issue.

mfb’s picture

mfb
they or he
commented
Version: 4.7.x-1.x-dev » 5.x-3.x-dev
Status: Needs work » Needs review
StatusFileSize
new paypal_has_shippable_item.patch1.43 KB

product_is_shippable() was being called incorrectly.
This is the smallest possible patch to fix it.

Note, paypal module needs other cleanup, which I will leave to some other issue...

skwashd’s picture

skwashd commented
Status: Needs review » Reviewed & tested by the community

I haven't tested the patch, but a visual review suggests that it is good to go. This problem is still present in 5.x-3-dev 10-May-2007 (and was in 4.7-2.0). Would be good to see this fixed in 5.x-3.1

mfb’s picture

mfb
they or he
commented

Any chance this could make it into e-commerce 3.2?

gordon’s picture

gordon commented
Status: Reviewed & tested by the community » Fixed

Thanks this has been committed to v5.x-3.x-dev and v4.7.x-3.x-dev

Will be available as a part of v3.2

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)