-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat(billing): refacto billing #14243
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.
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
Conversation
… prices for metered billing
…ernal component paths
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.
Greptile Summary
This PR introduces comprehensive automation for Stripe price management in Twenty's metered billing system, along with significant UI improvements for the billing interface. The core addition is a Node.js script (update-stripe-prices.js
) that automates the creation and archival of Stripe prices for metered billing based on workflow node runs. The script creates tiered pricing structures with 12 different credit tiers (ranging from 5,000 to 7.5M credits) for both monthly and yearly subscriptions, implementing proper idempotency using SHA-256 hashes to prevent duplicate price creation.
The frontend changes focus on improving the billing page organization and user experience. Components are being restructured with internal implementation details moved to a dedicated internal/
directory, following common patterns for separating public APIs from private components. The MeteredPriceSelector
component receives a major UX overhaul, transforming from a simple dropdown to a safer two-step confirmation process where users must first select a plan, then explicitly click an Upgrade/Downgrade button, and finally confirm their choice in a modal.
The billing system is also being simplified by removing support for daily and weekly subscription intervals, keeping only monthly and yearly periods which align with the actual supported intervals in the codebase. The SettingsBillingCreditsSection
component is updated to enable previously commented-out metered billing functionality, including the ability to dynamically select different credit plans with proper error handling and confirmation flows.
These changes work together to create a more robust and user-friendly metered billing system, with automated Stripe price management on the backend and an improved confirmation-based user interface on the frontend.
Confidence score: 3/5
- This PR requires careful review due to potential breaking changes and complex billing logic that could affect revenue
- Score reflects concerns about top-level await usage, enum value removal without migration checks, and removal of internationalization support
- Pay close attention to the Stripe script, enum changes, and MeteredPriceSelector component modifications
8 files reviewed, 3 comments
@@ -0,0 +1,175 @@ | |||
// Usage: STRIPE_API_KEY=sk_live_xxx PRODUCT_ID=prod_xxx node create-and-archive-prices.js |
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.
syntax: Comment mentions wrong filename 'create-and-archive-prices.js' but actual file is 'update-stripe-prices.js'
// Usage: STRIPE_API_KEY=sk_live_xxx PRODUCT_ID=prod_xxx node create-and-archive-prices.js | |
// Usage: STRIPE_API_KEY=sk_live_xxx PRODUCT_ID=prod_xxx node update-stripe-prices.js |
throw new Error('Meter not found'); | ||
} | ||
|
||
const formatCredits = (credits) => credits.toLocaleString('de-DE'); // exemple: 7.500.000 |
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.
syntax: Typo in comment: 'exemple' should be 'example'
const formatCredits = (credits) => credits.toLocaleString('de-DE'); // exemple: 7.500.000 | |
const formatCredits = (credits) => credits.toLocaleString('de-DE'); // example: 7.500.000 |
|
||
const makeNickname = (r) => `${formatCredits(r.credits)} Credits`; | ||
|
||
const makeIdemKey = (r) => |
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.
style: Function name 'makeIdemKey' uses abbreviation. Consider 'makeIdempotencyKey' for clarity
Context Used: Context - Avoid using abbreviations in the codebase; prefer full descriptive names, e.g., use 'record' instead of 'r'. (link)
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:56852 This environment will automatically shut down when the PR is closed or after 5 hours. |
…mproved typings and dropped unused intervals
…mproved typings and dropped unused intervals
…($0.00001 per credit)
…($0.00001 per credit)
…hook handling, and refine subscription phase logic
…page # Conflicts: # packages/twenty-front/src/generated-metadata/graphql.ts # packages/twenty-front/src/modules/billing/components/SettingsBillingSubscriptionInfo.tsx # packages/twenty-front/src/modules/billing/components/internal/MeteredPriceSelector.tsx
…line pricing structures, and enhance subscription schedule handling
…BillingSubscriptionInfo` component
📊 API Changes ReportGraphQL Schema ChangesGraphQL Schema Changes[log] [log] ✖ Field baseProduct was removed from object type BillingPlanOutput
GraphQL Metadata Schema ChangesGraphQL Metadata Schema Changes[log] [log] ✖ Field baseProduct was removed from object type BillingPlanOutput
|
@AMoreaux your tests are red! |
[selectedPriceId, meteredBillingPrices], | ||
); | ||
|
||
const isChanged = Boolean( |
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.
Boolean() not standard
selectedPriceId !== currentMeteredBillingPrice?.stripePriceId, | ||
); | ||
|
||
const isUpgrade = useMemo(() => { |
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.
let's not add useMemos, they are likely not useful
setSelectedPriceId(priceId); | ||
}; | ||
|
||
const confirmModalId = 'metered-price-change-confirmation-modal'; |
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 constant and should be UPPER_CASE
isTrialPeriod?: boolean; | ||
}; | ||
|
||
export const PlanTag = ({ plan, isTrialPeriod = false }: PlanTagProps) => { |
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 component is returning two tags and called PlanTag, this is a bit confusing
`; | ||
|
||
const StyledIconLabelContainer = styled.div` | ||
align-items: center; | ||
gap: ${({ theme }) => theme.spacing(1)}; | ||
color: ${({ theme }) => theme.font.color.tertiary}; | ||
display: flex; | ||
width: 120px; | ||
min-width: 0; /* allow label ellipsis within grid column */ |
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'm surprised here, we already have components to handle Ellipsis and we should re-use them
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.
TextWithOverflowingToolTip for instance
import { isDefined } from 'twenty-shared/utils'; | ||
import { useRecoilValue } from 'recoil'; | ||
|
||
export const useBillingPlan = () => { |
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 hook is problematic to me:
- it's an umbrella hook doing many things, this means that using it will trigger a re-render nightmare!
- all these getters are not idiomatic to me: the issue is that these are functions and that we use the function return in react which is new for react engine at each re-renders and therefore can trigger a re-render itself
I would recommend creating hooks (maybe not one for everything but more and to return the things directly
For instance:
usePlans()
useProducts()
useProduct(planKey)
etc...
…d options and abbreviation support
…ow`, remove redundant utils, and add tests for billing helpers
…ependencies, and update related components
…d `min-width` style in `SubscriptionInfoRowContainer`
…n `useBillingWording`, and clean up unused variables
…ervice` and clean up unused variable in subscription service test
…pe-prices` scripts
…ry, and add debug logging in billing settings
…ge handling, and improve credits formatting
…dLabel` to use options object for `decimals`
…d refactor `useBillingWording` for improved date handling
Big refacto/cleanup that addresses a performance issue and many things that were dirty as we added and added layers over months. It's not perfect yet but I think it goes in the right direction
… detection/ folder - Moved all detect* functions to utils/detection/ subfolder - Updated all import paths throughout the codebase - Fixed TypeScript errors in test files by using proper Jest mock typing - Updated formatDateISOStringToDateTimeSimplified test to use TimeFormat enum values - Improved code organization and maintainability
…ata values, and update skipped test syntax
…lean up trial period logic, and update billing price entity relations with nullable modifications
… prevent potential runtime errors
…tripePriceId` for metered prices
… prices for metered billing