Skip to content

Commit 40e5801

Browse files
BrunoQuaresmaBrunoQuaresma
committed
Apply Danny suggestions
1 parent fbf99be commit 40e5801

File tree

8 files changed

+123
-136
lines changed

8 files changed

+123
-136
lines changed

‎coderd/database/migrations/000221_notifications.up.sql

Lines changed: 13 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -50,35 +50,16 @@ CREATE TABLE notification_messages
5050
CREATE INDEX idx_notification_messages_status ON notification_messages (status);
5151

5252
-- TODO: autogenerate constants which reference the UUIDs
53-
INSERT INTO notification_templates
54-
(id, name, title_template, body_template, "group", actions)
55-
VALUES
56-
-- Workspace Deleted
57-
('f517da0b-cdc9-410f-ab89-a86107c420ed',
58-
'Workspace Deleted',
59-
E'Workspace "{{.Labels.name}}" deleted',
60-
E'Hi {{.UserName}}\n\nYour workspace **{{.Labels.name}}** was deleted.\nThe specified reason was "**{{.Labels.reason}}{{ if .Labels.initiator }} ({{ .Labels.initiator }}){{end}}**".',
61-
'Workspace Events',
62-
'[
63-
{
64-
"label": "View workspaces",
65-
"url": "{{ base_url }}/workspaces"
66-
},
67-
{
68-
"label": "View templates",
69-
"url": "{{ base_url }}/templates"
70-
}
71-
]'::jsonb),
72-
-- Workspace Marked as Dormant
73-
('123e4567-e89b-12d3-a456-426614174000',
74-
'Workspace Marked as Dormant',
75-
E'Workspace "{{.Labels.name}}" marked as dormant',
76-
E'Hi {{.UserName}}\n\nYour workspace **{{.Labels.name}}** has been marked as dormant.\nThe specified reason was "**{{.Labels.reason}}{{ if .Labels.initiator }} ({{ .Labels.initiator }}){{end}}**',
77-
'Workspace Events',
78-
'[
79-
{
80-
"label": "View workspace",
81-
"url": "{{ base_url }}/@{{.UserName}}/{{.Labels.name}}"
82-
},
83-
]'::jsonb);
84-
53+
INSERT INTO notification_templates (id, name, title_template, body_template, "group", actions)
54+
VALUES ('f517da0b-cdc9-410f-ab89-a86107c420ed', 'Workspace Deleted', E'Workspace "{{.Labels.name}}" deleted',
55+
E'Hi {{.UserName}}\n\nYour workspace **{{.Labels.name}}** was deleted.\nThe specified reason was "**{{.Labels.reason}}{{ if .Labels.initiator }} ({{ .Labels.initiator }}){{end}}**".',
56+
'Workspace Events', '[
57+
{
58+
"label": "View workspaces",
59+
"url": "{{ base_url }}/workspaces"
60+
},
61+
{
62+
"label": "View templates",
63+
"url": "{{ base_url }}/templates"
64+
}
65+
]'::jsonb);

‎coderd/database/migrations/000223_dormancy_notification_template.down.sql

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
DELETE notification_templates WHERE id = '123e4567-e89b-12d3-a456-426614174000';

‎coderd/database/migrations/000223_dormancy_notification_template.up.sql

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
-- TODO: autogenerate constants which reference the UUIDs
2+
INSERT INTO notification_templates
3+
(id, name, title_template, body_template, "group", actions)
4+
VALUES
5+
-- Workspace Marked as Dormant
6+
('123e4567-e89b-12d3-a456-426614174000',
7+
'Workspace Marked as Dormant',
8+
E'Workspace "{{.Labels.name}}" marked as dormant',
9+
E'Hi {{.UserName}}\n\nYour workspace **{{.Labels.name}}** has been marked as dormant.\nThe specified reason was "**{{.Labels.reason}}{{ if .Labels.initiator }} ({{ .Labels.initiator }}){{end}}**',
10+
'Workspace Events',
11+
'[
12+
{
13+
"label": "View workspace",
14+
"url": "{{ base_url }}/@{{.UserName}}/{{.Labels.name}}"
15+
},
16+
]'::jsonb);

‎coderd/provisionerdserver/provisionerdserver_test.go

Lines changed: 9 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -1602,7 +1602,7 @@ func TestNotifications(t *testing.T) {
16021602
t.Parallel()
16031603

16041604
ctx := context.Background()
1605-
notifEnq := &fakeNotificationEnqueuer{}
1605+
notifEnq := &testutil.FakeNotificationEnqueuer{}
16061606

16071607
srv, db, ps, pd := setup(t, false, &overrides{
16081608
notificationEnqueuer: notifEnq,
@@ -1680,17 +1680,17 @@ func TestNotifications(t *testing.T) {
16801680

16811681
if tc.shouldNotify {
16821682
// Validate that the notification was sent and contained the expected values.
1683-
require.Len(t, notifEnq.sent, 1)
1684-
require.Equal(t, notifEnq.sent[0].userID, user.ID)
1685-
require.Contains(t, notifEnq.sent[0].targets, template.ID)
1686-
require.Contains(t, notifEnq.sent[0].targets, workspace.ID)
1687-
require.Contains(t, notifEnq.sent[0].targets, workspace.OrganizationID)
1688-
require.Contains(t, notifEnq.sent[0].targets, user.ID)
1683+
require.Len(t, notifEnq.Sent, 1)
1684+
require.Equal(t, notifEnq.Sent[0].UserID, user.ID)
1685+
require.Contains(t, notifEnq.Sent[0].Targets, template.ID)
1686+
require.Contains(t, notifEnq.Sent[0].Targets, workspace.ID)
1687+
require.Contains(t, notifEnq.Sent[0].Targets, workspace.OrganizationID)
1688+
require.Contains(t, notifEnq.Sent[0].Targets, user.ID)
16891689
if tc.deletionReason == database.BuildReasonInitiator {
1690-
require.Equal(t, notifEnq.sent[0].labels["initiatedBy"], initiator.Username)
1690+
require.Equal(t, notifEnq.Sent[0].Labels["initiatedBy"], initiator.Username)
16911691
}
16921692
} else {
1693-
require.Len(t, notifEnq.sent, 0)
1693+
require.Len(t, notifEnq.Sent, 0)
16941694
}
16951695
})
16961696
}
@@ -1919,31 +1919,3 @@ func (s *fakeStream) cancel() {
19191919
s.canceled = true
19201920
s.c.Broadcast()
19211921
}
1922-
1923-
type fakeNotificationEnqueuer struct {
1924-
mu sync.Mutex
1925-
sent []*notification
1926-
}
1927-
1928-
type notification struct {
1929-
userID, templateID uuid.UUID
1930-
labels map[string]string
1931-
createdBy string
1932-
targets []uuid.UUID
1933-
}
1934-
1935-
func (f *fakeNotificationEnqueuer) Enqueue(_ context.Context, userID, templateID uuid.UUID, labels map[string]string, createdBy string, targets ...uuid.UUID) (*uuid.UUID, error) {
1936-
f.mu.Lock()
1937-
defer f.mu.Unlock()
1938-
1939-
f.sent = append(f.sent, &notification{
1940-
userID: userID,
1941-
templateID: templateID,
1942-
labels: labels,
1943-
createdBy: createdBy,
1944-
targets: targets,
1945-
})
1946-
1947-
id := uuid.New()
1948-
return &id, nil
1949-
}

‎coderd/workspaces.go

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ import (
2323
"github.com/coder/coder/v2/coderd/database/dbauthz"
2424
"github.com/coder/coder/v2/coderd/database/dbtime"
2525
"github.com/coder/coder/v2/coderd/database/provisionerjobs"
26-
"github.com/coder/coder/v2/coderd/dormancy"
2726
"github.com/coder/coder/v2/coderd/httpapi"
2827
"github.com/coder/coder/v2/coderd/httpmw"
2928
"github.com/coder/coder/v2/coderd/rbac"
@@ -35,6 +34,7 @@ import (
3534
"github.com/coder/coder/v2/coderd/wsbuilder"
3635
"github.com/coder/coder/v2/codersdk"
3736
"github.com/coder/coder/v2/codersdk/agentsdk"
37+
"github.com/coder/coder/v2/enterprise/coderd/dormancy"
3838
)
3939

4040
var (
@@ -957,21 +957,18 @@ func (api *API) putWorkspaceDormant(rw http.ResponseWriter, r *http.Request) {
957957
if req.Dormant && apiKey.UserID != workspace.OwnerID {
958958
initiator, err := api.Database.GetUserByID(ctx, apiKey.UserID)
959959
if err != nil {
960-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
961-
Message: "Internal error fetching user.",
962-
Detail: err.Error(),
963-
})
960+
api.Logger.Warn(ctx, "failed to get the initiator by id", slog.Error(err))
964961
return
965962
}
966963
dormancy.NotifyWorkspaceDormant(
967964
ctx,
968965
api.Logger,
969966
api.NotificationsEnqueuer,
970967
dormancy.WorkspaceDormantNotification{
971-
Workspace: workspace,
972-
InitiatedBy: initiator.Username,
973-
Reason: "requested by user",
974-
CreatedBy: "api",
968+
Workspace: workspace,
969+
Initiator: initiator.Username,
970+
Reason: "requested by user",
971+
CreatedBy: "api",
975972
},
976973
)
977974
}

‎coderd/workspaces_test.go

Lines changed: 28 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import (
1111
"os"
1212
"slices"
1313
"strings"
14-
"sync"
1514
"testing"
1615
"time"
1716

@@ -3516,8 +3515,9 @@ func TestNotifications(t *testing.T) {
35163515
t.Run("InitiatorNotOwner", func(t *testing.T) {
35173516
t.Parallel()
35183517

3518+
// Given
35193519
var (
3520-
notifyEnq = &fakeNotificationEnqueuer{}
3520+
notifyEnq = &testutil.FakeNotificationEnqueuer{}
35213521
client = coderdtest.New(t, &coderdtest.Options{
35223522
IncludeProvisionerDaemon: true,
35233523
NotificationEnqueuer: notifyEnq,
@@ -3532,25 +3532,30 @@ func TestNotifications(t *testing.T) {
35323532
)
35333533

35343534
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
3535-
defer cancel()
3535+
t.Cleanup(cancel)
35363536

3537+
// When
35373538
err := memberClient.UpdateWorkspaceDormancy(ctx, workspace.ID, codersdk.UpdateWorkspaceDormancy{
35383539
Dormant: true,
35393540
})
3541+
3542+
// Then
35403543
require.NoError(t, err, "mark workspace as dormant")
3541-
require.Len(t, notifyEnq.sent, 1)
3542-
require.Equal(t, notifyEnq.sent[0].userID, workspace.OwnerID)
3543-
require.Contains(t, notifyEnq.sent[0].targets, template.ID)
3544-
require.Contains(t, notifyEnq.sent[0].targets, workspace.ID)
3545-
require.Contains(t, notifyEnq.sent[0].targets, workspace.OrganizationID)
3546-
require.Contains(t, notifyEnq.sent[0].targets, workspace.OwnerID)
3547-
require.Equal(t, notifyEnq.sent[0].labels["initiatedBy"], member.Username)
3544+
require.Len(t, notifyEnq.Sent, 1)
3545+
require.Equal(t, notifyEnq.Sent[0].UserID, workspace.OwnerID)
3546+
require.Contains(t, notifyEnq.Sent[0].Targets, template.ID)
3547+
require.Contains(t, notifyEnq.Sent[0].Targets, workspace.ID)
3548+
require.Contains(t, notifyEnq.Sent[0].Targets, workspace.OrganizationID)
3549+
require.Contains(t, notifyEnq.Sent[0].Targets, workspace.OwnerID)
3550+
require.Equal(t, notifyEnq.Sent[0].Labels["initiatedBy"], member.Username)
35483551
})
35493552

35503553
t.Run("InitiatorIsOwner", func(t *testing.T) {
35513554
t.Parallel()
3555+
3556+
// Given
35523557
var (
3553-
notifyEnq = &fakeNotificationEnqueuer{}
3558+
notifyEnq = &testutil.FakeNotificationEnqueuer{}
35543559
client = coderdtest.New(t, &coderdtest.Options{
35553560
IncludeProvisionerDaemon: true,
35563561
NotificationEnqueuer: notifyEnq,
@@ -3564,20 +3569,24 @@ func TestNotifications(t *testing.T) {
35643569
)
35653570

35663571
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
3567-
defer cancel()
3572+
t.Cleanup(cancel)
35683573

3574+
// When
35693575
err := client.UpdateWorkspaceDormancy(ctx, workspace.ID, codersdk.UpdateWorkspaceDormancy{
35703576
Dormant: true,
35713577
})
3578+
3579+
// Then
35723580
require.NoError(t, err, "mark workspace as dormant")
3573-
require.Len(t, notifyEnq.sent, 0)
3581+
require.Len(t, notifyEnq.Sent, 0)
35743582
})
35753583

35763584
t.Run("ActivateDormantWorkspace", func(t *testing.T) {
35773585
t.Parallel()
35783586

3587+
// Given
35793588
var (
3580-
notifyEnq = &fakeNotificationEnqueuer{}
3589+
notifyEnq = &testutil.FakeNotificationEnqueuer{}
35813590
client = coderdtest.New(t, &coderdtest.Options{
35823591
IncludeProvisionerDaemon: true,
35833592
NotificationEnqueuer: notifyEnq,
@@ -3590,8 +3599,9 @@ func TestNotifications(t *testing.T) {
35903599
_ = coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID)
35913600
)
35923601

3602+
// When
35933603
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
3594-
defer cancel()
3604+
t.Cleanup(cancel)
35953605

35963606
// Make workspace dormant before activate it
35973607
err := client.UpdateWorkspaceDormancy(ctx, workspace.ID, codersdk.UpdateWorkspaceDormancy{
@@ -3600,46 +3610,13 @@ func TestNotifications(t *testing.T) {
36003610
require.NoError(t, err, "mark workspace as dormant")
36013611
// Clear notifications before activating the workspace
36023612
notifyEnq.Clear()
3613+
3614+
// Then
36033615
err = client.UpdateWorkspaceDormancy(ctx, workspace.ID, codersdk.UpdateWorkspaceDormancy{
36043616
Dormant: false,
36053617
})
36063618
require.NoError(t, err, "mark workspace as active")
3607-
require.Len(t, notifyEnq.sent, 0)
3619+
require.Len(t, notifyEnq.Sent, 0)
36083620
})
36093621
})
36103622
}
3611-
3612-
type fakeNotificationEnqueuer struct {
3613-
mu sync.Mutex
3614-
sent []*notification
3615-
}
3616-
3617-
type notification struct {
3618-
userID, templateID uuid.UUID
3619-
labels map[string]string
3620-
createdBy string
3621-
targets []uuid.UUID
3622-
}
3623-
3624-
func (f *fakeNotificationEnqueuer) Enqueue(_ context.Context, userID, templateID uuid.UUID, labels map[string]string, createdBy string, targets ...uuid.UUID) (*uuid.UUID, error) {
3625-
f.mu.Lock()
3626-
defer f.mu.Unlock()
3627-
3628-
f.sent = append(f.sent, &notification{
3629-
userID: userID,
3630-
templateID: templateID,
3631-
labels: labels,
3632-
createdBy: createdBy,
3633-
targets: targets,
3634-
})
3635-
3636-
id := uuid.New()
3637-
return &id, nil
3638-
}
3639-
3640-
func (f *fakeNotificationEnqueuer) Clear() {
3641-
f.mu.Lock()
3642-
defer f.mu.Unlock()
3643-
3644-
f.sent = nil
3645-
}

‎coderd/dormancy/notifications.go renamed to ‎enterprise/coderd/dormancy/notifications.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,10 @@ import (
99
)
1010

1111
type WorkspaceDormantNotification struct {
12-
Workspace database.Workspace
13-
InitiatedBy string
14-
Reason string
15-
CreatedBy string
12+
Workspace database.Workspace
13+
Initiator string
14+
Reason string
15+
CreatedBy string
1616
}
1717

1818
func NotifyWorkspaceDormant(
@@ -22,9 +22,9 @@ func NotifyWorkspaceDormant(
2222
notification WorkspaceDormantNotification,
2323
) {
2424
labels := map[string]string{
25-
"name": notification.Workspace.Name,
26-
"initiatedBy": notification.InitiatedBy,
27-
"reason": notification.Reason,
25+
"name": notification.Workspace.Name,
26+
"initiator": notification.Initiator,
27+
"reason": notification.Reason,
2828
}
2929
if _, err := enqueuer.Enqueue(
3030
ctx,

‎testutil/notifications.go

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
package testutil
2+
3+
import (
4+
"context"
5+
"sync"
6+
7+
"github.com/google/uuid"
8+
)
9+
10+
type FakeNotificationEnqueuer struct {
11+
mu sync.Mutex
12+
Sent []*Notification
13+
}
14+
15+
type Notification struct {
16+
UserID, TemplateID uuid.UUID
17+
Labels map[string]string
18+
CreatedBy string
19+
Targets []uuid.UUID
20+
}
21+
22+
func (f *FakeNotificationEnqueuer) Enqueue(_ context.Context, userID, templateID uuid.UUID, labels map[string]string, createdBy string, targets ...uuid.UUID) (*uuid.UUID, error) {
23+
f.mu.Lock()
24+
defer f.mu.Unlock()
25+
26+
f.Sent = append(f.Sent, &Notification{
27+
UserID: userID,
28+
TemplateID: templateID,
29+
Labels: labels,
30+
CreatedBy: createdBy,
31+
Targets: targets,
32+
})
33+
34+
id := uuid.New()
35+
return &id, nil
36+
}
37+
38+
func (f *FakeNotificationEnqueuer) Clear() {
39+
f.mu.Lock()
40+
defer f.mu.Unlock()
41+
42+
f.Sent = nil
43+
}

0 commit comments

Comments
 (0)