-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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 Github service bugs #6571
Fix Github service bugs #6571
Conversation
@@ -51,7 +51,7 @@ async def store_settings( | |||
# We check if the token is valid by getting the user | |||
# If the token is invalid, this will raise an exception | |||
github = GitHubService(settings.github_token, None) | |||
response = await github.get_user() | |||
response = await github.fetch_response('get_user') |
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.
Could we expose a load_user
method that returns the user and raises an exception? (It feels weird to have the internals of the github integration exposed here.)
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.
🍰 - I see you have these suggestions already in subsequent PRs.
Co-authored-by: tofarr <tofarr@gmail.com>
Co-authored-by: tofarr <tofarr@gmail.com>
Co-authored-by: tofarr <tofarr@gmail.com>
End-user friendly description of the problem this fixes or functionality that this introduces
Give a summary of what the PR does, explaining any non-trivial design decisions
This PR fixes a group of bugs. NOTE: this doesn't affect any existing functionality. However, these changes are blocking for implementing token refresh logic
The bugs fixed
"SAAS"
withAppMode.SAAS
)get_installation_ids
function means adict
object isn't return. This PR handles this edge case appropriatelyfetch_response
to call APIs that require refresh token logic (this PR fixed one such case whereget_user
was used in place offetch_response('get_user')
Link of any specific issues this addresses
To run this PR locally, use the following command: