From d01261a228fd6cda693623a85832bd567693512e Mon Sep 17 00:00:00 2001 From: Jonathan Date: Mon, 31 Jul 2017 11:51:43 -0400 Subject: PLT-7177: Change the default email frequency to 15 minutes if batching is enabled on the server. (#7036) * PLT-7177: Found default preference that needs to be changed * PLT-7177: Front end behaves as desired * PLT-7177: Changed default batching interval on server side * PLT-7177: Added unit tests for new default interval * PLT-7177: Removed unused import * PLT-7177: Renamed constants to increase clarity --- app/email_batching.go | 7 +- app/email_batching_test.go | 80 ++++++++++++++++++++++ app/notification.go | 7 +- model/preference.go | 4 +- webapp/components/admin_console/email_settings.jsx | 6 +- .../user_settings/email_notification_setting.jsx | 15 ++-- webapp/i18n/en.json | 2 +- 7 files changed, 104 insertions(+), 17 deletions(-) diff --git a/app/email_batching.go b/app/email_batching.go index 5e4a36daf..e69870814 100644 --- a/app/email_batching.go +++ b/app/email_batching.go @@ -140,13 +140,14 @@ func (job *EmailBatchingJob) checkPendingNotifications(now time.Time, handler fu // get how long we need to wait to send notifications to the user var interval int64 if result := <-pchan; result.Err != nil { - // default to 30 seconds to match the send "immediate" setting - interval, _ = strconv.ParseInt(model.PREFERENCE_DEFAULT_EMAIL_INTERVAL, 10, 64) + // use the default batching interval if an error ocurrs while fetching user preferences + interval, _ = strconv.ParseInt(model.PREFERENCE_EMAIL_INTERVAL_BATCHING_SECONDS, 10, 64) } else { preference := result.Data.(model.Preference) if value, err := strconv.ParseInt(preference.Value, 10, 64); err != nil { - interval, _ = strconv.ParseInt(model.PREFERENCE_DEFAULT_EMAIL_INTERVAL, 10, 64) + // // use the default batching interval if an error ocurrs while deserializing user preferences + interval, _ = strconv.ParseInt(model.PREFERENCE_EMAIL_INTERVAL_BATCHING_SECONDS, 10, 64) } else { interval = value } diff --git a/app/email_batching_test.go b/app/email_batching_test.go index 74fbea5c3..b5c18d378 100644 --- a/app/email_batching_test.go +++ b/app/email_batching_test.go @@ -191,3 +191,83 @@ func TestCheckPendingNotifications(t *testing.T) { t.Fatal("timed out waiting for second post notification") } } + +/** + * Ensures that email batch interval defaults to 15 minutes for users that haven't explicitly set this preference + */ +func TestCheckPendingNotificationsDefaultInterval(t *testing.T) { + Setup() + id1 := model.NewId() + job := MakeEmailBatchingJob(128) + + // bypasses recent user activity check + store.Must(Srv.Store.Status().SaveOrUpdate(&model.Status{ + UserId: id1, + LastActivityAt: 9999000, + })) + + job.pendingNotifications[id1] = []*batchedNotification{ + { + post: &model.Post{ + UserId: id1, + CreateAt: 10000000, + }, + }, + } + + // notifications should not be sent 1s after post was created, because default batch interval is 15mins + job.checkPendingNotifications(time.Unix(10001, 0), func(string, []*batchedNotification) {}) + if job.pendingNotifications[id1] == nil || len(job.pendingNotifications[id1]) != 1 { + t.Fatal("shouldn't have sent queued post") + } + + // notifications should be sent 901s after post was created, because default batch interval is 15mins + job.checkPendingNotifications(time.Unix(10901, 0), func(string, []*batchedNotification) {}) + if job.pendingNotifications[id1] != nil || len(job.pendingNotifications[id1]) != 0 { + t.Fatal("should have sent queued post") + } +} + +/** + * Ensures that email batch interval defaults to 15 minutes if user preference is invalid + */ +func TestCheckPendingNotificationsCantParseInterval(t *testing.T) { + Setup() + id1 := model.NewId() + job := MakeEmailBatchingJob(128) + + // bypasses recent user activity check + store.Must(Srv.Store.Status().SaveOrUpdate(&model.Status{ + UserId: id1, + LastActivityAt: 9999000, + })) + + // preference value is not an integer, so we'll fall back to the default 15min value + store.Must(Srv.Store.Preference().Save(&model.Preferences{{ + UserId: id1, + Category: model.PREFERENCE_CATEGORY_NOTIFICATIONS, + Name: model.PREFERENCE_NAME_EMAIL_INTERVAL, + Value: "notAnIntegerValue", + }})) + + job.pendingNotifications[id1] = []*batchedNotification{ + { + post: &model.Post{ + UserId: id1, + CreateAt: 10000000, + }, + }, + } + + // notifications should not be sent 1s after post was created, because default batch interval is 15mins + job.checkPendingNotifications(time.Unix(10001, 0), func(string, []*batchedNotification) {}) + if job.pendingNotifications[id1] == nil || len(job.pendingNotifications[id1]) != 1 { + t.Fatal("shouldn't have sent queued post") + } + + // notifications should be sent 901s after post was created, because default batch interval is 15mins + job.checkPendingNotifications(time.Unix(10901, 0), func(string, []*batchedNotification) {}) + if job.pendingNotifications[id1] != nil || len(job.pendingNotifications[id1]) != 0 { + t.Fatal("should have sent queued post") + } +} diff --git a/app/notification.go b/app/notification.go index d2eb44b3a..d145b21b3 100644 --- a/app/notification.go +++ b/app/notification.go @@ -332,13 +332,12 @@ func sendNotificationEmail(post *model.Post, user *model.User, channel *model.Ch } if *utils.Cfg.EmailSettings.EnableEmailBatching { var sendBatched bool - if result := <-Srv.Store.Preference().Get(user.Id, model.PREFERENCE_CATEGORY_NOTIFICATIONS, model.PREFERENCE_NAME_EMAIL_INTERVAL); result.Err != nil { - // if the call fails, assume it hasn't been set and use the default + // if the call fails, assume it hasn't been set and don't batch notifications for this user sendBatched = false } else { - // default to not using batching if the setting is set to immediate - sendBatched = result.Data.(model.Preference).Value != model.PREFERENCE_DEFAULT_EMAIL_INTERVAL + // if the user has chosen to receive notifications immediately, don't batch them + sendBatched = result.Data.(model.Preference).Value != model.PREFERENCE_EMAIL_INTERVAL_NO_BATCHING_SECONDS } if sendBatched { diff --git a/model/preference.go b/model/preference.go index f4737d06e..6bbe7326c 100644 --- a/model/preference.go +++ b/model/preference.go @@ -33,7 +33,9 @@ const ( PREFERENCE_CATEGORY_NOTIFICATIONS = "notifications" PREFERENCE_NAME_EMAIL_INTERVAL = "email_interval" - PREFERENCE_DEFAULT_EMAIL_INTERVAL = "30" // default to match the interval of the "immediate" setting (ie 30 seconds) + + PREFERENCE_EMAIL_INTERVAL_NO_BATCHING_SECONDS = "30" // the "immediate" setting is actually 30s + PREFERENCE_EMAIL_INTERVAL_BATCHING_SECONDS = "900" // fifteen minutes is 900 seconds ) type Preference struct { diff --git a/webapp/components/admin_console/email_settings.jsx b/webapp/components/admin_console/email_settings.jsx index d7694ebb6..e48bc46ab 100644 --- a/webapp/components/admin_console/email_settings.jsx +++ b/webapp/components/admin_console/email_settings.jsx @@ -21,9 +21,7 @@ export default class EmailSettings extends AdminSettings { super(props); this.getConfigFromState = this.getConfigFromState.bind(this); - this.handleSaved = this.handleSaved.bind(this); - this.renderSettings = this.renderSettings.bind(this); } @@ -138,11 +136,11 @@ export default class EmailSettings extends AdminSettings { , enableEmailBatchingDisabledText ]} - value={this.state.enableEmailBatching && !this.props.config.ClusterSettings.Enable && this.props.config.ServiceSettings.SiteURL} + value={this.state.enableEmailBatching && !this.props.config.ClusterSettings.Enable && Boolean(this.props.config.ServiceSettings.SiteURL)} onChange={this.handleChange} disabled={!this.state.sendEmailNotifications || this.props.config.ClusterSettings.Enable || !this.props.config.ServiceSettings.SiteURL} /> diff --git a/webapp/components/user_settings/email_notification_setting.jsx b/webapp/components/user_settings/email_notification_setting.jsx index 4dd15f0be..5abdc3384 100644 --- a/webapp/components/user_settings/email_notification_setting.jsx +++ b/webapp/components/user_settings/email_notification_setting.jsx @@ -28,13 +28,20 @@ export default class EmailNotificationSetting extends React.Component { super(props); this.submit = this.submit.bind(this); - this.expand = this.expand.bind(this); this.collapse = this.collapse.bind(this); - this.state = { - emailInterval: PreferenceStore.getInt(Preferences.CATEGORY_NOTIFICATIONS, Preferences.EMAIL_INTERVAL, Preferences.INTERVAL_IMMEDIATE) - }; + if (global.mm_config.EnableEmailBatching === 'true') { + // when email batching is enabled, the default interval is 15 minutes + this.state = { + emailInterval: PreferenceStore.getInt(Preferences.CATEGORY_NOTIFICATIONS, Preferences.EMAIL_INTERVAL, Preferences.INTERVAL_FIFTEEN_MINUTES) + }; + } else { + // otherwise, the default interval is immediately + this.state = { + emailInterval: PreferenceStore.getInt(Preferences.CATEGORY_NOTIFICATIONS, Preferences.EMAIL_INTERVAL, Preferences.INTERVAL_IMMEDIATE) + }; + } } handleChange(enableEmail, emailInterval) { diff --git a/webapp/i18n/en.json b/webapp/i18n/en.json index deec452d2..e138660b0 100755 --- a/webapp/i18n/en.json +++ b/webapp/i18n/en.json @@ -267,7 +267,7 @@ "admin.email.emailSuccess": "No errors were reported while sending an email. Please check your inbox to make sure.", "admin.email.enableEmailBatching.clusterEnabled": "Email batching cannot be enabled when High Availability mode is enabled.", "admin.email.enableEmailBatching.siteURL": "Email batching cannot be enabled unless the SiteURL is configured in Configuration > SiteURL.", - "admin.email.enableEmailBatchingDesc": "When true, users can have email notifications for multiple direct messages and mentions combined into a single email, configurable in Account Settings > Notifications.", + "admin.email.enableEmailBatchingDesc": "When true, users will have email notifications for multiple direct messages and mentions combined into a single email. Batching will occur at a default interval of 15 minutes, configurable in Account Settings > Notifications.", "admin.email.enableEmailBatchingTitle": "Enable Email Batching:", "admin.email.fullPushNotification": "Send full message snippet", "admin.email.genericPushNotification": "Send generic description with user and channel names", -- cgit v1.2.3-1-g7c22