Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
List users #270
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
List users #270
Changes from 4 commits
aa52e7d
af26209
f84167a
1fe5fd1
119cbf0
00415b9
5152e7d
42fbe9c
db11a45
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Does this include child orgs?
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.
I believe so. I'll update the text.
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.
Lines 70 - 73 is quite complex. Can we break this into multiple statements?
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.
I did break this out, and also found a way to do it using DataFrame.at[] instead of DataFrame.loc[]
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.
We don't typically test the implementation details of commands (the protected methods).
Can we change these tests such that their entry point occurs from command execution?
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.
Updated! Let me know if you'd like any further changes, but it now runs "users list" and works from that.
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.
There are also a couple other tests (test_get_role_id_returns_appropriate_role_id and test_get_users_dataframe_returns_dataframe) that test the details of internal methods, should those be updated as well? I think it might be difficult to test some of those things from command execution as they aren't directly exposed in the result.
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.
Yes, that would be nice.
Tests like
test_get_users_dataframe_returns_dataframe
are not needed. The fact that we use dataframes under the hood shouldn't matter.For testing
role_ids
logic, the test would look something thisAnd then you would verify that the role IDs from the mock responses have shown up in the output
(Mayne make the role ID more specific, like
1230123
or something)Additionally, I would really appreciate some tests around the logic that figures out
active
.^ These tests can just verify that py42 was called correctly if we are unable to verify these states via the output.
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.
I've added the additional tests for the active logic. I updated the role Id test as well, but roleId isn't actually included in the output, it's just passed as a parameter to the users.get_all() method that gets the users (where it's used as a filter). So I ended up just testing that the users.get_all() call has the appropriate parameter.