From 5b383dc768c52893ff69e64bb23cbb53bcf8503f Mon Sep 17 00:00:00 2001 From: Nathaniel Caza Date: Thu, 1 Jun 2023 12:01:50 -0500 Subject: [PATCH 1/4] add smoketest for issue #2889 --- test/smoke/slackdm2889_test.go | 57 ++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 test/smoke/slackdm2889_test.go diff --git a/test/smoke/slackdm2889_test.go b/test/smoke/slackdm2889_test.go new file mode 100644 index 0000000000..8ead2b3295 --- /dev/null +++ b/test/smoke/slackdm2889_test.go @@ -0,0 +1,57 @@ +package smoke + +import ( + "testing" + + "github.com/target/goalert/expflag" + "github.com/target/goalert/test/smoke/harness" +) + +// TestSlackDM2889 tests that slack DMs update correctly, even when the old status log config is present (issue #2889). +func TestSlackDM2889(t *testing.T) { + t.Parallel() + + sql := ` + insert into users (id, name, email, role) + values + ({{uuid "user"}}, 'bob', 'joe', 'user'); + insert into user_contact_methods (id, user_id, name, type, value) + values + ({{uuid "cm1"}}, {{uuid "user"}}, 'personal', 'SLACK_DM', {{slackUserID "bob"}}), + ({{uuid "cm2"}}, {{uuid "user"}}, 'personal', 'SMS', {{phone "bob"}}); + + update users set alert_status_log_contact_method_id = {{uuid "cm2"}} where id = {{uuid "user"}}; + + insert into user_notification_rules (user_id, contact_method_id, delay_minutes) + values + ({{uuid "user"}}, {{uuid "cm1"}}, 0), + ({{uuid "user"}}, {{uuid "cm1"}}, 30); + + insert into escalation_policies (id, name) + values + ({{uuid "eid"}}, 'esc policy'); + insert into escalation_policy_steps (id, escalation_policy_id) + values + ({{uuid "esid"}}, {{uuid "eid"}}); + insert into escalation_policy_actions (escalation_policy_step_id, user_id) + values + ({{uuid "esid"}}, {{uuid "user"}}); + + insert into services (id, escalation_policy_id, name) + values + ({{uuid "sid"}}, {{uuid "eid"}}, 'service'); + +` + h := harness.NewHarnessWithFlags(t, sql, "slack-dm-cm-type", expflag.FlagSet{expflag.SlackDM}) + defer h.Close() + h.SetConfigValue("Slack.InteractiveMessages", "true") + h.Trigger() // the user's account should get "linked" via compat mgr + + h.CreateAlert(h.UUID("sid"), "testing") + msg := h.Slack().User("bob").ExpectMessage("testing") + msg.Action("Close").Click() + + updated := msg.ExpectUpdate() + updated.AssertText("Closed", "testing") + updated.AssertActions() +} From 9a9afa04634ec4f48a02855656472a0494e79067 Mon Sep 17 00:00:00 2001 From: Nathaniel Caza Date: Thu, 1 Jun 2023 12:06:06 -0500 Subject: [PATCH 2/4] remove message rewrite from old status update code --- engine/message/db.go | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/engine/message/db.go b/engine/message/db.go index 04dbbefad5..90a564a093 100644 --- a/engine/message/db.go +++ b/engine/message/db.go @@ -48,7 +48,6 @@ type DB struct { sentByCMType *sql.Stmt - updateCMStatusUpdate *sql.Stmt cleanupStatusUpdateOptOut *sql.Stmt tempFail *sql.Stmt @@ -174,20 +173,6 @@ func NewDB(ctx context.Context, db *sql.DB, a *alertlog.Store, pausable lifecycl where msg.sent_at > $1 and cm.type = $2 `), - updateCMStatusUpdate: p.P(` - update outgoing_messages msg - set contact_method_id = usr.alert_status_log_contact_method_id - from users usr - where - msg.message_type = 'alert_status_update' and - ( - msg.last_status = 'pending' or - (msg.last_status = 'failed' and msg.next_retry_at notnull) - ) and - msg.contact_method_id != usr.alert_status_log_contact_method_id and - msg.user_id = usr.id and - usr.alert_status_log_contact_method_id notnull - `), cleanupStatusUpdateOptOut: p.P(` delete from outgoing_messages msg using user_contact_methods cm @@ -617,11 +602,6 @@ func (db *DB) _SendMessages(ctx context.Context, send SendFunc, status StatusFun return errors.Wrap(err, "get current time") } - _, err = tx.Stmt(db.updateCMStatusUpdate).ExecContext(execCtx) - if err != nil { - return errors.Wrap(err, "update status update CM preferences") - } - _, err = tx.Stmt(db.cleanupStatusUpdateOptOut).ExecContext(execCtx) if err != nil { return errors.Wrap(err, "clear disabled status updates") From 5e7eb5e334d01bde48777c70ac6ccb45c813ee87 Mon Sep 17 00:00:00 2001 From: Nathaniel Caza Date: Thu, 1 Jun 2023 12:12:01 -0500 Subject: [PATCH 3/4] ignore deprecated status CM --- graphql2/graphqlapp/user.go | 4 +--- user/store.go | 21 ++++++++++----------- user/user.go | 27 +++------------------------ 3 files changed, 14 insertions(+), 38 deletions(-) diff --git a/graphql2/graphqlapp/user.go b/graphql2/graphqlapp/user.go index 9db10844ae..a493bb342a 100644 --- a/graphql2/graphqlapp/user.go +++ b/graphql2/graphqlapp/user.go @@ -143,9 +143,7 @@ func (a *Mutation) UpdateUser(ctx context.Context, input graphql2.UpdateUserInpu if input.Email != nil { usr.Email = *input.Email } - if input.StatusUpdateContactMethodID != nil { - usr.AlertStatusCMID = *input.StatusUpdateContactMethodID - } + return a.UserStore.UpdateTx(ctx, tx, usr) }) return err == nil, err diff --git a/user/store.go b/user/store.go index e269adafa7..01b1b592a0 100644 --- a/user/store.go +++ b/user/store.go @@ -74,17 +74,16 @@ func NewStore(ctx context.Context, db *sql.DB) (*Store, error) { insert: p.P(` INSERT INTO users ( - id, name, email, avatar_url, role, alert_status_log_contact_method_id + id, name, email, avatar_url, role ) - VALUES ($1, $2, $3, $4, $5, $6) + VALUES ($1, $2, $3, $4, $5) `), update: p.P(` UPDATE users SET name = $2, - email = $3, - alert_status_log_contact_method_id = $4 + email = $3 WHERE id = $1 `), @@ -103,7 +102,7 @@ func NewStore(ctx context.Context, db *sql.DB) (*Store, error) { usersMissingProvider: p.P(` SELECT - id, name, email, avatar_url, role, alert_status_log_contact_method_id, false + id, name, email, avatar_url, role, false FROM users WHERE id not in (select user_id from auth_subjects where provider_id = $1) `), @@ -116,7 +115,7 @@ func NewStore(ctx context.Context, db *sql.DB) (*Store, error) { findMany: p.P(` SELECT - u.id, u.name, u.email, u.avatar_url, u.role, u.alert_status_log_contact_method_id, fav is distinct from null + u.id, u.name, u.email, u.avatar_url, u.role, fav is distinct from null FROM users u LEFT JOIN user_favorites fav ON fav.tgt_user_id = u.id AND fav.user_id = $2 @@ -131,7 +130,7 @@ func NewStore(ctx context.Context, db *sql.DB) (*Store, error) { findOneBySubject: p.P(` SELECT - u.id, u.name, u.email, u.avatar_url, u.role, u.alert_status_log_contact_method_id, false + u.id, u.name, u.email, u.avatar_url, u.role, false FROM auth_subjects s JOIN users u ON u.id = s.user_id WHERE s.provider_id = $1 AND s.subject_id = $2 @@ -139,7 +138,7 @@ func NewStore(ctx context.Context, db *sql.DB) (*Store, error) { findOne: p.P(` SELECT - u.id, u.name, u.email, u.avatar_url, u.role, u.alert_status_log_contact_method_id, fav is distinct from null + u.id, u.name, u.email, u.avatar_url, u.role, fav is distinct from null FROM users u LEFT JOIN user_favorites fav ON fav.tgt_user_id = u.id AND fav.user_id = $2 @@ -147,7 +146,7 @@ func NewStore(ctx context.Context, db *sql.DB) (*Store, error) { `), findOneForUpdate: p.P(` SELECT - id, name, email, avatar_url, role, alert_status_log_contact_method_id, false + id, name, email, avatar_url, role, false FROM users WHERE id = $1 FOR UPDATE @@ -161,7 +160,7 @@ func NewStore(ctx context.Context, db *sql.DB) (*Store, error) { findAll: p.P(` SELECT - id, name, email, avatar_url, role, alert_status_log_contact_method_id, false + id, name, email, avatar_url, role, false FROM users `), @@ -501,7 +500,7 @@ func (s *Store) removeUserFromRotation(ctx context.Context, tx *sql.Tx, userID, // Update id equivalent to calling UpdateTx(ctx, nil, u). func (s *Store) Update(ctx context.Context, u *User) error { return s.UpdateTx(ctx, nil, u) } -// UpdateTx allows updating a user name, email, and status update preference. +// UpdateTx allows updating a user name and email. func (s *Store) UpdateTx(ctx context.Context, tx *sql.Tx, u *User) error { err := permission.LimitCheckAny(ctx, permission.System, permission.Admin, permission.MatchUser(u.ID)) if err != nil { diff --git a/user/user.go b/user/user.go index 56b78e4ebc..37a129ae70 100644 --- a/user/user.go +++ b/user/user.go @@ -2,7 +2,6 @@ package user import ( "crypto/md5" - "database/sql" "encoding/hex" "fmt" @@ -28,7 +27,9 @@ type User struct { AvatarURL string // AlertStatusCMID defines a contact method ID for alert status updates. - AlertStatusCMID string `gorm:"column:alert_status_log_contact_method_id"` + // + // Deprecated: No longer used. + AlertStatusCMID string // The Role of the user Role permission.Role @@ -55,47 +56,32 @@ func (u User) ResolveAvatarURL(fullSize bool) string { type scanFn func(...interface{}) error func (u *User) scanFrom(fn scanFn) error { - var statusCM sql.NullString err := fn( &u.ID, &u.Name, &u.Email, &u.AvatarURL, &u.Role, - &statusCM, &u.isUserFavorite, ) - u.AlertStatusCMID = statusCM.String return err } func (u *User) userUpdateFields() []interface{} { - var statusCM sql.NullString - if u.AlertStatusCMID != "" { - statusCM.Valid = true - statusCM.String = u.AlertStatusCMID - } return []interface{}{ u.ID, u.Name, u.Email, - statusCM, } } func (u *User) fields() []interface{} { - var statusCM sql.NullString - if u.AlertStatusCMID != "" { - statusCM.Valid = true - statusCM.String = u.AlertStatusCMID - } return []interface{}{ u.ID, u.Name, u.Email, u.AvatarURL, u.Role, - statusCM, } } @@ -118,13 +104,6 @@ func (u User) Normalize() (*User, error) { ) } - if u.AlertStatusCMID != "" { - err = validate.Many( - err, - validate.UUID("AlertStatusCMID", u.AlertStatusCMID), - ) - } - err = validate.Many( err, validate.Name("Name", u.Name), From c902c920f1ee62c53c02433ff8498448747116ca Mon Sep 17 00:00:00 2001 From: Nathaniel Caza Date: Thu, 1 Jun 2023 12:14:32 -0500 Subject: [PATCH 4/4] update TestStatusUpdatesCancel to use new API --- test/smoke/statusupdatescancel_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/smoke/statusupdatescancel_test.go b/test/smoke/statusupdatescancel_test.go index ef76963c34..f52be900a9 100644 --- a/test/smoke/statusupdatescancel_test.go +++ b/test/smoke/statusupdatescancel_test.go @@ -75,7 +75,11 @@ func TestStatusUpdatesCancel(t *testing.T) { d2.ExpectSMS("first") h.Trigger() // cleanup subscription to cm2, since only cm1 is configured - h.GraphQLQueryUserT(t, h.UUID("user"), fmt.Sprintf(`mutation{updateUser(input:{id:"%s",statusUpdateContactMethodID:"%s"})}`, h.UUID("user"), h.UUID("cm2"))) + h.GraphQLQueryUserT(t, h.UUID("user"), fmt.Sprintf(` + mutation{ + updateUserContactMethod(input:{id:"%s",enableStatusUpdates: false}) + updateUserContactMethod(input:{id:"%s",enableStatusUpdates: true}) + }`, h.UUID("cm1"), h.UUID("cm2"))) h.Trigger() // cleanup subscription to cm1, now that cm2 is the only one configured doClose("first")