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

Allow SMS testing with numbers that are not PhoneTypeCode 0 #251

Merged

Conversation

damiendaemon
Copy link
Contributor

Not every number that can receive an SMS has PhoneTypeCode 0 - for example, toll-free numbers that can be obtained through Pinpoint and used for 2-way messaging.


This work was done by Dae.mn Team. http://dae.mn/ml
Damien.Duff@dae.mn Joe.Major@dae.mn Kyle.Redelinghuys@dae.mn

Not every number that can receive an SMS has PhoneTypeCode 0 - for example, Toll-free numbers that can be obtained through Pinpoint and used for
2-way messaging.
@damiendaemon
Copy link
Contributor Author

The diff without whitespace is more informative:
https://github.com/aws-samples/retail-demo-store/pull/251/files?w=1

Copy link
Contributor

@james-jory james-jory left a comment

Choose a reason for hiding this comment

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

Rather than just logging a message and allowing all phone types through, why not at least prevent known invalid phone types from getting through. Do you know what the phone type is for the Pinpoint 2-way number? Maybe "OTHER"? We should be able to at least deny LANDLINE and INVALID numbers.

https://docs.aws.amazon.com/pinpoint/latest/apireference/phone-number-validate.html#phone-number-validate-prop-numbervalidateresponse-phonetypecode

@damiendaemon
Copy link
Contributor Author

Do you know what the phone type is for the Pinpoint 2-way number? Maybe "OTHER"? We should be able to at least deny LANDLINE and INVALID numbers.

It's VOIP in my experience, also for services like https://receive-smss.com/

I have updated the PR.

Note that the error message does not make it through to the UI at present.

Have a good day!

Copy link
Contributor

@james-jory james-jory left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks.

Noted that we need to fix getting error messages like this back from the service and displayed to the user so they're more helpful.

@james-jory james-jory merged commit 431e635 into aws-samples:master Aug 6, 2021
@damiendaemon damiendaemon deleted the feature/testing-sms-notcode-0 branch November 10, 2021 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants