Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(billing): add missing subscription fields and audit logging to upgrade flow #2179

Merged
merged 3 commits into from
Oct 4, 2024
Merged

fix(billing): add missing subscription fields and audit logging to upgrade flow #2179

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aafe4d2
fix(billing): add missing subscription fields and audit logging to up…
mcstepp Oct 4, 2024
ebfa3ea
Merge branch 'main' into ENG-1359-missing-subscriptions
mcstepp Oct 4, 2024
0a75c1e
fix context properties
mcstepp Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 62 additions & 13 deletions apps/dashboard/app/(app)/settings/billing/stripe/success/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
import { EmptyPlaceholder } from "@/components/dashboard/empty-placeholder";
import { Code } from "@/components/ui/code";
import { insertAuditLogs } from "@/lib/audit";
import { getTenantId } from "@/lib/auth";
import { db, eq, schema } from "@/lib/db";
import { stripeEnv } from "@/lib/env";
import { PostHogClient } from "@/lib/posthog";
import { ingestAuditLogsTinybird } from "@/lib/tinybird";
import { currentUser } from "@clerk/nextjs";
import { defaultProSubscriptions } from "@unkey/billing";
import { headers } from "next/headers";
import { redirect } from "next/navigation";
import Stripe from "stripe";

Expand All @@ -18,10 +22,10 @@ type Props = {
export default async function StripeSuccess(props: Props) {
const { session_id, new_plan } = props.searchParams;
const tenantId = getTenantId();
if (!tenantId) {
const user = await currentUser();
if (!tenantId || !user) {
return redirect("/auth/sign-in");
}
const _user = await currentUser();

const ws = await db.query.workspaces.findFirst({
where: (table, { and, eq, isNull }) =>
Expand Down Expand Up @@ -72,19 +76,64 @@ export default async function StripeSuccess(props: Props) {
);
}

const isChangingPlan = new_plan && new_plan !== ws.plan;
const isUpgradingPlan = new_plan && new_plan !== ws.plan && new_plan === "pro";
const h = headers();

await db
.update(schema.workspaces)
.set({
stripeCustomerId: customer.id,
stripeSubscriptionId: session.subscription as string,
trialEnds: null,
...(isChangingPlan ? { plan: new_plan } : {}),
})
.where(eq(schema.workspaces.id, ws.id));
await db.transaction(async (tx) => {
await tx
.update(schema.workspaces)
.set({
stripeCustomerId: customer.id,
stripeSubscriptionId: session.subscription as string,
trialEnds: null,
...(isUpgradingPlan
? {
plan: new_plan,
planChanged: new Date(),
subscriptions: defaultProSubscriptions(),
planDowngradeRequest: null,
}
: {}),
})
.where(eq(schema.workspaces.id, ws.id));

if (isChangingPlan) {
if (isUpgradingPlan) {
await insertAuditLogs(tx, {
workspaceId: ws.id,
actor: { type: "user", id: user.id },
event: "workspace.update",
description: "Changed plan to 'pro'",
resources: [
{
type: "workspace",
id: ws.id,
},
],
context: {
location: h.get("x-forwarded-for") ?? process.env.VERCEL_REGION ?? "unknown",
userAgent: h.get("user-agent") ?? undefined,
},
});
await ingestAuditLogsTinybird({
workspaceId: ws.id,
actor: { type: "user", id: user.id },
event: "workspace.update",
description: "Changed plan to 'pro'",
resources: [
{
type: "workspace",
id: ws.id,
},
],
context: {
location: h.get("x-forwarded-for") ?? process.env.VERCEL_REGION ?? "unknown",
userAgent: h.get("user-agent") ?? undefined,
},
});
}
});
Comment on lines +100 to +134
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve audit logging implementation.

The addition of audit logging enhances the tracking of the upgrade process, which aligns with the PR objectives. However, there are a few points to consider:

  1. There's duplication in the data passed to both insertAuditLogs and ingestAuditLogsTinybird. Consider extracting this data into a shared variable to adhere to the DRY principle.

  2. The ingestAuditLogsTinybird function is called within the database transaction. This might not be ideal for performance, as it could potentially slow down the transaction.

Consider refactoring the audit logging implementation as follows:

const auditLogData = {
  workspaceId: ws.id,
  actor: { type: "user", id: user.id },
  event: "workspace.update",
  description: "Changed plan to 'pro'",
  resources: [{ type: "workspace", id: ws.id }],
  context: {
    location: h.get("x-forwarded-for") ?? process.env.VERCEL_REGION ?? "unknown",
    userAgent: h.get("user-agent") ?? undefined,
  },
};

await db.transaction(async (tx) => {
  // ... existing transaction code ...

  if (isUpgradingPlan) {
    await insertAuditLogs(tx, auditLogData);
  }
});

if (isUpgradingPlan) {
  await ingestAuditLogsTinybird(auditLogData);
}

This refactoring reduces duplication and moves the ingestAuditLogsTinybird call outside the transaction for better performance.


if (isUpgradingPlan) {
chronark marked this conversation as resolved.
Show resolved Hide resolved
PostHogClient.capture({
distinctId: tenantId,
event: "plan_changed",
Expand Down
Loading