-
Notifications
You must be signed in to change notification settings - Fork 531
7010/Support more flexible subscription intervals #7069
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
|
@Yopi is attempting to deploy a commit to the polar-sh Team on Vercel. A member of the Team first needs to authorize it. |
709f890 to
851e515
Compare
| {}, | ||
| ), | ||
| }) | ||
| } as schemas['ProductCreate']) |
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.
Not super impressed with having to do this, but Typescript couldn't figure the type out without it when I added the recurring_interval_count number / null.
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.
Yeah, it usually gets a bit tricky with those forms. We can live with it IMO :)
| required: 'This field is required when billing cycle is set', | ||
| min: { value: 1, message: 'Interval count must be at least 1' }, | ||
| max: { | ||
| value: 999, |
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 probably doesn't really make sense, but I'm also not sure what would make sense here out of a product perspective.
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.
Had the same debate with the trial configuration, and ended up also limiting to 1000. A bit arbitrary, but at least make sure we don't blow up datetimes.
| Set how often customers are billed (e.g., "every 2 | ||
| months") |
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 wanted to have an example here since it might not be intuitive, but we might also want to update the example based on the recurring_interval for it to be a bit more dynamic.
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.
Yeah, I guess we'll have quite a lot of back and forth on this UX. Will do fine for now.
| switch (interval) { | ||
| case 'day': | ||
| return ' / dy' | ||
| return ` /${prefix} dy` |
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.
It looks fine to me, but the text becomes a bit longer and I am guessing the intention was for it to be short given that it's being shortened (wk, etc).
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.
Looks great 👍
| recurring_interval_count: int | None = Field( | ||
| description=( | ||
| "Number of interval units of the subscription." | ||
| "If this is set to 1 the charge will happen every interval (e.g. every month)," |
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.
Same here, due to the possibility of it not being intuitive I wanted to give a clearer example.
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.
Looks great 👍
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.
Thank you @Yopi, very great and polished work 🙏
I suggested a few nitpicks; if you have time to address them, would be great 🙂
➕ if you can pass linters: uv run task lint && uv run task lint_types
server/polar/models/product.py
Outdated
| index=True, | ||
| default=None, | ||
| ) | ||
| recurring_interval_count: Mapped[int | None] = mapped_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 think this should be made non-nullable, especially since we take care to migrate existing data.
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.
That was my initial thought as well, but when recurring_interval is null I made this null as well. Otherwise we would need to give it a value. Perhaps setting it to 0 would make sense though?
server/polar/models/subscription.py
Outdated
| recurring_interval: Mapped[SubscriptionRecurringInterval] = mapped_column( | ||
| StringEnum(SubscriptionRecurringInterval), nullable=False, index=True | ||
| ) | ||
| recurring_interval_count: Mapped[int | None] = mapped_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.
See comment above about non-nullable
| recurring_interval_count: int | None = Field( | ||
| description=( | ||
| "Number of interval units of the subscription." | ||
| "If this is set to 1 the charge will happen every interval (e.g. every month)," |
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.
Looks great 👍
| if product.is_legacy_recurring_price: | ||
| prices = [checkout.product_price] | ||
| recurring_interval = prices[0].recurring_interval | ||
| recurring_interval_count = 1 |
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.
Good catch 😄
| {}, | ||
| ), | ||
| }) | ||
| } as schemas['ProductCreate']) |
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.
Yeah, it usually gets a bit tricky with those forms. We can live with it IMO :)
| required: 'This field is required when billing cycle is set', | ||
| min: { value: 1, message: 'Interval count must be at least 1' }, | ||
| max: { | ||
| value: 999, |
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.
Had the same debate with the trial configuration, and ended up also limiting to 1000. A bit arbitrary, but at least make sure we don't blow up datetimes.
| ) | ||
| recurring_interval_count: int | None = Field( | ||
| default=None, | ||
| ge=1, |
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.
We should also probably add the upper limit validation here.
| Set how often customers are billed (e.g., "every 2 | ||
| months") |
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.
Yeah, I guess we'll have quite a lot of back and forth on this UX. Will do fine for now.
| switch (interval) { | ||
| case 'day': | ||
| return ' / dy' | ||
| return ` /${prefix} dy` |
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.
Looks great 👍
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
851e515 to
12a9543
Compare
Sorry, I thought I had ran through the linter, but seems not. All should be good now. I pushed the changes you requested (made the |
This adds a recurring_interval_count column for the product and subscription models, allowing for a more granular control of the cycle for the subscription. We default the cycle to 1, which will conform the current data to every x (every month, every year, etc). We allow this to be nullable similar to the recurring_interval field.
…e it in the cycle calculation
12a9543 to
ce4a919
Compare
📋 Summary
Related Issue: Fixes #7010
This PR adds a recurring_interval_count in addition to the recurring_interval on the product and subscription models. Additionally it makes the cycle calculations for billing take the recurring_interval_count in consideration.
🎯 What
Backend:
Frontend
1 monthor1 year[ ]🤔 Why
To allow for more flexibility in billing intervals.
🔧 How
🧪 Testing
uv run task testfor backend,pnpm testfor frontend)uv run task lint && uv run task lint_typesfor backend)Test Instructions
🖼️ Screenshots/Recordings
📝 Additional Notes
✅ Pre-submission Checklist