Merge pull request #3060 from target/bug/2889-fix-slack-dm-status-upd…

…ates

fix slack dm status updates

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")

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

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()
+}

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")

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,23 +130,23 @@ 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
 		`),
 
 		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
 			WHERE u.id = $1
 		`),
 		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 {

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),