Skip to content

Display alert in "No seats" scenario #11768

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.

Sign up for GitHub

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 4 commits into from
Aug 2, 2022
Merged

Display alert in "No seats" scenario #11768

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
44fa57a
[server] Display Auth errors in the UI
geropl Aug 1, 2022
7efea0a
[dashboard] Move to new Alert component
geropl Aug 1, 2022
b773e5d
TEST COMMIT
geropl Aug 1, 2022
ca802d2
[dashboard] Review comment spacing
geropl Aug 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions components/dashboard/src/components/ErrorMessage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,15 @@

import FeedbackComponent from "../feedback-form/FeedbackComponent";
import { isGitpodIo } from "../utils";
import Alert from "./Alert";

function ErrorMessage(props: { imgSrc: string; imgAlt?: string; message: string }) {
return (
<>
<div className="mt-16 flex space-x-2 py-6 px-6 w-96 justify-between bg-gitpod-kumquat-light rounded-xl">
<div className="pr-3 self-center w-6">
<img src={props.imgSrc} alt={props.imgAlt || "An error message"} />
</div>
<div className="flex-1 flex flex-col">
<p className="text-gitpod-red text-sm">{props.message}</p>
</div>
<div className="mt-16 w-96">
<Alert closable={false} showIcon={true} type="error">
<span>{props.message}</span>
</Alert>
</div>
{isGitpodIo() && (
<FeedbackComponent
Expand Down
12 changes: 10 additions & 2 deletions components/server/ee/src/user/user-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,14 @@
*/

import { UserService, CheckSignUpParams, CheckTermsParams } from "../../../src/user/user-service";
import { User, WorkspaceTimeoutDuration, WORKSPACE_TIMEOUT_EXTENDED, WORKSPACE_TIMEOUT_EXTENDED_ALT, WORKSPACE_TIMEOUT_DEFAULT_LONG, WORKSPACE_TIMEOUT_DEFAULT_SHORT } from "@gitpod/gitpod-protocol";
import {
User,
WorkspaceTimeoutDuration,
WORKSPACE_TIMEOUT_EXTENDED,
WORKSPACE_TIMEOUT_EXTENDED_ALT,
WORKSPACE_TIMEOUT_DEFAULT_LONG,
WORKSPACE_TIMEOUT_DEFAULT_SHORT,
} from "@gitpod/gitpod-protocol";
import { inject } from "inversify";
import { LicenseEvaluator } from "@gitpod/licensor/lib";
import { Feature } from "@gitpod/licensor/lib/api";
Expand Down Expand Up @@ -78,7 +85,8 @@ export class UserServiceEE extends UserService {

// 1. check the license
const userCount = await this.userDb.getUserCount(true);
if (!this.licenseEvaluator.hasEnoughSeats(userCount)) {
if (userCount > -1) {
//!this.licenseEvaluator.hasEnoughSeats(userCount)) {
const msg = `Maximum number of users permitted by the license exceeded`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd word this slightly more specifically:

Suggested change
const msg = `Maximum number of users permitted by the license exceeded`;
const msg = `Unable to log in. The license used for this installation does not have enough seats. Please contact your administrator to increase the number of seats for your license.`;

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the copy suggestion, @lucasvaltl!

Some thoughts on the suggested copy:

Unable to log in.

Quite clear to read. But maybe we could surface the error instead?

The license used for this installation does not have enough seats.

Do we need to surface the license limits to the users? I'd expect us in the future to be a bit cautious about the error messages here to avoid exposing any relevant information.

Please contact your administrator to increase the number of seats for your license.

While surfacing a call-to-action (CTA) to users sounds good, I'd avoid vague actions like this which are usually not helpful, and sometimes it's not possible to perform in some large organizations.

Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Also, it'd be great if we could surface this CTA to request a new license for the admins when browsing the dashboard.

Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Adding one more alternative copy with a different copy structure, but we can work on improving the error messages in a later iteration. Cc @lucasvaltl

BEFORE AFTER
Screenshot 2022-08-01 at 9 46 00 PM Screenshot 2022-08-01 at 9 45 39 PM

Copy link
Contributor

@lucasvaltl lucasvaltl Aug 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm ok with this in general, but the terminology "instance user cap" seems off to me. We don't use it anywhere else, we usually talk about seats or just users. I'd suggest we go with "Maximum number of users reached. We could not sign you in because this instance has reached the maximum number of allowed users."

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, @lucasvaltl ! Agree, let's go with your updated suggestion:

Maximum number of users reached. We could not sign you in because this instance has reached the maximum number of allowed users."

Cc @geropl

throw AuthException.create("Cannot sign up", msg, { userCount, params });
}
Expand Down
2 changes: 1 addition & 1 deletion components/server/src/auth/generic-auth-provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,7 @@ export class GenericAuthProvider implements AuthProvider {

let message = "Authorization failed. Please try again.";
if (AuthException.is(err)) {
message = `Login was interrupted: ${err.message}`;
return this.sendCompletionRedirectWithError(response, { error: err.message });
Copy link
Member

Choose a reason for hiding this comment

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

LGTM!
I wonder why that's not done always in L.441. 🤔

}
if (this.isOAuthError(err)) {
message = "OAuth Error. Please try again."; // this is a 5xx response from authorization service
Expand Down