-
Notifications
You must be signed in to change notification settings - Fork 5
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
Pricing update #660
Pricing update #660
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Im not sure @commoddity if in |
FYI -- i purposely left the old plan types in place so as to try and maintain as much backwards compatibility as possible (especially for Enterprise). So there will be many references to the old plan types |
return ( | ||
<List center icon={<LuCheck size="18px" />} size="sm" spacing="xl"> | ||
<List.Item>Unlimited relays per month</List.Item> | ||
<List.Item>100,000 relays free per month</List.Item> |
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.
Just wondering if we want to change how the stripe worker handles this? Currently it doesn't really give 100,000 free relays, it just doesn't charge for the first million until the user hits 100,001 free relays.
eg. <= 100,000 relays = $0
100,001 - 1,000,000 relays = $2
1,000,001 relays = $4, etc.
Is that correct? If so maybe we should reword this. If not I'll need to make a change to the Stripe worker.
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.
No, the logic in the stripe worker is correct. I'll think about how to reword this.
app/utils/planUtils.ts
Outdated
@@ -32,6 +42,12 @@ export const getPlanName = (planType: PayPlanType) => { | |||
case PayPlanType.Enterprise: { | |||
return "Enterprise" | |||
} | |||
case PayPlanType.PlanUnlimited: { | |||
return "Enterprise" |
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.
Should this string be updated or is this intentional?
app/utils/planUtils.ts
Outdated
return "Enterprise" | ||
} | ||
case PayPlanType.PlanFree: { | ||
return "Enterprise" |
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.
Ditto this.
app/utils/updatePlan.server.ts
Outdated
@@ -30,7 +30,7 @@ export const updatePlan = async ({ | |||
} | |||
|
|||
if (limit) { | |||
options.input.enterpriseLimit = limit | |||
options.input.monthlyUserLimit = limit |
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 if statement should be removed for now since the monthly user limit is not yet fully supported. We will have to add some UI components to support allowing users to change this amount (in addition to the backend changes in Rate Limiter). For now I think we should remove the whole if (limit)
block.
Yeah I would expect it to fail if the price ID is wrong. TBH not sure there's a clear way to test this locally but let's see how it works in staging. |
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.
LGTM but let's wait for @Olshansk to review as well.
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.
Just a few questions / comments. Generally, I'm less worried about frontend code because it's not as critical.
@fredteumer All comments are optional but PTAL and merge tending /responding to them.
@commoddity Got one question in the for you.
Also, @fredteumer, for frontend changes, in the future, please add a screencap or screenshots in the PR description. Makes it easier to understand the changes as a reviewer.
export function isFree(planType: PayPlanType) { | ||
return planType === PayPlanType.PlanFree | ||
} | ||
|
||
export function isLegacyPlan(planType: PayPlanType) { | ||
return !( | ||
planType === PayPlanType.PayAsYouGoV0 || |
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.
Are we keeping these here intentionally?
If not, let's remove.
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.
yes to preserve any edge cases for backwards compatibility
@@ -13,11 +13,21 @@ export function isEnterprisePlan(planType: PayPlanType) { | |||
return planType === PayPlanType.Enterprise | |||
} | |||
|
|||
export function isUnlimitedPlan(planType: PayPlanType) { | |||
return planType === PayPlanType.PlanUnlimited |
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.
@commoddity I haven't followed on all the edge cases, but do we want/need to do an || planType === PayPlanType.Enterprise
here?
label: "Plan Type", | ||
value: getPlanName(account.planType), | ||
}, | ||
// { |
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.
Don't we want 100,000 here?
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 purely cosmetic -- im going to leave this out for now
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.
Just a few questions / comments. Generally, I'm less worried about frontend code because it's not as critical.
@fredteumer All comments are optional but PTAL and merge tending /responding to them.
@commoddity Got one question in the for you.
Also, @fredteumer, for frontend changes, in the future, please add a screencap or screenshots in the PR description. Makes it easier to understand the changes as a reviewer.
Co-authored-by: Daniel Olshansky <[email protected]>
Overview
Changes all the things wrt to Free / PAYG / Enterprise to Free / Unlimited