- Notifications
You must be signed in to change notification settings - Fork918
feat(site): show license utilization in general settings#15683
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
github-actionsbot commentedNov 29, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPageView.stories.tsxShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| {entitlements!.features.user_limit.actual}/ | ||
| {entitlements!.features.user_limit.limit} users) |
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.
Would it make sense to give these a similar treatment tolicenseUtilizationPercentage and check if we can evaluate them based on whetherentitlements is nil?
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.
Just a minor suggestion, otherwise LGTM (deferring approval for fe).
| entitlements?.features?.user_limit?.limit | ||
| ? entitlements.features.user_limit.actual / | ||
| entitlements.features.user_limit.limit | ||
| : undefined; |
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.
Suggestion: Instead of just doing the percentage here, we could prepare the data format in a more convenient way to use in the template:
constlicenseInfo=entitlements?.features?.user_limit?.actual&&entitlements?.features?.user_limit?.limit?{valid:true,actual:entitlements.features.user_limit.actual,limit:entitlements.features.user_limit.limit} :{valid:false,actual:0,limit:0,};
Might not need to even init the actual/limit for the zero case but added it just in case.
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.
Approving to unblock.
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.
It looks good to me. Thanks for digging into the problem and finding the right solution.
One minor thing, that we can do after merge, is to improve the design a bit. cc.:@chrifro
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.
nice!
site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPageView.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| @@ -42,6 +42,7 @@ const meta: Meta<typeof GeneralSettingsPageView> = { | |||
| deploymentDAUs: MockDeploymentDAUResponse, | |||
| invalidExperiments: [], | |||
| safeExperiments: [], | |||
| entitlements: undefined, | |||
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.
| entitlements: undefined, |
undefined is JavaScript's equivalent to a zero value, this doesn't do anything
…ralSettingsPageView.tsxCo-authored-by: ケイラ <mckayla@hey.com>
7e1ac2e intomainUh oh!
There was an error while loading.Please reload this page.
This PR is the first iteration towards#15297We cannot yet show license utilization over time, so we show currentlicense utilization.This is because we don't track user states over time. We only track thecurrent user state. A graph over time filtering by active users wouldtherefore not account for day to day changes in user state and beinaccurate.DB schema migrations and related updates will follow that allow us toshow license utilization over time.---------Co-authored-by: ケイラ <mckayla@hey.com>(cherry picked from commit7e1ac2e)
Uh oh!
There was an error while loading.Please reload this page.
This PR is the first iteration towards#15297
We cannot yet show license utilization over time, so we show current license utilization.
This is because we don't track user states over time. We only track the current user state. A graph over time filtering by active users would therefore not account for day to day changes in user state and be inaccurate.
DB schema migrations and related updates will follow that allow us to show license utilization over time.