-
Notifications
You must be signed in to change notification settings - Fork 18
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
Use pinpoint for designated templates #2152
Conversation
ba16317
to
0fa34c1
Compare
fc9eeb3
to
3cad558
Compare
3cad558
to
6e3d339
Compare
# "totalMessageParts": 1, | ||
# "totalMessagePrice": 0.00581, | ||
# "totalCarrierFee": 0.006 | ||
# } |
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.
Nice documentation to have in the code!
8f76363
to
6c090ce
Compare
case "phone is currently unreachable/unavailable": | ||
return NOTIFICATION_PERMANENT_FAILURE | ||
|
||
return None |
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.
I think this is the cleanest way to use matching, but I think it's even better with if/elifs:
if "blocked" in response_lower:
return NOTIFICATION_TECHNICAL_FAILURE
elif "invalid" in response_lower:
return NOTIFICATION_TECHNICAL_FAILURE
elif "is opted out" in response_lower:
return NOTIFICATION_PERMANENT_FAILURE
elif "unknown error" in response_lower:
return NOTIFICATION_TECHNICAL_FAILURE
elif "exceed max price" in response_lower:
return NOTIFICATION_TECHNICAL_FAILURE
elif "phone carrier is currently unreachable/unavailable" in response_lower:
return NOTIFICATION_TEMPORARY_FAILURE
elif "phone is currently unreachable/unavailable" in response_lower:
return NOTIFICATION_PERMANENT_FAILURE
else:
return None
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.
Yeah the match case isn't the most terse I have seen in a language unfortunately. I hope Python improves on that. I wouldn't mind the if...elif...else
. :\
current_app.logger.exception(f"Error processing Pinpoint results: {str(e)}") | ||
self.retry(queue=QueueNames.RETRY) | ||
|
||
return |
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.
Not sure we need this return
statement here.
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.
🔥
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.
🥳
Summary | Résumé
We add two main features here:
This code can be merged in and will sit unused unless
In other cases the normal sns code will be used to send the text.
Related Issues | Cartes liées
Test instructions | Instructions pour tester la modification
AWS_PINPOINT_SC_POOL_ID
env variable to the pool id in stagingAWS_PINPOINT_POOL_ID = "pool-b20333ce1e4e49309ba1db3bf94a3f57"
AWS_PINPOINT_SC_TEMPLATE_IDS
to a list of your sms template ids. Just one is ok, ieAWS_PINPOINT_SC_TEMPLATE_IDS = 000000-1111-2222-3333-444444444
AWS Pinpoint request finished in ...
in the celery logsAWS SNS request finished in ...
Release Instructions | Instructions pour le déploiement
None.
Reviewer checklist | Liste de vérification du réviseur