Skip to content
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

Update Mobilize connector URI, fix display of errors, fix auth issue #708

Merged
merged 5 commits into from
Jul 9, 2022

Conversation

shaunagm
Copy link
Collaborator

@shaunagm shaunagm commented Jul 6, 2022

1 - Most importantly, this updates the base URI for the Mobilize connector, which was no longer working. WARNING this changes from HTTP to HTTPS which may cause authentication issues? I don't know. The Mobilize Connector has its own custom requests code and doesn't use APIConnector, which I assume would handle the change fine.

2 - Also fixed an issue where the auth information wasn't getting passed from request_paginate to request, which lead to auth errors.

3 - Also (and this is the original reason I opened this PR):

The Mobilize connector uses the following logic to check for request errors:

if 'error' in r.json():

Unfortunately, for most unsuccessful API calls (including 404 file not found, 401 authentication, etc) no json is returned. Because we try to parse it anyway, the user sees an unreadable JSONDecodeError, like simplejson.errors.JSONDecodeError: Expecting value: line 1 column 1 (char 0) instead of the more helpful error message that requests would otherwise return.

This fix uses the helper method provided by requests, r.raise_for_status() to surface most errors, before moving on to the line if 'error' in r.json(): (which may still be helpful in a subset of cases).

The better fix here is to rewrite the Mobilize connector to use the APIConnector utility, which was created precisely so we wouldn't have to duplicate this kind of general request logic within each connector, but that's a bigger change I don't have time for right now.

@shaunagm shaunagm changed the title Raises readable errors when unsuccessful status codes returned Update Mobilize connector URI, fix display of errors Jul 6, 2022
@shaunagm shaunagm changed the title Update Mobilize connector URI, fix display of errors Update Mobilize connector URI, fix display of errors, fix auth issue Jul 7, 2022
@shaunagm shaunagm merged commit baabd1b into main Jul 9, 2022
@SorenSpicknall SorenSpicknall deleted the fix-mobilize-request-error branch September 26, 2022 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants