-
Notifications
You must be signed in to change notification settings - Fork 874
feat: notify about created user account #14010
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
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.
Looking good! Mostly small comments and nits.
🤖 Meticulous spotted visual differences in 11 of 1069 screens tested: view and approve differences detected. Last updated for commit 77c0c28. This comment will update as new commits are pushed. |
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.
LGTM, with non-blocking nit 👍
require.Contains(t, notifyEnq.Sent[0].Targets, workspace.OrganizationID) | ||
require.Contains(t, notifyEnq.Sent[0].Targets, workspace.OwnerID) | ||
require.Len(t, notifyEnq.Sent, 2) | ||
// notifyEnq.Sent[0] is an event for created user account |
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.
Nit: would be safer to validate this
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.
I left it like this on purpose, this is not the aim of this test. Less chance that it will require refining if we alter something in Notifications Enqueuer logic
Related: coder/internal#17
This PR extends
api.CreateUser
to notify owners and user admins when somebody manually creates a new user.