[Usage-based] Adjust Stripe usage limits to spec #13746

jankeromnes · 2022-10-11T08:46:53Z

Description

Adjust Stripe usage limits to spec

Related Issue(s)

Part of #13389
~~Depends on #13798~~

How to test

Create a team called "Gitpod [Something]"
Go to team billing
There shouldn't be a visible usage limit
Upgrade the team by adding a payment method
The default usage limit should be 1000 * team_size (but you can set it to any value)
Go to your individual billing settings
The usage limit should be 500 (not editable)
Upgrade by adding a payment method
The usage limit should now be 1000, and you should be able to set any value >= 1000 (but not < )
Cancel your individual user subscription
The limit should be back to 500 (not editable)

Release Notes

NONE

Documentation

Werft options:

/werft with-local-preview
If enabled this will build install/preview
/werft with-preview
/werft with-payment
/werft with-integration-tests=all
Valid options are all, workspace, webapp, ide

werft-gitpod-dev-com · 2022-10-11T08:47:12Z

started the job as gitpod-build-jx-adjust-usage-limits.1 because the annotations in the pull request description changed
(with .werft/ from main)

jankeromnes · 2022-10-11T12:18:12Z

Issue: Immediately after upgrading, the limit shows up wrong. After a refresh it's okay. I guess we need to refresh that state somehow.

jankeromnes · 2022-10-11T15:31:54Z

Okay, this now works almost as expected. 🎉

Except for the final step:

The limit should be back to 500 (not editable)

(When you cancel, the limit seems to remain unchanged? I think this should be fixed in the CostCenter as part of #13389)

components/dashboard/src/components/UsageBasedBillingConfig.tsx

easyCZ · 2022-10-12T08:20:46Z

components/dashboard/src/components/UsageBasedBillingConfig.tsx

    const [newLimit, setNewLimit] = useState<string | undefined>(
-        props.currentValue ? String(props.currentValue) : undefined,
+        typeof props.currentValue === "number" ? String(props.currentValue) : undefined,
    );


Why is the limit a string?

That's how the modal code was initially written 🤷 I left it unchanged, but I assume it's because <input> natively deal with strings (i.e. when you get the value of an input, it's always a string, even if it's of type="number").

My personal preference would be to have it a number, and always parse whatever value we get out of the input, but I generally tend not to change existing code based on my sole personal preference. Happy to refactor that bit if you also have a preference for a number state.

jldec

LGTM with the exception of step 11 (usage limit does not revert to 500 when you cancel)
I think that's ok for this PR - we have to decide if we will refund or give users the remaining credits.

Thank you @jankeromnes 🙏

/hold for final technical 👍

easyCZ · 2022-10-12T11:44:58Z

components/dashboard/src/components/UsageBasedBillingConfig.tsx

+                let limit = 1000;
+                const attrId = AttributionId.parse(attributionId);
+                if (attrId?.kind === "team") {
+                    const members = await getGitpodService().server.getTeamMembers(attrId.teamId);
+                    limit = 1000 * members.length;
+                }
+                await getGitpodService().server.subscribeToStripe(attributionId, setupIntentId, limit);


Thinking about this, I think we can shift this to server also.

Thanks! But I moved this out of server and into dashboard at Sven's request, because maybe in the future we'll want to ask users to define or confirm their usage limit while they're upgrading. I think we can leave this as is for now.

IMO, we should move it. If we'd like to suggest a limit which the user then confirms, we can suggest it in the Intent flow (teamSize * X), and send along with the subscribeToStripe call for actual persistence. We make the UI unnecessarily complex with the extra logic and introduce more failure modes.

Would like to also understand the context of Sven's request, do you have a link? What held true then, may no longer be the case. In general, the less logic the client side has, the better and given we're not doing the suggestion now .. YAGNI

Hmm, if the code works as is, I'd be keen to merge it as is, then figure out how to improve it in follow-up PRs. Happy to file a follow-up issue for this! 😇

Would like to also understand the context of Sven's request, do you have a link?

#13535 (comment)

YAGNI

Sorry, I can never remember what those acronyms mean, and I'm too lazy to look them up, so I just glaze over them and lose their meaning 😝 (also reminds me of https://acronyms-suck.com 😇)

Follow-up issue is here: #13835

easyCZ · 2022-10-12T11:45:17Z

The counterpart PR which adds validation is here #13798

geropl · 2022-10-13T07:11:03Z

Having a look here as well.

components/dashboard/src/components/UsageBasedBillingConfig.tsx

geropl

Code LGTM, tested and:

step 11. does not work (yet) as per this comment
there are further potential improvements, but those are out of scope IMO
So: ✔️

/hold for this nit.

easyCZ · 2022-10-13T08:18:18Z

Before landing this, we'll want to do a rebase to avoid having to do a get after set, and we should also shift more of the logic into server - we want these calls to be "transactional" on the server side, not on the client side (when one fails, we don't want it to fail in the client without the subsequent requests being done and us not having visibility into it)

jankeromnes · 2022-10-13T08:36:06Z

Many thanks for the reviews! Briefly taking this back to Draft in order to:

address nits
test [usage] Validate spending limits in UpdateCostCenter #13798 by rebasing on top of it

…g to Stripe

…its package you get when subscribed to Stripe as an individual

…limit modal

jankeromnes · 2022-10-13T09:18:57Z

Usage Limit input type

@geropl Fixed (it's now an <input type="text">)

Error message on personal upgrade

@geropl Known issue: #13177

Before landing this, we'll want to do a rebase to avoid having to do a get after set

@easyCZ Rebased and now using the new limit returned by subscribeToStripe

I've also uncoupled the Pull Requests again due to https://github.com/gitpod-io/gitpod/pull/13798/files -- happy to pair on that one if you'd like. Maybe it's a missing webhook? I see that the user cost center never goes back to other after cancel 👀

jankeromnes · 2022-10-13T09:39:39Z

Okay, majority of nits addressed, and this works 90% in preview environments (with a chance to work 100% in staging/production if webhooks work there -- in any case this problem pre-dates & is unrelated to this PR).

Happy to make any further adjustments as follow-up issues. I'll file these today, but please let me know if you think I forgot anything.

/unhold

easyCZ · 2022-10-13T11:06:08Z

components/server/ee/src/workspace/gitpod-server-impl.ts

+    async subscribeToStripe(
+        ctx: TraceContext,
+        attributionId: string,
+        setupIntentId: string,
+        usageLimit: number,
+    ): Promise<number | undefined> {


FWIW, this API is now super awkward in terms of the response. A consumer wouldn't expect to get a number as a response to subscribeToStripe. subscribe should return a Subscription, which can have a property of usageLimit.

roboquat added do-not-merge/work-in-progress do-not-merge/release-note-label-needed size/S labels Oct 11, 2022

jankeromnes force-pushed the jx/adjust-usage-limits branch from 3695ea1 to 3a08597 Compare October 11, 2022 08:50

roboquat added size/M and removed size/S labels Oct 11, 2022

jankeromnes force-pushed the jx/adjust-usage-limits branch from 3a08597 to a73cbac Compare October 11, 2022 09:01

roboquat added size/L and removed size/M labels Oct 11, 2022

jankeromnes marked this pull request as ready for review October 11, 2022 15:32

jankeromnes requested a review from a team October 11, 2022 15:32

roboquat removed the do-not-merge/work-in-progress label Oct 11, 2022

github-actions bot added the team: webapp Issue belongs to the WebApp team label Oct 11, 2022

easyCZ reviewed Oct 12, 2022

View reviewed changes

roboquat added release-note-none and removed do-not-merge/release-note-label-needed labels Oct 12, 2022

jldec approved these changes Oct 12, 2022

View reviewed changes

roboquat added the do-not-merge/hold label Oct 12, 2022

easyCZ reviewed Oct 12, 2022

View reviewed changes

geropl reviewed Oct 13, 2022

View reviewed changes

components/dashboard/src/components/UsageBasedBillingConfig.tsx Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

geropl approved these changes Oct 13, 2022

View reviewed changes

jankeromnes marked this pull request as draft October 13, 2022 08:36

roboquat added the do-not-merge/work-in-progress label Oct 13, 2022

jankeromnes force-pushed the jx/adjust-usage-limits branch from eaf0685 to 7e53b57 Compare October 13, 2022 08:40

roboquat added size/XL and removed size/L labels Oct 13, 2022

jankeromnes changed the base branch from main to mp/usage-cost-center-validations October 13, 2022 08:41

roboquat added size/L and removed size/XL labels Oct 13, 2022

jankeromnes added 5 commits October 13, 2022 09:15

[server][dashboard] Always explicitly set a usage limit when upgradin…

de60d60

…g to Stripe

[dashboard] Don't offer to set a usage limit lower than the base cred…

36a98a2

…its package you get when subscribed to Stripe as an individual

[dashboard] When upgrading to Stripe, also refresh the usage limit state

d140307

[dashboard] Add proper form submission and error validation to usage …

52f0710

…limit modal

[server] Make 'subscribeToStripe' return the new usage limit

f0b4217

jankeromnes force-pushed the jx/adjust-usage-limits branch from 7e53b57 to f0b4217 Compare October 13, 2022 09:15

roboquat added size/XXL and removed size/L labels Oct 13, 2022

jankeromnes changed the base branch from mp/usage-cost-center-validations to main October 13, 2022 09:15

roboquat added size/L and removed size/XXL labels Oct 13, 2022

roboquat removed the do-not-merge/hold label Oct 13, 2022

jankeromnes marked this pull request as ready for review October 13, 2022 09:39

jankeromnes removed the do-not-merge/work-in-progress label Oct 13, 2022

roboquat merged commit 53c1a1f into main Oct 13, 2022

roboquat deleted the jx/adjust-usage-limits branch October 13, 2022 09:45

jankeromnes mentioned this pull request Oct 13, 2022

Improve default usage limit UX when upgrading to Stripe #13835

Closed

easyCZ reviewed Oct 13, 2022

View reviewed changes

jankeromnes mentioned this pull request Oct 13, 2022

Adjust cost centers and usage limits depending to spec #13389

Closed

roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Oct 14, 2022

[Usage-based] Adjust Stripe usage limits to spec #13746

[Usage-based] Adjust Stripe usage limits to spec #13746

Uh oh!

Conversation

jankeromnes commented Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue(s)

How to test

Release Notes

Documentation

Werft options:

Uh oh!

werft-gitpod-dev-com bot commented Oct 11, 2022

Uh oh!

jankeromnes commented Oct 11, 2022

Uh oh!

jankeromnes commented Oct 11, 2022 • edited by werft-gitpod-dev-com bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

easyCZ Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

jankeromnes Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jldec left a comment

Choose a reason for hiding this comment

Uh oh!

easyCZ Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

jankeromnes Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

easyCZ Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

jankeromnes Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jankeromnes Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

easyCZ commented Oct 12, 2022

Uh oh!

geropl commented Oct 13, 2022

Uh oh!

Uh oh!

This comment was marked as resolved.

geropl left a comment

Choose a reason for hiding this comment

Uh oh!

easyCZ commented Oct 13, 2022

Uh oh!

jankeromnes commented Oct 13, 2022

Uh oh!

jankeromnes commented Oct 13, 2022 • edited by werft-gitpod-dev-com bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jankeromnes commented Oct 13, 2022

Uh oh!

easyCZ Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jankeromnes commented Oct 11, 2022 •

edited

Loading

jankeromnes commented Oct 11, 2022 •

edited by werft-gitpod-dev-com bot

Loading

jankeromnes Oct 12, 2022 •

edited

Loading

jankeromnes Oct 13, 2022 •

edited

Loading

jankeromnes commented Oct 13, 2022 •

edited by werft-gitpod-dev-com bot

Loading