fix: publishing multiple notifications

leg100 · May 17, 2023 · 96f9a85 · 96f9a85
1 parent 220cb3f
commit 96f9a85
Show file tree

Hide file tree

Showing 4 changed files with 40 additions and 13 deletions.
diff --git a/internal/notifications/cache_test.go b/internal/notifications/cache_test.go
@@ -8,9 +8,9 @@ import (
 )
 
 func TestCache_New(t *testing.T) {
-	nc1 := newTestConfig(t, DestinationSlack, "http://example.com")
-	nc2 := newTestConfig(t, DestinationSlack, "http://example.com")
-	nc3 := newTestConfig(t, DestinationGCPPubSub, "gcppubsub://project1/topic1")
+	nc1 := newTestConfig(t, "", DestinationSlack, "http://example.com")
+	nc2 := newTestConfig(t, "", DestinationSlack, "http://example.com")
+	nc3 := newTestConfig(t, "", DestinationGCPPubSub, "gcppubsub://project1/topic1")
 
 	cache := newTestCache(t, nil, nc1, nc2, nc3)
 
@@ -20,8 +20,8 @@ func TestCache_New(t *testing.T) {
 
 func TestCache_AddRemove(t *testing.T) {
 	cache := newTestCache(t, nil)
-	nc1 := newTestConfig(t, DestinationSlack, "http://example.com")
-	nc2 := newTestConfig(t, DestinationSlack, "http://example.com")
+	nc1 := newTestConfig(t, "", DestinationSlack, "http://example.com")
+	nc2 := newTestConfig(t, "", DestinationSlack, "http://example.com")
 
 	err := cache.add(nc1)
 	require.NoError(t, err)

diff --git a/internal/notifications/notifier.go b/internal/notifications/notifier.go
@@ -125,6 +125,7 @@ func (s *notifier) handleRun(ctx context.Context, r *run.Run) error {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
+	var ws *workspace.Workspace
 	for _, cfg := range s.configs {
 		if cfg.WorkspaceID != r.WorkspaceID {
 			// skip configs for other workspaces
@@ -143,15 +144,18 @@ func (s *notifier) handleRun(ctx context.Context, r *run.Run) error {
 			// skip config with no matching trigger
 			continue
 		}
-		// retrieve workspace because client might want to provide workspace
-		// info in the notification
+		// Retrieve workspace if not already retrieved. We do this in order to
+		// furnish the notification below with further information.
 		//
 		// TODO: this is rather expensive. We should either:
 		// (a) cache workspaces, either in the notifier or upstream; or
 		// (b) add workspace info to run itself
-		ws, err := s.GetWorkspace(ctx, r.WorkspaceID)
-		if err != nil {
-			return err
+		if ws == nil {
+			var err error
+			ws, err = s.GetWorkspace(ctx, r.WorkspaceID)
+			if err != nil {
+				return err
+			}
 		}
 		client, ok := s.clients[*cfg.URL]
 		if !ok {
@@ -166,7 +170,9 @@ func (s *notifier) handleRun(ctx context.Context, r *run.Run) error {
 			hostname:  s.Hostname(),
 		}
 		s.V(3).Info("publishing notification", "notification", msg)
-		return client.Publish(ctx, msg)
+		if err := client.Publish(ctx, msg); err != nil {
+			return err
+		}
 	}
 	return nil
 }
diff --git a/internal/notifications/notifier_test.go b/internal/notifications/notifier_test.go
@@ -78,3 +78,23 @@ func TestNotifier_handleRun(t *testing.T) {
 		})
 	}
 }
+
+// TestNotifier_handleRun_multiple tests handleRun() publishing multiple
+// notifications
+func TestNotifier_handleRun_multiple(t *testing.T) {
+	ctx := context.Background()
+	planningRun := &run.Run{
+		Status:      internal.RunPlanning,
+		WorkspaceID: "ws-123",
+	}
+	config1 := newTestConfig(t, "ws-123", DestinationGCPPubSub, "", TriggerPlanning)
+	config2 := newTestConfig(t, "ws-123", DestinationSlack, "", TriggerPlanning)
+
+	published := make(chan *run.Run, 2)
+	notifier := newTestNotifier(t, &fakeFactory{published}, config1, config2)
+
+	err := notifier.handleRun(ctx, planningRun)
+	require.NoError(t, err)
+	assert.Equal(t, planningRun, <-published)
+	assert.Equal(t, planningRun, <-published)
+}
diff --git a/internal/notifications/test_helpers.go b/internal/notifications/test_helpers.go
@@ -52,12 +52,13 @@ func newTestCache(t *testing.T, f clientFactory, configs ...*Config) *cache {
 	return cache
 }
 
-func newTestConfig(t *testing.T, dst Destination, url string) *Config {
-	cfg, err := NewConfig(uuid.NewString(), CreateConfigOptions{
+func newTestConfig(t *testing.T, workspaceID string, dst Destination, url string, triggers ...Trigger) *Config {
+	cfg, err := NewConfig(workspaceID, CreateConfigOptions{
 		Name:            internal.String(uuid.NewString()),
 		DestinationType: dst,
 		Enabled:         internal.Bool(true),
 		URL:             internal.String(url),
+		Triggers:        triggers,
 	})
 	require.NoError(t, err)
 	return cfg