Long story short, I have a programmer looking into fixing some of the issues for uc_stripe that have directly affected me:

1) comments not working

2) addition of idempotency

3) update to work with the latest php-stripe library (not so much of an issue as a preference. Latest version is 7.28)

What I need to know is the best plan of attack for getting things fixed and included back into the community. I know the normal approach is to create a new issue for each "problem" and submit a patch. Sorry, "ain't nobody got time for that." What I will probably do, is create a new issue tagged as v4.0 and upload the entire zip. I believe the main changes are strictly with uc_stripe.module, however there will probably be enough changes that are all over the place and I just don't have the time to deal with each and every issue, especially when I'm having to relay stuff I don't even fully comprehend...

For some context, according to the programmer there are a few functions under the hood that should seriously consider being fixed. It's over my head so I'll just re-post his comments.

The uc_stripe.module contains these lines:

// When a payment fails, remove the Submit Order button because it will most
// likely fail again. Instead, the customer should hit back to try again.
if(isset($_SESSION['stripe']['payment_failed'])){
$form['actions']['submit']['#type'] = 'hidden';
}

I did not like this when I first saw it a few days ago, and I still do not like it. So instead of hiding the button, I would rename it to "Retry". After clicking the Retry a few times, the customer could still click the Back button. What do you think?

---
Unfortunately the module is still plagued by the "charge" and "confirm" naming confusion, if I understand it correctly. For example, the payment may be successful (done via the ajax call, executed in the function called "confirm"), but later when the server checks the payment in the function named "charge", and this checking fails, then the module sets the "payment_failed" value, which will result in the hiding of the "Submit Order" button.
In other words, there is a huge difference between "Stripe server said that Payment Failed" and "Unable to connect to Stripe Server to check payment result", but the module makes no difference.

---
I hope that my explanation was clear, but if not: the main thing is that in addition to the idempotency thing, the network errors should be handled better to completely avoid double payments.

I was confused with whether the problem was with Stripe or UC Stripe. His response:

This confusion is in the uc_stripe module.
The reason could be that other payment modules always have a "charge" function that is called when you click the "Submit" button, and uc_stripe also has such a function. However, in the stripe module the actual payment is done via ajax (in function _uc_stripe_confirm_payment), and the usual "charge" function does no charging, only checking.

Some other modules as examples:
/**
* Main handler for processing credit card transactions.
*/
function uc_authorizenet_charge($order_id, $amount, $data) {
/**
* Charges card.
*/
function uc_cybersource_charge($order_id, $amount, $data) {
/**
* Processes a credit card payment through Website Payments Pro.
*/
function uc_paypal_wpp_charge($order_id, $amount, $data) {

So other modules do indeed charge in their "charge" functions, but uc_stripe does not. It is not bad, just unusual compared to other modules.

So the network error problem I mentioned could be described like this: when charging in _uc_stripe_confirm_payment() is successful via ajax but checking the result later in uc_stripe_charge() fails, the customer is forced to click the "Back" button and pay again, not knowing that the payment was already done.
(the function names look confusing, don't you think?)

This issue is the start of the contributions I am doing, but don't have time to fully deal with in the issue queue of v3.x. I'll keep things posted here, but mainly wanted to share some of the things I'm working on. If the maintainers need this separated out, then I'm going to have to ask someone in the community to reverse engineer what I present and port it back to 3.x branch. Otherwise I suggest we move to a new targeted version which isn't ideal either...

Sorry for being a pain but I'll do what I can to make this go as smooth as possible.

Comments

philsward created an issue. See original summary.

andraeray’s picture

andraeray commented

Hi Philsward, any contributions are welcome. Separating the changes is the most helpful so that each change can be properly tested, but I understand you may not have the time to do this.

1) I haven't looked at the comment issue much, I don't have any recommendation there.
2) For idempotency, you can probably just generate the idempotent key by doing a hash of order id, total, and possibly order created date. (I'm trying to be careful so that it works for users with recurring payments as well)
3) Just look at the stripe change log breaking changes I think it's pretty safe to upgrade since we don't use many stripe functions.

Originally, Charge() used to do the charge, it was straight forward. But after the SCA changes you create a payment intent, then attempt to process the intent, and check the status to see if it succeeded or needs extra authentication. So it doesn't become as straight forward anymore.

andraeray’s picture

andraeray commented

I have implemented the idempotency key in this ticket:
#3074679: Prevent user from making multiple accidental charges. using the order id, payment amount, and payment Id worked out to be a good key.

I removed the code that disables the "Submit Order" and just defaulted to normal UC cart settings. At the time I didn't consider that a network issue could have prevented it and it might succeed on a retry.

If you would like to submit a patch to name it retry please go ahead.

philsward’s picture

philsward commented

@AndraeRay sounds good. I'll have my guy take a look at your changes and compare with his work to see if there's anything more that can be done.

I'll note that he did get the idempotent key working, however I have had one order come through with two charges to stripe. Transactions were approximately 2 min apart from each other so it might be that pesky "network" issue.

philsward’s picture

philsward commented

@AndraeRay on a side note, I looked through some of the other issues that I'm tagged in and the network discussion came up which triggered a conversation I had with my programmer buddy. He mentioned that Stripe is doing some ajax loading under the hood which he felt is kind of un-necessary to getting the transactions to go through.

We discussed getting rid of the ajax, but haven't had a chance to hammer it out yet.

Thought I'd pass it along and put a bug in your ear. See if you had any thoughts on it.

andraeray’s picture

andraeray commented

I believe Ajax is a requirement. I don't know if you can get rid of it entirely. You might be able to add some webhooks in certain places though.

philsward’s picture

philsward commented

Relayed Thought #1

The _uc_stripe_confirm_payment() function calls drupal_json_decode() with 2 parameters (should be 1), and it should check whether the decoding was successful, it should check whether data contains the order_id, then it should check whether order_load was successful, then it should check whether it belongs to the user.

When looking for the stripe customer ID, there is a check whether the order id and the user id are different, but how would it be possible that user A pays the order of user B? Probably this case should not be allowed at all.

philsward’s picture

philsward commented

Relayed Thought #2

The _uc_stripe_confirm_payment() function checks whether the posted data contains payment_intent_id value, and if yes, it retrieves it and then it confirms it and then it saves in the order data, and all this is done without any validation. It should check whether retrieve was successful, as it is done in uc_stripe_charge(), and it should check whether confirm was successful.

philsward’s picture

philsward commented

Relayed Thought #3

For context, I had a bunch of double charges on the 17th of April (4 or 5 in one day). I am using the coding from my guy which has its own flavor of implementing the idempotency on it, but that hasn't been the problem. The problem has been with the submit button being removed on click and the customer clicking the back button. Hopefully this is enough back-story to help with below.

I think we need a fundamental change, and it seems I have to return to my confusion about these things:
- why is the ajax function called "confirm", when it is doing the charge
- why is the usual "charge" function just checking the result, not charging at all

And I am completely surprised that the ajax function charges AND confirms the payment, which is not logical and may add to the double payment problem.

For your information:
When uc_stripe tells the Stripe server to make the charge (in the ajax function), there is a parameter called "confirmation_method" which can be automatic or manual. And uc_stripe sends "manual" value, which means: "requires your server to initiate each payment attempt with an explicit confirmation.". The uc_stripe also sends a "confirm" parameter with "true" value, which is probably ignored, because it is not mentioned in the documentation. But... then uc_stripe immediately tells the Stripe server to confirm the charge (still in the ajax function).

But logical confirmation should not be done here, only when the Review Order form is submitted, and the so-called "charge" function is called. At least that should decrease the chances of double payments, but... as you saw my double order emails once, even the order can be completed twice.

(That last comment about double emails was in regards to testing the network response of each respective server, using a sandbox of my live site)

philsward’s picture

philsward commented

Reply to Comment #6

I believe Ajax is a requirement. I don't know if you can get rid of it entirely. You might be able to add some webhooks in certain places though.

AndraeRay mentioned webhooks, which means that the Stripe server connects to your server to tell the status of the payment.

This is exactly what the uc_paypal module does, and probably others.

The Stripe documentation says:
"It is technically possible to use polling instead of webhooks to monitor for changes caused by asynchronous operations—repeatedly retrieving a PaymentIntent so that you can check its status—but this is markedly less reliable and may pose challenges if used at scale. Stripe enforces rate limiting on API requests, so exercise caution should you decide to use polling."

So yes, with the current uc_stripe method, polling would be needed, so that as long as the status is "processing", it should retry. BUT the module does not even do polling, it just gives error to the client, while Stripe status may be "succeeded" in a few milliseconds or seconds.

So, to increase reliability, we need to use webhooks.

philsward’s picture

philsward commented
Version: 8.x-3.x-dev » 7.x-3.x-dev

Corrected branch version.