feat: update Stripe plan in PlansStorage#set
#318
Conversation
We'd like to let users change their plan directly from console or the CLI, which means we need to teach `PlansStorage#set` how to update Stripe. I've introduced a new `BillingProvider` interface to start abstracting Stripe functionality and especially to make it easier to test systems that interact with Stripe. I have not done a thorough audit of the various ways we interact with Stripe, but I'd like to move toward having all Stripe interactions go through `BillingProvider`. The linter is currently failing because I made the `account` field in the the billing customer schema optional and a fair amount of the billing logic relies on it being not-`undefined`. I could just go in and update logic to deal with undefined `account` but wanted to get some feedback from @alanshaw before I dig into that: I can see arguments for either option (roughly "we might want to track customers who don't have an account set up in Stripe" vs "the point of this table is to track Stripe accounts") but testing will require a bit more work if we want to preserve non-optionality (since the test stores don't get initialized from the test billing provider in the same way - in production they get initialized via the webhook handler, which doesn't currently have an analog in testing code). Either of the options here will require a bit more work, so I'd love to know if anyone has strong feelings before I finish this off.
upload-api/stores/plans.js
Outdated
| if (hasCustomerResponse.error) { | ||
| return hasCustomerResponse | ||
| } else { | ||
| return { error: new Failure(`billing provider does not have customer for ${customer}`) } |
There was a problem hiding this comment.
So this would maybe be a BillingCustomerNotFoundError that we can use in the types for PlansStorage?
There was a problem hiding this comment.
ah yep - I want to replace all the Failures in this PR with typed error responses like that, great catch!
upload-api/stores/plans.js
Outdated
| } | ||
| } | ||
| } catch (/** @type {any} */ err) { | ||
| return { error: err } |
There was a problem hiding this comment.
Just let it throw...this is what ucanto will do for us essentially.
For sure - tried to explain this in the PR description but it was end-of-day stream of consciousness. Tests for Taken on its own, this suggests to me that we should rework the test and perhaps make It's also entirely possible that this case is already supported without making |
We had a quick huddle about `Ucanto.Result` inspired by a discussion in `w3infra` (storacha/w3infra#318 (comment)) and I committed to writing up a proposal describing the tradeoffs of using _only_ `Result` for error handling vs using `Result` plus normal JavaScript error handling. I've proposed we standardize on `Result` purity and offered to take a pass through the code to ensure we are wrapping all possible thrown errors in error `Result`s but want to get feedback from the team before I do that! Comments sincerely requested - I, for one, could pretty easily be swayed to the "error `Result`s for known errors, thrown errors for unexpected stuff" camp, so if you feel strongly please let us know!
linter still failing because I'm returning unexpected error types, need to update w3up one more time..
it gets initialized before our system, so should always have a record.
Co-authored-by: Alan Shaw <[email protected]>
also respect no-implicit-coercion
found this in the organization secrets in GitHub, so let's re-use it
I also rolled this in Stripe
I was not doing it correctly, which is I think why the integration tests are failing
I updated them in the Stripe test environment to match what we use in production, since we don't support using different plan names across the board.
|
created #320 because the integration environment for this PR is broken - integration tests pass there! |
We'd like to let users change their plan directly from console or the CLI, which means we need to teach
PlansStorage#sethow to update Stripe.I've introduced a new
BillingProviderinterface to start abstracting Stripe functionality and especially to make it easier to test systems that interact with Stripe. I have not done a thorough audit of the various ways we interact with Stripe, but I'd like to move toward having all Stripe interactions go throughBillingProvider.The linter is currently failing because I made the
accountfield in the the billing customer schema optional and a fair amount of the billing logic relies on it being not-undefined. I could just go in and update logic to deal with undefinedaccountbut wanted to get some feedback from @alanshaw before I dig into that: I can see arguments for either option (roughly "we might want to track customers who don't have an account set up in Stripe" vs "the point of this table is to track Stripe accounts") but testing will require a bit more work if we want to preserve non-optionality (since the test stores don't get initialized from the test billing provider in the same way - in production they get initialized via the webhook handler, which doesn't currently have an analog in testing code). Either of the options here will require a bit more work, so I'd love to know if anyone has strong feelings before I finish this off.