- Notifications
You must be signed in to change notification settings - Fork952
feat: add template delete notification#14250
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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, but we need to send to all template admins & owners.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Almost there 👍
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
coderd/templates_test.go Outdated
| require.NoError(t, err) | ||
| // Then: the template owners and template admins should receive the notification | ||
| shouldBeNotified := []uuid.UUID{anotherOwner.ID, tmplAdmin.ID} |
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 think it'd be good to explicitly specify that the first user is not receiving the notification because they initiated it.
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 think it'd also be a good idea here to create another admin but with a different role likeuser admin, and validate that they don't receive the notification either.
Uh oh!
There was an error while loading.Please reload this page.
coderd/templates_test.go Outdated
| func TestNotifyDeletedTemplate(t *testing.T) { | ||
| t.Parallel() | ||
| t.Run("OnlyNotifyOwnersAndTemplateAdmins", func(t *testing.T) { |
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.
Please add a separate test to validate the functionality in isolation; you can then reduce some complexity in this test and keep it nicely scoped.
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.
Not sure if I got it. What should I separate and what complexity does it solve?
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.
Almost there! Thanks for taking the feedback onboard so well
coderd/templates_test.go Outdated
| @@ -1334,9 +1334,50 @@ func TestTemplateNotifications(t *testing.T) { | |||
| t.Run("Delete", func(t *testing.T) { | |||
| t.Parallel() | |||
| t.Run("SendNotification", func(t *testing.T) { | |||
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.
Now this is a great test! Needs a little work but it's very nice.
Doesn't this feel better to read now?
coderd/templates_test.go Outdated
| _, anotherOwner := coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.RoleOwner()) | ||
| _, tmplAdmin := coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.RoleTemplateAdmin()) | ||
| coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.RoleMember()) | ||
| coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.RoleUserAdmin()) | ||
| // When: template is deleted | ||
| // When:thetemplate is deleted by the owner |
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.
Which owner? You have the first user, which is performing this action, andanotherOwner.
| // When: the template is deleted bythe owner | |
| // When: the template is deleted byfirstUser |
coderd/templates_test.go Outdated
| deleteNotifications := make([]*testutil.Notification, 0) | ||
| for _, n := range notifyEnq.Sent { | ||
| if n.TemplateID == notifications.TemplateTemplateDeleted { | ||
| deleteNotifications = append(deleteNotifications, n) | ||
| } | ||
| } | ||
| require.Len(t, deleteNotifications, 1) | ||
| require.Contains(t, deleteNotifications[0].TemplateID, notifications.TemplateTemplateDeleted) | ||
| require.Contains(t, deleteNotifications[0].UserID, templateAdmin.ID) | ||
| require.Contains(t, deleteNotifications[0].Targets, template.ID) | ||
| require.Contains(t, deleteNotifications[0].Targets, template.OrganizationID) | ||
| require.Equal(t, deleteNotifications[0].Labels["name"], template.Name) | ||
| require.Equal(t, deleteNotifications[0].Labels["initiator"], coderdtest.FirstUserParams.Username) |
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 think you could simplify this to show that the notification is not received at all in this test if there's only one owner.
In the other test you're checking for exactly the same thing.
Tests should validate one (or as close to one) aspect of business logic at a time, IMHO.
coderd/templates_test.go Outdated
| t.Run("Delete", func(t *testing.T) { | ||
| t.Parallel() | ||
| t.Run("SendNotification", func(t *testing.T) { |
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.
| t.Run("SendNotification",func(t*testing.T) { | |
| t.Run("InitiatorIsNotNotified",func(t*testing.T) { |
6f1951e intomainUh oh!
There was an error while loading.Please reload this page.
Preview:
Related to:coder/internal#16