Skip to content
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

[UBP] Add getUsageLimit and setUsageLimit methods to server #12767

Merged
merged 2 commits into from
Sep 14, 2022

Conversation

andrew-farries
Copy link
Contributor

@andrew-farries andrew-farries commented Sep 8, 2022

Description

Add two new methods to the server API for getting and setting usage limits. Both new functions take an attributionId and work for both users and teams.

For backwards compatibility, leave the getUsageLimitForTeam and setUsageLimitForTeam methods as they are still used by the dashboard, but change them to be implemented in terms of the more general get/set methods.

Related Issue(s)

Part of #12685

How to test

  • Create a team with Gitpod in the name in the preview environment and sign the team up for usage based pricing.
  • Run the following in the browser developer console:
await window._gp.gitpodService.server.setUsageLimit('team:<teamId of the new team>', 90)
  • Then:
await window._gp.gitpodService.server.getUsageLimit('team:<teamId of the new team>')

The team usage limit can also be updated from the team billing page.

The usage limits for the team are correctly persisted.

Setting usage limits from the the UI for individual users will be done in a later PR.

Release Notes

NONE

Documentation

Werft options:

  • /werft with-preview
  • /werft with-payment

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-af-usage-limit-set-get-for-users.2 because the annotations in the pull request description changed
(with .werft/ from main)

@andrew-farries andrew-farries marked this pull request as draft September 8, 2022 14:43
@andrew-farries andrew-farries force-pushed the af/usage-limit-set-get-for-users branch from 1b147ee to 150fcd2 Compare September 9, 2022 06:01
@roboquat roboquat added size/L and removed size/M labels Sep 9, 2022
@andrew-farries andrew-farries force-pushed the af/usage-limit-set-get-for-users branch from 150fcd2 to 791eea4 Compare September 9, 2022 06:31
@roboquat roboquat added size/M and removed size/L labels Sep 9, 2022
@andrew-farries andrew-farries force-pushed the af/usage-limit-set-get-for-users branch from 791eea4 to 3a73019 Compare September 9, 2022 06:42
@andrew-farries andrew-farries changed the title [UBP] Add (get|set)UsageLimitForUser methods to server [UBP] Add getUsageLimitFor and setUsageLimitFor methods to server Sep 9, 2022
@andrew-farries andrew-farries marked this pull request as ready for review September 9, 2022 07:02
@andrew-farries andrew-farries force-pushed the af/usage-limit-set-get-for-users branch from 5d9e40f to d7fbf86 Compare September 9, 2022 07:52
@andrew-farries andrew-farries changed the title [UBP] Add getUsageLimitFor and setUsageLimitFor methods to server [UBP] Add getUsageLimit and setUsageLimit methods to server Sep 9, 2022
@andrew-farries
Copy link
Contributor Author

This has some conflicts to resolve now that #12776 is merged (putting costcenter access behind an API).

Moving back to draft until that's done.

@andrew-farries
Copy link
Contributor Author

/hold because it's based on #12843

Base automatically changed from af/fix-usage-sugar-file to main September 12, 2022 06:00
roboquat pushed a commit that referenced this pull request Sep 12, 2022
In particular avoid the use of `||` against an enum type that can be 0.

See:

#12767 (comment)
@roboquat roboquat added size/L and removed size/M labels Sep 12, 2022
switch (attrId.kind) {
case "team":
const team = await this.guardTeamOperation(attrId.teamId, "update");
await this.ensureStripeApiIsAllowed({ team });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This checks for m.mode === "usage-based" || (m.mode === "chargebee" && !!m.canUpgradeToUBB) which seems like I could then change my spendinglimit although I haven't put my credit card into stripe, yet.
Maybe we should reconsider the special 'chargebee' case? What was the idea of it? cc @geropl

Copy link
Member

@svenefftinge svenefftinge Sep 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove the ensureStripeApiIsAllowed checks here as well. The check below testing the billingStrategy !== "stripe" is doing what we need and is more explicit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@svenefftinge Originally, Chargebee and Usage-Limit was strictly independent. After last weeks discussion, they are not anymore.

Let's remove the ensureStripeApiIsAllowed checks here as well.

💯 Agreed!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrew-farries do you agree to remove the calls to ensureStripeApiIsAllowed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes; I've removed them in a650ae7

components/server/ee/src/workspace/gitpod-server-impl.ts Outdated Show resolved Hide resolved
components/server/ee/src/workspace/gitpod-server-impl.ts Outdated Show resolved Hide resolved
components/server/ee/src/workspace/gitpod-server-impl.ts Outdated Show resolved Hide resolved
@andrew-farries andrew-farries force-pushed the af/usage-limit-set-get-for-users branch from 850610e to 568f5f2 Compare September 12, 2022 06:30
@roboquat roboquat added size/M and removed size/L labels Sep 12, 2022
@andrew-farries
Copy link
Contributor Author

/unhold because #12843 is merged.

@andrew-farries andrew-farries force-pushed the af/usage-limit-set-get-for-users branch from 568f5f2 to b86fba1 Compare September 12, 2022 07:31
const user = this.checkAndBlockUser("getUsageLimit");
if (attrId.kind === "team") {
const team = await this.guardTeamOperation(attrId.teamId, "get");
await this.ensureStripeApiIsAllowed({ team });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if I wasn't clear. I meant to remove these calls for both attribution kinds (user and team).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this check aswell.

@andrew-farries
Copy link
Contributor Author

andrew-farries commented Sep 13, 2022

Needs rebased again now that #12903 landed 😅

@andrew-farries andrew-farries force-pushed the af/usage-limit-set-get-for-users branch from f02cdb4 to 35e207c Compare September 13, 2022 14:43
Add two new methods to the server API for getting and setting usage
limits. Both new functions take an attributionId and work for both users
and teams.

For backwards compatibility, leave the `getUsageLimitForTeam` and
`setUsageLimitForTeam` methods as they are still used by the dashboard,
but change them to be implemented in terms of the more general `get/set`
methods.
@andrew-farries andrew-farries force-pushed the af/usage-limit-set-get-for-users branch from 35e207c to 6d9c770 Compare September 14, 2022 05:54
Also the `guardTeamOperation` call as both are not needed given the
`guardCostCenterAccess` call below.
Copy link
Member

@svenefftinge svenefftinge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@roboquat roboquat merged commit 5350b91 into main Sep 14, 2022
@roboquat roboquat deleted the af/usage-limit-set-get-for-users branch September 14, 2022 06:58
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Sep 14, 2022
iQQBot pushed a commit that referenced this pull request Sep 14, 2022
In particular avoid the use of `||` against an enum type that can be 0.

See:

#12767 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/M team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants