Merge pull request #1879 from target/slack-oncall-multichan

engine/schedulemanager: fix sending messages to multiple Slack channels at the same time

Author: mastercactapus

engine/schedulemanager/update.go

@@ -5,6 +5,7 @@ import (
 	"database/sql"
 	"encoding/json"
 	"fmt"
+	"sort"
 	"time"
 
 	"github.com/google/uuid"
@@ -234,7 +235,7 @@ func (db *DB) update(ctx context.Context) error {
 	}
 
 	// Notify changed schedules
-	needsOnCallNotification := make(map[string]uuid.UUID)
+	needsOnCallNotification := make(map[string][]uuid.UUID)
 	for schedID := range changedSchedules {
 		data := scheduleData[schedID]
 		if data == nil {
@@ -245,15 +246,15 @@ func (db *DB) update(ctx context.Context) error {
 				continue
 			}
 
-			needsOnCallNotification[schedID] = r.ChannelID
+			needsOnCallNotification[schedID] = append(needsOnCallNotification[schedID], r.ChannelID)
 		}
 	}
 
 	for schedID, data := range scheduleData {
 		var hadChange bool
 		for i, r := range data.V1.OnCallNotificationRules {
 			if r.NextNotification != nil && !r.NextNotification.After(now) {
-				needsOnCallNotification[schedID] = r.ChannelID
+				needsOnCallNotification[schedID] = append(needsOnCallNotification[schedID], r.ChannelID)
 			}
 
 			newTime := nextOnCallNotification(now.In(tz[schedID]), r)
@@ -277,10 +278,18 @@ func (db *DB) update(ctx context.Context) error {
 		}
 	}
 
-	for schedID, chanID := range needsOnCallNotification {
-		_, err = tx.StmtContext(ctx, db.scheduleOnCallNotification).ExecContext(ctx, uuid.New(), chanID, schedID)
-		if err != nil {
-			return err
+	for schedID, chanIDs := range needsOnCallNotification {
+		sort.Slice(chanIDs, func(i, j int) bool { return chanIDs[i].String() < chanIDs[j].String() })
+		var lastID uuid.UUID
+		for _, chanID := range chanIDs {
+			if chanID == lastID {
+				continue
+			}
+			lastID = chanID
+			_, err = tx.StmtContext(ctx, db.scheduleOnCallNotification).ExecContext(ctx, uuid.New(), chanID, schedID)
+			if err != nil {
+				return err
+			}
 		}
 	}
 

smoketest/harness/slack.go

@@ -24,7 +24,8 @@ type SlackChannel interface {
 type slackServer struct {
 	h *Harness
 	*mockslack.Server
-	channels map[string]*slackChannel
+	hasFailure bool
+	channels   map[string]*slackChannel
 }
 
 type slackChannel struct {
@@ -47,7 +48,7 @@ func (s *slackServer) WaitAndAssert() {
 		s.h.Trigger()
 		var hasFailure bool
 		for _, ch := range s.channels {
-			hasFailure = hasFailure || ch.hasUnexpectedMessages()
+			hasFailure = s.hasFailure || hasFailure || ch.hasUnexpectedMessages()
 		}
 
 		if hasFailure {
@@ -95,6 +96,7 @@ func (ch *slackChannel) ExpectMessage(keywords ...string) {
 
 		select {
 		case <-timeout.C:
+			ch.h.slack.hasFailure = true
 			ch.h.t.Fatalf("timeout waiting for slack message: Channel=%s; ID=%s; keywords=%v\nGot: %#v", ch.name, ch.id, keywords, ch.h.slack.Messages(ch.id))
 		default:
 		}

smoketest/oncallnotify_test.go

@@ -34,16 +34,18 @@ func TestOnCallNotify(t *testing.T) {
 
 	insert into notification_channels (id, type, name, value)
 	values
-		({{uuid "chan"}}, 'SLACK', '#test', {{slackChannelID "test"}});
-	
+		({{uuid "chan1"}}, 'SLACK', '#test1', {{slackChannelID "test1"}}),
+		({{uuid "chan2"}}, 'SLACK', '#test2', {{slackChannelID "test2"}});
+
 	insert into schedule_data (schedule_id, data)
 	values
-		({{uuid "sid"}}, '{"V1":{"OnCallNotificationRules": [{"ChannelID": {{uuidJSON "chan"}} }]}}');
+		({{uuid "sid"}}, '{"V1":{"OnCallNotificationRules": [{"ChannelID": {{uuidJSON "chan1"}} }, {"ChannelID": {{uuidJSON "chan2"}} }]}}');
 `
 	h := harness.NewHarness(t, sql, "outgoing-messages-schedule-id")
 	defer h.Close()
 
-	h.Slack().Channel("test").ExpectMessage("on-call", "testschedule", "bob")
+	h.Slack().Channel("test1").ExpectMessage("on-call", "testschedule", "bob")
+	h.Slack().Channel("test2").ExpectMessage("on-call", "testschedule", "bob")
 
 	h.FastForward(time.Hour)
 
@@ -56,5 +58,6 @@ func TestOnCallNotify(t *testing.T) {
 	})
 	require.NoError(t, err)
 
-	h.Slack().Channel("test").ExpectMessage("on-call", "testschedule", "bob", "joe")
+	h.Slack().Channel("test1").ExpectMessage("on-call", "testschedule", "bob", "joe")
+	h.Slack().Channel("test2").ExpectMessage("on-call", "testschedule", "bob", "joe")
 }

smoketest/oncallnotifytod_test.go

@@ -26,11 +26,12 @@ func TestOnCallNotifyTOD(t *testing.T) {
 
 	insert into notification_channels (id, type, name, value)
 	values
-		({{uuid "chan"}}, 'SLACK', '#test', {{slackChannelID "test"}});
+		({{uuid "chan1"}}, 'SLACK', '#test1', {{slackChannelID "test1"}}),
+		({{uuid "chan2"}}, 'SLACK', '#test2', {{slackChannelID "test2"}});
 	
 	insert into schedule_data (schedule_id, data)
 	values
-		({{uuid "sid"}}, '{"V1":{"OnCallNotificationRules": [{"ChannelID": {{uuidJSON "chan"}}, "Time": "00:00" }]}}');
+		({{uuid "sid"}}, '{"V1":{"OnCallNotificationRules": [{"ChannelID": {{uuidJSON "chan1"}}, "Time": "00:00" },{"ChannelID": {{uuidJSON "chan1"}}, "Time": "01:00" },{"ChannelID": {{uuidJSON "chan2"}}, "Time": "00:00" }]}}');
 `
 	h := harness.NewHarness(t, sql, "outgoing-messages-schedule-id")
 	defer h.Close()
@@ -39,6 +40,7 @@ func TestOnCallNotifyTOD(t *testing.T) {
 
 	h.FastForward(24 * time.Hour)
 
-	h.Slack().Channel("test").ExpectMessage("on-call", "testschedule", "bob")
-
+	// should only send 1 message to each channel
+	h.Slack().Channel("test1").ExpectMessage("on-call", "testschedule", "bob")
+	h.Slack().Channel("test2").ExpectMessage("on-call", "testschedule", "bob")
 }