Fix pre-auth behavior

[bpcreech]

Before this change we were erroneously marking pre-auth'd charges as status=pending, when they're actually status=succeeded. We were (accidentally) working around this incorrect behavior in pre-auth'd PaymentIntents.

To get this right we have to actually split the _trigger_payment method into two: a check for payment authorization (which we do on construction even for Charges created with capture=false), and a separate routine to actually capture the charge (which we do on construction for non-pre-auth'd charges, and on _api_capture for pre-auth'd charges). We also split the PaymentIntent._api_confirm method into two for more control of error handling. We then adjust the PaymentIntent wrapper to fit.

This also fixes a tiny mistake in the Charge refund test; it was asserting the wrong variable.

This also fixes a minor CI problem by using a venv. (I can't reproduce this in my local fork, implying this is a runner change rolling out now... not sure if it's intentional or not, but using a venv seems like a good idea regardless.)

[bpcreech]

Actually sorry for the spam... there's still a flaw in this, for PaymentIntents we shouldn't check authorization until confirm. (So we need to not return a 402 on a PaymentIntent creation if confirm=false.)

Not sure if this PR makes that any worse, gotta think about it. Will consider and fix.

[bpcreech]

Actually sorry for the spam... there's still a flaw in this, for PaymentIntents we shouldn't check authorization until confirm. (So we need to not return a 402 on a PaymentIntent creation if confirm=false.)

Not sure if this PR makes that any worse, gotta think about it. Will consider and fix.

Actually this was working fine, what a pleasant surprise! :) I added a more elaborate test to verify it.

This should be good to review now

[bpcreech]

@adrienverge this one's ready for review if you want! We're using it at Via now (in an internal fork I want to get rid of :) ).

[adrienverge]

Sure! We'll look at it as soon as we have some time to review.

Good it to merge local edits upstream 👍 and by the way, do you plan to propose other ones? If it's the case, it's easier for us to test them all at once.

[bpcreech]

@adrienverge I do propose other ones. They are kind of based on each other in a way that Github isn't good at supporting.

I could send one big one after this if that is useful. The remaining stuff is a series of increasingly small things.

Also I have some things you might find suspicious: testing undocumented and/or deprecated Stripe behavior... I'll probably separate those out to avoid controversy on the more mainstream changes. :)

[adrienverge]

Hello @bpcreech, OK understood. Your contributions are welcome, especially since they are clean and useful :) For next ones: as you wish. GitHub can show PRs with multiple commits inside them pretty nicely. You can also open a PR mentioning that it's based on another one, and we'll only look at the last commit.

---

About this one, it looks good, but our test suite detected a problem. I digged to identify the problem, and it seems that your current implementation changes behavior for card 4000000000000341 (a card that you can associate to a customer but is supposed to fail upon payment, see documentation). With such a card, both Stripe and current Localstripe seem to accept creating a subscription (POST /v1/subscriptions succeeds with HTTP 200, although the subscription is incomplete) whereas your modification makes the call fail with HTTP 402.

Here are minimal reproducers.

• With real Stripe:
  - Create a customer
  - Associate card 4000000000000341 and make it the default payment method
  - Create a subscription: it succeeds (with status: incomplete instead of active)

  curl -u $SK: $HOST/v1/subscriptions -d customer=$cus -d items[0][plan]=any-plan-of-your-choice
  HTTP 200
  {
    "id": "sub_1Q7d…",
    …
    "status": "incomplete",
    …
  }

• Same with Localstripe master:

  #!/bin/bash
  set -eux
  HOST=http://localhost:8420
  SK=sk_test_12345
  
  # Create a plan
  curl -sSfg -u $SK: $HOST/v1/plans -d id=yearly-plan \
     -d name=Name \
     -d amount=20000 \
     -d currency=eur \
     -d interval=year || true
  
  # Create a customer
  cus=$(curl -sSfg -u $SK: $HOST/v1/customers \
             -d [email protected] \
        | grep -oE 'cus_\w+' | head -n 1)
  
  # Create a payment method "the right way", with setup intents and all.
  # But I've also tried with "the simple way", i.e. POST /v1/customers/$cus/cards,
  # and the result is the same.
  res=$(curl -sSfg -u $SK: -X POST $HOST/v1/setup_intents)
  seti=$(echo "$res" | grep '"id"' | grep -oE 'seti_\w+' | head -n 1)
  seti_secret=$(echo $res | grep -oE 'seti_\w+_secret_\w+' | head -n 1)
  res=$(curl -sSfg $HOST/v1/setup_intents/$seti/confirm \
             -d key=pk_test_sldkjflaksdfj \
             -d client_secret=$seti_secret \
             -d payment_method_data[type]=card \
             -d payment_method_data[card][number]=4000000000000341 \
             -d payment_method_data[card][cvc]=242 \
             -d payment_method_data[card][exp_month]=4 \
             -d payment_method_data[card][exp_year]=2030 \
             -d payment_method_data[billing_details][address][postal_code]=42424)
  pm=$(echo "$res" | grep '"payment_method"' | grep -oE 'pm_\w+' | head -n 1)
  curl -u $SK: $HOST/v1/payment_methods/$pm/attach -d customer=$cus
  
  # Set this payment method the default one
  curl -sSfg -u $SK: $HOST/v1/customers/$cus \
       -d invoice_settings[default_payment_method]=$pm
  
  # Create a subscription
  curl -sSfg -u $SK: $HOST/v1/subscriptions \
       -d customer=$cus \
       -d items[0][plan]=yearly-plan

  This succeeds, the subscription is created and has status: incomplete.

• With this pull request (commit d00c513): the same script fails with HTTP 402.

In my opinion, failing with a HTTP 402 would make sense. I think this is how Stripe behaves for simple payments like POST /v1/charges. But it's not how they do for subscriptions, so we need to adapt...

[bpcreech]

ah good to know! we don't use subscriptions at Via so I didn't catch that

I'll see if I can add the test you created and fix the behavior (EDIT ah you already did the first part! awesome)

[bpcreech]

okay, @adrienverge this should be ready for re-review now!

I modified it so that all the paths to Charge creation have to explicitly request raise-on-failure by way of an on_failure_now callback, and the Invoice creation doesn't do that.

As part of that I split up PaymentIntent._api_confirm into two functions: the "front door" for the API, and the "back door" which is called by Invoices. That way we can pass in the on_failure_now argument.

[bpcreech]

Also added a small CI fix: it looks like the Github actions runner is now set to require a venv (which for some reason I can't repro on my own fork)

[H--o-l]

Also added a small CI fix: it looks like the Github actions runner is now set to require a venv (which for some reason I can't repro on my own fork)

@bpcreech thanks for seeing and investigating the issue. I wasn't entirely convinced with the venv solution and I was curious so I investigated further and proposed another solution: #234
Sorry I have not waited for your feedback or contribution for that one, it was about the CI so I assumed it wasn't the subject you were the most interested in and more on us to fix it.

I ran your PR on our test suite and no issue was detected this time, that's great. I will take more time tomorrow to read the code and spot issues if any.

In the meantime, if you want, you can rebase on master, the CI will be fixed (or later, no hurry)

[bpcreech]

no problem, rebased!

[H--o-l]

Hi!

The code looks clean and smart.

It's a bit too hard for me to understand every cases. I tried but it's too much time, so I trust you to have been carreful on the details (other people uses Localstripe, if you break their integration they will not be happy 😅 )

I let Adrien tells if it looks OK to him too or not, on my side good to go 👍

[adrienverge]

Looks good!

Thanks @bpcreech and @H--o-l 👍

The CI environments seems to have new problems, but tests pass OK on my computer, so I'll merge, and look at those independently.