LG-10123: Add data model for notification SMS numbers#8682
Conversation
sheldon-b
force-pushed
the
sbachstein/lg-10123-in-person-sms-data-model
branch
from
1859259 to
e0f801a
Compare
sheldon-b
requested review from
a team and
allthesignals
and removed request for
a team
db/primary_migrate/20230627213457_add_notification_phone_configurations_table.rb
Outdated
Show resolved
Hide resolved
sheldon-b
force-pushed
the
sbachstein/lg-10123-in-person-sms-data-model
branch
from
e0f801a to
968d19b
Compare
zachmargolis
left a comment
zachmargolis
left a comment
There was a problem hiding this comment.
LGTM
Maybe we can add it later when we get there, but we could also add user.notification_phone_configurations as a has_many ..., :through relation? docs link
… for sms notifications
sheldon-b
force-pushed
the
sbachstein/lg-10123-in-person-sms-data-model
branch
from
fb1afc7 to
4bc9ef4
Compare
| return '' if phone.blank? | ||
|
|
||
| formatted = Phonelib.parse(phone).national | ||
| formatted[0..-5].gsub(/\d/, '*') + formatted[-4..-1] |
There was a problem hiding this comment.
This could use some fresh eyes -- for now I've just copied the implementation over from PhoneConfiguration. A separate PR/ticket will refresh this
There was a problem hiding this comment.
it looks good to me. What will the purpose of the refresh be?
There was a problem hiding this comment.
It has a different interface from the above format method -- this one returns an empty string for invalid phone numbers and format returns nil. And this one is focused on national numbers whereas format has explicit support for international. That's all I have in mind
|
|
||
| def formatted_phone | ||
| Phonelib.parse(phone).international | ||
| PhoneFormatter.format(phone) |
There was a problem hiding this comment.
It took me some digging, but I'm confident that these two are equivalent; PhoneFormatter.format sends 'US' as the country_code, which the code you're replacing does not, but we set Phonelib's default country to 'US' in config/initializers/phonelib.rb.
| return '' if phone.blank? | ||
|
|
||
| formatted = Phonelib.parse(phone).national | ||
| formatted[0..-5].gsub(/\d/, '*') + formatted[-4..-1] |
There was a problem hiding this comment.
it looks good to me. What will the purpose of the refresh be?
4 participants
🎫 Ticket
LG-10123
🛠 Summary of changes