Skip to content
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

Rescue notify api errors #3276

Merged
merged 1 commit into from
Oct 15, 2024
Merged

Rescue notify api errors #3276

merged 1 commit into from
Oct 15, 2024

Conversation

rjlynch
Copy link
Contributor

@rjlynch rjlynch commented Oct 4, 2024

Notify uses the https://pypi.org/project/phonenumbers/ library to
validate phone numbers. There's some scenarios where our regex permits a
phone number that libphonenumber doesn't allow, causing notify to return
an error when sending to that number.
We've gone for rescuing this api error and adding a validation error to
the the form. We may at some point, especially if we move message sending
into a background job, want to investigate using a gem like
https://github.com/daddyz/phonelib that will provide stricter phone
number validations.

@rjlynch rjlynch added the deploy Deploy a review app for this PR label Oct 4, 2024
@rjlynch rjlynch marked this pull request as ready for review October 4, 2024 15:29
@rjlynch rjlynch force-pushed the LUPEYALPHA-1116/notify-errors branch from 78c66a8 to d2209e1 Compare October 14, 2024 15:37
@rjlynch rjlynch requested a review from asmega October 14, 2024 16:15
Notify uses the https://pypi.org/project/phonenumbers/ library to
validate phone numbers. There's some scenarios where our regex permits a
phone number that libphonenumber doesn't allow, causing notify to return
an error when sending to that number.
We've gone for rescuing this api error and adding a validation error to
the the form. We may at some point, especially if we move message sending
into a background job, want to investigate using a gem like
https://github.com/daddyz/phonelib that will provide stricter phone
number validations.
@rjlynch rjlynch force-pushed the LUPEYALPHA-1116/notify-errors branch from d2209e1 to a7073ad Compare October 15, 2024 15:38
@rjlynch rjlynch merged commit d5d2c26 into master Oct 15, 2024
14 checks passed
@rjlynch rjlynch deleted the LUPEYALPHA-1116/notify-errors branch October 15, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy Deploy a review app for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants