Skip to content

SMS opt-in fixes#5919

Merged
zachmargolis merged 3 commits intomainfrom
margolis-sms-opt-in-fixes
Feb 8, 2022
Merged

SMS opt-in fixes#5919
zachmargolis merged 3 commits intomainfrom
margolis-sms-opt-in-fixes

Conversation

@zachmargolis
Copy link
Contributor

Some follow-ups to #5894 based on testing we did today

  • Update links to help at SP to be external (give them the little ➡️ and open in new tab)
  • Update SmsSender to fail faster (don't retry OPT_OUT errors across regions)

* Troubleshooting options: link to SP should be in new tab
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.

👍

* heading_tag: Tag name for heading. Does not affect visual appearance. Defaults to :h2.
* heading: Heading text.
* options: List of link options to display, as an array of hashes with `url`, `text`, `external` values.
* options: List of link options to display, as an array of hashes with `url`, `text`, `new_tab` values.
Copy link
Contributor

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 🤔

{options.map(({ url, text, isExternal }) => (
<li key={url}>
<BlockLink url={url} isNewTab={isExternal}>

Comment on lines +49 to +50
response.error.is_a?(OptOutError) ||
response.error.is_a?(PermanentFailureError)
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean IdentityConfig.store.allow_sms_resubscribe? Or something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean IdentityConfig.store.allow_sms_resubscribe? Or something else?

Yes. In other words, should this logic only apply if we're allowing SMS resubscribe in the environment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense 👍

return response if response.success?
if response.success? ||
response.error.is_a?(OptOutError) ||
response.error.is_a?(PermanentFailureError)
Copy link
Contributor

Choose a reason for hiding this comment

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

For considering PermanetnFailureError, should we limit it to the cases like what we're matching with this logic?

# Sometimes AWS Pinpoint returns PERMANENT_FAILURE with an "opted out" message
# instead of an OPT_OUT error
# @param [Aws::Pinpoint::Types::MessageResult] message_response_result
def permanent_failure_opt_out?(message_response_result)
message_response_result.delivery_status == 'PERMANENT_FAILURE' &&
message_response_result.status_message&.include?('opted out')
end

On the other hand, if it is a permanent failure, regardless the specific, maybe we don't want failover anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So... that logic you linked turns PERMANENT_FAILURE into OptOutError classes (because it seemed to be logically what it was representing)

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

@zachmargolis zachmargolis merged commit f26b6a3 into main Feb 8, 2022
@zachmargolis zachmargolis deleted the margolis-sms-opt-in-fixes branch February 8, 2022 18:58
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.

2 participants