LG-14442: Add error handling and invalid character check to public usps locations controller#11470
Conversation
changelog: Internal, In-person Proofing, Adding graceful error handling and analytics in public usps locations controller
jennyverdeyen
force-pushed
the
jverdeyen/LG-14442-handle-usps-errors-public-po-search
branch
from
2ea2856 to
a053ada
Compare
eileen-nava
left a comment
eileen-nava
left a comment
There was a problem hiding this comment.
Looks good to me. I left two questions. No blocking feedback.
|
|
||
| include IppHelper | ||
|
|
||
| rescue_from ActionController::InvalidAuthenticityToken, |
There was a problem hiding this comment.
@jennyverdeyen What would cause an ActionController::InvalidAuthenticityToken error to occur?
There was a problem hiding this comment.
Good question! My understanding is that it occurs when a there is a missing or mismatched CSRF token: https://dev.to/ben/actioncontroller-invalidauthenticitytoken-what-s-going-on-here-2828
I'm not sure more specifically how it could be triggered in this controller, but I deduced it would be worth catching since it was also being caught in the other version of this controller
There was a problem hiding this comment.
This controller is "public" and includes skip_forgery_protection, which means the CSRF token is not being checked. Because of that, it should be safe to remove ActionController::InvalidAuthenticityToken.
There was a problem hiding this comment.
@mitchellhenke Thanks for the explanation, that makes sense! I'll remove it.
There was a problem hiding this comment.
This was educational, thanks. 🙏🏻
| ) | ||
|
|
||
| unless candidate.has_valid_address? | ||
| raise UspsLocationsError.new |
There was a problem hiding this comment.
What's the benefit of using raise UspsLocationsError.new instead of raise UspsLocationsError? (I'm asking because I'm curious, I'm not asking because I think it needs to be changed. I see both patterns being used in our codebase.)
There was a problem hiding this comment.
Ooh also a great question. These sources offer an explanation that they are essentially the same: https://blog.arkency.com/ruby-raise-exception-dot-new-or-raise-exception-theyre-both-the-same/
https://stackoverflow.com/questions/31373514/difference-between-rails-raise-standarderror-new-and-raise-standarderror
I think I'll use the simpler one!
There was a problem hiding this comment.
Sounds good. Thanks for linking the blog. 🙏🏻
| module Public | ||
| class UspsLocationsError < StandardError | ||
| def initialize | ||
| super('Unsupported characters in address field.') |
There was a problem hiding this comment.
no biggie, but I noticed sometimes there's a pattern to add these error messages as constants. Just wanted to pop that in here as food for thought but non-blocking.
jennyverdeyen
deleted the
jverdeyen/LG-14442-handle-usps-errors-public-po-search
branch
5 participants
🎫 Ticket
Link to the relevant ticket:
LG-14442
🛠 Summary of changes
Adds error handling to the public USPS locations controller. Also adds a check for invalid characters before making the call to USPS, otherwise throwing a UspsLocationsError. Analytics added for when errors occur.
📜 Testing Plan
In the identity-site which calls this public controller, the front end checks should prevent invalid characters from being submitted before the controller should have to handle it. So instead of a manual test plan, automated tests have been added to test that this check and the error handling works.