alert -> ok with reminders enabled should send

pkg/services/alerting/notifiers/base.go

@@ -51,19 +51,26 @@ func defaultShouldNotify(context *alerting.EvalContext, sendReminder bool, frequ
 		return false
 	}
 
+	if context.PrevAlertState == context.Rule.State && sendReminder {
 		// Do not notify if interval has not elapsed
 		lastNotify := time.Unix(notificationState.SentAt, 0)
-	if sendReminder && !lastNotify.IsZero() && lastNotify.Add(frequency).After(time.Now()) {
+		if !lastNotify.IsZero() && lastNotify.Add(frequency).After(time.Now()) {
 			return false
 		}
 
-	// Do not notify if alert state if OK or pending even on repeated notify
-	if sendReminder && (context.Rule.State == models.AlertStateOK || context.Rule.State == models.AlertStatePending) {
+		// Do not notify if alert state is OK or pending even on repeated notify
+		if context.Rule.State == models.AlertStateOK || context.Rule.State == models.AlertStatePending {
 			return false
 		}
+	}
 
 	// Do not notify when we become OK for the first time.
-	if (context.PrevAlertState == models.AlertStatePending) && (context.Rule.State == models.AlertStateOK) {
+	if context.PrevAlertState == models.AlertStatePending && context.Rule.State == models.AlertStateOK {
+		return false
+	}
+
+	// Do not notify when we OK -> Pending
+	if context.PrevAlertState == models.AlertStateOK && context.Rule.State == models.AlertStatePending {
 		return false
 	}
 

pkg/services/alerting/notifiers/base_test.go

@@ -20,34 +20,34 @@ func TestShouldSendAlertNotification(t *testing.T) {
 		newState     m.AlertStateType
 		sendReminder bool
 		frequency    time.Duration
-		journals     *m.AlertNotificationState
+		state        *m.AlertNotificationState
 
 		expect bool
 	}{
 		{
 			name:         "pending -> ok should not trigger an notification",
-			newState:     m.AlertStatePending,
-			prevState:    m.AlertStateOK,
+			newState:     m.AlertStateOK,
+			prevState:    m.AlertStatePending,
 			sendReminder: false,
-			journals:     &m.AlertNotificationState{},
+			state:        &m.AlertNotificationState{},
 
 			expect: false,
 		},
 		{
 			name:         "ok -> alerting should trigger an notification",
-			newState:     m.AlertStateOK,
-			prevState:    m.AlertStateAlerting,
+			newState:     m.AlertStateAlerting,
+			prevState:    m.AlertStateOK,
 			sendReminder: false,
-			journals:     &m.AlertNotificationState{},
+			state:        &m.AlertNotificationState{},
 
 			expect: true,
 		},
 		{
 			name:         "ok -> pending should not trigger an notification",
-			newState:     m.AlertStateOK,
-			prevState:    m.AlertStatePending,
+			newState:     m.AlertStatePending,
+			prevState:    m.AlertStateOK,
 			sendReminder: false,
-			journals:     &m.AlertNotificationState{},
+			state:        &m.AlertNotificationState{},
 
 			expect: false,
 		},
@@ -56,66 +56,77 @@ func TestShouldSendAlertNotification(t *testing.T) {
 			newState:     m.AlertStateOK,
 			prevState:    m.AlertStateOK,
 			sendReminder: false,
-			journals:     &m.AlertNotificationState{},
+			state:        &m.AlertNotificationState{},
 
 			expect: false,
 		},
 		{
-			name:         "ok -> alerting should trigger an notification",
+			name:         "ok -> ok with reminder should not trigger an notification",
 			newState:     m.AlertStateOK,
-			prevState:    m.AlertStateAlerting,
+			prevState:    m.AlertStateOK,
 			sendReminder: true,
-			journals:     &m.AlertNotificationState{},
+			state:        &m.AlertNotificationState{},
+
+			expect: false,
+		},
+		{
+			name:         "alerting -> ok should trigger an notification",
+			newState:     m.AlertStateOK,
+			prevState:    m.AlertStateAlerting,
+			sendReminder: false,
+			state:        &m.AlertNotificationState{},
 
 			expect: true,
 		},
 		{
-			name:         "ok -> ok with reminder should not trigger an notification",
+			name:         "alerting -> ok should trigger an notification when reminders enabled",
 			newState:     m.AlertStateOK,
-			prevState:    m.AlertStateOK,
+			prevState:    m.AlertStateAlerting,
+			frequency:    time.Minute * 10,
 			sendReminder: true,
-			journals:     &m.AlertNotificationState{},
+			state:        &m.AlertNotificationState{SentAt: tnow.Add(-time.Minute).Unix()},
 
-			expect: false,
+			expect: true,
 		},
 		{
-			name:         "alerting -> alerting with reminder and no journaling should trigger",
+			name:         "alerting -> alerting with reminder and no state should trigger",
 			newState:     m.AlertStateAlerting,
 			prevState:    m.AlertStateAlerting,
 			frequency:    time.Minute * 10,
 			sendReminder: true,
-			journals:     &m.AlertNotificationState{},
+			state:        &m.AlertNotificationState{},
 
 			expect: true,
 		},
 		{
-			name:         "alerting -> alerting with reminder and successful recent journal event should not trigger",
+			name:         "alerting -> alerting with reminder and last notification sent 1 minute ago should not trigger",
 			newState:     m.AlertStateAlerting,
 			prevState:    m.AlertStateAlerting,
 			frequency:    time.Minute * 10,
 			sendReminder: true,
-			journals:     &m.AlertNotificationState{SentAt: tnow.Add(-time.Minute).Unix()},
+			state:        &m.AlertNotificationState{SentAt: tnow.Add(-time.Minute).Unix()},
 
 			expect: false,
 		},
 		{
-			name:         "alerting -> alerting with reminder and failed recent journal event should trigger",
+			name:         "alerting -> alerting with reminder and last notifciation sent 11 minutes ago should trigger",
 			newState:     m.AlertStateAlerting,
 			prevState:    m.AlertStateAlerting,
 			frequency:    time.Minute * 10,
 			sendReminder: true,
+			state:        &m.AlertNotificationState{SentAt: tnow.Add(-11 * time.Minute).Unix()},
+
 			expect: true,
-			journals:     &m.AlertNotificationState{SentAt: tnow.Add(-time.Hour).Unix()},
 		},
 	}
 
 	for _, tc := range tcs {
 		evalContext := alerting.NewEvalContext(context.TODO(), &alerting.Rule{
-			State: tc.newState,
+			State: tc.prevState,
 		})
 
-		evalContext.Rule.State = tc.prevState
-		if defaultShouldNotify(evalContext, true, tc.frequency, tc.journals) != tc.expect {
+		evalContext.Rule.State = tc.newState
+		if defaultShouldNotify(evalContext, tc.sendReminder, tc.frequency, tc.state) != tc.expect {
 			t.Errorf("failed test %s.\n expected \n%+v \nto return: %v", tc.name, tc, tc.expect)
 		}
 	}