AlertingNG: Fix TODOs in email notification channel (#33169)

* AlertingNG: Fix TODOs in email notification channel

Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>

* Test fixup

* Remove the receiver field it is not needed for the email notification

Co-authored-by: Josue Abreu <josue@grafana.com>

pkg/services/ngalert/notifier/alertmanager.go

@@ -352,7 +352,7 @@ func (am *Alertmanager) buildReceiverIntegrations(receiver *apimodels.PostableAp
 		}
 		switch r.Type {
 		case "email":
-			n, err = channels.NewEmailNotifier(cfg, externalURL)
+			n, err = channels.NewEmailNotifier(cfg, externalURL, am.Settings.AppURL)
 		case "pagerduty":
 			n, err = channels.NewPagerdutyNotifier(cfg, tmpl, externalURL)
 		case "slack":

pkg/services/ngalert/notifier/channels/email.go

@@ -3,12 +3,12 @@ package channels
 import (
 	"context"
 	"net/url"
+	"path"
 
 	gokit_log "github.com/go-kit/kit/log"
 	"github.com/prometheus/alertmanager/notify"
 	"github.com/prometheus/alertmanager/template"
 	"github.com/prometheus/alertmanager/types"
-	"github.com/prometheus/common/model"
 
 	"github.com/grafana/grafana/pkg/bus"
 	"github.com/grafana/grafana/pkg/infra/log"
@@ -27,11 +27,12 @@ type EmailNotifier struct {
 	AutoResolve bool
 	log         log.Logger
 	externalUrl *url.URL
+	appURL      string
 }
 
 // NewEmailNotifier is the constructor function
 // for the EmailNotifier.
-func NewEmailNotifier(model *models.AlertNotification, externalUrl *url.URL) (*EmailNotifier, error) {
+func NewEmailNotifier(model *models.AlertNotification, externalUrl *url.URL, appURL string) (*EmailNotifier, error) {
 	if model.Settings == nil {
 		return nil, alerting.ValidationError{Reason: "No Settings Supplied"}
 	}
@@ -52,6 +53,7 @@ func NewEmailNotifier(model *models.AlertNotification, externalUrl *url.URL) (*E
 		Addresses:    addresses,
 		SingleEmail:  singleEmail,
 		AutoResolve:  autoResolve,
+		appURL:       appURL,
 		log:          log.New("alerting.notifier.email"),
 		externalUrl:  externalUrl,
 	}, nil
@@ -59,11 +61,6 @@ func NewEmailNotifier(model *models.AlertNotification, externalUrl *url.URL) (*E
 
 // Notify sends the alert notification.
 func (en *EmailNotifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error) {
-	// TODO(codesome): make sure the receiver name is added in the ctx before calling this.
-	ctx = notify.WithReceiverName(ctx, "email-notification-channel") // Dummy.
-	// TODO(codesome): make sure the group labels is added in the ctx before calling this.
-	ctx = notify.WithGroupLabels(ctx, model.LabelSet{}) // Dummy.
-
 	// We only need ExternalURL from this template object. This hack should go away with https://github.com/prometheus/alertmanager/pull/2508.
 	data := notify.GetTemplateData(ctx, &template.Template{ExternalURL: en.externalUrl}, as, gokit_log.NewNopLogger())
 
@@ -74,15 +71,14 @@ func (en *EmailNotifier) Notify(ctx context.Context, as ...*types.Alert) (bool,
 			Subject: title,
 			Data: map[string]interface{}{
 				"Title":             title,
-				"Receiver":          data.Receiver,
 				"Status":            data.Status,
 				"Alerts":            data.Alerts,
 				"GroupLabels":       data.GroupLabels,
 				"CommonLabels":      data.CommonLabels,
 				"CommonAnnotations": data.CommonAnnotations,
 				"ExternalURL":       data.ExternalURL,
-				"RuleUrl":           "TODO",
+				"RuleUrl":           path.Join(en.appURL, "/alerting/list"),
-				"AlertPageUrl":      "TODO",
+				"AlertPageUrl":      path.Join(en.appURL, "/alerting/list?alertState=firing&view=state"),
 			},
 			To:          en.Addresses,
 			SingleEmail: en.SingleEmail,

pkg/services/ngalert/notifier/channels/email_test.go

@@ -1,12 +1,18 @@
 package channels
 
 import (
+	"context"
 	"net/url"
 	"testing"
 
+	"github.com/prometheus/alertmanager/template"
+	"github.com/prometheus/alertmanager/types"
+	"github.com/prometheus/common/model"
+	"github.com/stretchr/testify/require"
+
+	"github.com/grafana/grafana/pkg/bus"
 	"github.com/grafana/grafana/pkg/components/simplejson"
 	"github.com/grafana/grafana/pkg/models"
-	"github.com/stretchr/testify/require"
 )
 
 func TestEmailNotifier(t *testing.T) {
@@ -23,41 +29,71 @@ func TestEmailNotifier(t *testing.T) {
 			Settings: settingsJSON,
 		}
 
-		_, err := NewEmailNotifier(model, externalURL)
+		_, err := NewEmailNotifier(model, externalURL, "")
 		require.Error(t, err)
 	})
 
-	t.Run("from settings", func(t *testing.T) {
+	t.Run("with the correct settings it should not fail and produce the expected command", func(t *testing.T) {
-		json := `{"addresses": "ops@grafana.org"}`
+		json := `{"addresses": "someops@example.com;somedev@example.com"}`
 		settingsJSON, err := simplejson.NewJson([]byte(json))
 		require.NoError(t, err)
 
 		emailNotifier, err := NewEmailNotifier(&models.AlertNotification{
-			Name:     "ops",
+			Name: "ops",
-			Type:     "email",
+			Type: "email",
+
 			Settings: settingsJSON,
-		}, externalURL)
+		}, externalURL, "")
 
 		require.NoError(t, err)
-		require.Equal(t, "ops", emailNotifier.Name)
-		require.Equal(t, "email", emailNotifier.Type)
-		require.Equal(t, []string{"ops@grafana.org"}, emailNotifier.Addresses)
-	})
 
-	t.Run("from settings with two emails", func(t *testing.T) {
+		expected := map[string]interface{}{}
-		json := `{"addresses": "ops@grafana.org;dev@grafana.org"}`
+		bus.AddHandlerCtx("test", func(ctx context.Context, cmd *models.SendEmailCommandSync) error {
-		settingsJSON, err := simplejson.NewJson([]byte(json))
+			expected["subject"] = cmd.SendEmailCommand.Subject
+			expected["to"] = cmd.SendEmailCommand.To
+			expected["single_email"] = cmd.SendEmailCommand.SingleEmail
+			expected["template"] = cmd.SendEmailCommand.Template
+			expected["data"] = cmd.SendEmailCommand.Data
+
+			return nil
+		})
+
+		alerts := []*types.Alert{
+			{
+				Alert: model.Alert{
+					Labels:      model.LabelSet{"alertname": "AlwaysFiring", "severity": "warning"},
+					Annotations: model.LabelSet{"runbook_url": "http://fix.me"},
+				},
+			},
+		}
+
+		ok, err := emailNotifier.Notify(context.Background(), alerts...)
 		require.NoError(t, err)
+		require.True(t, ok)
 
-		emailNotifier, err := NewEmailNotifier(&models.AlertNotification{
+		require.Equal(t, map[string]interface{}{
-			Name:     "ops",
+			"subject":      "[firing:1]  (AlwaysFiring warning)",
-			Type:     "email",
+			"to":           []string{"someops@example.com", "somedev@example.com"},
-			Settings: settingsJSON,
+			"single_email": false,
-		}, externalURL)
+			"template":     "ng_alert_notification.html",
-
+			"data": map[string]interface{}{
-		require.NoError(t, err)
+				"Title":  "[firing:1]  (AlwaysFiring warning)",
-		require.Equal(t, "ops", emailNotifier.Name)
+				"Status": "firing",
-		require.Equal(t, "email", emailNotifier.Type)
+				"Alerts": template.Alerts{
-		require.Equal(t, []string{"ops@grafana.org", "dev@grafana.org"}, emailNotifier.Addresses)
+					template.Alert{
+						Status:      "firing",
+						Labels:      template.KV{"alertname": "AlwaysFiring", "severity": "warning"},
+						Annotations: template.KV{"runbook_url": "http://fix.me"},
+						Fingerprint: "15a37193dce72bab",
+					},
+				},
+				"GroupLabels":       template.KV{},
+				"CommonLabels":      template.KV{"alertname": "AlwaysFiring", "severity": "warning"},
+				"CommonAnnotations": template.KV{"runbook_url": "http://fix.me"},
+				"ExternalURL":       "http://localhost",
+				"RuleUrl":           "/alerting/list",
+				"AlertPageUrl":      "/alerting/list?alertState=firing&view=state",
+			},
+		}, expected)
 	})
 }