-
Notifications
You must be signed in to change notification settings - Fork 37
Add monthly subscriptions #23
Conversation
75d78cd to
0a55b22
Compare
775fafc to
9a3ee27
Compare
|
|
||
| before_action :set_account, only: %i[ edit update ] | ||
|
|
||
| def index |
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.
This is a simple admin screen where you can modify the card count in a given account. It is intended mostly to test this in staging.
| class Admin::StatsController < AdminController | ||
| layout "public" | ||
|
|
||
| def show |
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.
Moved to the gem, but unrelated to the billing system.
| end | ||
|
|
||
| private | ||
| def dispatch_stripe_event(event) |
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.
Hey @zachasme I would love your eyes on how we are handling stripe webhooks and on the flow in general.
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.
Looks great @jorgemanrubia!
I only see one potential issue that have bitten me in the past: Stripe doesn’t guarantee the delivery of events in the order that they’re generated (WTF?). If I recall correctly, a successful checkout session will generate:
checkout.session.completedcustomer.subscription.createdcustomer.subscription.updated
almost always in that order. Additionally, event data will correspond to the time it was dispatched, so if you handle created after updated you could end up in a bad state.
I have found the best solution is not to rely on any data provided in event payloads, except identifiers. I would recommended something like:
when "checkout.session.completed"
sync_subscription(event.data.object.subscription) if event.data.object.mode == "subscription"
when "customer.subscription.updated"
when "customer.subscription.deleted"
sync_subscription(event.data.object.id)
#...
def sync_subscription(stripe_id)
stripe_subscription = Stripe::Subscription.retrieve(stripe_id)
if subscription = find_subscription_by_customer(stripe_subscription.customer)
cancellation_reason = stripe_subscription.cancellation_details.reason
subscription.update! #...
end
endI also think stripe dispatches customer.subscription.deleted when payment fails, in which case you can use stripe_subscription.cancellation_details.reason instead of listening for invoice.payment_failed.
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.
Thanks @zachasme, super useful, I will tackle this tomorrow.
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.
Hey @zachasme could you have a second look at the changes 🙏?
b855ebc to
7df55bc
Compare
Events can land out of order. See #23 (comment)
78e65b2 to
acdd2ca
Compare
| when "checkout.session.completed" | ||
| sync_new_subscription(event.data.object.subscription, plan_key: event.data.object.metadata["plan_key"]) if event.data.object.mode == "subscription" | ||
| when "customer.subscription.updated", "customer.subscription.deleted" | ||
| sync_subscription(event.data.object.id) |
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.
I still think there's going to be an issue if checkout.session.completed comes in last, since the subscription plan_key is being validated for presence. I'd try and stick to calling sync_subscription directly from all branches, and read the plan_key from subscription metadata in all cases.
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.
Oh, the subscription is already created with a plan_key before checkout, so probably not an issue. But generally I still think we should avoid branching logic based on event type.
| secrets_escaped = `kamal secrets fetch \ | ||
| --adapter 1password \ | ||
| --account basecamp \ | ||
| --from "Deploy/Fizzy" \ | ||
| "Development/STRIPE_SECRET_KEY" \ | ||
| "Development/STRIPE_MONTHLY_V1_PRICE_ID" 2>/dev/null` | ||
|
|
||
| secrets_json = secrets_escaped.gsub(/\\(.)/, '\1') | ||
| secrets = JSON.parse(secrets_json) | ||
|
|
||
| stripe_secret_key = secrets["Deploy/Fizzy/Development/STRIPE_SECRET_KEY"] | ||
| stripe_price_id = secrets["Deploy/Fizzy/Development/STRIPE_MONTHLY_V1_PRICE_ID"] |
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.
@jorgemanrubia curious if you looked at using op run for handling the secrets, rather than Kamal?
It ought to take care of some of these details for you, and would let you move the secret & env names into a file like .stripe-env rather than have them inside the script. Then you could exec both the stripe listen and bin/dev with the necessary secrets in place.
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.
@kevinmcconnell thanks, that would be an improvement indeed. I will track this one in Basecamp, as I want to finish up other stuff this week first.
0abd3cf to
4ff06fc
Compare
…nner Instead: - Always let you create drafts when pressing "Add card" - Prevent creations of published cards via API - Prevent publication of cards in all cases when the limit is exceeded
Before, we were relying on just changing the cards_count in account, but this could create problems where the system to calculate the next card number fails due to the unique constraint.
Co-authored-by: Matt Almeida <[email protected]>
6f2e97c to
77c5729
Compare
77c5729 to
3a2d2ad
Compare
Events can land out of order. See basecamp/fizzy-saas#23 (comment)
https://3.basecamp.com/2914079/buckets/37331921/todos/9346492118
See basecamp/fizzy#2050