Skip to content
This repository has been archived by the owner on Jul 12, 2023. It is now read-only.

Add Multi-SMS template columns to realm db #1325

Merged
merged 13 commits into from
Dec 14, 2020
Merged

Add Multi-SMS template columns to realm db #1325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
34c450d
add to db
whaught Dec 11, 2020
85d56d2
include labels
whaught Dec 11, 2020
241f740
validation
whaught Dec 11, 2020
379f836
unit test
whaught Dec 11, 2020
04af347
retrieve realm in test
whaught Dec 11, 2020
ee0f80c
Switch to hstore
whaught Dec 11, 2020
52dd047
Merge remote-tracking branch 'upstream/main' into multi-template
whaught Dec 11, 2020
9f8176c
name
whaught Dec 11, 2020
0051e43
if not exists
whaught Dec 11, 2020
9e5d08c
needless log
whaught Dec 11, 2020
19862e1
Merge remote-tracking branch 'upstream/main' into multi-template
whaught Dec 14, 2020
b264825
Merge remote-tracking branch 'upstream/main' into multi-template
whaught Dec 14, 2020
e748e01
review things
whaught Dec 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion pkg/database/migrations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ func (db *Database) getMigrations(ctx context.Context) *gormigrate.Gormigrate {
{
ID: initState,
Migrate: func(tx *gorm.DB) error {
return nil
// Create required extensions on new DB so AutoMigrate doesn't fail.
return tx.Exec("CREATE EXTENSION IF NOT EXISTS hstore").Error
},
Rollback: func(tx *gorm.DB) error {
return nil
Expand Down Expand Up @@ -1827,6 +1828,21 @@ func (db *Database) getMigrations(ctx context.Context) *gormigrate.Gormigrate {
return nil
},
},
{
ID: "00076-EnableExtension_hstore",
sethvargo marked this conversation as resolved.
Show resolved Hide resolved
Migrate: func(tx *gorm.DB) error {
return tx.Exec("CREATE EXTENSION IF NOT EXISTS hstore").Error
},
},
{
ID: "00077-AddAlternateSMSTemplates",
Migrate: func(tx *gorm.DB) error {
return tx.Exec(`ALTER TABLE realms ADD COLUMN IF NOT EXISTS alternate_sms_templates hstore`).Error
},
Rollback: func(tx *gorm.DB) error {
return tx.Exec(`ALTER TABLE realms DROP COLUMN IF EXISTS alternate_sms_templates`).Error
},
},
})
}

Expand Down
62 changes: 42 additions & 20 deletions pkg/database/realm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import (
"github.com/microcosm-cc/bluemonday"

"github.com/jinzhu/gorm"
"github.com/jinzhu/gorm/dialects/postgres"
"github.com/lib/pq"
"github.com/russross/blackfriday/v2"
)
Expand Down Expand Up @@ -142,7 +143,8 @@ type Realm struct {
LongCodeDuration DurationSeconds `gorm:"type:bigint; not null; default: 86400"` // default 24h

// SMS configuration
SMSTextTemplate string `gorm:"type:text; not null; default: 'This is your Exposure Notifications Verification code: [longcode] Expires in [longexpires] hours'"`
SMSTextTemplate string `gorm:"type:text; not null; default: 'This is your Exposure Notifications Verification code: [longcode] Expires in [longexpires] hours'"`
SMSTextAlternateTemplates postgres.Hstore `gorm:"column:alternate_sms_templates; type:hstore"`

// SMSCountry is an optional field to hint the default phone picker country
// code.
Expand Down Expand Up @@ -358,28 +360,21 @@ func (r *Realm) BeforeSave(tx *gorm.DB) error {
r.AddError("longCodeDuration", "must be no more than 24 hours")
}

if r.EnableENExpress {
if !strings.Contains(r.SMSTextTemplate, SMSENExpressLink) {
r.AddError("SMSTextTemplate", fmt.Sprintf("must contain %q", SMSENExpressLink))
}
if strings.Contains(r.SMSTextTemplate, SMSRegion) {
r.AddError("SMSTextTemplate", fmt.Sprintf("cannot contain %q - this is automatically included in %q", SMSRegion, SMSENExpressLink))
}
if strings.Contains(r.SMSTextTemplate, SMSLongCode) {
r.AddError("SMSTextTemplate", fmt.Sprintf("cannot contain %q - the long code is automatically included in %q", SMSLongCode, SMSENExpressLink))
}

} else {
// Check that we have exactly one of [code] or [longcode] as template substitutions.
if c, lc := strings.Contains(r.SMSTextTemplate, SMSCode), strings.Contains(r.SMSTextTemplate, SMSLongCode); !(c || lc) || (c && lc) {
r.AddError("SMSTextTemplate", fmt.Sprintf("must contain exactly one of %q or %q", SMSCode, SMSLongCode))
r.validateSMSTemplate(r.SMSTextTemplate)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API is a bit weird since it doesn't return anything. I don't have a better solution though.

if r.SMSTextAlternateTemplates != nil {
for l, t := range r.SMSTextAlternateTemplates {
if t == nil || *t == "" {
r.AddError("SMSTextTemplate", fmt.Sprintf("no template for label %s", l))
continue
}
if l == "" {
r.AddError("SMSTextTemplate", fmt.Sprintf("no label for template %s", *t))
continue
}
r.validateSMSTemplate(*t)
}
}

// Check template length.
if l := len(r.SMSTextTemplate); l > SMSTemplateMaxLength {
r.AddError("SMSTextTemplate", fmt.Sprintf("must be %v characters or less, current message is %v characters long", SMSTemplateMaxLength, l))
}
// Check expansion length based on settings.
fakeCode := fmt.Sprintf(fmt.Sprintf("\\%0%d\\%d", r.CodeLength), 0)
fakeLongCode := fmt.Sprintf(fmt.Sprintf("\\%0%d\\%d", r.LongCodeLength), 0)
Expand Down Expand Up @@ -434,6 +429,33 @@ func (r *Realm) BeforeSave(tx *gorm.DB) error {
return nil
}

// validateSMSTemplate is a helper method to validate a single SMSTemplate.
// Errors are returned by appending them to the realm's Errorable fields.
func (r *Realm) validateSMSTemplate(t string) {
whaught marked this conversation as resolved.
Show resolved Hide resolved
if r.EnableENExpress {
if !strings.Contains(t, SMSENExpressLink) {
r.AddError("SMSTextTemplate", fmt.Sprintf("must contain %q", SMSENExpressLink))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these errors aren't going to be right, since they are for SMSTextTemplate

validation is going to be tricky...

this is where I think breaking these out into a separate table so you had a stable ID that could be part of a form field ID would be valuable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should Take the field name as another param to this function. We could also prefix the error with the label name for alt templates.

That being said, we'll mark these as required in the HTML, so the only way someone gets to these errors is if they do some significant mucking about. I think it's probably fine to keep this as-is and create a follow-up issue to make it betterer in the future. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can also include the label in the error message - I'm worried about a user not being able to tell where the error is
and therefore not be able to fix it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine w/ doing the follow up when the UI is added.

}
if strings.Contains(t, SMSRegion) {
r.AddError("SMSTextTemplate", fmt.Sprintf("cannot contain %q - this is automatically included in %q", SMSRegion, SMSENExpressLink))
}
if strings.Contains(t, SMSLongCode) {
r.AddError("SMSTextTemplate", fmt.Sprintf("cannot contain %q - the long code is automatically included in %q", SMSLongCode, SMSENExpressLink))
}

} else {
// Check that we have exactly one of [code] or [longcode] as template substitutions.
if c, lc := strings.Contains(t, SMSCode), strings.Contains(t, SMSLongCode); !(c || lc) || (c && lc) {
r.AddError("SMSTextTemplate", fmt.Sprintf("must contain exactly one of %q or %q", SMSCode, SMSLongCode))
}
}

// Check template length.
if l := len(t); l > SMSTemplateMaxLength {
r.AddError("SMSTextTemplate", fmt.Sprintf("must be %d characters or less, current message is %v characters long", SMSTemplateMaxLength, l))
}
}

// GetCodeDurationMinutes is a helper for the HTML rendering to get a round
// minutes value.
func (r *Realm) GetCodeDurationMinutes() int {
Expand Down
39 changes: 38 additions & 1 deletion pkg/database/realm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ func TestValidation(t *testing.T) {
db, _ := testDatabaseInstance.NewDatabase(t, nil)
os.Setenv("ENX_REDIRECT_DOMAIN", "https://en.express")

valid := "State of Wonder, COVID-19 Exposure Verification code [code]. Expires in [expires] minutes. Act now!"

cases := []struct {
Name string
Input *Realm
Expand Down Expand Up @@ -191,12 +193,47 @@ func TestValidation(t *testing.T) {
},
Error: "SMSTextTemplate when expanded, the result message is too long (3168 characters). The max expanded message is 918 characters",
},
{
Name: "valid",
Input: &Realm{
Name: "a",
CodeLength: 6,
LongCodeLength: 12,
EnableENExpress: false,
SMSTextTemplate: valid,
},
},
{
Name: "alternate_sms_template",
Input: &Realm{
Name: "a",
EnableENExpress: false,
SMSTextTemplate: valid,
SMSTextAlternateTemplates: map[string]*string{"alternate1": nil},
},
Error: "no template for label alternate1",
},
{
Name: "alternate_sms_template valid",
Input: &Realm{
Name: "b",
CodeLength: 6,
LongCodeLength: 12,
EnableENExpress: false,
SMSTextTemplate: valid,
SMSTextAlternateTemplates: map[string]*string{"alternate1": &valid},
},
},
}

for _, tc := range cases {
t.Run(tc.Name, func(t *testing.T) {
if err := db.SaveRealm(tc.Input, SystemTest); err == nil {
t.Fatalf("expected error: %q got: nil", tc.Error)
if tc.Error != "" {
t.Fatalf("expected error: %q got: nil", tc.Error)
}
} else if tc.Error == "" {
t.Fatalf("expected no error, got %q", err.Error())
} else if !strings.Contains(err.Error(), tc.Error) {
t.Fatalf("wrong error, want: %q got: %q", tc.Error, err.Error())
}
Expand Down