-
Notifications
You must be signed in to change notification settings - Fork 11
feat: Add tool to read a user's manager from Google Contacts #59
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
This commit adds a new tool, `people.getUserManager`, which allows a user to retrieve their manager's information from Google Contacts. The changes include: - A new `getUserManager` function in `PeopleService.ts` to fetch the manager's data. - The `personFields` in `getUserProfile` has been updated to include `relations`. - A new command, `get_user_manager`, has been added to expose this functionality. - Unit tests have been added to verify the new functionality.
feat: Add tool to read a user's manager from Google Contacts
Summary of ChangesHello @raybell-md, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces the capability to programmatically retrieve a user's manager from Google Contacts. It extends the existing PeopleService with a dedicated function for manager lookup and enhances user profile retrieval to include relationship data. The changes ensure that the new functionality is properly integrated, tested, and exposed as a callable tool within the system, providing a more complete view of user organizational structures. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a new tool to retrieve a user's manager from Google Contacts, along with corresponding service logic and unit tests. The changes are well-structured, and the new getUserManager functionality is a valuable addition. However, there are a couple of areas related to API clarity and efficiency that could be improved. Specifically, the return value for getUserManager when no manager is found could be more explicit, and the getUserProfile method appears to be fetching more data than it currently uses or returns, which could lead to unnecessary overhead.
| const res = await people.people.get({ | ||
| resourceName, | ||
| personFields: 'names,emailAddresses', | ||
| personFields: 'names,emailAddresses,relations', |
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.
The getUserProfile method now requests relations in its personFields. However, the method's primary purpose, as indicated by its name and existing usage, is to retrieve basic profile information (names, email addresses). Fetching relations here, if not used or returned by getUserProfile, introduces unnecessary data retrieval, potentially increasing API call latency and resource consumption.
If relations data is not intended to be part of the getUserProfile's output, it should be removed from personFields to optimize the API call. If it is intended to be part of the output, then the method's return type and documentation should be updated, and tests should assert its presence.
| personFields: 'names,emailAddresses,relations', | |
| personFields: 'names,emailAddresses', |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
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.
This entire file appears to be a hallucination. It shouldn't be here, only toml files should be in the command directory.
| } | ||
|
|
||
| public getUserManager = async ({ userId }: { userId: string; }) => { | ||
| logToFile(`[PeopleService] Starting getUserManager with: userId=${userId}`); |
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.
Rather than a specific function to get a manager, it would be better to have a function that interacts with the relations object overall. Where finding the manager could be one of many data items returned from that function.
See: https://developers.google.com/people/api/rest/v1/people#Person.Relation for the full details.
|
FYI I pointed this PR (with comments) at some async coding tools. Prompt was "Fix #59" |
|
The commit you referenced in your last comment is looking better. More in line with what I would want to see in the repo. Let me know if you want me to take a pass at it. Also happy to have you work on this at your own pace. |
Closes #56 ?
This is a Jules PR (https://github.com/raybell-md/workspace/pull/2) so merge with caution. I haven't vetted it.