Work through the Stripe integration checklist

[joshsmith]

Problem

We should maybe fan this out into separate issues, but following the checklist here: https://stripe.com/docs/checklist

Subtasks

• Verify edge case handling (I know at least one of the test values fails in weird ways for us)
• Review API error handling - Once you've gone live is an unfortunate time to discover you've not properly written your code to handle every possible error type, including those that should "never" happen. Be certain your code is defensive, handling not just the common errors, but all possibilities.
• Review your logging - Regularly examine your logs to ensure they're storing all the information you may need and they aren't storing anything of a sensitive nature (e.g., credit card details or personally identifiable information).
• Ensure you've registered your production webhooks
• Subscribe to the API announce mailing list (from our team email)

I know our logging is not very verbose around errors with Stripe right now, in particular #540 raises some issues around this with 500 errors. Not sure how to improve here.

[begedin]

API Error handling information

Stripe itself returns one of api_connection_error, api_error, authentication_error, card_error, invalid_request_error, or rate_limit_error.

stripity_stripe, however, can return:

• %APIErrorResponse{} - we handle this one in several places, but should make sure we handle it in all places and correctly. In fact, in Prevent swallowing of stripe errors #546, we explicitly stop "handling" these, so an exception can be raised and we can get it reported by sentry. This one is thrown when we try to perform a request, everything else is in order, but the request fails for some reason (parameters wrong, record missing, etc.)

stripity-stripe can also return these, but we do not handle them at all.

• %APIRateLimitingResponse{} - this one sounds like it could need a whole new feature. If related to webhook handling, we might need an event processing queue of some sort that works in the background. If triggered by a user action, we need some way to inform the user of "our payment processing servers being busy" or something.

• %AuthenticationErrorResponse{} - should not be happening if our environment is set up. Should be fine to just let it be caught by Sentry and reported.

• %HTTPClientFailed{} - if this one happens, probably something wrong with the addon internally.

• %OAuthAPIErrorResponse{} - seems to be returned in place of APIErrorResponse for the oauth related endpoints. Not sure yet how to handle these.

[joshsmith]

@begedin it would be good to run through every Stripe card error there is and see what might happen. Also to run through the others.

[joshsmith]

Comment collapsed as outdated

I'm not sure about the API rate limiting.

[joshsmith]

One thing that was mentioned to me earlier today that will help prevent some issues in the first place is to take advantage of doing spec on the types for the fn signatures and let static analysis do some of the work that testing might otherwise do, i.e. let Dialyzer really do its job.

[joshsmith]

Our interfaces right now also appear to be very open on a lot of functions, allowing random maps. Possible that we want to lock this down further by being more explicit in what we whitelist here.

[begedin]

Looked further into error handling. As said in another PR, stripity_stripe does not raise errors at all, other than the library specific MissingAPIKeys error. Instead, it returns a Stripe.APIErrorResponse struct if a stripe request produced an error.

This struct includes, as far as I can tell, all types of stripe errors, including card errors, other than the oauth-related errors.

When creating a card, the client will first try to go with creating a token using the javascript stripe library. If that fails, it will produce a stripe card error client-side, so we should make sure we handle those correctly. I believe we mentioned rendering card validation errors to, at some point.

If the token creation on the client succeeds, there is no reason it should not succeed on the API, when we actually create and store the card. If something does happen, the way our StripeService.XService modules work (or at least, ought to work) is that they use a with statement which will return an {:ok, something}, or an {:error, something}. This includes an {:error, %Stripe.APIErrorResponse{}} if stripity-stripe encounters trouble. At that point, unless we have some specific handling, which as far as I can tell, we do not have at all, there will be a function mismatch error at the controller level and that's it.

In turn, the client will get back a 500 response.

What I would like to do is

1. Make a pass through all of our service modules
2. Make sure none of them swallow errors
3. Basically remove the else blocks from all with statemets to avoid error swallowing.
4. Only add else blocks if we have cases which we handle in a special way.

What do I mean by "cases which we handle in a special way"

If we take a look at this

def update(%StripeConnectCard{} = record, %{} = attributes) do
  with {:ok, %Stripe.Card{} = updated_stripe_card} <- update_on_stripe(record, attributes)
  do
    {:ok, updated_stripe_card}
  else
    {:error, %Stripe.APIErrorResponse{} = error} -> {:error, error}
    _ -> {:error, :unhandled}
  end
end

The single line in the with block expects an {:ok, %Stripe.Card}. If the call to update_on_stripe/2 returns anything else, we will trigger the else block. In the case of a %Stripe.APIErrorResponse{}, that response will be passed out of the module, into the controller. In the case of any other error, it will get swallowed and replaced with :unhandled, the information from that error lost completely.

I wrote this and I think this is bad. I think we should outright remove the else block entirely from that call. That way, update/2 will return either {:ok, updated_stripe_card} or basically any {:error, error} returned by stripity-stripe.

Then, our controller will only expect {:ok, updated_stripe_card}and there will be a function mismatch error if anything bad happens, which gets reported by sentriy, etc.

If we need to handle some very specific form of a Stripe.APIErrorResponse, for example, one that contains a specific code or message, by explicitly doing something specific (pulling things out of thin air here, but for example, if we want to send a notification/email`, then we should add the following else block

else
  {:error, some_specific_match_which_we_do_handle} -> perform_handling
  any_other_unexpected_response -> any_other_unexpected_response
end

That way, errors that we do cover somehow, get covered, while anything else is still allowed to throw a function mismatch.

The reasoning here is, "anything else" is unexpected and should not happen, so it should raise an exception.

In short

A bunch of our when do else end blocks are outright swallowing errors. We should consolidate these and only swallow errors which we actually handle. Anything else should be let through and allowed to cause a function mismatch.

[begedin]

The above, combined with proper speccing and possibly restricting map-matching where it's necessary ought to make the whole system much more robust and easier to handle.

[joshsmith]

Should your requests begin to receive 429 HTTP errors, reduce the frequency of your requests. Each failed request is perfectly safe to retry as rate limiting takes place before any other action and prevents the request from being processed. When reducing your request frequency, we recommend an exponential backoff by first waiting one second before trying again. If your request continues to receive the same response, wait two seconds, then four seconds, etc.

Don't think we need to worry about this too much right now, but if we ever do, an exponential backoff seems appropriate.

[joshsmith]

For the first item on this list, we use the values supplied here https://stripe.com/docs/testing in order to test:

• Credit card numbers
  • successful payment
  • international cards
  • specific responses and errors
• Disputes
• Managed accounts
  • routing numbers
  • account numbers
  • tax IDs

We should see how our backend and our UI handles these and make some notes about the handling.