Closed Bug 1022635 Opened 5 years ago Closed 5 years ago
Profile .reverse _geocode can raise Connection Error blocking User Profile .save()
UserProfile.reverse_geocode depends on an external service which can fail or we can fail to connect to it, for example when developing mozillians.org without an internet connection. Currently we only handle HTTPError errors in UserProfile.reverse_geocode but a ConnectionError is possible, as well as other errors. I suggest that we move the external service exception handling to geo.lookup and broanden it to handle also ConnectionError exceptions. If things break geo.lookup should raise a GeoLookupException instead of ConnectionError, HTTPError or other errors. UserProfile.save() should not block saving user's profile. Also we should properly log and monitor lookup errors (to graphite and with admin emails) and also message users on the website when we fail to geocode their location.
When mapbox fixes their issue with ID-based queries(some just flat-out fail for no reason, theyre aware of the bug), we won't need to query their API server side for every single profile save anymore, we can move it primarily into the JS.
Is there a public bug url that mapbox tracks this fix? If yes can you please create a bug to track the removal of our server side code when mapbox's bug is fixed?
(hit enter a bit too early) Meanwhile we need to fix this in our code b/c otherwise things can break, correct?
Yeah, we do need to fix this meanwhile.
Commits pushed to master at https://github.com/mozilla/mozillians https://github.com/mozilla/mozillians/commit/5ef3310e044599b5ea1e9f0a1e4cd5871def40fe [Fix Bug 1022635] Improve error handling on mapbox api calls https://github.com/mozilla/mozillians/commit/0720b6dd1a86022950a70096a929fb70b996f423 Merge pull request #984 from Sancus/geocodingfixes [geolocation] Fixes for bugs 1029326 and 1022635
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
There's no way to test for this on the site short of mapbox going down, and it's covered by unit tests quite well.
Bumping to verified as [qa-] per comment 6.. thanks sancus
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.