feat: add template delete notification - #14250
Conversation
dannykopping
left a comment
There was a problem hiding this comment.
Looking good, but we need to send to all template admins & owners.
| 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.
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.
I think it'd also be a good idea here to create another admin but with a different role like user admin, and validate that they don't receive the notification either.
| func TestNotifyDeletedTemplate(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| t.Run("OnlyNotifyOwnersAndTemplateAdmins", func(t *testing.T) { |
There was a problem hiding this comment.
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.
Not sure if I got it. What should I separate and what complexity does it solve?
dannykopping
left a comment
There was a problem hiding this comment.
Almost there! Thanks for taking the feedback onboard so well
| t.Run("Delete", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| t.Run("SendNotification", func(t *testing.T) { |
There was a problem hiding this comment.
Now this is a great test! Needs a little work but it's very nice.
Doesn't this feel better to read now?
| coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.RoleUserAdmin()) | ||
|
|
||
| // When: template is deleted | ||
| // When: the template is deleted by the owner |
There was a problem hiding this comment.
Which owner? You have the first user, which is performing this action, and anotherOwner.
| // When: the template is deleted by the owner | |
| // When: the template is deleted by firstUser |
| 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.
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.
| t.Run("Delete", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| t.Run("SendNotification", func(t *testing.T) { |
There was a problem hiding this comment.
| t.Run("SendNotification", func(t *testing.T) { | |
| t.Run("InitiatorIsNotNotified", func(t *testing.T) { |
Preview:
Related to: coder/internal#16