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

Add get_aliases() function to Google Admin connector #769

Merged
merged 1 commit into from
Nov 21, 2022

Conversation

crayolakat
Copy link
Collaborator

Also fix existing tests for them to make more sense

@shaunagm
Copy link
Collaborator

Overall this looks good to me - just had one question. Sorry to take so long to review!

@crayolakat
Copy link
Collaborator Author

@shaunagm No problem, what is your question?

@shaunagm
Copy link
Collaborator

I left a comment on line 50 of test_google_admin.py. In case it's not showing up for you, it's "Can you walk me through why we need to change the tests for these other methods? Are you just doing some cleanup, or did something actually change?"

@crayolakat
Copy link
Collaborator Author

The get_all_group_members method is supposed to take a group key/ID as a parameter and return all the emails in that group. I noticed in the original test_all_group_members test that it was taking the string group_key as a parameter instead of the key/ID of one of the test groups defined in mock_all_groups. It makes more sense to pass in a group key, such as 1, to test the method, so that's why I changed that test. Nothing in the code regarding the get_all_group_members function changed though, or any other Google Admin function

'{"groups": [{"email": "[email protected]", "id": 1}, {"email": '
'"[email protected]", "id": 2}, {"email": "[email protected]", "id": '
'3}]}'.encode()
'{"groups": [{"aliases": ["[email protected]", "[email protected]"], "e'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you walk me through why we need to change the tests for these other methods? Are you just doing some cleanup, or did something actually change?

@shaunagm
Copy link
Collaborator

Gotcha @crayolakat - thanks for the explanation! Merging now.

@shaunagm shaunagm merged commit 9a00b10 into move-coop:main Nov 21, 2022
@crayolakat crayolakat deleted the kathy_get_aliases branch November 21, 2022 23:32
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