PayPal only allows 1 transaction per invoice Id, so if a developer processes test order 1 on a development box, then order 1 will fail on the live box when it is processed.

Cheers,

Antoine

CommentFileSizeAuthor
#8 uc_payflowpro-test-invoice-num.patch1.97 KBkwinters
#7 uc_payflowpro-1012766_1.patch756 bytesjantoine
#1 uc_payflowpro-1012766.patch618 bytesjantoine

Comments

jantoine’s picture

jantoine commented
Status: Active » Needs review
StatusFileSize
new uc_payflowpro-1012766.patch618 bytes

The attached patch generates a large unique Id to send as the invoice Id when in test mode to avoid this issue.

Cheers,

Antoine

kwinters’s picture

kwinters commented
Status: Needs review » Needs work

I don't really like this approach to avoiding it. The ids that uniqid() creates aren't easy to map back to a Drupal order ID. Assuming PFP will allow this many characters, etc. I'd prefer the pattern to be something like [orderid]-TEST-[uniq], which is relatable, obviously a test, and still unique. Second best would be [orderid]-[uniq].

kwinters’s picture

kwinters commented

A nitpick: the comment doesn't explain *why* we would do this, just what it does.

jantoine’s picture

jantoine commented

PFP does accept the length of uniqid().

Because I used this only for testing, I never needed to map the uniqid() back to the order Id, I always used the PNRef number returned by PFP to map an order to payments. I do however like the idea of adding '[order-id]-TEST-' before the 'uniqid'.

This actually brings up a question: would this module ever need to support multiple payments per order (i.e. layaway payments)? I think this is something that Ubercart can support and we should therefor not be using the order-id by itself for live orders. I propose we change the module to always use '[order-id]-[uniqid]' for live payments and '[order-id]-[uniqid]-TEST for test payments.

Let me know what you think? I will start testing to ensure that PFP will accept these new values.

P.S. I took the comments out just before generating the patch ;) I'll put them back in.

Cheers,

Antoine

kwinters’s picture

kwinters commented

Normally multiple payments per order would use the recurring system, and it's never come up before as a bug report, so I'm hesitant to change live behaviors. It's hard to tell if anything might rely on the old behavior.

Make sure PFP accepts the new pattern with large order IDs, too.

jantoine’s picture

jantoine commented

I haven't looked at the recurring code of the module or the documentation for PFP. If recurring payments use the same order-id for each payment, it could be the PFP allows this for recurring payments and may even require that an identical payment-id be used to link recurring payments? For now I'll just work on changing it for test cases.

Cheers,

Antoine

jantoine’s picture

jantoine commented
Status: Needs work » Needs review
StatusFileSize
new uc_payflowpro-1012766_1.patch756 bytes

A new patch is attached that formats the InvoiceId like [order_id]-[uniqid]-TEST and includes a comment as to why we would use this vs. just the order_id. I added '-TEST' to the end in case down the road we decide using [order_id]-[uniqid] for live orders is a good idea. I also sent a successful test order to PFP.

Cheers,

Antoine

kwinters’s picture

kwinters commented
StatusFileSize
new uc_payflowpro-test-invoice-num.patch1.97 KB

New version attached. Changes:

* Also change the invoice number for recurring profiles.
* Put "TEST" before the random characters, since the manager.paypal.com truncates and was cutting the word TEST.

If feasible, please provide the patches in unified format (-u) -- they apply cleaner in Eclipse for whatever reason.