From 61974383d7ca2a66fd2962375c60046e407d0e71 Mon Sep 17 00:00:00 2001 From: Katarina Yang <69819079+yangkb09@users.noreply.github.com> Date: Wed, 19 Jan 2022 14:25:52 -0500 Subject: [PATCH] Refactor: Change sqlstore.inTransaction to SQLStore.WithTransactionalDBSession in alert files (#43815) * Refactor: Change sqlstore.inTransaction to SQLStore.WithTransactionalDBSession in alert files * Fix: Revert second SaveAlerts back to normal func * Refactor: Update tests so functions are now SQLStore methods * Fix: Refactor pauseAlert and pauseAllAlerts to be SQLStore methods * Fix: Refactor SaveAlerts to be SQLStore method * Refactor: Update SaveAlerts func signature to have correct arguments * Refactor: Define sqlStore * Chore: Delete commented out code * Chore: Remove unused SaveAlertsCommand --- pkg/models/alert.go | 8 ---- pkg/services/sqlstore/alert.go | 34 +++------------ pkg/services/sqlstore/alert_test.go | 67 ++++++++--------------------- 3 files changed, 25 insertions(+), 84 deletions(-) diff --git a/pkg/models/alert.go b/pkg/models/alert.go index b4da89d143c..452e0fa44c6 100644 --- a/pkg/models/alert.go +++ b/pkg/models/alert.go @@ -127,14 +127,6 @@ func (a *Alert) GetTagsFromSettings() []*Tag { return tags } -type SaveAlertsCommand struct { - DashboardId int64 - UserId int64 - OrgId int64 - - Alerts []*Alert -} - type PauseAlertCommand struct { OrgId int64 AlertIds []int64 diff --git a/pkg/services/sqlstore/alert.go b/pkg/services/sqlstore/alert.go index b884a888767..98902413698 100644 --- a/pkg/services/sqlstore/alert.go +++ b/pkg/services/sqlstore/alert.go @@ -15,14 +15,13 @@ import ( var timeNow = time.Now func (ss *SQLStore) addAlertQueryAndCommandHandlers() { - bus.AddHandler("sql", SaveAlerts) bus.AddHandler("sql", ss.HandleAlertsQuery) bus.AddHandler("sql", ss.GetAlertById) bus.AddHandler("sql", ss.GetAllAlertQueryHandler) bus.AddHandler("sql", ss.SetAlertState) bus.AddHandler("sql", ss.GetAlertStatesForDashboard) - bus.AddHandler("sql", PauseAlert) - bus.AddHandler("sql", PauseAllAlerts) + bus.AddHandler("sql", ss.PauseAlert) + bus.AddHandler("sql", ss.PauseAllAlerts) } func (ss *SQLStore) GetAlertById(ctx context.Context, query *models.GetAlertByIdQuery) error { @@ -174,7 +173,7 @@ func deleteAlertDefinition(dashboardId int64, sess *DBSession) error { } func (ss *SQLStore) SaveAlerts(ctx context.Context, dashID int64, alerts []*models.Alert) error { - return ss.WithTransactionalDbSession(context.Background(), func(sess *DBSession) error { + return ss.WithTransactionalDbSession(ctx, func(sess *DBSession) error { existingAlerts, err := GetAlertsByDashboardId2(dashID, sess) if err != nil { return err @@ -192,25 +191,6 @@ func (ss *SQLStore) SaveAlerts(ctx context.Context, dashID int64, alerts []*mode }) } -func SaveAlerts(ctx context.Context, cmd *models.SaveAlertsCommand) error { - return inTransaction(func(sess *DBSession) error { - existingAlerts, err := GetAlertsByDashboardId2(cmd.DashboardId, sess) - if err != nil { - return err - } - - if err := updateAlerts(existingAlerts, cmd.Alerts, sess); err != nil { - return err - } - - if err := deleteMissingAlerts(existingAlerts, cmd.Alerts, sess); err != nil { - return err - } - - return nil - }) -} - func updateAlerts(existingAlerts []*models.Alert, alerts []*models.Alert, sess *DBSession) error { for _, alert := range alerts { update := false @@ -344,8 +324,8 @@ func (ss *SQLStore) SetAlertState(ctx context.Context, cmd *models.SetAlertState }) } -func PauseAlert(ctx context.Context, cmd *models.PauseAlertCommand) error { - return inTransaction(func(sess *DBSession) error { +func (ss *SQLStore) PauseAlert(ctx context.Context, cmd *models.PauseAlertCommand) error { + return ss.WithTransactionalDbSession(ctx, func(sess *DBSession) error { if len(cmd.AlertIds) == 0 { return fmt.Errorf("command contains no alertids") } @@ -378,8 +358,8 @@ func PauseAlert(ctx context.Context, cmd *models.PauseAlertCommand) error { }) } -func PauseAllAlerts(ctx context.Context, cmd *models.PauseAllAlertCommand) error { - return inTransaction(func(sess *DBSession) error { +func (ss *SQLStore) PauseAllAlerts(ctx context.Context, cmd *models.PauseAllAlertCommand) error { + return ss.WithTransactionalDbSession(ctx, func(sess *DBSession) error { var newState string if cmd.Paused { newState = string(models.AlertStatePaused) diff --git a/pkg/services/sqlstore/alert_test.go b/pkg/services/sqlstore/alert_test.go index 8fece7a975d..1f7d1fdb5e5 100644 --- a/pkg/services/sqlstore/alert_test.go +++ b/pkg/services/sqlstore/alert_test.go @@ -34,7 +34,6 @@ func TestAlertingDataAccess(t *testing.T) { var sqlStore *SQLStore var testDash *models.Dashboard - var cmd models.SaveAlertsCommand var items []*models.Alert setup := func(t *testing.T) { @@ -56,14 +55,7 @@ func TestAlertingDataAccess(t *testing.T) { }, } - cmd = models.SaveAlertsCommand{ - Alerts: items, - DashboardId: testDash.Id, - OrgId: 1, - UserId: 1, - } - - err = SaveAlerts(context.Background(), &cmd) + err = sqlStore.SaveAlerts(context.Background(), testDash.Id, items) require.Nil(t, err) } @@ -91,7 +83,7 @@ func TestAlertingDataAccess(t *testing.T) { stateDateBeforePause := alert.NewStateDate t.Run("can pause all alerts", func(t *testing.T) { - err := pauseAllAlerts(t, true) + err := sqlStore.pauseAllAlerts(t, true) require.Nil(t, err) t.Run("cannot updated paused alert", func(t *testing.T) { @@ -117,7 +109,7 @@ func TestAlertingDataAccess(t *testing.T) { }) t.Run("unpausing alerts should update their NewStateDate again", func(t *testing.T) { - err := pauseAllAlerts(t, false) + err := sqlStore.pauseAllAlerts(t, false) require.Nil(t, err) alert, _ = getAlertById(t, insertedAlert.Id, sqlStore) stateDateAfterUnpause := alert.NewStateDate @@ -162,14 +154,7 @@ func TestAlertingDataAccess(t *testing.T) { modifiedItems := items modifiedItems[0].Name = "Name" - modifiedCmd := models.SaveAlertsCommand{ - DashboardId: testDash.Id, - OrgId: 1, - UserId: 1, - Alerts: modifiedItems, - } - - err := SaveAlerts(context.Background(), &modifiedCmd) + err := sqlStore.SaveAlerts(context.Background(), testDash.Id, items) t.Run("Can save alerts with same dashboard and panel id", func(t *testing.T) { require.Nil(t, err) @@ -189,7 +174,7 @@ func TestAlertingDataAccess(t *testing.T) { }) t.Run("Updates without changes should be ignored", func(t *testing.T) { - err3 := SaveAlerts(context.Background(), &modifiedCmd) + err3 := sqlStore.SaveAlerts(context.Background(), testDash.Id, items) require.Nil(t, err3) }) }) @@ -220,8 +205,7 @@ func TestAlertingDataAccess(t *testing.T) { }, } - cmd.Alerts = multipleItems - err := SaveAlerts(context.Background(), &cmd) + err := sqlStore.SaveAlerts(context.Background(), testDash.Id, multipleItems) t.Run("Should save 3 dashboards", func(t *testing.T) { require.Nil(t, err) @@ -236,8 +220,7 @@ func TestAlertingDataAccess(t *testing.T) { t.Run("should updated two dashboards and delete one", func(t *testing.T) { missingOneAlert := multipleItems[:2] - cmd.Alerts = missingOneAlert - err = SaveAlerts(context.Background(), &cmd) + err = sqlStore.SaveAlerts(context.Background(), testDash.Id, missingOneAlert) t.Run("should delete the missing alert", func(t *testing.T) { query := models.GetAlertsQuery{DashboardIDs: []int64{testDash.Id}, OrgId: 1, User: &models.SignedInUser{OrgRole: models.ROLE_ADMIN}} @@ -259,14 +242,7 @@ func TestAlertingDataAccess(t *testing.T) { }, } - cmd := models.SaveAlertsCommand{ - Alerts: items, - DashboardId: testDash.Id, - OrgId: 1, - UserId: 1, - } - - err := SaveAlerts(context.Background(), &cmd) + err := sqlStore.SaveAlerts(context.Background(), testDash.Id, items) require.Nil(t, err) err = DeleteDashboard(context.Background(), &models.DeleteDashboardCommand{ @@ -293,7 +269,7 @@ func TestPausingAlerts(t *testing.T) { sqlStore := InitTestDB(t) testDash := insertTestDashboard(t, sqlStore, "dashboard with alerts", 1, 0, false, "alert") - alert, err := insertTestAlert("Alerting title", "Alerting message", testDash.OrgId, testDash.Id, simplejson.New()) + alert, err := insertTestAlert("Alerting title", "Alerting message", testDash.OrgId, testDash.Id, simplejson.New(), sqlStore) require.Nil(t, err) stateDateBeforePause := alert.NewStateDate @@ -307,7 +283,7 @@ func TestPausingAlerts(t *testing.T) { insertedAlert := alertQuery.Result[0] t.Run("when paused", func(t *testing.T) { - _, err := pauseAlert(t, testDash.OrgId, insertedAlert.Id, true) + _, err := sqlStore.pauseAlert(t, testDash.OrgId, insertedAlert.Id, true) require.Nil(t, err) t.Run("the NewStateDate should be updated", func(t *testing.T) { @@ -320,7 +296,7 @@ func TestPausingAlerts(t *testing.T) { }) t.Run("when unpaused", func(t *testing.T) { - _, err := pauseAlert(t, testDash.OrgId, insertedAlert.Id, false) + _, err := sqlStore.pauseAlert(t, testDash.OrgId, insertedAlert.Id, false) require.Nil(t, err) t.Run("the NewStateDate should be updated again", func(t *testing.T) { @@ -333,17 +309,17 @@ func TestPausingAlerts(t *testing.T) { }) }) } -func pauseAlert(t *testing.T, orgId int64, alertId int64, pauseState bool) (int64, error) { +func (ss *SQLStore) pauseAlert(t *testing.T, orgId int64, alertId int64, pauseState bool) (int64, error) { cmd := &models.PauseAlertCommand{ OrgId: orgId, AlertIds: []int64{alertId}, Paused: pauseState, } - err := PauseAlert(context.Background(), cmd) + err := ss.PauseAlert(context.Background(), cmd) require.Nil(t, err) return cmd.ResultCount, err } -func insertTestAlert(title string, message string, orgId int64, dashId int64, settings *simplejson.Json) (*models.Alert, error) { +func insertTestAlert(title string, message string, orgId int64, dashId int64, settings *simplejson.Json, ss *SQLStore) (*models.Alert, error) { items := []*models.Alert{ { PanelId: 1, @@ -356,15 +332,8 @@ func insertTestAlert(title string, message string, orgId int64, dashId int64, se }, } - cmd := models.SaveAlertsCommand{ - Alerts: items, - DashboardId: dashId, - OrgId: orgId, - UserId: 1, - } - - err := SaveAlerts(context.Background(), &cmd) - return cmd.Alerts[0], err + err := ss.SaveAlerts(context.Background(), dashId, items) + return items[0], err } func getAlertById(t *testing.T, id int64, ss *SQLStore) (*models.Alert, error) { @@ -376,11 +345,11 @@ func getAlertById(t *testing.T, id int64, ss *SQLStore) (*models.Alert, error) { return q.Result, err } -func pauseAllAlerts(t *testing.T, pauseState bool) error { +func (ss *SQLStore) pauseAllAlerts(t *testing.T, pauseState bool) error { cmd := &models.PauseAllAlertCommand{ Paused: pauseState, } - err := PauseAllAlerts(context.Background(), cmd) + err := ss.PauseAllAlerts(context.Background(), cmd) require.Nil(t, err) return err }