-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Introduce BillingMode (1/4) #11812
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
Introduce BillingMode (1/4) #11812
Conversation
/werft run 👍 started the job as gitpod-build-gpl-billing-mode.1 |
/werft run 👍 started the job as gitpod-build-gpl-billing-mode.2 🤞 test timeouts... |
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.
Many thanks @geropl for powering through the first implementation and then the cleaner split! 💯
The code looks good to me (modulo some minor comments, please take a look and either address or dismiss), and the tests really build a ton of confidence here. Another thank you for always diligently adding lots of tests alongside your PRs 💯 I think these play a major role in Gitpod not falling apart as complexity grows. 💪
Would have been nice to be able to test this manually as well (e.g. with preview & payment & chargebee enabled, and double-checking the billing mode in various scenarii) but I guess we can do that in follow-up PRs too, so not a blocker.
/hold
}, | ||
// user: transition chargebee -> UBB | ||
{ | ||
name: "user: chargbee paid personal (cancelled) + team seat", |
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.
The endDate
in the future threw me off a bit. Maybe you could clarify this comment a bit?
chargebee paid personal (cancelled but still active)
(Had forgotten about the two dimensions "cancelled" vs "active")
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.
- cancellationDate is the date when we click "cancel"
- endDate is when the subscription is no longer effective (currently: end of billing period)
Instead of setting endDate to "end of billing period", I think should prorate and set it to "cancellationDate + 24h", to give room for a safe transition period from Chargebee to Stripe (as discussed here internally).
☁️ I'm just realizing that the current implementation here does not support that use-case very well, though. 😢
If you are still on a team seat, and deliberately want to upgrade to "personal usage-based", then you also want to ripe the benefits of that. @jankeromnes Would be awesome if we can find 10mins of sync time on this and adjust.
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.
Addressed here.
One follow-up remark: As a sanity check, I tried to make your tests fail on purpose, by making a random change in the expectations: 89cde61 However, it seems that my build went through without complaining. 😬 How can we be certain that the tests are actually executed? |
started the job as gitpod-build-gpl-billing-mode.3 because the annotations in the pull request description changed |
/werft run 👍 started the job as gitpod-build-gpl-billing-mode.4 |
started the job as gitpod-build-gpl-billing-mode.5 because the annotations in the pull request description changed |
@AlexTugarev After discussing with @jankeromnes I'm actively working on further simplyfing this. sorry for not communicating earlier on the PR 😕 |
9b1097f
to
ccc20fa
Compare
@jankeromnes I came up with a new approach which addresses all of your comments here (complexity), here (cleared modifiers: cmp. The core change is that UBB now is "gready": If you have at least one UBB team Seat, or a personal UBB subscription, that "overrides" your Chargebee one. This way we can ensure you receive the benefits of UBB (workspace classes) immediately. |
mode: "none", | ||
}; | ||
|
||
export function showChargebeeBilling(billingMode: BillingMode): 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.
These functions are not used nor tested, yet, but helped me greatly when thinking about how we want to used BillingMode later on. Left them in for reference and to ensure our usage is in line with the implementation/derivation logic.
|
||
// 1. UBB enabled? | ||
if (!isUsageBasedBillingEnabled) { | ||
// No Chargebee subscription, and UBB is not enabled: chargebee (free) |
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.
Nit: This comment is inaccurate (we don't know that here are no chargebee subscriptions because we didn't check yet)
// 2. Any Chargbee TeamSubscription2 (old Team Subscriptions are not relevant here, as they are not associated with a team) | ||
const teamSubscription = await this.teamSubscription2Db.findForTeam(team.id, now); | ||
if (teamSubscription && TeamSubscription2.isActive(teamSubscription, now)) { | ||
if (isUsageBasedBillingEnabled && TeamSubscription2.isCancelled(teamSubscription, 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.
Nit: I think you don't need to check isUsageBasedBillingEnabled
here (we know it's true
, because otherwise we would have already bailed out in the UBB check above)
Oops:
|
@jankeromnes Sorry, it's: |
Thanks! Cannot double-approve: Please fix the two conflicts and feel free to merge! 🛹 😁 |
Thank you, @jankeromnes! 🚀 🙏 |
ccc20fa
to
a1435e3
Compare
/unhold Did rebase, and ran tests again 🙃 |
Description
This PR introduces
BiIllingMode
, which at it's core is an enum which tells you how a given user (and their usage) is billed. We use this abstraction to guide the (gradual) rollout of Usage-based Pricing, and later to phase out Chargebee.This changeset does not change any behavior, but rather adds the interface, methods to derive the current state for a given
User
/Team
and tests for said implementation. It's 1 of 4 in a stack.Related Issue(s)
Context: #11402
How to test
Release Notes
Documentation
Werft options: