Skip to content

Commit 0f29293

Browse files
dannykoppingdannykopping
committed
Review feedback
Signed-off-by: Danny Kopping <danny@coder.com>
1 parent ba5f7c6 commit 0f29293

File tree

18 files changed

+420
-167
lines changed

18 files changed

+420
-167
lines changed

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717
"github.com/open-policy-agent/opa/topdown"
1818

1919
"cdr.dev/slog"
20+
2021
"github.com/coder/coder/v2/coderd/rbac/policy"
2122
"github.com/coder/coder/v2/coderd/rbac/rolestore"
2223

@@ -1471,6 +1472,13 @@ func (q *querier) GetLogoURL(ctx context.Context) (string, error) {
14711472
return q.db.GetLogoURL(ctx)
14721473
}
14731474

1475+
func (q *querier) GetNotificationMessagesByStatus(ctx context.Context, arg database.GetNotificationMessagesByStatusParams) ([]database.NotificationMessage, error) {
1476+
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceSystem); err != nil {
1477+
return nil, err
1478+
}
1479+
return q.db.GetNotificationMessagesByStatus(ctx, arg)
1480+
}
1481+
14741482
func (q *querier) GetOAuth2ProviderAppByID(ctx context.Context, id uuid.UUID) (database.OAuth2ProviderApp, error) {
14751483
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceOauth2App); err != nil {
14761484
return database.OAuth2ProviderApp{}, err

‎coderd/database/dbmem/dbmem.go

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -944,10 +944,10 @@ func (q *FakeQuerier) AcquireNotificationMessages(_ context.Context, arg databas
944944
}
945945

946946
// Mimic mutation in database query.
947-
nm.UpdatedAt = sql.NullTime{Time: time.Now(), Valid: true}
947+
nm.UpdatedAt = sql.NullTime{Time: dbtime.Now(), Valid: true}
948948
nm.Status = database.NotificationMessageStatusLeased
949949
nm.StatusReason = sql.NullString{String: fmt.Sprintf("Enqueued by notifier %d", arg.NotifierID), Valid: true}
950-
nm.LeasedUntil = sql.NullTime{Time: time.Now().Add(time.Second * time.Duration(arg.LeaseSeconds)), Valid: true}
950+
nm.LeasedUntil = sql.NullTime{Time: dbtime.Now().Add(time.Second * time.Duration(arg.LeaseSeconds)), Valid: true}
951951

952952
out = append(out, database.AcquireNotificationMessagesRow{
953953
ID: nm.ID,
@@ -1836,7 +1836,7 @@ func (q *FakeQuerier) EnqueueNotificationMessage(_ context.Context, arg database
18361836
Targets: arg.Targets,
18371837
CreatedBy: arg.CreatedBy,
18381838
// Default fields.
1839-
CreatedAt: time.Now(),
1839+
CreatedAt: dbtime.Now(),
18401840
Status: database.NotificationMessageStatusPending,
18411841
}
18421842

@@ -2740,6 +2740,26 @@ func (q *FakeQuerier) GetLogoURL(_ context.Context) (string, error) {
27402740
return q.logoURL, nil
27412741
}
27422742

2743+
func (q *FakeQuerier) GetNotificationMessagesByStatus(_ context.Context, arg database.GetNotificationMessagesByStatusParams) ([]database.NotificationMessage, error) {
2744+
err := validateDatabaseType(arg)
2745+
if err != nil {
2746+
return nil, err
2747+
}
2748+
2749+
var out []database.NotificationMessage
2750+
for _, m := range q.notificationMessages {
2751+
if len(out) > int(arg.Limit) {
2752+
return out, nil
2753+
}
2754+
2755+
if m.Status == arg.Status {
2756+
out = append(out, m)
2757+
}
2758+
}
2759+
2760+
return out, nil
2761+
}
2762+
27432763
func (q *FakeQuerier) GetOAuth2ProviderAppByID(_ context.Context, id uuid.UUID) (database.OAuth2ProviderApp, error) {
27442764
q.mutex.Lock()
27452765
defer q.mutex.Unlock()

‎coderd/database/dbmetrics/dbmetrics.go

Lines changed: 7 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/dbmock/dbmock.go

Lines changed: 15 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ CREATE INDEX idx_notification_messages_status ON notification_messages (status);
5252
-- TODO: autogenerate constants which reference the UUIDs
5353
INSERT INTO notification_templates (id, name, title_template, body_template, "group", actions)
5454
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}}**".',
55+
E'Hi {{.UserName}}\n\nYour workspace **{{.Labels.name}}** was deleted.\nThe specified reason was "**{{.Labels.reason}}{{ if .Labels.initiatedBy }} ({{ .Labels.initiatedBy }}){{end}}**".',
5656
'Workspace Events', '[
5757
{
5858
"label": "View workspaces",

‎coderd/database/querier.go

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries.sql.go

Lines changed: 47 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/notifications.sql

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,3 +125,6 @@ WHERE id IN
125125
FROM notification_messages AS nested
126126
WHERE nested.updated_at < NOW() - INTERVAL '7 days');
127127

128+
-- name: GetNotificationMessagesByStatus :many
129+
SELECT * FROM notification_messages WHERE status = @status LIMIT sqlc.arg('limit')::int;
130+

‎coderd/notifications/dispatch/smtp.go

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ import (
2020

2121
"cdr.dev/slog"
2222

23-
"github.com/coder/coder/v2/coderd/database"
2423
"github.com/coder/coder/v2/coderd/notifications/render"
2524
"github.com/coder/coder/v2/coderd/notifications/types"
2625
markdown "github.com/coder/coder/v2/coderd/render"
@@ -53,10 +52,6 @@ func NewSMTPHandler(cfg codersdk.NotificationsEmailConfig, log slog.Logger) *SMT
5352
return &SMTPHandler{cfg: cfg, log: log}
5453
}
5554

56-
func (*SMTPHandler) NotificationMethod() database.NotificationMethod {
57-
return database.NotificationMethodSmtp
58-
}
59-
6055
func (s *SMTPHandler) Dispatcher(payload types.MessagePayload, titleTmpl, bodyTmpl string) (DeliveryFunc, error) {
6156
// First render the subject & body into their own discrete strings.
6257
subject, err := markdown.PlaintextFromMarkdown(titleTmpl)

‎coderd/notifications/dispatch/webhook.go

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import (
1313

1414
"cdr.dev/slog"
1515

16-
"github.com/coder/coder/v2/coderd/database"
1716
"github.com/coder/coder/v2/coderd/notifications/types"
1817
markdown "github.com/coder/coder/v2/coderd/render"
1918
"github.com/coder/coder/v2/codersdk"
@@ -40,11 +39,6 @@ func NewWebhookHandler(cfg codersdk.NotificationsWebhookConfig, log slog.Logger)
4039
return &WebhookHandler{cfg: cfg, log: log, cl: &http.Client{}}
4140
}
4241

43-
func (*WebhookHandler) NotificationMethod() database.NotificationMethod {
44-
// TODO: don't use database types
45-
return database.NotificationMethodWebhook
46-
}
47-
4842
func (w *WebhookHandler) Dispatcher(payload types.MessagePayload, titleTmpl, bodyTmpl string) (DeliveryFunc, error) {
4943
if w.cfg.Endpoint.String() == "" {
5044
return nil, xerrors.New("webhook endpoint not defined")

‎coderd/notifications/manager.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,10 @@ import (
1616
"github.com/coder/coder/v2/codersdk"
1717
)
1818

19+
var (
20+
ErrInvalidDispatchTimeout = xerrors.New("dispatch timeout must be less than lease period")
21+
)
22+
1923
// Manager manages all notifications being enqueued and dispatched.
2024
//
2125
// Manager maintains a notifier: this consumes the queue of notification messages in the store.
@@ -53,6 +57,13 @@ type Manager struct {
5357
// helpers is a map of template helpers which are used to customize notification messages to use global settings like
5458
// access URL etc.
5559
func NewManager(cfg codersdk.NotificationsConfig, store Store, log slog.Logger) (*Manager, error) {
60+
// If dispatch timeout exceeds lease period, it is possible that messages can be delivered in duplicate because the
61+
// lease can expire before the notifier gives up on the dispatch, which results in the message becoming eligible for
62+
// being re-acquired.
63+
if cfg.DispatchTimeout.Value() >= cfg.LeasePeriod.Value() {
64+
return nil, ErrInvalidDispatchTimeout
65+
}
66+
5667
return &Manager{
5768
log: log,
5869
cfg: cfg,
@@ -82,6 +93,8 @@ func (m *Manager) WithHandlers(reg map[database.NotificationMethod]Handler) {
8293
// Manager requires system-level permissions to interact with the store.
8394
// Run is only intended to be run once.
8495
func (m *Manager) Run(ctx context.Context) {
96+
m.log.Info(ctx, "started")
97+
8598
m.runOnce.Do(func() {
8699
// Closes when Stop() is called or context is canceled.
87100
go func() {

‎coderd/notifications/manager_test.go

Lines changed: 16 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55
"encoding/json"
66
"sync/atomic"
77
"testing"
8-
"time"
98

109
"github.com/google/uuid"
1110
"github.com/stretchr/testify/assert"
@@ -15,8 +14,8 @@ import (
1514
"cdr.dev/slog"
1615
"cdr.dev/slog/sloggers/slogtest"
1716

18-
"github.com/coder/coder/v2/coderd/coderdtest"
1917
"github.com/coder/coder/v2/coderd/database"
18+
"github.com/coder/coder/v2/coderd/database/dbgen"
2019
"github.com/coder/coder/v2/coderd/database/dbmem"
2120
"github.com/coder/coder/v2/coderd/database/dbtestutil"
2221
"github.com/coder/coder/v2/coderd/notifications"
@@ -32,7 +31,7 @@ func TestBufferedUpdates(t *testing.T) {
3231
if !dbtestutil.WillUsePostgres() {
3332
t.Skip("This test requires postgres")
3433
}
35-
ctx, logger, db, ps := setup(t)
34+
ctx, logger, db := setup(t)
3635
interceptor := &bulkUpdateInterceptor{Store: db}
3736

3837
santa := &santaHandler{}
@@ -45,19 +44,15 @@ func TestBufferedUpdates(t *testing.T) {
4544
enq, err := notifications.NewStoreEnqueuer(cfg, interceptor, defaultHelpers(), logger.Named("notifications-enqueuer"))
4645
require.NoError(t, err)
4746

48-
client := coderdtest.New(t, &coderdtest.Options{Database: db, Pubsub: ps})
49-
user := coderdtest.CreateFirstUser(t, client)
47+
user := dbgen.User(t, db, database.User{})
5048

5149
// given
52-
if _, err := enq.Enqueue(ctx, user.UserID, notifications.TemplateWorkspaceDeleted, map[string]string{"nice": "true"}, ""); true {
53-
require.NoError(t, err)
54-
}
55-
if _, err := enq.Enqueue(ctx, user.UserID, notifications.TemplateWorkspaceDeleted, map[string]string{"nice": "true"}, ""); true {
56-
require.NoError(t, err)
57-
}
58-
if _, err := enq.Enqueue(ctx, user.UserID, notifications.TemplateWorkspaceDeleted, map[string]string{"nice": "false"}, ""); true {
59-
require.NoError(t, err)
60-
}
50+
_, err = enq.Enqueue(ctx, user.ID, notifications.TemplateWorkspaceDeleted, map[string]string{"nice": "true"}, "")
51+
require.NoError(t, err)
52+
_, err = enq.Enqueue(ctx, user.ID, notifications.TemplateWorkspaceDeleted, map[string]string{"nice": "true"}, "")
53+
require.NoError(t, err)
54+
_, err = enq.Enqueue(ctx, user.ID, notifications.TemplateWorkspaceDeleted, map[string]string{"nice": "false"}, "")
55+
require.NoError(t, err)
6156

6257
// when
6358
mgr.Run(ctx)
@@ -94,7 +89,6 @@ func TestBuildPayload(t *testing.T) {
9489
"my_url": func() string { return url },
9590
}
9691

97-
ctx := context.Background()
9892
db := dbmem.New()
9993
interceptor := newEnqueueInterceptor(db,
10094
// Inject custom message metadata to influence the payload construction.
@@ -107,7 +101,7 @@ func TestBuildPayload(t *testing.T) {
107101
},
108102
}
109103
out, err := json.Marshal(actions)
110-
require.NoError(t, err)
104+
assert.NoError(t, err)
111105

112106
return database.FetchNewMessageMetadataRow{
113107
NotificationName: "My Notification",
@@ -122,19 +116,17 @@ func TestBuildPayload(t *testing.T) {
122116
enq, err := notifications.NewStoreEnqueuer(defaultNotificationsConfig(database.NotificationMethodSmtp), interceptor, helpers, logger.Named("notifications-enqueuer"))
123117
require.NoError(t, err)
124118

119+
ctx := testutil.Context(t, testutil.WaitShort)
120+
125121
// when
126122
_, err = enq.Enqueue(ctx, uuid.New(), notifications.TemplateWorkspaceDeleted, nil, "test")
127123
require.NoError(t, err)
128124

129125
// then
130-
select {
131-
case payload := <-interceptor.payload:
132-
require.Len(t, payload.Actions, 1)
133-
require.Equal(t, label, payload.Actions[0].Label)
134-
require.Equal(t, url, payload.Actions[0].URL)
135-
case <-time.After(testutil.WaitShort):
136-
t.Fatalf("timed out")
137-
}
126+
payload := testutil.RequireRecvCtx(ctx, t, interceptor.payload)
127+
require.Len(t, payload.Actions, 1)
128+
require.Equal(t, label, payload.Actions[0].Label)
129+
require.Equal(t, url, payload.Actions[0].URL)
138130
}
139131

140132
func TestStopBeforeRun(t *testing.T) {
@@ -184,10 +176,6 @@ type santaHandler struct {
184176
nice atomic.Int32
185177
}
186178

187-
func (*santaHandler) NotificationMethod() database.NotificationMethod {
188-
return database.NotificationMethodSmtp
189-
}
190-
191179
func (s *santaHandler) Dispatcher(payload types.MessagePayload, _, _ string) (dispatch.DeliveryFunc, error) {
192180
return func(ctx context.Context, msgID uuid.UUID) (retryable bool, err error) {
193181
if payload.Labels["nice"] != "true" {

0 commit comments

Comments
 (0)