-
Notifications
You must be signed in to change notification settings - Fork 166
SMS opt-in fixes #5919
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
SMS opt-in fixes #5919
Changes from all commits
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 | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -45,7 +45,11 @@ def send(message:, to:, country_code:, otp: nil) | |||||||||||||||
| ) | ||||||||||||||||
| finish = Time.zone.now | ||||||||||||||||
| response = build_response(pinpoint_response, start: start, finish: finish) | ||||||||||||||||
| return response if response.success? | ||||||||||||||||
| if response.success? || | ||||||||||||||||
| response.error.is_a?(OptOutError) || | ||||||||||||||||
| response.error.is_a?(PermanentFailureError) | ||||||||||||||||
|
Comment on lines
+49
to
+50
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. Should we consider the SMS opt-in configuration in deciding to fail fast or allow failover?
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. Do you mean
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.
Yes. In other words, should this logic only apply if we're allowing SMS resubscribe in the environment?
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. My thinking is no, this logic should apply always, opt out means "don't send me SMS" so I think we don't want to failover or retry if we get that response. We have older opt-out detecting logic that shows a flash error "you have opted out" (that isn't actionable), so this would result in that
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. Makes sense 👍
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. For considering identity-idp/lib/telephony/pinpoint/sms_sender.rb Lines 177 to 183 in 12f214f
On the other hand, if it is a permanent failure, regardless the specific, maybe we don't want failover anyways.
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. So... that logic you linked turns I figure other permanent failures might include numbers that don't exist, or something so they seemed worth terminating on as well: https://docs.aws.amazon.com/pinpoint/latest/apireference/apps-application-id-messages.html |
||||||||||||||||
| return response | ||||||||||||||||
| end | ||||||||||||||||
| PinpointHelper.notify_pinpoint_failover( | ||||||||||||||||
| error: response.error, | ||||||||||||||||
| region: sms_config.region, | ||||||||||||||||
|
|
||||||||||||||||
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.
Whoops 😅 Thanks for fixing.
Interestingly, for reasons I cannot recall, we call it both things in the JSX implementation 🤔
identity-idp/app/javascript/packages/components/troubleshooting-options.jsx
Lines 34 to 36 in 12f214f