Display alert in "No seats" scenario #11768

geropl · 2022-08-01T11:51:21Z

Description

Related Issue(s)

How to test

try to signup with GitHub here
notice how you're brought back to the login-screen, which should shows this message:

Note: the "feedback" panel is only shown on gitpod.io.

Release Notes

display an alert on signup if a user cannot login because they license does not cover for that additional seat

Documentation

Werft options:

/werft with-preview

geropl · 2022-08-01T11:52:31Z

/werft run

👍 started the job as gitpod-build-gpl-9972-no-seats.2
(with .werft/ from main)

With preview this time

geropl · 2022-08-01T15:14:02Z

/hold Until the TEST COMMIT is removed

geropl · 2022-08-01T15:16:01Z

@AlexTugarev Could you verify if the approach I took here is legit?

@gtsiolis Was the switch to <Alert ... >...<Alert/> what you had in mind here? 🤔

lucasvaltl

LGTM, thanks for looking into this! :)

lucasvaltl · 2022-08-01T15:22:38Z

components/server/ee/src/user/user-service.ts

@@ -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`;


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.`;

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.

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

gtsiolis

Nice one, @geropl! ✨

Left one comment about the style of the alert and another one about the copy used there.

Regarding the question in #11768 (comment): Yes, the alert component is exactly what I had in mind for this case. 🏀

Approving from UX perspective. ✔️

Approving as this also needs a review from @gitpod-io/engineering-webapp. 🏓

gtsiolis · 2022-08-01T18:48:51Z

components/server/ee/src/user/user-service.ts

@@ -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`;


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.

components/dashboard/src/components/ErrorMessage.tsx

gtsiolis · 2022-08-01T20:41:08Z

components/server/ee/src/user/user-service.ts

@@ -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`;


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

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."

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

Co-authored-by: George Tsiolis <tsiolis.g@gmail.com>

AlexTugarev · 2022-08-02T07:09:07Z

components/server/src/auth/generic-auth-provider.ts

@@ -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 });


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

AlexTugarev

LGTM!

geropl · 2022-08-02T07:28:51Z

/unhold

geropl · 2022-08-02T07:29:59Z

@lucasvaltl @gtsiolis I'd like to defer the wording discussion, also because I think @jldec would want to review that is will (he's ooo this week). 👍

lucasvaltl · 2022-08-02T07:34:38Z

That's fine with me - feel free to pick something and then we can iterate on it later on :)

gtsiolis · 2022-08-02T07:49:16Z

Sounds good! Opened a follow-up issue for the wording in #11791. Cc @lucasvaltl @geropl

roboquat added do-not-merge/work-in-progress do-not-merge/release-note-label-needed size/XS release-note and removed do-not-merge/release-note-label-needed labels Aug 1, 2022

geropl added 3 commits August 1, 2022 14:28

[server] Display Auth errors in the UI

44fa57a

[dashboard] Move to new Alert component

7efea0a

TEST COMMIT

b773e5d

geropl force-pushed the gpl/9972-no-seats branch from a5a28a7 to b773e5d Compare August 1, 2022 14:28

roboquat added size/S and removed size/XS labels Aug 1, 2022

geropl marked this pull request as ready for review August 1, 2022 15:13

geropl requested a review from a team August 1, 2022 15:13

roboquat removed the do-not-merge/work-in-progress label Aug 1, 2022

github-actions bot added the team: webapp Issue belongs to the WebApp team label Aug 1, 2022

roboquat added the do-not-merge/hold label Aug 1, 2022

lucasvaltl approved these changes Aug 1, 2022

View reviewed changes

gtsiolis approved these changes Aug 1, 2022

View reviewed changes

[dashboard] Review comment spacing

ca802d2

Co-authored-by: George Tsiolis <tsiolis.g@gmail.com>

AlexTugarev reviewed Aug 2, 2022

View reviewed changes

AlexTugarev approved these changes Aug 2, 2022

View reviewed changes

roboquat removed the do-not-merge/hold label Aug 2, 2022

roboquat merged commit 74bf2aa into main Aug 2, 2022

roboquat deleted the gpl/9972-no-seats branch August 2, 2022 07:29

gtsiolis mentioned this pull request Aug 2, 2022

Update error message copy when maximum number of users reached per license #11791

Open

roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Aug 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display alert in "No seats" scenario #11768

Display alert in "No seats" scenario #11768

geropl commented Aug 1, 2022 •

edited

Loading

geropl commented Aug 1, 2022 •

edited by werft-gitpod-dev-com bot

Loading

geropl commented Aug 1, 2022

geropl commented Aug 1, 2022

lucasvaltl left a comment

lucasvaltl Aug 1, 2022

gtsiolis Aug 1, 2022

gtsiolis Aug 1, 2022

gtsiolis left a comment

gtsiolis Aug 1, 2022

gtsiolis Aug 1, 2022

lucasvaltl Aug 2, 2022 •

edited

Loading

gtsiolis Aug 2, 2022

AlexTugarev Aug 2, 2022

AlexTugarev left a comment

geropl commented Aug 2, 2022

geropl commented Aug 2, 2022

lucasvaltl commented Aug 2, 2022

gtsiolis commented Aug 2, 2022 •

edited

Loading

	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.`;

Display alert in "No seats" scenario #11768

Display alert in "No seats" scenario #11768

Conversation

geropl commented Aug 1, 2022 • edited Loading

Description

Related Issue(s)

How to test

Release Notes

Documentation

Werft options:

geropl commented Aug 1, 2022 • edited by werft-gitpod-dev-com bot Loading

geropl commented Aug 1, 2022

geropl commented Aug 1, 2022

lucasvaltl left a comment

Choose a reason for hiding this comment

lucasvaltl Aug 1, 2022

Choose a reason for hiding this comment

gtsiolis Aug 1, 2022

Choose a reason for hiding this comment

gtsiolis Aug 1, 2022

Choose a reason for hiding this comment

gtsiolis left a comment

Choose a reason for hiding this comment

gtsiolis Aug 1, 2022

Choose a reason for hiding this comment

gtsiolis Aug 1, 2022

Choose a reason for hiding this comment

lucasvaltl Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

gtsiolis Aug 2, 2022

Choose a reason for hiding this comment

AlexTugarev Aug 2, 2022

Choose a reason for hiding this comment

AlexTugarev left a comment

Choose a reason for hiding this comment

geropl commented Aug 2, 2022

geropl commented Aug 2, 2022

lucasvaltl commented Aug 2, 2022

gtsiolis commented Aug 2, 2022 • edited Loading

geropl commented Aug 1, 2022 •

edited

Loading

geropl commented Aug 1, 2022 •

edited by werft-gitpod-dev-com bot

Loading

lucasvaltl Aug 2, 2022 •

edited

Loading

gtsiolis commented Aug 2, 2022 •

edited

Loading