Support for Twilio messaging service #1526
Conversation
googlebot
added
the
cla: yes
|
/hold This may not be a good idea if we wish to steer users away from this feature of Twilio. Hold for discussion. |
|
My high-level feedback is that a MessagingServiceSid is basically a "from" number. The only functional change is the query parameter key that gets sent. From a UX perspective, I think it would be better to use the existing "from" number field (updated some text in the UI). Then, it's trivial for us to detect if the value is a phone number or from number (MSIDs always start with MG). |
I have merged them into one field in the UI - I've left the placeholder as "from number" but updated the small text detail |
pkg/database/migrations.go
Outdated
| ID: "00083-AddTwilioMessageSid", | ||
| Migrate: func(tx *gorm.DB) error { | ||
| return multiExec(tx, | ||
| `ALTER TABLE sms_configs ADD COLUMN IF NOT EXISTS twilio_messaging_service_sid varchar(255)`) |
There was a problem hiding this comment.
Sorry, I was trying to say: I don't think we want a new field/column at all.
- Make sure
fromcan handle 255 characters - Add something in sms.go:
if strings.HasPrefix(from, "MG") { q.Add("MessagingServiceSid", from) } else { q.Add("From", from) }
That should dramatically reduce complexity and avoid boolean logic for if msid else from.
There was a problem hiding this comment.
sure - expanded the existing db column instead.
pkg/database/migrations.go
Outdated
| }, | ||
| Rollback: func(tx *gorm.DB) error { | ||
| return multiExec(tx, | ||
| `ALTER TABLE sms_configs ALTER COLUMN twilio_from_number TYPE varchar(16)`) |
There was a problem hiding this comment.
Let's not rollback the size change, that will lose data unnecessarily.
There was a problem hiding this comment.
sure - we do have similar rollbacks when we've done this in the past:
eg: "ALTER TABLE verification_codes ALTER COLUMN long_code TYPE varchar(20)",
which may also be scary and destroy data
There was a problem hiding this comment.
hmm yea we shouldn't roll back to a smaller column
|
[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 |
|
New changes are detected. LGTM label has been removed. |
Proposed Changes
Release Note