From 50915875c022aed67d8e44cc1ebbfda74c16343c Mon Sep 17 00:00:00 2001 From: Rowan Seymour Date: Thu, 14 Jan 2021 12:55:40 -0500 Subject: [PATCH] Simplify FCM client code --- core/hooks/send_messages.go | 2 +- core/msgio/android.go | 38 +++++++++++---------------------- core/msgio/android_test.go | 15 +++++++++++-- core/msgio/send.go | 8 +++++-- core/msgio/send_test.go | 4 ++-- core/tasks/broadcasts/worker.go | 2 +- mailroom.go | 5 +++++ services/tickets/utils.go | 2 +- 8 files changed, 42 insertions(+), 34 deletions(-) diff --git a/core/hooks/send_messages.go b/core/hooks/send_messages.go index eb9a2fc94..387c8b14d 100644 --- a/core/hooks/send_messages.go +++ b/core/hooks/send_messages.go @@ -35,6 +35,6 @@ func (h *sendMessagesHook) Apply(ctx context.Context, tx *sqlx.Tx, rp *redis.Poo msgs = append(msgs, sceneMsgs...) } - msgio.SendMessages(ctx, tx, rp, msgs) + msgio.SendMessages(ctx, tx, rp, nil, msgs) return nil } diff --git a/core/msgio/android.go b/core/msgio/android.go index 0917214fc..67c80db81 100644 --- a/core/msgio/android.go +++ b/core/msgio/android.go @@ -1,7 +1,6 @@ package msgio import ( - "sync" "time" "github.com/nyaruka/mailroom/config" @@ -12,27 +11,9 @@ import ( "github.com/sirupsen/logrus" ) -var clientInit sync.Once -var fcmClient *fcm.Client - -func init() { - clientInit.Do(func() { - if config.Mailroom.FCMKey == "" { - logrus.Error("fcm not configured, no syncing of android channels") - return - } - - var err error - fcmClient, err = fcm.NewClient(config.Mailroom.FCMKey) - if err != nil { - panic(errors.Wrap(err, "unable to create FCM client")) - } - }) -} - // SyncAndroidChannels tries to trigger syncs of the given Android channels via FCM -func SyncAndroidChannels(channels []*models.Channel) { - if fcmClient == nil { +func SyncAndroidChannels(fc *fcm.Client, channels []*models.Channel) { + if fc == nil { logrus.Warn("skipping Android sync as instance has not configured FCM") return } @@ -52,7 +33,7 @@ func SyncAndroidChannels(channels []*models.Channel) { } start := time.Now() - _, err := fcmClient.Send(sync) + _, err := fc.Send(sync) if err != nil { // log failures but continue, relayer will sync on its own @@ -63,7 +44,14 @@ func SyncAndroidChannels(channels []*models.Channel) { } } -// SetFCMClient sets the FCM client. Used for testing. -func SetFCMClient(client *fcm.Client) { - fcmClient = client +// CreateFCMClient creates an FCM client based on the configured FCM API key +func CreateFCMClient() *fcm.Client { + if config.Mailroom.FCMKey == "" { + return nil + } + client, err := fcm.NewClient(config.Mailroom.FCMKey) + if err != nil { + panic(errors.Wrap(err, "unable to create FCM client")) + } + return client } diff --git a/core/msgio/android_test.go b/core/msgio/android_test.go index b7724b488..a442763b9 100644 --- a/core/msgio/android_test.go +++ b/core/msgio/android_test.go @@ -8,6 +8,7 @@ import ( "github.com/nyaruka/gocommon/jsonx" "github.com/nyaruka/goflow/utils" + "github.com/nyaruka/mailroom/config" "github.com/nyaruka/mailroom/core/models" "github.com/nyaruka/mailroom/core/msgio" "github.com/nyaruka/mailroom/testsuite" @@ -69,7 +70,7 @@ func TestSyncAndroidChannels(t *testing.T) { mockFCM := newMockFCMEndpoint("FCMID3") defer mockFCM.Stop() - msgio.SetFCMClient(mockFCM.Client("FCMKEY123")) + fc := mockFCM.Client("FCMKEY123") // create some Android channels channel1ID := testdata.InsertChannel(t, db, models.Org1, "A", "Android 1", []string{"tel"}, "SR", map[string]interface{}{"FCM_ID": ""}) // no FCM ID @@ -83,7 +84,7 @@ func TestSyncAndroidChannels(t *testing.T) { channel2 := oa.ChannelByID(channel2ID) channel3 := oa.ChannelByID(channel3ID) - msgio.SyncAndroidChannels([]*models.Channel{channel1, channel2, channel3}) + msgio.SyncAndroidChannels(fc, []*models.Channel{channel1, channel2, channel3}) // check that we try to sync the 2 channels with FCM IDs, even tho one fails assert.Equal(t, 2, len(mockFCM.Messages)) @@ -94,3 +95,13 @@ func TestSyncAndroidChannels(t *testing.T) { assert.Equal(t, "sync", mockFCM.Messages[0].CollapseKey) assert.Equal(t, map[string]interface{}{"msg": "sync"}, mockFCM.Messages[0].Data) } + +func TestCreateFCMClient(t *testing.T) { + config.Mailroom.FCMKey = "1234" + + assert.NotNil(t, msgio.CreateFCMClient()) + + config.Mailroom.FCMKey = "" + + assert.Nil(t, msgio.CreateFCMClient()) +} diff --git a/core/msgio/send.go b/core/msgio/send.go index 6b251952d..850886890 100644 --- a/core/msgio/send.go +++ b/core/msgio/send.go @@ -3,6 +3,7 @@ package msgio import ( "context" + "github.com/edganiukov/fcm" "github.com/nyaruka/mailroom/core/models" "github.com/apex/log" @@ -10,7 +11,7 @@ import ( ) // SendMessages tries to send the given messages via Courier or Android syncing -func SendMessages(ctx context.Context, db models.Queryer, rp *redis.Pool, msgs []*models.Msg) { +func SendMessages(ctx context.Context, db models.Queryer, rp *redis.Pool, fc *fcm.Client, msgs []*models.Msg) { // messages to be sent by courier, organized by contact courierMsgs := make(map[models.ContactID][]*models.Msg, 100) @@ -61,7 +62,10 @@ func SendMessages(ctx context.Context, db models.Queryer, rp *redis.Pool, msgs [ // if we have any android messages, trigger syncs for the unique channels if len(androidChannels) > 0 { - SyncAndroidChannels(androidChannels) + if fc == nil { + fc = CreateFCMClient() + } + SyncAndroidChannels(fc, androidChannels) } // any messages that didn't get sent should be moved back to pending (they are queued at creation to save an diff --git a/core/msgio/send_test.go b/core/msgio/send_test.go index 95e69add1..ec60a6d2c 100644 --- a/core/msgio/send_test.go +++ b/core/msgio/send_test.go @@ -62,7 +62,7 @@ func TestSendMessages(t *testing.T) { mockFCM := newMockFCMEndpoint("FCMID3") defer mockFCM.Stop() - msgio.SetFCMClient(mockFCM.Client("FCMKEY123")) + fc := mockFCM.Client("FCMKEY123") // create some Andoid channels androidChannel1ID := testdata.InsertChannel(t, db, models.Org1, "A", "Android 1", []string{"tel"}, "SR", map[string]interface{}{"FCM_ID": "FCMID1"}) @@ -151,7 +151,7 @@ func TestSendMessages(t *testing.T) { rc.Do("FLUSHDB") mockFCM.Messages = nil - msgio.SendMessages(ctx, db, rp, msgs) + msgio.SendMessages(ctx, db, rp, fc, msgs) assertCourierQueueSizes(t, rc, tc.QueueSizes, "courier queue sizes mismatch in '%s'", tc.Description) diff --git a/core/tasks/broadcasts/worker.go b/core/tasks/broadcasts/worker.go index 7ebcd402b..a7c5364eb 100644 --- a/core/tasks/broadcasts/worker.go +++ b/core/tasks/broadcasts/worker.go @@ -171,6 +171,6 @@ func SendBroadcastBatch(ctx context.Context, db *sqlx.DB, rp *redis.Pool, bcast return errors.Wrapf(err, "error creating broadcast messages") } - msgio.SendMessages(ctx, db, rp, msgs) + msgio.SendMessages(ctx, db, rp, nil, msgs) return nil } diff --git a/mailroom.go b/mailroom.go index 3661e82a9..1a232d5e8 100644 --- a/mailroom.go +++ b/mailroom.go @@ -192,6 +192,11 @@ func (mr *Mailroom) Start() error { log.Info("elastic ok") } + // warn if we won't be doing FCM syncing + if config.Mailroom.FCMKey == "" { + logrus.Error("fcm not configured, no syncing of android channels") + } + for _, initFunc := range initFunctions { initFunc(mr) } diff --git a/services/tickets/utils.go b/services/tickets/utils.go index 4aadb9c34..7f45f4f4c 100644 --- a/services/tickets/utils.go +++ b/services/tickets/utils.go @@ -110,7 +110,7 @@ func SendReply(ctx context.Context, db *sqlx.DB, rp *redis.Pool, store storage.S return nil, errors.Wrapf(err, "error creating message batch") } - msgio.SendMessages(ctx, db, rp, msgs) + msgio.SendMessages(ctx, db, rp, nil, msgs) return msgs[0], nil }