LG-8788: USPS controller, refactor error handling#7812
Conversation
allthesignals
force-pushed
the
wmg/8788-usps-locations-refactor
branch
from
e50b27c to
fd45cea
Compare
|
This is one situation where we are using analytics for something that isn't really typical for analytics_events. Here, we're treating error scenarios as things that belong in analytics, so the code looks awkward. In most cases, AnalyticsEvents' methods being called with a splat from a hashable value. Errors are not hashable, so we have to manually map stuff. I wonder if there's a way to make Faraday error subclasses hashable or describable in some way? |
| Faraday::TimeoutError => :unprocessable_entity, | ||
| Faraday::BadRequestError => :unprocessable_entity, | ||
| Faraday::ForbiddenError => :unprocessable_entity, |
There was a problem hiding this comment.
I wish there were some dictionary of canonical HTTP errors that had things like status code and Rails' symbol. Rack::Utils has this but it's just a simple map. I was hoping Faraday error subclasses would have this info so I could just remap to the correct one!
But, I can't be sure that when Faraday::TimeoutError is instantiated, it's actually being instantiated with the correct response_body. If something times out, is it because the client gave up on waiting, or the server explicitly returned a timeout? 🤷 That's where we'd just lean on the resonse_body (and hash that!) but it seems like that's not always available...
| render json: {}, status: :internal_server_error | ||
| response = proofer.request_facilities(candidate) | ||
| else | ||
| response = proofer.request_pilot_facilities |
There was a problem hiding this comment.
(I know this PR was already merged - I was looking it over while reviewing the refactor of the address search controller and had a question while doing that.)
Why are we returning the pilot facilities here? What's the use case where a user would see this response? Since we've now rolled out nationwide, I'm unclear on when a user would hit this scenario.
There was a problem hiding this comment.
Yeah, that's a good question, I don't know haha. I want to remove it all but wasn't sure if product wanted it for some reason.
There was a problem hiding this comment.
Cool! We can check in with product about this. I'll ask in slack tomorrow.
|
I know this was already merged, but I wanted to chime in and say that I think it looks great! Thanks, @allthesignals ! 🙏🏻 |
3 participants
🎫 Ticket
Link to the relevant ticket.
🛠 Summary of changes
Attempt to refactor error handling by leveraging
rescue_fromRails controller feature.I'm not in love with the approach and open to suggestions!
📜 Testing Plan
Provide a checklist of steps to confirm the changes.