-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[dashboard] add license tab to the admin dashboard #9343
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
Looking at this now! 👀 |
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.
Thanks for adding this, @nandajavarma! ✨
Looks great so far! Added some comments below.
From UX perspective this is getting closer to the finish line. We should probably ask for a review from someone to take a closer look at the code before merging, too. 🏁
7ff11d6
to
5c4f191
Compare
cefdfd4
to
e955551
Compare
/werft run with-clean-slate 👍 started the job as gitpod-build-nvn-license-dash.133 |
Hey @gtsiolis ! Could you give the changes another look when you get a chance, I have modified the code to reflect the changes you suggested: |
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.
Thanks for the ping, @nandajavarma! 🏓
This is almost at the finish line! 🏁
Left a few more comments below.
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.
Thanks for the great work, Nandaja! 🚀
I left a few comments.
Together with the flow chart @lucasvaltl is working on about the different states the license could reach (valid, expired, too many users, wrong domain, etc.) we need to think about how we could test every case (unit tests?) and how the license page actually looks like for each case. But that's follow-up work.
return [licenseLevel("Inactive"), additionalLicenseInfo("Free"), alertMessage()]; | ||
} | ||
|
||
function professionalPlan(userCount: number, seats: number, trial: boolean, validUntil: string): ReactElement[] { |
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.
In this code, the validUntil
param is used for Trial licenses only. However, every license could have a limited validation (I expect that we usually issue 1-year licenses that need to be renewed). Right?
@corneliusludmann Triggered also from the comment in #9343 (comment) (Cc @nandajavarma), I'd also like to better understand the options that exist and the matrix of possible use cases depending on which option a user picks to install Gitpod, so that we make the license page flexible to support all cases. Please, keep me in the loop or ping me in a follow up issue. 🏓 |
Hi, would you mind making me (jldec) admin on the preview env so that I can look at the admin UI. |
Here's what I see when I look at the preview env (I have narrowed my browser viewport). The comments are just first impressions to someone non-familiar with all the details - not intended as criticism 🙏. Generally speaking I do like the look of the design.
|
/werft run with-clean-slate 👍 started the job as gitpod-build-nvn-license-dash.142 |
I was going through another admin epic, and noticed that the Admin Dashboard should no longer be limited to license holders. This means that Admin Dashboard could always be shown as checked in the left box :) See ebb90c6 and #6932 (comment) - cc: @mrsimonemms |
@jldec My understanding was that, the bit about Correct me if I am wrong @mrsimonemms |
Whether the admin dashboard is available or not is outside of the scope of this pull request, in my opinion. We need to fix this on the license code level (if needed) and not in the UI. Nevertheless, good heads up, @jldec! 🙏 @lucasvaltl We need to consider this in our license evaluation work and check implementation vs. what we really want. Homepage tells currently that admin dashboard is a EE feature but I think Jürgen is right here that the admin dashboard is intentional always available (code-wise). |
927115a
to
63f3df6
Compare
16f7750
to
8a87054
Compare
Sorry about the late update on this PR. Holidays did make things a bit late. The preview environment got cleaned up, and your accounts are not there anymore. I can grant admin access as and when necessary.
Let me know your thoughts! And apologies for the very long waiting time on this PR. |
8a87054
to
8009fa2
Compare
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.
Thanks @nandajavarma for tackling all pending discussions!
FWIW, regarding responsive layout mentioned in #9343 (comment) is part of a larger topic affecting more parts of the dashboard, see #4050. In case there's anything we could do ad-hoc to improve the responsive layout in every PR that introduces new components or patterns we could do that! However, it's also ok or even preferred to leave this out of the scope of this PR.
UX in this PR now looks mergable! 🛹
Do you think it's worth passing this to someone else to take a closer look at the code changes? 🏀
Co-authored-by: George Tsiolis <[email protected]>
/werft run with-clean-slate 👍 started the job as gitpod-build-nvn-license-dash.150 |
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.
Thank you @nandajavarma - this looks much more polished now.
You have addressed all of my feedback in your comments or in fixes.
LGTM
Description
This PR adds a Licence page to the
admin
dashboard. This will help the self-hosted user to understand the features they have available, user limit, etc. The information that are currently available on the UI are:Design specs for this can be found under the issue 8413
Related Issue(s)
Fixes #8413
How to test
Admin
>License
tab has been tested for the following scenarios on a self-hosted setup (it won't be available for SaaS):installer
without a license key (default license)The testing can be done by manually inserting users to the
d_b_user
table of themysql
pod to exceed the user limits to test all scenarios.Release Notes
Documentation