-
Notifications
You must be signed in to change notification settings - Fork 166
LG-10123: Add data model for notification SMS numbers #8682
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
dc37a53
758bd2b
443dec1
5ac840a
05ed1be
5f06653
4bc9ef4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| class NotificationPhoneConfiguration < ApplicationRecord | ||
| include EncryptableAttribute | ||
|
|
||
| belongs_to :in_person_enrollment, inverse_of: :notification_phone_configuration | ||
| validates :encrypted_phone, presence: true | ||
|
|
||
| encrypted_attribute(name: :phone) | ||
|
|
||
| def formatted_phone | ||
| PhoneFormatter.format(phone) | ||
| end | ||
|
|
||
| def masked_phone | ||
| PhoneFormatter.mask(phone) | ||
| end | ||
|
|
||
| def friendly_name | ||
| :phone | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,4 +5,11 @@ def self.format(phone, country_code: nil) | |
| country_code = DEFAULT_COUNTRY if country_code.nil? && !phone&.start_with?('+') | ||
| Phonelib.parse(phone, country_code)&.international | ||
| end | ||
|
|
||
| def self.mask(phone) | ||
| return '' if phone.blank? | ||
|
|
||
| formatted = Phonelib.parse(phone).national | ||
| formatted[0..-5].gsub(/\d/, '*') + formatted[-4..-1] | ||
|
Comment on lines
+10
to
+13
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could use some fresh eyes -- for now I've just copied the implementation over from PhoneConfiguration. A separate PR/ticket will refresh this
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it looks good to me. What will the purpose of the refresh be?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It has a different interface from the above |
||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| class AddNotificationPhoneConfigurationsTable < ActiveRecord::Migration[7.0] | ||
| def change | ||
| create_table :notification_phone_configurations do |t| | ||
| t.references :in_person_enrollment, null: false, index: { name: 'index_notification_phone_configurations_on_enrollment_id', unique: true } | ||
| t.text :encrypted_phone, null: false, comment: 'Encrypted phone number to send notifications to' | ||
| t.timestamps | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| FactoryBot.define do | ||
| Faker::Config.locale = :en | ||
|
|
||
| factory :notification_phone_configuration do | ||
| phone { '+1 202-555-1212' } | ||
| in_person_enrollment { association :in_person_enrollment } | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| require 'rails_helper' | ||
|
|
||
| RSpec.describe NotificationPhoneConfiguration do | ||
| describe 'Associations' do | ||
| it { is_expected.to belong_to(:in_person_enrollment) } | ||
| it { is_expected.to validate_presence_of(:encrypted_phone) } | ||
| end | ||
|
|
||
| let(:phone) { '+1 703 555 1212' } | ||
|
|
||
| let(:notification_phone_configuration) { create(:notification_phone_configuration, phone: phone) } | ||
|
|
||
| describe 'creation' do | ||
| it 'stores an encrypted form of the phone number' do | ||
| expect(notification_phone_configuration.encrypted_phone).to_not be_blank | ||
| end | ||
| end | ||
|
|
||
| describe 'encrypted attributes' do | ||
| it 'decrypts phone' do | ||
| expect(notification_phone_configuration.phone).to eq phone | ||
| end | ||
|
|
||
| context 'with unnormalized phone' do | ||
| let(:phone) { ' 555 555 5555 ' } | ||
| let(:normalized_phone) { '555 555 5555' } | ||
|
|
||
| it 'normalizes phone' do | ||
| expect(notification_phone_configuration.phone).to eq normalized_phone | ||
| end | ||
| end | ||
| end | ||
|
|
||
| describe '#masked_phone' do | ||
| let(:notification_phone_configuration) do | ||
| build(:notification_phone_configuration, phone: phone) | ||
| end | ||
| let(:phone) { '+1 703 555 1212' } | ||
|
|
||
| subject(:masked_phone) { notification_phone_configuration.masked_phone } | ||
|
|
||
| it 'masks the phone number, leaving the last 4 digits' do | ||
| expect(masked_phone).to eq('(***) ***-1212') | ||
| end | ||
|
|
||
| context 'with a blank phone number' do | ||
| let(:phone) { ' ' } | ||
|
|
||
| it 'is the empty string' do | ||
| expect(masked_phone).to eq('') | ||
| end | ||
| end | ||
|
|
||
| context 'with an international number' do | ||
| let(:phone) { '+212 636-023853' } | ||
|
|
||
| it 'keeps the groupings and leaves the last 4 digits' do | ||
| expect(masked_phone).to eq('****-**3853') | ||
gina-yamada marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| end | ||
| end | ||
| end | ||
| end | ||
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.
It took me some digging, but I'm confident that these two are equivalent;
PhoneFormatter.formatsends'US'as the country_code, which the code you're replacing does not, but we set Phonelib's default country to'US'inconfig/initializers/phonelib.rb.