-
-
Notifications
You must be signed in to change notification settings - Fork 8
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
Fix site-wide 404 & Pass along HTTP status info to error handler #123
Conversation
…dividual package pages
Big 👍 on this! Reviewing the diff now... Also, not crashing on 404, sounds like another big 👍 , lol. |
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.
Looks good overall, I had some comments, they're all style things though, so I can approve it as-is basically.
Hope this isn't too thorough/small details.
Also, I am not sure which part of the backend this corresponds to, so I can't personally verify that this works correctly with the backend. Just reading the code from this diff only, and surrounding lines of code for context. With that level of info, it looks reasonable, and passing along specific error codes is SO, SO GOOD from a debugging standpoint than "everything is code 500". Love having more error codes passed to the backend!!! ❤️
Checking if the indentation was as deep as intended is pretty straight-forward, and the rest are style things about some logic checks that I don't feel strongly about. Overall 👍 regardless of these comments. The approve above is valid for merging as-is, if you've considered the comments and determine that no changes are intended/desired. |
@DeeDeeG Thanks a ton for giving this one a review, lots of fantastic feedback, especially about shortening those logic checks. I'll go ahead and address those properly tomorrow, just giving a nod to show I've seen it, thanks! |
@DeeDeeG Thanks a ton for the feedback, sorry it took some time to get too. I've gone ahead and addressed this feedback and updated the commit following all three of your well thought out suggestions. Thanks again and feel free to let me know what you think! |
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.
Again, approved as-is, with optional style feedback.
I waffled a bit on the style of one of the lines (see suggestion below). THAT SAID, the PR is generally great, and I don't mean to be picky regarding feedback. So I emphasize that the feedback is once again optional.
Approve stands with/without the suggested code change. Feel free to merge with or without applying the suggested diff.
Co-authored-by: DeeDeeG <[email protected]>
With the brand new logging we have setup, I was able to find a bug occurring in prod. While the error handler expected an object, it was only being given a
404
status code number during the site wide 404 error. Which would cause it to crash.This PR fixes this, ensuring that site wide 404's are caught properly by the generic error handler.
Additionally, I've gone ahead and added some logic that lets other errors, such as those when displaying an individual package page, can bubble up the HTTP status code they received to the error handler, providing the HTTP error code the user will actually receive. Which can help make the logs easier to inspect, and keeps things much more accurate