allow specifiction of realm alert SMS numbers #2237
Conversation
<td> | ||
{{if .DeletedAt}} | ||
<span class="bi bi-x-square-fill text-danger me-1" | ||
data-bs-toggle="tooltip" title="phone number is disabled and will not receive notifications - it will be deleted in a few days"></span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data-bs-toggle="tooltip" title="phone number is disabled and will not receive notifications - it will be deleted in a few days"></span> | |
data-bs-toggle="tooltip" title="This phone number is disabled and will NOT receive notifications - it will be deleted in a few days"></span> |
data-bs-toggle="tooltip" title="phone number is disabled and will not receive notifications - it will be deleted in a few days"></span> | ||
{{else}} | ||
<span class="bi bi-check-square-fill text-success me-1" | ||
data-bs-toggle="tooltip" title="phone number is enabled and will receive notifications"></span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data-bs-toggle="tooltip" title="phone number is enabled and will receive notifications"></span> | |
data-bs-toggle="tooltip" title="This phone number is enabled and will receive notifications"></span> |
<thead> | ||
<tr> | ||
<th scope="col">Name</th> | ||
<th scope="col" width="200" class="d-none d-md-table-cell">Phone Number</th> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<th scope="col" width="200" class="d-none d-md-table-cell">Phone Number</th> | |
<th scope="col" width="200" class="d-none d-md-table-cell">Phone number</th> |
<a href="/realm/notifications/{{.ID}}/enable" id="enable-rap-{{.ID}}" | ||
class="d-block text-danger" | ||
data-method="patch" | ||
data-confirm="Are you sure you want to resume sending notifications to '{{.Name}}'?" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data-confirm="Are you sure you want to resume sending notifications to '{{.Name}}'?" | |
data-confirm="Are you sure you want to resume sending notifications to {{.PhoneNumber}} ({{.Name}})?" |
<a href="/realm/notifications/{{.ID}}/disable" id="disable-rap-{{.ID}}" | ||
class="d-block text-danger" | ||
data-method="patch" | ||
data-confirm="Are you sure you want to disable sending notifications to '{{.Name}}'?" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data-confirm="Are you sure you want to disable sending notifications to '{{.Name}}'?" | |
data-confirm="Are you sure you want to disable sending notifications to {{.PhoneNumber}} ({{.Name}})?" |
@@ -23,6 +23,11 @@ type FeatureConfig struct { | |||
// TODO(sethvargo): default to true and remove in 1.0.4+. | |||
NotifyAnomalies bool `env:"NOTIFY_ANOMALIES"` | |||
|
|||
// RealmNotificationSMSAlerts enables sending realm specific alerts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we still use "Alerts" here?
now := time.Now().UTC() | ||
realmAdminPhone.DeletedAt = &now | ||
if err := c.db.SaveRealmAdminPhone(currentRealm, realmAdminPhone, currentUser); err != nil { | ||
flash.Error("Failed to disable realm alerts to phone number: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flash.Error("Failed to disable realm alerts to phone number: %v", err) | |
flash.Error("Failed to disable realm notifications: %v", err) |
return | ||
} | ||
|
||
flash.Alert("Successfully disabled realm alerts for '%v'", realmAdminPhone.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flash.Alert("Successfully disabled realm alerts for '%v'", realmAdminPhone.Name) | |
flash.Alert("Successfully disabled realm notifications for '%v'", realmAdminPhone.Name) |
controller.InternalError(w, r, c.h, err) | ||
return | ||
} else if !hasSMS { | ||
flash.Alert("There is no SMS configuration for this realm, so any enabled notifications will not be sent via SMS.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda feel like we shouldn't allow this without an sms config set.
I also question if we should use the realm's SMS config or require a system sms config. In the most recent incident, using the realm's SMS config would have NOT triggered an alert.
@@ -1057,6 +1057,29 @@ func (r *Realm) ListSMSSigningKeys(db *Database) ([]*SMSSigningKey, error) { | |||
return keys, nil | |||
} | |||
|
|||
func (r *Realm) ListAdminPhones(db *Database, p *pagination.PageParams, scopes ...Scope) ([]*NotificationPhone, *pagination.Paginator, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doc
@mikehelmick should we consider putting an artificial cap on the number of notification phone numbers per realm? 10 seems like a really generous number. |
towards #2236
Proposed Changes
Menu
Phone number panel
Release Note