Skip to content

Commit 0a2b3b4

Browse files
fix(billing): cleanup 4 Copilot nits on subscription_changed analytics (#3772) (#3773)
- Remove redundant try/catch around AnalyticsService.capture() — never throws - Fix comment: billing.plan.changed → plan.changed (matches billingEvents.emit) - Fix stale-event fixture: add previous_attributes.items so stale guard fires - Add same-plan test: exercises previousPlan === newPlan guard with items present
1 parent c860b0f commit 0a2b3b4

2 files changed

Lines changed: 52 additions & 22 deletions

File tree

modules/billing/services/billing.webhook.service.js

Lines changed: 12 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -508,25 +508,19 @@ const handleSubscriptionUpdated = async (subscription, event) => {
508508
}
509509

510510
// Emit analytics observability event for downstreams running PostHog (no-op otherwise).
511-
// Mirrors the internal billing.plan.changed event but lands in the analytics pipeline.
511+
// Mirrors the internal plan.changed event but lands in the analytics pipeline.
512+
// AnalyticsService.capture() swallows its own errors — no outer try/catch needed.
512513
if (AnalyticsService.isConfigured()) {
513-
try {
514-
AnalyticsService.capture({
515-
distinctId: organizationId,
516-
event: 'subscription_changed',
517-
source: 'stripe-webhook',
518-
properties: {
519-
previousPlan,
520-
newPlan,
521-
isDowngrade,
522-
},
523-
});
524-
} catch (capErr) {
525-
logger.error('[billing.webhook] analytics capture subscription_changed failed (non-fatal)', {
526-
organizationId,
527-
error: capErr?.message ?? String(capErr),
528-
});
529-
}
514+
AnalyticsService.capture({
515+
distinctId: organizationId,
516+
event: 'subscription_changed',
517+
source: 'stripe-webhook',
518+
properties: {
519+
previousPlan,
520+
newPlan,
521+
isDowngrade,
522+
},
523+
});
530524
}
531525

532526
// Plan switch mid-cycle = refresh the active week snapshot to the new plan.

modules/billing/tests/billing.webhook.subscription-changed-analytics.unit.tests.js

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -225,8 +225,8 @@ describe('billing.webhook.service — subscription_changed analytics emit:', ()
225225
});
226226

227227
describe('when plan did not change', () => {
228-
test('does not capture subscription_changed', async () => {
229-
// No previous_attributes.items — no plan change detected
228+
test('does not capture subscription_changed when previous_attributes.items is absent', async () => {
229+
// No previous_attributes.items — plan-change branch is skipped entirely
230230
await BillingWebhookService.handleSubscriptionUpdated(
231231
_mkStripeSubscription(),
232232
_mkEvent({
@@ -239,15 +239,51 @@ describe('billing.webhook.service — subscription_changed analytics emit:', ()
239239
);
240240
expect(subscriptionChangedCalls).toHaveLength(0);
241241
});
242+
243+
test('does not capture subscription_changed when previousPlan === newPlan (same-plan guard)', async () => {
244+
// previous_attributes.items IS present but the plan is the same — the
245+
// `previousPlan !== newPlan` guard should prevent the emit.
246+
await BillingWebhookService.handleSubscriptionUpdated(
247+
_mkStripeSubscription({
248+
items: {
249+
data: [{ price: { id: 'price_growth', metadata: { planId: 'growth' } }, current_period_start: 1700000000 }],
250+
},
251+
}),
252+
_mkEvent({
253+
data: {
254+
previous_attributes: {
255+
items: { data: [{ price: { id: 'price_growth', metadata: { planId: 'growth' } } }] },
256+
},
257+
},
258+
}),
259+
);
260+
261+
const subscriptionChangedCalls = mockAnalyticsCapture.mock.calls.filter(
262+
([arg]) => arg.event === 'subscription_changed',
263+
);
264+
expect(subscriptionChangedCalls).toHaveLength(0);
265+
});
242266
});
243267

244268
describe('when event is stale (updateIfEventNewer returns null)', () => {
245269
test('does not capture subscription_changed', async () => {
270+
// Fixture includes previous_attributes.items so the stale-event guard is what
271+
// actually prevents capture (not the absence of a plan change).
246272
mockSubscriptionRepository.updateIfEventNewer.mockResolvedValue(null);
247273

248274
await BillingWebhookService.handleSubscriptionUpdated(
249-
_mkStripeSubscription(),
250-
_mkEvent(),
275+
_mkStripeSubscription({
276+
items: {
277+
data: [{ price: { id: 'price_growth', metadata: { planId: 'growth' } }, current_period_start: 1700000000 }],
278+
},
279+
}),
280+
_mkEvent({
281+
data: {
282+
previous_attributes: {
283+
items: { data: [{ price: { id: 'price_free', metadata: { planId: 'free' } } }] },
284+
},
285+
},
286+
}),
251287
);
252288

253289
const subscriptionChangedCalls = mockAnalyticsCapture.mock.calls.filter(

0 commit comments

Comments
 (0)