-
Notifications
You must be signed in to change notification settings - Fork 690
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 Discord OAuth Flow and Bot Public Thread with History Response Feature #1129
base: master
Are you sure you want to change the base?
Conversation
…threads and responding with the chat history features
Thanks @AveryYay , I will review the code and assist you in completing the PR integration |
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.
Thanks @AveryYay , good code! left some comments below.
And in terms of functionality, I hope you refer to camel.bots.slack.slack_app.py
and integrate the oauth process into camel.bots.discord.discord_app.py
(you can create camel.bots.discord
to store module content)
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.
thanks @AveryYay , left some comments below.
|
||
user_data = await self.oauth_client.get_user_info(access_token) | ||
logger.info(f"User data: {user_data}") | ||
await self.shutdown_oauth_server() |
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.
await self.shutdown_oauth_server() |
Server is a long-term service, so it's better to let users manually shut it down
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.
Example can be combined with the usage of chat_agent
to integrate existing historical records into the memory
of chat_agent
.
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.
Thanks @AveryYay ! Overall LGTM, left some comments below, It would be better if we could also add docstring for DiscordOAuth
BTW, you can run pre-commit run --all-files
before the push to ensure the code pass pre-commit check
token: Optional[str] = None, | ||
client_id: Optional[str] = None, | ||
client_secret: Optional[str] = None, | ||
redirect_uri: str = "http://localhost:8000/callback", |
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.
use REDIRECT_URI
instead?
response = await client.post(TOKEN_URL, data=data, headers=headers) | ||
response_data = response.json() |
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.
before fetch the data, check the status code first like below?
if response.status_code != 200:
logger.error("Failed to exchange code for token)
return None
user_response = await client.get(USER_URL, headers=headers) | ||
return user_response.json() |
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.
same as above
self.server.should_exit = True | ||
logger.info("Shutting down the OAuth server.") |
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.
should we add await here to make sure the server is fully stopped?
Description
This PR introduces 2 components:
Motivation and Context
This change adds two distinct functionalities:
Types of changes
What types of changes does your code introduce? Put an
x
in all the boxes that apply:Implemented Tasks
Checklist
Go over all the following points, and put an
x
in all the boxes that apply.If you are unsure about any of these, don't hesitate to ask. We are here to help!