-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[Feat] Add Microsoft Outlook - Mail Actions #3419
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
Conversation
Created _create_message function to compose emails with Graph APIh Added send mail function
…tionality Removed content_bytes from filter query as Microsoft uses queries on base attachment file which does have content_bytes and is specific to fileattachments Also fixed some mypy type issues
- Improved email validation logic and reduced bloated code - Add _create_recipients() to allow both 'email' and 'Name <email>' formats - Switched BeautifulSoup to html2text - Rename 'id' to 'message_id' - Add server_close() - Add dependency to pyproject.toml Fixed some docstring
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…date dependencies
Add fixtures and tests covering send_email, create_draft_email, and send_draft_email with success, failure, and validation scenarios.
…tachments methods covering success, failure, and edge cases including metadata-only retrieval and file saving functionality.
|
Hi @Wendong-Fan |
Regarding this issue, I think this is unrelated to my pr and the reason I am getting this is because i pushed uv lock file due to which we tied creating new environment instead of one in cache |
|
I have added the remaining tests and function Also I choose update_draft_message instead of update_message as updating sent message only changes data on our side like changing cc recipients wont send them mail again, so it wouldnt be very helpful for a llm toolkit in my opinion or even in general other than faking a sent mail. I have completed the mail part of actions for now but feel free to let me know if there are any suggestions or change either regarding this pr or in general that might help me. Thanks |
|
Also to make reviewers life easier 1 Go to https://portal.azure.com/ Thats it Now just add these creds to env (for personal account dont add tenant id or if you want add "common" ) and the code should work Later we should also add all this to example file ( I didnt create it since i dont have open ai credits) |
Hey @Tanuj-Taneja1, I will have a look at this issue and see why this may be happening, also moving forward feel free to message me with any concerns or issues you may face! |
No problem, I'll work on that thanks for the heads up and I'll make sure to let you know if I have any questions about the colab file! |
|
Hey @Tanuj-Taneja1, Thank you so much for your patience, I have added an enhance PR (#3492) that fixes some of the async functionality as well as the way the port is assigned, since you must set a fixed port for use in the OAUTH process I allow the user to set the port in the env variable, would be great if you could review this when possible so we can merge this feature! |
Hi @waleedalzarooni |
Yep, you're totally right I will reset to the original conventions in the enhanced PR |
…ect… PR#3419 (#3492) Co-authored-by: Tanuj Taneja <[email protected]>
|
everything looks good! some minor issues in this PR are around auth lifecycle and runtime robustness:
created an enhance pr, #3578 feel free to check it! @Tanuj-Taneja1 |
|
hi @MuggleJinx if you have time,please take a look at this pr again! |
MuggleJinx
left a comment
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! LGTM!
|
Hi @fengju0213
I think that is still the case, I am still getting browser login with each init. Can you confirm that from your side? |
Hi figured out the reason of this and mentioned the reason in #3578 |
Yes, that’s expected. It’s just that the previous persistence logic didn’t take effect. Now it can be saved properly, but to skip verification under the current implementation, a refresh token path needs to be provided. We could also change it to be passed automatically. |
Actually previously even in browser auth persistance was there just not in refresh_token_file_path but using TokenCachePersistenceOptions. Browser auth is when either user didnt provide refresh_token_file_path or when authentication using it fails. For Browser auth TokenCachePersistenceOptions stored refresh token and access token internally. Your current implentation is exactly what AuthorizationCodeCredential and TokenCachePersistenceOptions did internally. Feel free to refer this : https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/identity/Azure.Identity/samples/TokenCache.md |
Thanks for the clear explanation. I also reviewed the Azure Identity docs you linked. Based on that, to achieve “reuse credentials without repeated browser prompts,” it seems the official way is to enable persistent caching via TokenCachePersistenceOptions. Are you suggesting we switch to using the official TokenCachePersistenceOptions for this? |
Yes we can just revert to our original logic, it used TokenCachePersistenceOptions. |
okay,we can merge it first,then we can improve the implement |
Wendong-Fan
left a comment
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 @Tanuj-Taneja1 's contribution and @waleedalzarooni @fengju0213 @MuggleJinx help with reviewing, I added one enhance PR: #3608, feel free to review and let me know if there's anything we want to discuss further
Description
Fixes #3201
Hi @Wendong-Fan
I have completed the mail actions part of Outlook toolkit and this pr is ready to review.
Also I wanted to split other outlook related actions (like calender) into seperate file, so if that is ok we can rename this file to something mail related.
Reference:
msgraph
msgraph (some docs like send mail are in users section for some reason)
authorizationcodecredential for auth
(This is a edited conversation, older conversation was draft related)