Skip to content
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

Update App Notifications #12670

Merged
merged 2 commits into from
Sep 15, 2022
Merged

Update App Notifications #12670

merged 2 commits into from
Sep 15, 2022

Conversation

AlexTugarev
Copy link
Member

@AlexTugarev AlexTugarev commented Sep 6, 2022

Description

Dismiss Usage Limit notifications automatically on resolution, e.g. on increase of Usage Limit.

Related Issue(s)

Fixes #12547

How to test

  • Have 2 accounts on a team with UB enabled.
  • Decrease usage limit and drive into the limit notification.
  • Increase the spending limit on one account.
  • See the notification is dismissed for both accounts.

Release Notes

Dismiss Usage Limit notifications automatically.

Documentation

Werft options:

  • /werft with-preview
  • /werft with-payment

@AlexTugarev
Copy link
Member Author

AlexTugarev commented Sep 8, 2022

/werft run

👍 started the job as gitpod-build-at-update-notifications.2
(with .werft/ from main)

@AlexTugarev
Copy link
Member Author

AlexTugarev commented Sep 8, 2022

/werft run

👍 started the job as gitpod-build-at-update-notifications.3
(with .werft/ from main)

@jldec
Copy link
Contributor

jldec commented Sep 12, 2022

/werft run

👍 started the job as gitpod-build-at-update-notifications.4
(with .werft/ from main)

@AlexTugarev AlexTugarev force-pushed the at/update-notifications branch 2 times, most recently from 21e0e6a to 5ed594a Compare September 12, 2022 12:33
@AlexTugarev
Copy link
Member Author

AlexTugarev commented Sep 13, 2022

/werft run

👍 started the job as gitpod-build-at-update-notifications.7
(with .werft/ from main)

@AlexTugarev AlexTugarev force-pushed the at/update-notifications branch from 5ed594a to be0ecad Compare September 13, 2022 12:23
@AlexTugarev AlexTugarev marked this pull request as ready for review September 13, 2022 12:24
@AlexTugarev AlexTugarev requested a review from a team September 13, 2022 12:24
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Sep 13, 2022
@AlexTugarev
Copy link
Member Author

AlexTugarev commented Sep 13, 2022

@jldec, please have a look. 🙏🏻
(Hopefully, the latest build will succeed 🤞🏻 )

@AlexTugarev AlexTugarev force-pushed the at/update-notifications branch from be0ecad to b759be3 Compare September 13, 2022 12:36
@svenefftinge
Copy link
Member

tbh, it seems a bit overkill to roll out push events for this. @jldec why do we need to show the limit everywhere in the dashboard? Would it not be sufficient to check and display the limit on workspace-start?

@jldec
Copy link
Contributor

jldec commented Sep 13, 2022

@svenefftinge, @AlexTugarev, the idea behind a global dashboard notification from the start was that this message is really intended for team owners who may not be starting workspaces all the time. (We also envisaged email notifications at some (80%) threshold before the limit is reached.)

If the notification mechanism is really expensive (implementation cost, complexity), we could pull back and ship just the notification/error on start workspace for now, but if we have something that works globally, I believe users would prefer that because it makes the out-of-credits status much clearer.

@svenefftinge
Copy link
Member

this message is really intended for team owners who may not be starting workspaces all the time

I don't think people who don't start workspaces hang out on the dashboard. They would get nudged by developers who see the information when starting a workspace.

@jldec
Copy link
Contributor

jldec commented Sep 14, 2022

@svenefftinge Maybe I'm nitpicking, but the UX of a dashboard notification does feel superior. Happy to roll-back if you think the implementation or complexity cost is not worth it.

Error-on-start only (no dashboard notification):

  • developer gets error on workspace start
  • contacts team owner
  • team owner first confirms error by starting a workspace
  • team owner sees error and confirms that it is because "usage limit exceeded"

Dashboard notification

  • any team member sees error (on workspace start or in dashboard)
  • contacts team owner
  • team owner immediately sees "usage limit exceeded" in dashhboard - no additional confirmation required

Copy link
Contributor

@jldec jldec left a comment

Choose a reason for hiding this comment

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

Thank you @AlexTugarev
LGTM - tested as described in PR description and the notification works as advertised.

/hold in case you decide to pull back, based on discussion with @svenefftinge.

@svenefftinge
Copy link
Member

Anyone should see the status of usage vs limits when looking at the billing page - no need to start a workspace. But I shared my concerns and don't want to further block this if you think it's good.

Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

Anyone should see the status of usage vs limits when looking at the billing page - no need to start a workspace. But I shared my concerns and don't want to further block this if you think it's good.

I feel having (reactive) notifications like this can lift the UX to a new level, and I think we should aim for that. Notifications like this have been requested for a couple of usecases and the abstraction and mechanism solves the it nicely. Treating the paywall as a first "test driver" felt like the natural choice.

@AlexTugarev I left some cleanup comments, and a potential follow-up, but besides: Looks good to go! 👍

@AlexTugarev AlexTugarev force-pushed the at/update-notifications branch from b759be3 to 3c149cf Compare September 15, 2022 12:48
@AlexTugarev
Copy link
Member Author

Thanks for the review, @geropl! Changes applied. Build is running...

@AlexTugarev AlexTugarev force-pushed the at/update-notifications branch from 3c149cf to 721b359 Compare September 15, 2022 13:55
@AlexTugarev
Copy link
Member Author

Fixed compile errors after rebase merge conflicts.

@AlexTugarev AlexTugarev force-pushed the at/update-notifications branch from 721b359 to faa927e Compare September 15, 2022 13:57
@AlexTugarev
Copy link
Member Author

Typechecking Werft Typescript files
STDOUT: platform-delete-preview-environment.ts(3,10): error TS2305: Module '"./util/preview"' has no exported member 'HarvesterPreviewEnvironment'.
platform-delete-preview-environment.ts(3,39): error TS2305: Module '"./util/preview"' has no exported member 'PreviewEnvironment'.

looks unrelated. updated again.

@AlexTugarev
Copy link
Member Author

/hold cancel

@roboquat roboquat merged commit eb8e1d7 into main Sep 15, 2022
@roboquat roboquat deleted the at/update-notifications branch September 15, 2022 14:25
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Sep 16, 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 size/L team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spending limit notification is too sticky
5 participants