Skip to content

[admin] Allow credit adjustments #14594

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

Merged
merged 1 commit into from
Nov 22, 2022
Merged

Conversation

svenefftinge
Copy link
Member

@svenefftinge svenefftinge commented Nov 11, 2022

Description

Allows admins to view and alter the credit balance as well as adjusting the usage limit of team and user cost centers.

The spending in billing setting was showing the <usage of the current period> / <usageLimit> which is ignoring the total credit balance of an account. I have changed/fixed this in this PR as well.
Since we now can issue one time credits (or even debits) and because users can go over their spending limit (with running workspaces), we compute the spending so that if a user has a positive balance we add the amount to the spending limit on the right. Also, the percentage bar below would be 0% instead of a negative number.
Here's an example where I granted 30 credits (in addition to a 500 spending limit and some small usage)

Screenshot 2022-11-21 at 10 43 08

The case where someone uses more than their usage limit, is show like this:

Screenshot 2022-11-21 at 10 44 50

For the admin dashboard I opted for separating balance and usage limit:
Screenshot 2022-11-21 at 10 46 49

Related Issue(s)

Fixes #14591
Fixes #14739
Fixes #14367

How to test

Release Notes

NONE

Documentation

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-sefftinge-ubp-grant-free-credits-14591.1 because the annotations in the pull request description changed
(with .werft/ from main)

@svenefftinge
Copy link
Member Author

svenefftinge commented Nov 14, 2022

/werft run

👍 started the job as gitpod-build-sefftinge-ubp-grant-free-credits-14591.2
(with .werft/ from main)

@svenefftinge svenefftinge force-pushed the sefftinge/ubp-grant-free-credits-14591 branch from c00c344 to 1b22a00 Compare November 15, 2022 13:57
@roboquat roboquat added size/L and removed size/XS labels Nov 15, 2022
@svenefftinge svenefftinge force-pushed the sefftinge/ubp-grant-free-credits-14591 branch from 1b22a00 to ba17816 Compare November 15, 2022 14:03
@roboquat roboquat added size/XS and removed size/L labels Nov 15, 2022
@svenefftinge svenefftinge reopened this Nov 15, 2022
@roboquat roboquat added size/L and removed size/XS labels Nov 15, 2022
@svenefftinge svenefftinge force-pushed the sefftinge/ubp-grant-free-credits-14591 branch from 1540756 to ba17816 Compare November 15, 2022 14:17
@roboquat roboquat added size/XS and removed size/L labels Nov 15, 2022
@svenefftinge svenefftinge reopened this Nov 15, 2022
@roboquat roboquat added size/L and removed size/XS labels Nov 15, 2022
@svenefftinge
Copy link
Member Author

svenefftinge commented Nov 15, 2022

/werft run

👍 started the job as gitpod-build-sefftinge-ubp-grant-free-credits-14591.8
(with .werft/ from main)

@svenefftinge svenefftinge force-pushed the sefftinge/ubp-grant-free-credits-14591 branch 5 times, most recently from 1c33b93 to f6d393c Compare November 16, 2022 17:22
@jldec
Copy link
Contributor

jldec commented Nov 21, 2022

@svenefftinge - did you forget to push your changes to the branch?
I don't see the behavior described in the latest PR description above.
Screenshot 2022-11-21 at 12 29 46

@svenefftinge svenefftinge force-pushed the sefftinge/ubp-grant-free-credits-14591 branch from eef621d to 040b1ef Compare November 21, 2022 12:54
@roboquat roboquat added size/XXL and removed size/XL labels Nov 21, 2022
Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

Thanks, @svenefftinge!

Looks great overall! UX could still be somewhat confusing in some edge cases or missing some user feedback on errors, but changes here look like a good great first iteration. 🛹

Left a few minor UX comments below. 🏓

}
}}
>
Save
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: Probably not the best line to comment about this, but we could probably surface these errors we return in console when entering invalid values. Possibly good for a follow up issue. 🏓

Screenshot 2022-11-21 at 2 40 40 PM (2)

<Modal
visible={editSpendingLimit}
onClose={() => setEditSpendingLimit(false)}
title="Update Usage Limit"
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: When trying to update the usage limit for a team I get the following console error and the limit does not update. Is this known or expected?

Uncaught (in promise) Error: Failed to get Stripe Subscription ID for 'team:0faf0b45-1e5f-4c3c-b5ef-6bbe22f1da9f'

}
}}
>
Save
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Probably not the best line to comment about this, but when a user upgrades to UBP, the error returned when trying to change the usage limit from the admin dashboard below 1000 mentions again the 500 limit. Should this become 1000 or shall we change the error to become more generic?

/ {usageLimit} Credit{usageLimit === 1 ? "" : "s"}
</span>
<span className="text-gray-900 dark:text-gray-100">{used}</span>
<span className="text-gray-400 dark:text-gray-500"> / {totalAvailable} Credits</span>
Copy link
Contributor

@gtsiolis gtsiolis Nov 21, 2022

Choose a reason for hiding this comment

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

thought: Cross-posting for future reference, in case we'd like to separate the monthly quota from the additional credits to explain further how and which credits are carried over to the next month, how some credits could expire after some months, and more. Feel to ignore this for now.

Frame 541

@svenefftinge svenefftinge force-pushed the sefftinge/ubp-grant-free-credits-14591 branch from 040b1ef to 3d43ba9 Compare November 21, 2022 14:05
@jldec
Copy link
Contributor

jldec commented Nov 21, 2022

UI nit - it's possible to grant negative credits using the up/down controls, but not by typing a minus.

I think we should allow negative credit grants with a minus e.g. if an admin makes a mistake in a positive grant and wants to undo it with a negative grant.

@jldec
Copy link
Contributor

jldec commented Nov 21, 2022

We should allow admins to enter a reason for updating the limit (just like they have for adding credits)

@jldec
Copy link
Contributor

jldec commented Nov 21, 2022

Updating the limit is effectively a permanent change to the user's plan, right?
-> We should also update "Current plan" section in the UI with the new limit.

Screenshot 2022-11-21 at 17 08 04

Copy link
Member

@AlexTugarev AlexTugarev left a comment

Choose a reason for hiding this comment

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

(wrong type)

Copy link
Member

@AlexTugarev AlexTugarev left a comment

Choose a reason for hiding this comment

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

Checked code in components/server, LGTM!
See a few minor remarks in comments, but that's definitely not blocking.

George reported some issues/errors, which I'm not sure if we need to address right away. If you think they should be addressed in a follow-up, we can merge it as is.
/hold

Copy link
Member

@easyCZ easyCZ left a comment

Choose a reason for hiding this comment

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

Broadly looks okay. I'd personally like that a reason - human readable explanation of the credit be included to help us understand retrospectively the WHY

Comment on lines +436 to +418
min={-50000}
max={50000}
Copy link
Member

Choose a reason for hiding this comment

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

[nit] Feels arbitrary, and if we want to enforce it, perhaps should do that on the backend anyway. I'd remove.

@svenefftinge svenefftinge force-pushed the sefftinge/ubp-grant-free-credits-14591 branch from 3d43ba9 to c12f3b8 Compare November 22, 2022 08:32
@svenefftinge svenefftinge force-pushed the sefftinge/ubp-grant-free-credits-14591 branch from c12f3b8 to d97b1e0 Compare November 22, 2022 08:49
@svenefftinge
Copy link
Member Author

We should allow admins to enter a reason for updating the limit (just like they have for adding credits)

Yes, would be good, but it's not low-hanging. Can you create a follow-up issue?

@svenefftinge
Copy link
Member Author

UI nit - it's possible to grant negative credits using the up/down controls, but not by typing a minus.

Works for me. What browser are you on?

@svenefftinge
Copy link
Member Author

/unhold

@roboquat roboquat merged commit e044c1d into main Nov 22, 2022
@roboquat roboquat deleted the sefftinge/ubp-grant-free-credits-14591 branch November 22, 2022 09:31
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Nov 22, 2022
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/XXL team: webapp Issue belongs to the WebApp team
Projects
None yet
7 participants