Skip to content

Fix #847 Log an exception when response status is not OK #848

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

Merged
merged 5 commits into from
Jun 17, 2020
Merged

Fix #847 Log an exception when response status is not OK #848

merged 5 commits into from
Jun 17, 2020

Conversation

Chouvic
Copy link
Contributor

@Chouvic Chouvic commented Jun 16, 2020

By providing the status code and its reason to logger, the error message would be more explicit for debugger whenever there is an issue related to resource token or other errors.

Fixes #847

Description of the Change

Added a checker to log an exception when the response from authentication server is not successful. With such change, the real issue with the response would not be changed to a JSONDecoderError which would be a distraction for debugging.

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

…is not successful

By providing the status code and its reason to logger, the error message would be more explicit for debugger whenever there is an issue related to resource token or other errors.
@coveralls
Copy link

coveralls commented Jun 16, 2020

Pull Request Test Coverage Report for Build 1405

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 94.806%

Totals Coverage Status
Change from base Build 1396: 0.02%
Covered Lines: 1296
Relevant Lines: 1367

💛 - Coveralls

Copy link
Contributor

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please add a test for the proposed change?

@Chouvic
Copy link
Contributor Author

Chouvic commented Jun 16, 2020

can you please add a test for the proposed change?
@auvipy
Sure thanks, do you know how to set up the testing environment for this?
I've set up a virtual environment and installed Django, oauthlib. It asked for settings OAUTH2_PROVIDER. Do I need to make django-oauth-toolkit as a Django project first?

@Chouvic Chouvic requested a review from auvipy June 16, 2020 15:45
@Chouvic
Copy link
Contributor Author

Chouvic commented Jun 16, 2020

A unit test is added to check the expected logger information.

@auvipy
Copy link
Contributor

auvipy commented Jun 17, 2020

please update the changelog and document if needed.

@Chouvic
Copy link
Contributor Author

Chouvic commented Jun 17, 2020

please update the changelog and document if needed.

A CHANGLOG is added. I have checked the documentation, currently, it seems there is no section to cover things related to APIs with the authentication server.

@auvipy auvipy merged commit d392c60 into django-oauth:master Jun 17, 2020
@n2ygk n2ygk mentioned this pull request Sep 19, 2024
5 tasks
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.

Return appropriate error messages when response from authentication server is not successful
3 participants