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

feat: Add oauth flow for querybook github integration #1497

Merged
merged 9 commits into from
Oct 30, 2024

Conversation

zhangvi7
Copy link
Contributor

@zhangvi7 zhangvi7 commented Oct 16, 2024

OAuth flow for querybook github integration feature

Test Plan

Datadoc rightside bar:
image
Modal:
Screenshot 2024-10-16 at 11 37 18 AM
GitHub Oauth:
Screenshot 2024-10-16 at 11 37 39 AM
Post OAuth Flow (To be changed):
Screenshot 2024-10-16 at 11 50 02 AM

@zhangvi7 zhangvi7 force-pushed the github/oauth branch 2 times, most recently from 500a138 to f0b1c90 Compare October 16, 2024 16:23
querybook/server/lib/github_integration/__init__.py Outdated Show resolved Hide resolved
return github_manager.github_integration_callback()


# Test GitHub OAuth Flow
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's not have the test code here. move to a separate test file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will remove

Comment on lines 29 to 31
def save_github_token(self, token: str) -> None:
flask_session["github_access_token"] = token
LOG.debug("Saved GitHub token to session")
Copy link
Collaborator

Choose a reason for hiding this comment

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

will save to the user properties as well?

Copy link
Contributor Author

@zhangvi7 zhangvi7 Oct 25, 2024

Choose a reason for hiding this comment

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

Will handle refactoring to encrypt in db in later PR

Comment on lines 77 to 80
@flask_app.route(GITHUB_OAUTH_CALLBACK)
def github_callback() -> str:
github_manager = get_github_manager()
return github_manager.github_integration_callback()
Copy link
Collaborator

Choose a reason for hiding this comment

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

would be better to just put this inside the datasources/github.py together with other github routes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can leave here to keep OAuth flow logic together in GitHub manager, similar to gspread_exporter.py

window.receiveChildMessage = receiveMessage;

// If the user closes the authentication window manually, clean up
const timer = setInterval(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why it needs a timer here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

detects if the user manually closed the authentication window to throw an error authentication process incomplete

return config

def save_github_token(self, token: str) -> None:
flask_session["github_access_token"] = token
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's define a const for the key github_access_token as it will be used by other module

@zhangvi7 zhangvi7 merged commit 3320361 into pinterest:github-integration Oct 30, 2024
4 checks passed
zhangvi7 added a commit to zhangvi7/querybook that referenced this pull request Oct 30, 2024
* feat: Add oauth flow for querybook github integration
* link datadoc to github directory
* feat: Add Datadoc serializing util
@zhangvi7 zhangvi7 deleted the github/oauth branch November 1, 2024 16:46
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