-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Polish handling of BillingMode #12130
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
/werft run 👍 started the job as gitpod-build-gpl-ubp.2 |
started the job as gitpod-build-gpl-ubp.3 because the annotations in the pull request description changed |
Thanks! Code looks good, now moving on to (manual) tests. |
Question:
|
/hold Have to remove the usage timestamp fix commit as it clashes with a parallel fix. |
We discussed @jankeromnes comment above: It's very well possible to not have UBP on your user. In this case, because the Team had not paid plan, yet. |
/unhold Commit removed. @jankeromnes PR should be good to review - sorry for the UI glitch 🙇 |
Many thanks for finishing up this PR @geropl! I went through the entire "How to test" process, but found a few possible small problems.
Right after signup, and a second time later after randomly refreshing once (but it's hard to reproduce), I got this strange error: I believe this is when a backend service returns an error, but it's missing an HTTP error code (it's supposed to say something like But maybe it's just a preview problem and not related to this PR?
✅ Additionally, this wasn't part of the test flow, but I notice that the Team Billing UI has some unfortunate flickering: Instead of briefly showing the Chargebee UI before the Usage-Based UI shows up, I think this initial state should be a loading indicator. Can be done in a follow-up though.
❌ Actually, there is this: Is this not expected?
✅ (although I think this should change in the future when we implement usage-based billing for individual users)
✅
❌ It didn't show the "set spending limit" UI, but started a workspace successfully instead:
✅ (selector is still visible)
❔ basically worked, although I needed to hard-refresh once in order to see the "Billing" menu entry (instead of the "Plans" and "Team Plans" entries which were still there for some reason). After that single refresh, everything is there, although the "Billing" menu entry now takes a long time before it shows up.
✅ (same as above -- menu is eventually correct, but it takes a long time before the "Billing" menu entry shows up) |
Thank you for your thorough review, @jankeromnes ! 🚀
The value of the
Would you have time after 10:30 to help me fix these? 🙏 Feels like it could be simple, but I'm too unfamiliar with the Dashboard to be confident.
Are there any logs on the |
I tried with a [email protected] user -- same problem:
Would be more than happy to! 👍 🎨
I didn't check for server errors, but I'm pretty sure the gitpod/components/proxy/conf/Caddyfile Lines 277 to 279 in aa2c51c
|
@jankeromnes and I synced, and created this commit with resulting fixes. 👍 |
Thanks @geropl! Unfortunately, while doing one last check, I found another problem 🙈 it looks like the Chargebee team billing UI is now gone entirely: Notice that I have one team with UB flag enabled, and another team with UB flag disabled. EDIT: However, when I delete the team with with UB enabled, it fixes the Charbebee UI of the team with UB disabled. |
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.
Alright! I think we're there now. 🎉 Thanks for iterating until it works!
Approving to unblock merging, but holding because there is still one problem that I raised in this comment:
❌ It didn't show the "set spending limit" UI, but started a workspace successfully instead:
The testing plan says that, when you create a "Gitpod (Something)" team, and try to start a workspace, you get blocked by a "Spending Limit" modal.
This is not the case.
Instead, it continues to start workspaces with no issues, and I have to upgrade a team to Usage-Based, and then set a Spending Limit to very low (e.g. 0), in order to actually see the "Spending Limit" modal on workspace start.
However, this is likely a bug in the test scenario, and not a bug in the code, right? 😇
/hold
I tested this before the rebase. As it's not critical, and more important that we don't show an arkward error message as before, I'd say let's merge this one (as it's big enough), and I will re-test and take care in a follow up. Mentioned earlier, but here again: thank you for the through review @jankeromnes ! 🙏 |
/unhold |
Description
This PR includes a couple of alignments around BillingMode. It's reviewable by commit. As all changes share the same context I think it does not make sense to further split this up. 🧘
BillingMode.canSetWorkspaceClass
returnstrue
for your BillingModeMakeremovedmoment
time offset work on Usage List ViewfindCustomerByUserId
and replace withfindCustomerByTeamId
Related Issue(s)
Fixes #12098
Fixes #12090
Fixes #12149
How to test
Gitpod-heroes
or soRelease Notes
Documentation
Werft options: