-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Conversation
Hey @mxstbr - in the midst of working through the payments v2 upgrade work. I'm building this with some of the following requirements:
I'm not happy with how v1 (current) payments work is implemented, mainly because we're doing the following:
This first commit is my brainstorm of how an API might work here; I've gone with a Stripe class that gets invoked with a I've tried to architect it to require as few network requests to Stripe as possible; it should truly only be one request to get a customer, but I have fallbacks to make new network requests to stripe to get the latest information. If you look at that commit I set up a dummy route where you can see demo usage, and you could visit const { Stripe } = require('../../models/stripe');
const Customer = new Stripe({ customerId: 'cus_CD6DWvSmbuVxUi' });
const hasAnalytics = await Customer.hasAnalytics(); Overall I'm fairly happy with this approach, and in theory should be easier to write tests against. Here are key benefits in my mind:
Here's how this would affect the rest of the system:
Here's where I'm stuck:
Here's where I'd love your thoughts:
Anything else come to mind? You can also just tell me if this approach is a bad idea in general :) |
Oh, and obviously this isn't done - just a proof of concept! I'd need to write a bunch more methods and do a whole bunch o testing. |
44325e3 is an alternative implementation that solves a couple problems:
Example: const hasAnalytics = await StripeCustomer.init({ customerId: 'cus_CD6DWvSmbuVxUi' })
.hasAnalytics()
.catch(err => console.log('Error', err.message));
console.log(hasAnalytics) // true
// note you can init with no arguments as well - not sure if this is confusing or not
const newCustomer = await StripeCustomer.init()
.createCustomer({ email: 'briandlovin+test2@gmail.com' })
.catch(err => console.log('Error', err.message));
console.log('newCustomer', newCustomer); // customer object At this point I'm mostly just doing research around how this kind of composition should work; I'm finding a lot of articles that would bemoan my use of |
LOL. Just got off a call with Kelly. tl;dr: I'm glad I wrote this PR because I learned a lot about JavaScript, but Kelly has like a 10x more sane and manageable approach to this that makes a lot more sense. Here's how it's going to work (still open to suggestions, but this feels pretty sane to me):
Still totally open to feedback on this @mxstbr but that method feels way more sane to me: we can isolate each webhook event into its own function that mirrors the source of truth in our db, which means we get to keep all of the same logic we have in our resolvers for checking user/community features! The cost is more storage in our db, but the pros are huge to this approach by decoupling our resolvers entirely from Stripe's infra. |
c8b2db7 is a proof of concept of the above messages.
So this worker is called Pluto, the purpose of which is to make us dat money. It works by receiving Stripe webhook events, figures out a handler, and then resolves itself with a 200 response to Stripe. If this looks good, tomorrow I'll set up the tables proposed above, the transformation layer (eg stripe customer => SpectrumCustomer), and start implementing handlers! They should all be fairly straightforward, and I'll only work on ones with the highest impact; we can do cool stuff here down the road about dispersing async jobs to hermes to send emails about failed payments, expiring cards, receipts, etc (and remove the /api/stripe endpoint from iris!) |
Haha damn, you went well in the weeds there! Glad you learned a ton about JavaScript 😊 The worker implementation makes sense to me. 👍 |
Okay, I'm going a bit in circles here, mostly because we keep coming up with complex edge cases that break some of my original thinking. Here's a new approach we could explore:
The outcome of this approach is the following:
Some downsides of this:
Some data model changes we'd want to consider:
What do you think? |
The more I think about this, the more appealing it is in terms of managing complexity at this early stage. Basically we can just stop caring about counting moderator seats, checking to see if a private channel is available to post to based on the community's payments records, etc. Instead all of the features just work without needed all this extra permission logic...then we just bill based on what's being used each month. |
Update: in touch with a field engineer at Stripe who has some ideas he wants to test on the use case I've attempted to implement, and the one describe above. Will follow up and work on something else in the meantime. |
…o payments-api-v2
Quick note on progress here @mxstbr for feedback:
As a result of these changes, and after the migration, we'll have the following data scenario: Between stripe webooks and this changefeed, we will always be in sync with Stripe. |
Actually I need to change some code so that this will be true, since I was originally trying to ensure that all stripe customers have an email on file; but this doesn't actually matter for us as long as we require an admin email before they can create any subscriptions. |
@@ -78,4 +78,52 @@ describe('Community settings overview tab', () => { | |||
cy.contains(community.description); | |||
cy.contains(community.website); | |||
}); | |||
|
|||
it.only('should allow managing branded login settings', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't leave the .only
there 😉 Danger will complain!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops! Forgot to remove, thanks :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excited man. Let's get this on alpha!
Wow I have no clue wtf is going on with these tests, so many are failing but untouched in this branch. Additionally, I'm totally unable to run any tests locally: Curious @mxstbr if you're able to run tests locally on this branch? |
A bit annoying that everything passes locally but fails in ci, I think because of a db mismatch :/ Commenting out the problematic test (that passes locally) for now to just get this to pass, and I'll re-integrate it into my other tests branch later. |
Yay! All tests pass <3 |
This PR updates all of our payments logic to an entirely new system that lives on a new worker, Pluto. All of our paid features can now be purchased a-la-carte. Additionally, this PR supports granting communities an open-source/education/non-profit status which grants them a free private channel and moderator seat.