Skip to content

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

Merged
merged 26 commits into from
Jul 30, 2024
Merged

feat: notify about created user account #14010

merged 26 commits into from
Jul 30, 2024

Conversation

mtojek
Copy link
Member

@mtojek mtojek commented Jul 25, 2024

Related: coder/internal#17

This PR extends api.CreateUser to notify owners and user admins when somebody manually creates a new user.

@mtojek mtojek self-assigned this Jul 25, 2024
@mtojek mtojek marked this pull request as ready for review July 29, 2024 14:35
@mtojek mtojek requested a review from dannykopping July 29, 2024 14:35
Copy link
Contributor

@dannykopping dannykopping left a 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.

Copy link

alwaysmeticulous bot commented Jul 30, 2024

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

@mtojek mtojek requested a review from dannykopping July 30, 2024 12:06
Copy link
Contributor

@dannykopping dannykopping left a 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
Copy link
Contributor

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

Copy link
Member Author

@mtojek mtojek Jul 30, 2024

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

@mtojek mtojek merged commit cf1fcab into main Jul 30, 2024
29 of 30 checks passed
@mtojek mtojek deleted the 17-account-notifs branch July 30, 2024 13:37
@github-actions github-actions bot locked and limited conversation to collaborators Jul 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants