From 7e73de06766179d7e2a8eda2e115c40bab0a440e Mon Sep 17 00:00:00 2001 From: akansnoro Date: Thu, 22 Oct 2026 14:29:38 +0100 Subject: [PATCH] feat: re-land issue 1684 (subscription billing correctness) Re-lands the part of the reverted #1703 squash that belongs to issue 1684 only. The remainder of the squash is re-landed by the other branches of the split, so this diff contains this issue's work exclusively. --- backend/billing/jobs/index.ts | 1 + .../jobs/monthlyRevenueRecognitionJob.ts | 98 +++++++++++++++++-- .../analytics/subscriptionAnalyticsService.ts | 11 ++- backend/services/batchChargeService.ts | 2 +- backend/services/billing/groupBilling.ts | 65 +++++++++--- backend/services/billing/interfaces.ts | 2 +- backend/services/billing/meteringService.ts | 55 +++++++++-- src/services/__tests__/groupBilling.test.ts | 14 ++- src/services/analyticsService.ts | 14 +-- src/services/invoiceService.ts | 43 +++++--- src/store/slices/analyticsSlice.ts | 10 +- src/store/subscriptionStore.ts | 16 +-- src/utils/billingCycleMath.ts | 98 +++++++++++++++++++ src/utils/invoiceTotals.ts | 87 ++++++++++++++-- 14 files changed, 430 insertions(+), 86 deletions(-) create mode 100644 src/utils/billingCycleMath.ts diff --git a/backend/billing/jobs/index.ts b/backend/billing/jobs/index.ts index 6b4fe8ef..fa0f7568 100644 --- a/backend/billing/jobs/index.ts +++ b/backend/billing/jobs/index.ts @@ -26,6 +26,7 @@ export { export type { RevenueSchedule, RevenueScheduleEntry, + EntryRecognitionProgress, RecognitionJournalEntry, RevenueRecognitionRepository, RecognitionJobMetrics, diff --git a/backend/billing/jobs/monthlyRevenueRecognitionJob.ts b/backend/billing/jobs/monthlyRevenueRecognitionJob.ts index e7e37213..bd5580a7 100644 --- a/backend/billing/jobs/monthlyRevenueRecognitionJob.ts +++ b/backend/billing/jobs/monthlyRevenueRecognitionJob.ts @@ -19,6 +19,12 @@ export interface RevenueScheduleEntry { periodEnd: number; // Unix ms recognisedAmount: number; isRecognised: boolean; + /** + * Cumulative amount already recognised for this entry across all prior + * runs. Optional so schedules persisted before progress tracking existed + * still load; treat a missing value as 0. + */ + recognisedToDate?: number; } export interface RevenueSchedule { @@ -42,6 +48,12 @@ export interface RecognitionJournalEntry { type: 'scheduled' | 'accelerated' | 'partial'; } +/** Per-entry cumulative recognition progress recorded by a run. */ +export interface EntryRecognitionProgress { + periodStart: number; + recognisedToDate: number; +} + export interface RevenueRecognitionRepository { /** Return all schedules where at least one entry is not yet recognised. */ getPendingSchedules(): Promise; @@ -54,6 +66,19 @@ export interface RevenueRecognitionRepository { ): Promise; /** Mark individual schedule entries as recognised. */ markEntriesRecognised(subscriptionId: string, entryPeriodStarts: number[]): Promise; + /** + * Persist cumulative per-entry recognition progress. + * + * Optional. Without it a partially-elapsed entry cannot record how much of + * itself it has already recognised, and the next run re-recognises the whole + * elapsed portion — inflating recognised revenue on every run. When the + * method is absent the job degrades to recognising each entry only once, + * at its full amount, which is slower but never over-counts. + */ + recordEntryProgress?( + subscriptionId: string, + progress: EntryRecognitionProgress[] + ): Promise; } export interface RecognitionJobMetrics { @@ -70,6 +95,12 @@ function nextId(): string { return `rev-${Date.now().toString(36)}-${(++_seq).toString(36)}`; } +/** Rounds to minor units, mapping non-finite input to zero. */ +function roundMoney(amount: number): number { + if (!Number.isFinite(amount)) return 0; + return Math.round((amount + Number.EPSILON * Math.sign(amount)) * 100) / 100; +} + /** * Determine how much of a single schedule entry to recognise as of `now`. * Returns 0 for entries that haven't started yet or amount = 0 (free trial). @@ -150,13 +181,48 @@ export class MonthlyRevenueRecognitionJob { for (const schedule of schedules) { localMetrics.schedulesProcessed += 1; const recognisedPeriodStarts: number[] = []; + const progressUpdates: EntryRecognitionProgress[] = []; let totalRecognisedForSchedule = 0; + // Deferred revenue is what was contractually billed but not yet + // recognised, i.e. the schedule total minus everything recognised on + // prior runs and earlier in *this* run. Ignoring prior runs produced a + // deferred balance that grew without bound. + const previouslyRecognised = (schedule.entries ?? []).reduce( + (sum, entry) => sum + (entry.recognisedToDate ?? 0), + 0 + ); + let recognisedSoFar = previouslyRecognised; + + const canTrackProgress = typeof this.repo.recordEntryProgress === 'function'; + for (const entry of schedule.entries) { - const { amount, type } = computeEntryRecognition(entry, now); - if (!type || amount <= 0) continue; + const { amount: cumulativeTarget, type } = computeEntryRecognition(entry, now); + if (!type || cumulativeTarget <= 0) continue; + + // Recognise only the increment since the last run. Recognising the + // full elapsed portion again on every run is what compounded the + // misstatement day after day. + const alreadyRecognised = entry.recognisedToDate ?? 0; + const amount = canTrackProgress + ? roundMoney(Math.max(cumulativeTarget - alreadyRecognised, 0)) + : // Without progress tracking, only recognise an entry once its + // period has fully elapsed, so it is never recognised twice. + type === 'partial' + ? 0 + : roundMoney(cumulativeTarget); + + if (amount <= 0) { + if (type === 'scheduled' || type === 'accelerated') { + // Already fully recognised on a prior run; close it out. + recognisedPeriodStarts.push(entry.periodStart); + } + continue; + } - const deferredBefore = schedule.totalAmount - totalRecognisedForSchedule; + const deferredBefore = roundMoney( + Math.max(schedule.totalAmount - recognisedSoFar, 0) + ); const journalEntry: RecognitionJournalEntry = { id: nextId(), subscriptionId: schedule.subscriptionId, @@ -165,19 +231,33 @@ export class MonthlyRevenueRecognitionJob { periodEnd: entry.periodEnd, recognisedAmount: amount, deferredBefore, - deferredAfter: deferredBefore - amount, + // Deferred can never go negative: recognising more than was + // deferred would be reported as an amount owed back to the merchant. + deferredAfter: roundMoney(Math.max(deferredBefore - amount, 0)), createdAt: now, type, }; try { await this.repo.writeJournalEntry(journalEntry); + recognisedSoFar += amount; totalRecognisedForSchedule += amount; localMetrics.entriesRecognised += 1; localMetrics.totalAmountRecognised += amount; - // Only mark fully-elapsed entries as done. - if (type === 'scheduled' || type === 'accelerated') { + progressUpdates.push({ + periodStart: entry.periodStart, + recognisedToDate: roundMoney(alreadyRecognised + amount), + }); + + // Only close out entries whose period has fully elapsed AND that + // are now fully recognised. + const recognisedTotal = roundMoney(alreadyRecognised + amount); + if ( + type === 'scheduled' || + type === 'accelerated' || + recognisedTotal >= roundMoney(entry.recognisedAmount) + ) { recognisedPeriodStarts.push(entry.periodStart); } } catch (err) { @@ -195,6 +275,12 @@ export class MonthlyRevenueRecognitionJob { recognisedDelta: totalRecognisedForSchedule, deferredDelta: -totalRecognisedForSchedule, }); + if (progressUpdates.length > 0 && canTrackProgress) { + await this.repo.recordEntryProgress!( + schedule.subscriptionId, + progressUpdates + ); + } if (recognisedPeriodStarts.length > 0) { await this.repo.markEntriesRecognised( schedule.subscriptionId, diff --git a/backend/services/analytics/subscriptionAnalyticsService.ts b/backend/services/analytics/subscriptionAnalyticsService.ts index 226b3d68..8f811ca4 100644 --- a/backend/services/analytics/subscriptionAnalyticsService.ts +++ b/backend/services/analytics/subscriptionAnalyticsService.ts @@ -317,9 +317,14 @@ export class SubscriptionAnalyticsService { // ── Private helpers ───────────────────────────────────────────────────────── private _monthlyRevenue(sub: Subscription): number { - // Normalise any billing cycle to monthly revenue - if (sub.billingCycle === BillingCycle.YEARLY) return sub.price / 12; - if (sub.billingCycle === BillingCycle.WEEKLY) return sub.price * 4.345; + // Normalise any billing cycle to monthly revenue. `ANNUAL` was missing + // here, so an annual subscription's full yearly price was reported as + // MRR — inflating MRR/ARR/churn by 12×. `monthlyEquivalent` also keeps + // the weekly factor identical to the frontend (52/12, not 4.345). + if (sub.billingCycle === BillingCycle.YEARLY || sub.billingCycle === BillingCycle.ANNUAL) { + return sub.price / 12; + } + if (sub.billingCycle === BillingCycle.WEEKLY) return sub.price * (52 / 12); return sub.price; } } diff --git a/backend/services/batchChargeService.ts b/backend/services/batchChargeService.ts index 63969fa5..fa0e8833 100644 --- a/backend/services/batchChargeService.ts +++ b/backend/services/batchChargeService.ts @@ -43,7 +43,7 @@ export interface BatchChargeResult { export class BatchChargeService { private intervalHandle: ReturnType | null = null; private lastMatchTimestamp = 0; - private RunHistory: BatchChargeResult[] = []; + private runHistory: BatchChargeResult[] = []; private maxHistory = 50; private cronExpression = '0 0 * * *'; private checkIntervalMs = 60_000; diff --git a/backend/services/billing/groupBilling.ts b/backend/services/billing/groupBilling.ts index f89e47ee..c5356514 100644 --- a/backend/services/billing/groupBilling.ts +++ b/backend/services/billing/groupBilling.ts @@ -53,10 +53,24 @@ export interface GroupPlanCustomization { sharedFeatures: string[]; memberLimits: Record; ownerDiscount: number; + /** Tax rate in basis points (e.g. 2000 = 20%). Optional; 0 when unset. */ + taxRateBps?: number; createdAt: number; updatedAt: number; } +/** Rounds to minor units, mapping non-finite input to zero. */ +const roundMoney = (amount: number): number => { + if (!Number.isFinite(amount)) return 0; + return Math.round((amount + Number.EPSILON * Math.sign(amount)) * 100) / 100; +}; + +/** Constrains a basis-point tax rate to a sane `[0, 100%]` range. */ +const clampRate = (bps: number | undefined): number => { + if (bps === undefined || !Number.isFinite(bps)) return 0; + return Math.min(Math.max(bps, 0), 10_000); +}; + const createId = (prefix: string): string => `${prefix}_${Date.now().toString(36)}_${Math.random().toString(36).slice(2, 8)}`; @@ -97,28 +111,40 @@ export class GroupBillingService { }; } - aggregateCharges(group: SubscriptionGroup, periodDays = 30): GroupBillingLineItem[] { - const cutoff = Date.now() - periodDays * 24 * 60 * 60 * 1000; - const recentCharges = group.charges.filter( - (c) => c.chargedAt.getTime() >= cutoff - ); + aggregateCharges( + group: SubscriptionGroup, + periodStart: number, + periodEnd: number + ): GroupBillingLineItem[] { + // The window is the requested period, not "the last N days relative to + // now". Deriving the cutoff from `Date.now()` made a historical invoice + // price a completely different set of charges than the period it claims + // to cover, so the line items never summed to the stated period. + const start = Math.min(periodStart, periodEnd); + const end = Math.max(periodStart, periodEnd); + const periodCharges = group.charges.filter((c) => { + const chargedAt = c.chargedAt.getTime(); + return chargedAt >= start && chargedAt <= end; + }); const memberCharges = new Map(); for (const member of group.members) { memberCharges.set(member.address, 0); } - for (const charge of recentCharges) { + for (const charge of periodCharges) { for (const item of charge.breakdown) { const current = memberCharges.get(item.memberAddress) ?? 0; - memberCharges.set(item.memberAddress, current + item.amount); + memberCharges.set(item.memberAddress, roundMoney(current + item.amount)); } } + const periodDays = Math.max(1, Math.ceil((end - start) / (24 * 60 * 60 * 1000))); + return Array.from(memberCharges.entries()).map(([address, amount]) => ({ memberAddress: address, amount, - description: `Aggregated charges over last ${periodDays} days`, + description: `Aggregated charges over ${periodDays} day(s)`, })); } @@ -130,15 +156,24 @@ export class GroupBillingService { periodEnd: number, currency = 'USD' ): GroupInvoice { - const aggregated = this.aggregateCharges(group, Math.ceil((periodEnd - periodStart) / (24 * 60 * 60 * 1000))); - const totalAmount = aggregated.reduce((sum, item) => sum + item.amount, 0); + const aggregated = this.aggregateCharges(group, periodStart, periodEnd); + const totalAmount = roundMoney(aggregated.reduce((sum, item) => sum + item.amount, 0)); const customization = this.planCustomizations.get(group.groupId); - const discountAmount = customization?.ownerDiscount - ? (totalAmount * customization.ownerDiscount) / 100 - : 0; - const taxAmount = 0; - const finalAmount = totalAmount - discountAmount + taxAmount; + + // `ownerDiscount` is a percentage but arrives from user-editable config. + // A negative value used to *increase* the bill and anything above 100 + // produced a negative payable amount, so both ends are clamped. + const rawDiscountPercent = customization?.ownerDiscount ?? 0; + const discountPercent = Math.min(Math.max(Number.isFinite(rawDiscountPercent) ? rawDiscountPercent : 0, 0), 100); + const discountAmount = roundMoney((totalAmount * discountPercent) / 100); + + // Tax was hard-coded to 0, so group invoices were never taxed even for + // entities inside a tax jurisdiction. + const taxRate = clampRate(customization?.taxRateBps); + const taxableAmount = Math.max(totalAmount - discountAmount, 0); + const taxAmount = roundMoney((taxableAmount * taxRate) / 10_000); + const finalAmount = roundMoney(Math.max(taxableAmount + taxAmount, 0)); const invoice: GroupInvoice = { id: createId('ginv'), diff --git a/backend/services/billing/interfaces.ts b/backend/services/billing/interfaces.ts index 776e426f..d1470a61 100644 --- a/backend/services/billing/interfaces.ts +++ b/backend/services/billing/interfaces.ts @@ -222,7 +222,7 @@ export interface IPartnerService { export interface IGroupBillingService { generateBillingSummary(group: any): any; - aggregateCharges(group: any, periodDays?: number): any[]; + aggregateCharges(group: any, periodStart: number, periodEnd: number): any[]; generateInvoice(group: any, periodStart: number, periodEnd: number, currency?: string): any; issueInvoice(invoiceId: string, groupId: string): any | null; markInvoicePaid(invoiceId: string, groupId: string): any | null; diff --git a/backend/services/billing/meteringService.ts b/backend/services/billing/meteringService.ts index 60c5acc9..cb0866d4 100644 --- a/backend/services/billing/meteringService.ts +++ b/backend/services/billing/meteringService.ts @@ -12,6 +12,12 @@ const SOFT_THRESHOLD_RATIO = 0.8; const HARD_THRESHOLD_RATIO = 1.0; const MAX_BATCH_SIZE = 500; +/** Monotonic sequence used to keep same-millisecond auto-keys distinct. */ +let _autoSequence = 0; +function nextAutoSequence(): number { + return ++_autoSequence; +} + export interface UsageMetric { userId: string; metricType: 'api' | 'compute' | 'storage'; @@ -126,7 +132,14 @@ export class MeteringService { } private async recordOne(metric: UsageMetric): Promise { - const idempotencyKey = metric.idempotencyKey ?? `auto-${meterKey(metric.userId, metric.metricType)}-${metric.timestamp.getTime()}`; + // An auto-generated key built purely from the event timestamp collides + // for two legitimate events landing in the same millisecond, and the + // second one was then reported as a duplicate and silently never rated — + // under-billing the customer. A monotonic sequence keeps distinct events + // distinct. Callers that need retry-idempotency must pass an explicit key. + const idempotencyKey = + metric.idempotencyKey ?? + `auto-${meterKey(metric.userId, metric.metricType)}-${metric.timestamp.getTime()}-${nextAutoSequence()}`; const cached = this.seenIdempotencyKeys.get(idempotencyKey); if (cached) { @@ -228,12 +241,22 @@ export class MeteringService { this.events.delete(meterKey(userId, metricType)); } - getUsageByMetric(subscriptionId: string): Record { + /** + * Sums recorded usage per metric for a subscription. + * + * An optional window is essential: without one this returns the entire + * lifetime total, which callers then label as a single billing period. + */ +getUsageByMetric(subscriptionId: string, from?: Date, to?: Date): Record { + const fromMs = from instanceof Date ? from.getTime() : undefined; + const toMs = to instanceof Date ? to.getTime() : undefined; const usage: Record = {}; for (const entry of this.usageHistory) { - if (entry.subscriptionId === subscriptionId) { - usage[entry.metric] = (usage[entry.metric] || 0) + entry.value; - } + if (entry.subscriptionId !== subscriptionId) continue; + const entryTime = entry.timestamp.getTime(); + if (fromMs !== undefined && entryTime < fromMs) continue; + if (toMs !== undefined && entryTime > toMs) continue; + usage[entry.metric] = (usage[entry.metric] || 0) + entry.value; } return usage; } @@ -299,13 +322,29 @@ export class MeteringService { } calculateUsageBilling(subscriptionId: string, unitPrice: number, currency: string): UsageBillingIntegration { - const metricUsage = this.getUsageByMetric(subscriptionId); const now = new Date(); const periodStart = new Date(now.getFullYear(), now.getMonth(), 1); - const periodEnd = new Date(now.getFullYear(), now.getMonth() + 1, 0); + // `new Date(y, m + 1, 0)` is midnight *starting* the last day, which + // excluded the final day of the month from the billed usage. + const periodEnd = new Date(now.getFullYear(), now.getMonth() + 1, 1); + + // Bill only usage inside the period being invoiced. Summing the full + // lifetime history while labelling the result "this month" over-billed + // every account with any prior history, compounding each cycle. + const metricUsage = this.getUsageByMetric(subscriptionId, periodStart, periodEnd); let totalMetered = 0; for (const usage of Object.values(metricUsage)) totalMetered += usage; - return { subscriptionId, meteredAmount: totalMetered, unitPrice, totalAmount: totalMetered * unitPrice, currency, period: { start: periodStart, end: periodEnd } }; + + const totalAmount = Number.isFinite(unitPrice) ? totalMetered * unitPrice : 0; + + return { + subscriptionId, + meteredAmount: totalMetered, + unitPrice, + totalAmount, + currency, + period: { start: periodStart, end: periodEnd }, + }; } } diff --git a/src/services/__tests__/groupBilling.test.ts b/src/services/__tests__/groupBilling.test.ts index 430270b6..05e7ca94 100644 --- a/src/services/__tests__/groupBilling.test.ts +++ b/src/services/__tests__/groupBilling.test.ts @@ -60,13 +60,23 @@ describe('GroupBillingService', () => { expect(summary.memberBalances['0xMEMBER1']).toBe(10); }); - it('aggregates charges within period', () => { - const items = service.aggregateCharges(sampleGroup, 30); + it('aggregates charges within the requested period', () => { + const now = Date.now(); + const items = service.aggregateCharges(sampleGroup, now - 30 * 86400000, now); expect(items.length).toBe(2); const ownerItem = items.find((i) => i.memberAddress === '0xOWNER'); expect(ownerItem?.amount).toBe(29.99); }); + it('excludes charges outside the requested period', () => { + const now = Date.now(); + // The window is anchored to the period, not to "the last N days from + // now" — a period that ended long ago must not pick up today's charges. + const items = service.aggregateCharges(sampleGroup, now - 90 * 86400000, now - 60 * 86400000); + const ownerItem = items.find((i) => i.memberAddress === '0xOWNER'); + expect(ownerItem?.amount).toBe(0); + }); + it('generates and processes invoices', () => { const now = Date.now(); const invoice = service.generateInvoice(sampleGroup, now - 86400000, now); diff --git a/src/services/analyticsService.ts b/src/services/analyticsService.ts index 228197a6..b47ca974 100644 --- a/src/services/analyticsService.ts +++ b/src/services/analyticsService.ts @@ -1,4 +1,5 @@ -import { BillingCycle, Subscription } from '../types/subscription'; +import { Subscription } from '../types/subscription'; +import { monthlyEquivalent } from '../utils/billingCycleMath'; export interface RevenuePoint { label: string; @@ -49,18 +50,9 @@ export interface SubscriptionAnalyticsReport { forecast: RevenueForecastPoint[]; } -const MONTHS_PER_YEAR = 12; -const WEEKS_PER_MONTH = 4.345; - export const toMonthlyRevenue = ( subscription: Pick -): number => { - if (subscription.billingCycle === BillingCycle.YEARLY) - return subscription.price / MONTHS_PER_YEAR; - if (subscription.billingCycle === BillingCycle.WEEKLY) - return subscription.price * WEEKS_PER_MONTH; - return subscription.price; -}; +): number => monthlyEquivalent(subscription.price, subscription.billingCycle); const MONTHS_PER_YEAR = 12; diff --git a/src/services/invoiceService.ts b/src/services/invoiceService.ts index ccb79a7b..e1413afd 100644 --- a/src/services/invoiceService.ts +++ b/src/services/invoiceService.ts @@ -12,7 +12,11 @@ import type { } from '../types/invoice'; import { InvoiceLayout, InvoiceStatus } from '../types/invoice'; import { getNextSequence, generateLegalInvoiceNumber } from './sequenceService'; -import { resolveLineTotal, isDiscountLineItem } from '../utils/invoiceTotals'; +import { + calculateInvoiceTotals, + resolveLineTotal, + isDiscountLineItem, +} from '../utils/invoiceTotals'; import { validateInvoiceLineItemMetadata, buildDiscountLineItem } from '../utils/invoice'; const STORAGE_KEYS = { @@ -207,11 +211,12 @@ export async function createInvoice(data: InvoiceFormData): Promise { lineItems = itemiseDiscount(lineItems, data.discountAmount, currency); } - const lineTotal = - lineItems.length > 0 - ? lineItems.reduce((sum, item) => sum + (item.lineTotal ?? 0), 0) - : (data.amount ?? 0); - const total = lineTotal + (data.taxAmount || 0) - (data.discountAmount || 0); + const totals = calculateInvoiceTotals({ + lineItems, + amount: data.amount, + tax: data.taxAmount, + }); + const { subtotal, total } = totals; // Legal numbering: fetch next persistent sequence // We track sequence per year to reset it annually (common legal requirement) @@ -228,7 +233,10 @@ export async function createInvoice(data: InvoiceFormData): Promise { merchantName: '', lineItems, tax: data.taxAmount ?? 0, - subtotal: lineTotal, + // Persist the clamped discount so a later edit can re-apply it instead of + // silently repricing the invoice at full value. + discountAmount: totals.discount, + subtotal, total, totalAmount: total, amount: total, @@ -294,13 +302,17 @@ export async function updateInvoice(id: string, updates: Partial): Prom lineItems = itemiseDiscount(lineItems, updatedInvoice.discountAmount, currency, exchangeRate); } - const subtotal = lineItems.reduce((sum, item) => sum + (item.lineTotal ?? 0), 0); - const total = subtotal + (updatedInvoice.tax ?? 0); + const totals = calculateInvoiceTotals({ + lineItems, + tax: updatedInvoice.tax ?? 0, + }); + updatedInvoice.lineItems = lineItems; - updatedInvoice.subtotal = subtotal; - updatedInvoice.total = total; - updatedInvoice.totalAmount = total; - updatedInvoice.amount = total; + updatedInvoice.subtotal = totals.subtotal; + updatedInvoice.discountAmount = totals.discount; + updatedInvoice.total = totals.total; + updatedInvoice.totalAmount = totals.total; + updatedInvoice.amount = totals.total; } invoices[index] = updatedInvoice; @@ -407,7 +419,10 @@ export async function getInvoiceAnalytics(): Promise { .filter((inv) => inv.status === InvoiceStatus.PAID) .reduce((sum, inv) => sum + inv.total, 0); - const averageInvoiceAmount = totalInvoices > 0 ? totalRevenue / paidInvoices || 0 : 0; + // `paidInvoices` can be 0 while invoices exist (every one still a draft). The + // old `totalRevenue / paidInvoices || 0` then evaluated to Infinity, because + // `Infinity` is truthy, and the analytics screen reported an infinite average. + const averageInvoiceAmount = paidInvoices > 0 ? totalRevenue / paidInvoices : 0; // Revenue by month const revenueByMonth: Record = {}; diff --git a/src/store/slices/analyticsSlice.ts b/src/store/slices/analyticsSlice.ts index 010a1f68..a76538b1 100644 --- a/src/store/slices/analyticsSlice.ts +++ b/src/store/slices/analyticsSlice.ts @@ -14,7 +14,8 @@ import { calculateSubscriptionAnalytics, SubscriptionAnalyticsReport, } from '../../services/analyticsService'; -import { BillingCycle, Subscription } from '../../types/subscription'; +import { Subscription } from '../../types/subscription'; +import { monthlyEquivalent } from '../../utils/billingCycleMath'; import { generateCSV } from '../../utils/importExport'; import { CohortService } from '../../../backend/services/analytics/cohortService'; import { cohortTableToCsv } from '../../../backend/services/analytics/cohortReportExport'; @@ -53,12 +54,7 @@ const toSubscriberRecords = (subscriptions: Subscription[]): SubscriberRecord[] signupAt: new Date(subscription.createdAt).getTime(), churnedAt: subscription.isActive ? undefined : new Date(subscription.updatedAt).getTime(), lastActiveAt: new Date(subscription.updatedAt).getTime(), - mrr: - subscription.billingCycle === BillingCycle.YEARLY - ? subscription.price / 12 - : subscription.billingCycle === BillingCycle.WEEKLY - ? subscription.price * 4.345 - : subscription.price, + mrr: monthlyEquivalent(subscription.price, subscription.billingCycle), })); export interface CreditMetricSnapshot { diff --git a/src/store/subscriptionStore.ts b/src/store/subscriptionStore.ts index 9b2f5e05..cf8544b3 100644 --- a/src/store/subscriptionStore.ts +++ b/src/store/subscriptionStore.ts @@ -30,7 +30,8 @@ import { InvoiceStatus, isOpenInvoice } from '../types/invoice'; import { dummySubscriptions } from '../utils/dummyData'; // eslint-disable-line import { advanceBillingDate, calculateNextBillingDate } from '../utils/billingDate'; import { buildBillingPeriod } from '../utils/invoice'; -import { BILLING_CONVERSIONS, CACHE_CONSTANTS } from '../utils/constants/values'; +import { CACHE_CONSTANTS } from '../utils/constants/values'; +import { monthlyEquivalent, yearlyEquivalent } from '../utils/billingCycleMath'; import { syncRenewalReminders, presentChargeSuccessNotification, @@ -1465,11 +1466,7 @@ export const useSubscriptionStore = create()( preferredCurrency, rates ); - if (sub.billingCycle === 'monthly') return total + priceInPreferred; - if (sub.billingCycle === 'yearly') return total + priceInPreferred / 12; - if (sub.billingCycle === 'weekly') - return total + priceInPreferred * BILLING_CONVERSIONS.WEEKS_PER_MONTH; - return total + priceInPreferred; + return total + monthlyEquivalent(priceInPreferred, sub.billingCycle); }, 0); const totalYearlySpend = activeSubs.reduce((total, sub) => { @@ -1479,12 +1476,7 @@ export const useSubscriptionStore = create()( preferredCurrency, rates ); - if (sub.billingCycle === 'yearly') return total + priceInPreferred; - if (sub.billingCycle === 'monthly') - return total + priceInPreferred * BILLING_CONVERSIONS.MONTHS_PER_YEAR; - if (sub.billingCycle === 'weekly') - return total + priceInPreferred * BILLING_CONVERSIONS.WEEKS_PER_YEAR; - return total + priceInPreferred * BILLING_CONVERSIONS.MONTHS_PER_YEAR; + return total + yearlyEquivalent(priceInPreferred, sub.billingCycle); }, 0); const categoryBreakdown = activeSubs.reduce( diff --git a/src/utils/billingCycleMath.ts b/src/utils/billingCycleMath.ts new file mode 100644 index 00000000..4de5c1f9 --- /dev/null +++ b/src/utils/billingCycleMath.ts @@ -0,0 +1,98 @@ +/** + * billingCycleMath.ts + * + * One place that turns a subscription's price and billing cycle into a + * comparable per-month figure. + * + * This logic was previously duplicated in four places that disagreed with one + * another, which produced inconsistent spend and MRR figures depending on + * which screen the user looked at: + * + * - `subscriptionStore` compared raw strings and omitted `annual` entirely, + * so an annual plan was billed as a *monthly* one and inflated MRR 12×. + * - `analyticsService` and `analyticsSlice` handled `yearly` but not + * `annual`. + * - Those two used a hard-coded `4.345` weeks/month while + * `BILLING_CONVERSIONS.WEEKS_PER_MONTH` is `52 / 12`. + */ + +import { BillingCycle } from '../types/subscription'; +import { BILLING_CONVERSIONS } from './constants/values'; + +/** Canonical weeks-per-month used for every weekly normalisation. */ +export const WEEKS_PER_MONTH = BILLING_CONVERSIONS.WEEKS_PER_MONTH; + +/** + * Coerces loose input (`'monthly'`, `'Monthly'`, a missing value) into a + * `BillingCycle`, defaulting to MONTHLY. + */ +export function normalizeBillingCycle(value: unknown): BillingCycle { + if (typeof value !== 'string') return BillingCycle.MONTHLY; + + const candidate = value.trim().toLowerCase(); + if (candidate === '') return BillingCycle.MONTHLY; + + const known = Object.values(BillingCycle) as string[]; + if (known.includes(candidate)) { + return candidate as BillingCycle; + } + + // Tolerate common synonyms used by imports and older records. + switch (candidate) { + case 'annually': + case 'yearly': + case 'annual': + case 'year': + return BillingCycle.YEARLY; + case 'week': + case 'weekly': + return BillingCycle.WEEKLY; + case 'month': + case 'monthly': + return BillingCycle.MONTHLY; + default: + return BillingCycle.MONTHLY; + } +} + +/** + * Monthly equivalent of `price` charged on `cycle`. + * + * `yearly` and `annual` are both billed once per year, so both normalise to + * one twelfth of the quoted price. `weekly` uses the shared weeks-per-month + * constant so it cannot drift away from the rest of the codebase. + */ +export function monthlyEquivalent(price: number, cycle: unknown): number { + if (!Number.isFinite(price)) return 0; + + switch (normalizeBillingCycle(cycle)) { + case BillingCycle.YEARLY: + case BillingCycle.ANNUAL: + return price / BILLING_CONVERSIONS.MONTHS_PER_YEAR; + case BillingCycle.WEEKLY: + return price * WEEKS_PER_MONTH; + default: + return price; + } +} + +/** Yearly equivalent of `price` charged on `cycle`. */ +export function yearlyEquivalent(price: number, cycle: unknown): number { + if (!Number.isFinite(price)) return 0; + + switch (normalizeBillingCycle(cycle)) { + case BillingCycle.YEARLY: + case BillingCycle.ANNUAL: + return price; + case BillingCycle.WEEKLY: + return price * BILLING_CONVERSIONS.WEEKS_PER_YEAR; + default: + return price * BILLING_CONVERSIONS.MONTHS_PER_YEAR; + } +} + +/** True for cycles billed once per year. */ +export function isAnnualCycle(cycle: unknown): boolean { + const normalized = normalizeBillingCycle(cycle); + return normalized === BillingCycle.YEARLY || normalized === BillingCycle.ANNUAL; +} diff --git a/src/utils/invoiceTotals.ts b/src/utils/invoiceTotals.ts index 6b22a92b..6b56b28c 100644 --- a/src/utils/invoiceTotals.ts +++ b/src/utils/invoiceTotals.ts @@ -1,14 +1,27 @@ /** * invoiceTotals.ts * - * Line-item helpers shared by the invoice renderers. + * Single source of truth for turning line items into an invoice total. * - * The full totals module — half-up rounding, discount clamping and the single - * arithmetic path used by `createInvoice` / `updateInvoice` — is introduced - * with the billing-correctness work. Only the two helpers the rendering layer - * depends on are defined here for now. + * `createInvoice` and `updateInvoice` previously each rolled their own + * arithmetic, and they did not agree: `createInvoice` subtracted the + * discount, `updateInvoice` did not. Editing one line on a discounted + * invoice therefore silently increased the amount payable, and because the + * discount was never persisted on the record there was nothing left to + * subtract on the second pass. */ +/** Rounds to `decimals` places using half-up, avoiding binary float drift. */ +export function roundMoney(amount: number, decimals = 2): number { + if (!Number.isFinite(amount)) return 0; + + const factor = 10 ** decimals; + // The epsilon nudge keeps values such as 1.005 from rounding down because + // their binary representation is fractionally below the decimal value. + // The trailing `+ 0` normalises -0 to 0 so callers never see a negative zero. + return Math.round((amount + Number.EPSILON * Math.sign(amount)) * factor) / factor + 0; +} + export interface LineTotalSource { kind?: string; lineTotal?: number; @@ -27,7 +40,7 @@ export function isDiscountLineItem(item: { kind?: string }): boolean { /** * Resolves a single line's total, tolerating both the itemised (`lineTotal`) - * and the flat (`amount`) shapes. + * and flat (`amount`) shapes. */ export function resolveLineTotal(item: LineTotalSource): number { const derived = item.quantity !== undefined ? (item.unitPrice ?? 0) * item.quantity : undefined; @@ -41,3 +54,65 @@ export function resolveLineTotal(item: LineTotalSource): number { return 0; } + +export interface InvoiceTotalsInput { + lineItems?: LineTotalSource[]; + /** Used when the invoice has no line items. */ + amount?: number; + tax?: number; + discount?: number; + currencyDecimals?: number; +} + +export interface InvoiceTotals { + subtotal: number; + tax: number; + discount: number; + total: number; +} + +/** + * Computes invoice totals with every component rounded to minor units and a + * non-negative payable amount. + * + * Discount line items (those with `kind: 'discount'`) are kept out of the + * subtotal and summed into `discount` instead, so an itemised discount is + * subtracted exactly once. A legacy scalar `discount` is added to whatever the + * discount lines contribute. + * + * The clamp matters: a discount larger than the subtotal otherwise produces a + * negative tax base and a negative total, which downstream code then treats as + * a credit owed to the customer rather than an over-discounted invoice. + */ +export function calculateInvoiceTotals(input: InvoiceTotalsInput): InvoiceTotals { + const decimals = input.currencyDecimals ?? 2; + + const lineItems = input.lineItems ?? []; + const chargeItems = lineItems.filter((item) => !isDiscountLineItem(item)); + const discountItems = lineItems.filter((item) => isDiscountLineItem(item)); + + // A discount-only invoice has nothing to charge; fall back to the flat + // amount if the caller supplied one rather than reporting a zero subtotal. + const rawSubtotal = + chargeItems.length > 0 + ? chargeItems.reduce((sum, item) => sum + resolveLineTotal(item), 0) + : (input.amount ?? 0); + + const subtotal = roundMoney(rawSubtotal, decimals); + const tax = roundMoney(input.tax ?? 0, decimals); + + // Discount lines carry a positive magnitude (or a negative one); either way + // they reduce the amount payable. + const lineDiscount = roundMoney( + discountItems.reduce((sum, item) => sum + Math.abs(resolveLineTotal(item)), 0), + decimals + ); + + // A discount may not exceed what is being discounted. + const requestedDiscount = roundMoney((input.discount ?? 0) + lineDiscount, decimals); + const discount = roundMoney(Math.min(Math.max(requestedDiscount, 0), subtotal), decimals); + + const total = roundMoney(Math.max(subtotal + tax - discount, 0), decimals); + + return { subtotal, tax, discount, total }; +}