Make error handler code in views.py more DRY

[miketaylr]

See @karlcow's comment at #928 (comment).

[miketaylr]

@deepthivenkat do you want to take a look at this one?

[deepthivenkat]

Yes..I will try this

[deepthivenkat]

yes! I will try this

On Sat, Mar 5, 2016 at 12:57 AM, Mike Taylor [email protected]
wrote:

@deepthivenkat https://github.com/deepthivenkat do you want to take a
look at this one?

—
Reply to this email directly or view it on GitHub
#929 (comment)
.

[deepthivenkat]

#949

[deepthivenkat]

#958 r? @karlcow r? @miketaylr
Sorry for the delay. @karlcow : I have considered the suggestions you have made for my previous pull.I have added generic methods(api_call(request), api_message(code)) for the following error codes : 400,401,403,404 and 500. However, I had to maintain separate methods for error handling messages for 429 and GitHubError. Is there any specific reason to make use of json.dump() instead of jsonify in app.errorhandler(429) ?

[karlcow]

@deepthivenkat no reason except history, you can perfectly change it. That would be better for consistency. Thanks for catching that.

Just be aware that

• json.dumps() returns a JSON file
• while jsonify() is part of flask and returns a flask.Response()

Feel free to add a commit for fixing this.

[deepthivenkat]

r? @karlcow

[karlcow]

Thanks @deepthivenkat
A couple of issues.

• nosetests reports failure. We need to understand why.
• When you commit things, you need to make sure that your commit explains what it means. See for example

3240856 #710 Adds parameters to save all frames for an animated GIF
577dcca #710 Optimizes images further for JPEG and PNG
3ab1512 #710 Adjusts to TypeError. e not needed

[deepthivenkat]

r? @karlcow Thanks for the review comments.Sorry for committing though the tests failed.I will make sure I run all the tests before performing commit from now on.
Three tests failed. Two tests failed because I hard coded 404 in api_message method instead of passing code as argument.
The third test was written by me. I intended to assert if I get 403 when I try to check other users activity.But for that the user needs to be logged in first.So I was getting 401 instead of 403.
Since this test was not correct, I removed it.
Now all 3 tests are passing.

I have maintained the same branch since I needed the code changes that I have done previously.

[karlcow]

Thanks @deepthivenkat

The GitHubError was introduced by @miketaylr for issue #32 I guess sending back whichever error Github message was.
deepthivenkat@d3676ad

I want to have a second opinion for this. Because if it's just about 401, maybe this piece of code can disappear given we are handling it.

[deepthivenkat]

@karlcow So we throw 401 for the following two cases?

1. Unauthorized, log in.
2. Github error when they try to login

Are they not two different errors?

[karlcow]

They might be I want to know which error GitHub is sending back and because @miketaylr worked on it. He might remember. :) If not, we can try to test this again.

[deepthivenkat]

No problem. I will check which error it is returning back and come back with the details

[deepthivenkat]

@karlcow

GitHubError occurs when github request fails:
If request passes, the response is in JSON format which is decodeded and returned as dictionary.
If request fails, a JSON object similar to above is returned.
We can access the exact error message using GitHubError.response.json()['message']

Other possible status codes due to client error alone could be 400 and 422(Unprocessable entity)

The following is a sample response for a failed request :
{
"message": "Validation Failed",
"errors": [
{
"message": "Must include at least one user, organization, or repository",
"resource": "Search",
"field": "q",
"code": "invalid"
}
],
"documentation_url": "https://developer.github.com/v3/search/#search-code"
}

I also checked how the GitHubError class looks like.One way to check the error code that this class returns in out app is to flash the error code in app.errorhandler.

But I do not know what I should append to webcompat url (in my case localhost:5000) so that I will get a GitHubError.Is there any tests written that asserts GitHubError? Please help

[deepthivenkat]

r? @karlcow @miketaylr
Hello!
Mike confirmed that the Githuberror was added just to handle 401 error code. So I removed it.