Add Multi-SMS template columns to realm db #1325
Conversation
googlebot
added
the
cla: yes
| // 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) |
There was a problem hiding this comment.
This API is a bit weird since it doesn't return anything. I don't have a better solution though.
| func (r *Realm) validateSMSTemplate(t string) { | ||
| if r.EnableENExpress { | ||
| if !strings.Contains(t, SMSENExpressLink) { | ||
| r.AddError("SMSTextTemplate", fmt.Sprintf("must contain %q", SMSENExpressLink)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm fine w/ doing the follow up when the UI is added.
| func (r *Realm) validateSMSTemplate(t string) { | ||
| if r.EnableENExpress { | ||
| if !strings.Contains(t, SMSENExpressLink) { | ||
| r.AddError("SMSTextTemplate", fmt.Sprintf("must contain %q", SMSENExpressLink)) |
There was a problem hiding this comment.
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?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sethvargo, whaught The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold I hear you on errors - I was intending to solve that in the follow up with more complete UX |
Issue #1320
Proposed Changes