Conversation
**Why**: So users don't get 500 errors when signing in. This is a follow up to the previous commit which adds the following: - Rescue Twilio errors when signing in and display the OTP verification page with an error message, allowing the user to try the phone call option, or signing in with their personal key - Capture the country code and context along with Twilio errors so we can more easily see which countries are affected - Don't change the user's OTP delivery preference if the last preference they used resulted in a Twilio error. For example, if a user normally uses voice, but for some reason didn't get the phone call, and then tries to send an SMS to a landline, resulting in an error message, we should leave their preference as voice so they won't get an error the next time they sign in. - Add the phone number country to the Exception Notification emails
| end | ||
| end | ||
|
|
||
| def update_otp_delivery_preference_if_needed |
There was a problem hiding this comment.
This makes a lot more sense here
| redirect_back(fallback_location: account_url) | ||
| end | ||
|
|
||
| # rubocop:disable Metrics/MethodLength |
There was a problem hiding this comment.
This method is a bit long. What would you think about moving the top part into a base_analytics_hash_for_exception method or something like that?
There was a problem hiding this comment.
yeah, I can do that. Ideally, we'd get rid of the background jobs for voice/sms and use a Form Object instead, and we can move all this logic inside there, and avoid using exceptions for control flow.
|
Knowing that we are planning on refactoring this, I'm going to merge this as is, just so we have something we can deploy if necessary. As of earlier this morning, Twilio has disabled their Lookup feature for us, so users are no longer seeing 500 errors. They are investigating why numbers that were able to receive SMS in the past are now being treated as landlines that can't receive SMS. |
**Why**: This TODO that had been previously removed got added back by mistake in #2308 and I forgot to remove it before merging the PR.
| User UUID: <%= @kontroller.analytics_user.uuid %> | ||
| <% user = @kontroller.analytics_user %> | ||
| User UUID: <%= user.uuid %> | ||
| User's Country (based on phone): <%= Phonelib.parse(user.phone).country %> |
There was a problem hiding this comment.
@kontroller.analytics_user may be nil, so in some circumstances the exception notifier itself is exploding here
There was a problem hiding this comment.
If there is a controller, analytics_user cannot be nil. If there is no current_user, it is an instance of AnonymousUser. Perhaps what is happening is that @kontroller can be nil in some cases. Do you have a specific scenario?
There was a problem hiding this comment.
There won't be a controller if rails dies before serving a request. Encountered this in my environment.
There was a problem hiding this comment.
ERROR: Failed to generate exception summary:
ActionView::Template::Error: undefined method `uuid' for nil:NilClass
/srv/idp/releases/chef/app/views/exception_notifier/_session.text.erb:19:in `_app_views_exception_notifier__session_text_erb__269420484499780557_47443798649360'
There was a problem hiding this comment.
Actually that message pretty strongly points to analytics_user being nil when the controller is present.
There was a problem hiding this comment.
In this case triggered by a redis connection failure.
A Redis::CannotConnectError occurred in #:
Error connecting to Redis on redis.login.gov.internal:6379 (Errno::ECONNREFUSED)
lib/utf8_sanitizer.rb:20:in `call'
There was a problem hiding this comment.
I see it now in the logs. Looks like it's happening before it gets to the controller. I will fix.
4 participants
Why: So users don't get 500 errors when signing in.
This is a follow up to the previous commit which adds the following:
Rescue Twilio errors when signing in and display the OTP verification
page with an error message, allowing the user to try the phone call
option, or signing in with their personal key
Capture the country code and context along with Twilio errors so we
can more easily see which countries are affected
Don't change the user's OTP delivery preference if the last preference
they used resulted in a Twilio error. For example, if a user normally
uses voice, but for some reason didn't get the phone call, and then
tries to send an SMS to a landline, resulting in an error message, we
should leave their preference as voice so they won't get an error the
next time they sign in.
Add the phone number country to the Exception Notification emails
Hi! Before submitting your PR for review, and/or before merging it, please
go through the following checklist:
For DB changes, check for missing indexes, check to see if the changes
affect other apps (such as the dashboard), make sure the DB columns in the
various environments are properly populated, coordinate with devops, plan
migrations in separate steps.
For route changes, make sure GET requests don't change state or result in
destructive behavior. GET requests should only result in information being
read, not written.
For encryption changes, make sure it is compatible with data that was
encrypted with the old code.
For secrets changes, make sure to update the S3 secrets bucket with the
new configs in all environments.
Do not disable Rubocop or Reek offenses unless you are absolutely sure
they are false positives. If you're not sure how to fix the offense, please
ask a teammate.
When reading data, write tests for nil values, empty strings,
and invalid formats.
When calling
redirect_toin a controller, use_url, not_path.When adding user data to the session, use the
user_sessionhelperinstead of the
sessionhelper so the data does not persist beyond the user'ssession.
When adding a new controller that requires the user to be fully
authenticated, make sure to add
before_action :confirm_two_factor_authenticated.