[Spike] Investigate our Stripe Integration

[lin-d-hop]

Description

We've been having a continuous run of Stripe related S2s since we switched to SCA. It is really causing a lot of strife to our support teams and to our users who are very aware that they are losing sales.

It seems likely that in the migration to SCA we implemented Stripe incorrectly. This spike is intended to deep dive into our Stripe integration to try and understand if we can improve.

Expected Behavior

Stripe payments should work reliably 99.9999% of the time outside of user error.

Actual Behaviour

Frequent failed payments, missing orders and general errors, seemingly related to asynch checks.

Severity

Assigning S2 as this is intended to be a super issue over the existing steady flow S2s. As a spike we simply want to understand the scale of the work required to repair our stripe integration and the probability of reducing the frequency of issues.

Your Environment

• Version used:
• Browser name and version:
• Operating System and version (desktop or mobile):

Related Issues and Conversations

Slack post on #bugs
#7809
#7767
#8019

[andrewpbrett]

First, a bit of a solo retro on SCA

We made a really big change to the way payments work on OFN when introducing SCA.

Initially, we didn't even have a separate state for payments that were awaiting SCA authorization and just relied on inferring it based on their cvv_response_message (bad idea). We now have a distinct state, but we're still finding places where we'd previously made the assumption that payments are immediately either processed or marked as failed. #8059 is a good example of this.

We also have three different ways that payments happen: front office, back office, and subscription. There are necessary differences between these three flows. Subscriptions in particular can be a bit obtuse on their own, so adding a big change to payments on top led to a lot of bugs that were hard to track down.

What to do now

As far as I can tell, the closest thing we currently have that could be called an "incorrect" implementation is that we've diverged from the official ActiveMerchant gem by using a decorator where we override some methods, most notably the store method for storing a credit card token. We originally added the decorator because we were stuck on an older version of ActiveMerchant (because we were stuck on an older version of Rails).

So removing that decorator should be on our priority list. I've started a PR (#8071) to do so, and it led me straight to an example of the other big problem with our Stripe implementation: tests. In this commit, 6c2d949, I was able to remove the decorator and get the build to green; however, a quick smoke test showed that the checkout was broken in spite of the green build. I'm going to add a failing test that passes with the new changes, but this is a pretty common pattern that I've seen with our Stripe tests: because we're using stubs, it's pretty easy to get to a "passing" test suite that doesn't reflect what happens in reality. Stubs are necessary in some instances but need to be used pretty carefully/thoughtfully.

Finishing the PR should be less than a day's work left.

In the past we've discussed adding the VCR gem for integration tests; #7767 was created after we first tried to remove the decorator. I think we should also prioritize putting some dev time, maybe 10-15 hours at first, toward adding some basic integration tests that use VCR and cover things like checking out with various test card numbers.

Those are the top two priorities (removing the decorator and adding VCR tests).

There are some other items that we can do to get closer to Lynne's 99.9999% goal. There are a few places that could really use some refactoring; with improvements to the test suite, this will be easier to do with confidence.

The first is around backoffice payments; luckily we have some unfinished work from Pau in #7809 that will be a good first step. Estimate of 1-2 dev days to load it back into your head, make any necessary updates, and make sure there's no additional tests needed.

The second is the (already ongoing) work around order and payment states.

The third would be to refactor subscriptions. Pau also started some work here that we could continue. This could obviously turn into a really large project so we might want to do another spike here to determine what makes the most sense to break off as a manageable piece.

I'll stop there since this has already gone on longer than a spike probably should :) @lin-d-hop let me know if you have other questions or feel free to close this one if this comment is sufficient.

[mkllnk]

👍

I find this a very helpful assessment with important next steps. I've been advocating for VCR for a while but never had the time to actually implement it. It actually speeds up development because you don't have to come up with mock data, it's automatically captured from the real test API.

[lin-d-hop]

@andrewpbrett this is great to read (sorry for the long delay)

I agree that improving active merchant and tests could be priorities - should be classed as tech debt.

What about this suggestion from Cillian - is this relevant?

[andrewpbrett]

Thanks @lin-d-hop. I think @cillian's suggestion is intended to solve the issue of "Paid orders that aren't appearing in the system", which I think we've tracked down to a combination of #8090 and #8143 (comment). So while we could set up listening on that webhook, it would take some time to set up correctly (in fact it might require each instance manager to configure something) and essentially just be a double check at this point. Unless we continue to see payments taken on Stripe but not marked complete in OFN, I'd recommend we hold off on listening to that webhook.

I'll close this spike since I think all of the remaining actions have their own issues and/or PR's.