Skip to content
This repository was archived by the owner on Nov 13, 2021. It is now read-only.

Per-country sender ID (LG-5060)#51

Merged
zachmargolis merged 3 commits intomainfrom
margolis-per-country-sender-id
Sep 3, 2021
Merged

Per-country sender ID (LG-5060)#51
zachmargolis merged 3 commits intomainfrom
margolis-per-country-sender-id

Conversation

@zachmargolis
Copy link
Contributor

I chose to add country_code as a param instead of adding a dependency on Phonelib to parse the numbers inside the gem, which added a lot of noise to the diff.

We're planning to move this back inside the IDP, so maybe it would't be that bad to depend on Phonelib? Open to feedback here,

**Why**: We don't want to send our sender_id to countries
where it is not registered, this changes the behavior in
India
Copy link

@orenyk orenyk left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

We're planning to move this back inside the IDP, so maybe it would't be that bad to depend on Phonelib? Open to feedback here,

Seems like a compelling argument to use it, but don't feel too strongly one way or the other 🤷

@zachmargolis
Copy link
Contributor Author

zachmargolis commented Sep 3, 2021

We're planning to move this back inside the IDP, so maybe it would't be that bad to depend on Phonelib? Open to feedback here,

Seems like a compelling argument to use it, but don't feel too strongly one way or the other 🤷

My other thought, was we can save a few Phonelib.parse with passing in the country and not having to parse again... and small changes to these more heavily trafficked endpoints have added up to big time savings in the past 18F/identity-idp#4330 ➡️ prev example 📉 ... gonna keep this for now

@zachmargolis zachmargolis merged commit b73e77c into main Sep 3, 2021
@zachmargolis zachmargolis deleted the margolis-per-country-sender-id branch September 3, 2021 15:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants