List users #270
List users #270
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ |
tests/cmds/test_users.py
Outdated
| assert isinstance(result, DataFrame) | ||
|
|
||
|
|
||
| def test_get_users_dataframe_calls_get_all_users_with_correct_parameters( |
There was a problem hiding this comment.
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.
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.
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.
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 this
def test_list_users_includes_expected_role_ids()
And then you would verify that the role IDs from the mock responses have shown up in the output
assert "12" in result.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.
def test_list_users_when_given_inactive_uses_active_equals_false()
def test_list_users_when_given_active_and_inactive_raises_error()
def test_list_users_when_given_excluding_active_and_inactive_uses_active_equals_none()
^ 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.
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.
src/code42cli/cmds/users.py
Outdated
| required=False, | ||
| type=str, | ||
| default=None, | ||
| help="Limit users to only those in the organization you specify. ", |
There was a problem hiding this comment.
Does this include child orgs?
There was a problem hiding this comment.
I believe so. I'll update the text.
src/code42cli/cmds/users.py
Outdated
| def _get_role_id(sdk, role_name): | ||
| try: | ||
| roles_dataframe = DataFrame.from_records(sdk.users.get_available_roles().data) | ||
| return str( |
There was a problem hiding this comment.
Lines 70 - 73 is quite complex. Can we break this into multiple statements?
There was a problem hiding this comment.
I did break this out, and also found a way to do it using DataFrame.at[] instead of DataFrame.loc[]
| assert "Error: --inactive can't be used with: --active" in result.output | ||
|
|
||
|
|
||
| def test_list_users_when_given_excluding_active_and_inactive_uses_active_equals_none( |
There was a problem hiding this comment.
technically don't need the word _given_ in the test signature (not a blocker though)
Description of Change
This adds a "users list" command and parameters to filter on active status, org Uid, and role name.
This partially addresses #259, but more work will follow in additional PRs.
Testing Procedure
PR Checklist