fix(billing): ensure all tenants have valid Stripe customer IDs #5
Reference in New Issue
Block a user
Delete Branch "userAdityaa/caravel:billing-tenants-id"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Problem
Tenants could be created with empty
stripe_customer_id, breaking invoice/webhook ownership mapping and causing ambiguous billing behavior.Solution
stripe_customer_idcustomer_idis missingTesting
Closes #2
@@ -0,0 +16,4 @@WHEN trim(NEW.stripe_customer_id) = ''BEGINSELECT RAISE(ABORT, 'stripe_customer_id cannot be empty');END;This is all too heavy handed, validation is already done at the application level, the column should just be changed to non-null in migration 1.
@@ -372,0 +368,4 @@// Ensure tenant exists and always has a Stripe customer id.match state.api.query.get_tenant(&pubkey).await {Ok(Some(existing_tenant)) => {if existing_tenant.stripe_customer_id.trim().is_empty() {This branch should never happen because tenant creation fails below if stripe customer fails to be created. We don't have any data that needs to be migrated. Just make the customer id not null.
@@ -267,0 +281,4 @@.execute(&mut *tx).await?;let activity = Self::insert_activity(&mut tx, "update_tenant", "tenant", pubkey).await?;Skip activity logging, this is just an implementation detail
Addressed the feedback, and also added tests to cover the flow.
Just a couple comments, and then feel free to merge.
Re: tests, it's totally up to you, but I generally go without until the app is stable, then add them with the correct behavior so that I'm not slowing myself down maintaining incorrect tests. But again, up to you.
@@ -131,0 +139,4 @@"tenant {} has no stripe_customer_id",tenant.pubkey));}This can be removed, it should be impossible
@@ -445,0 +460,4 @@pub async fn stripe_create_customer(&self, tenant_pubkey: &str) -> Result<String> {if self.stripe_secret_key.trim().is_empty() {return Err(anyhow!("missing STRIPE_SECRET_KEY"));}This should be validated on startup (in the constructor) and fail there instead
I’ll push all the updates shortly, including PRs for the other open issues as well.
I’ll keep the tests in the codebase for now since writing them as I go is part of my usual workflow.
I’ll make sure to update them as the behavior evolves so they stay accurate and useful.
bad81dbbb8to6faf7d3fa0LGTM, is this still a WIP or ready to merge?
was just checking everything from my side, removing WIP if the changes looks good to you.