Skip to content

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

Merged
merged 9 commits into from
Apr 22, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ how a consumer would use the library (e.g. adding unit tests, updating documenta

## Unreleased

- `code42 users list` command
- `--org-uid` filters on org membership.
- `--role-name` filters on users having a particular role.
- `--active` and `--inactive` filter on user status.

### Fixed

- Bug where some CSV outputs on Windows would have an extra newline between the rows.
Expand Down
84 changes: 84 additions & 0 deletions src/code42cli/cmds/users.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
import click
from pandas import DataFrame

from code42cli.click_ext.groups import OrderedGroup
from code42cli.click_ext.options import incompatible_with
from code42cli.errors import Code42CLIError
from code42cli.options import format_option
from code42cli.options import sdk_options
from code42cli.output_formats import DataFrameOutputFormatter


@click.group(cls=OrderedGroup)
@sdk_options(hidden=True)
def users(state):
"""Manage users within your Code42 environment"""
pass


org_uid_option = click.option(
"--org-uid",
required=False,
type=str,
default=None,
help="Limit users to only those in the organization you specify. ",
Copy link
Contributor

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?

Copy link
Contributor Author

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.

)
role_name_option = click.option(
"--role-name",
required=False,
type=str,
default=None,
help="Limit results to only users having the specified role.",
)
active_option = click.option(
"--active", is_flag=True, help="Limits results to only active users.", default=None,
)
inactive_option = click.option(
"--inactive",
is_flag=True,
help="Limits results to only deactivated users.",
cls=incompatible_with("active"),
)


@users.command(name="list")
@org_uid_option
@role_name_option
@active_option
@inactive_option
@format_option
@sdk_options()
def list_users(state, org_uid, role_name, active, inactive, format):
"""List users in your Code42 environment."""
if inactive:
active = False
if role_name:
role_id = _get_role_id(state.sdk, role_name)
else:
role_id = None
df = _get_users_dataframe(state.sdk, org_uid, role_id, active)
if df.empty:
click.echo("No results found.")
else:
formatter = DataFrameOutputFormatter(format)
formatter.echo_formatted_dataframe(df)


def _get_role_id(sdk, role_name):
try:
roles_dataframe = DataFrame.from_records(sdk.users.get_available_roles().data)
return str(
Copy link
Contributor

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?

Copy link
Contributor Author

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[]

roles_dataframe.loc[
roles_dataframe["roleName"] == role_name, "roleId"
].array[0]
)
except KeyError:
raise Code42CLIError("Role with name {} not found.".format(role_name))


def _get_users_dataframe(sdk, org_uid, role_id, active):
users_generator = sdk.users.get_all(active=active, org_uid=org_uid, role_id=role_id)
users_list = []
for page in users_generator:
users_list.extend(page["users"])
return DataFrame.from_records(users_list)
2 changes: 2 additions & 0 deletions src/code42cli/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
from code42cli.cmds.legal_hold import legal_hold
from code42cli.cmds.profile import profile
from code42cli.cmds.securitydata import security_data
from code42cli.cmds.users import users
from code42cli.options import sdk_options

BANNER = """\b
Expand Down Expand Up @@ -80,5 +81,6 @@ def cli(state, python):
cli.add_command(legal_hold)
cli.add_command(profile)
cli.add_command(devices)
cli.add_command(users)
cli.add_command(audit_logs)
cli.add_command(cases)
88 changes: 88 additions & 0 deletions tests/cmds/test_users.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
import json

import pytest
from pandas.core.frame import DataFrame
from py42.response import Py42Response
from requests import Response

from code42cli.cmds.users import _get_role_id
from code42cli.cmds.users import _get_users_dataframe
from code42cli.main import cli

TEST_ROLE_RETURN_DATA = {"data": [{"roleName": "Customer Cloud Admin", "roleId": "12"}]}

TEST_USERS_RESPONSE = {
"users": [
{
"userId": "1234",
"userUid": "997962681513153325",
"status": "Active",
"username": "[email protected]",
"creationDate": "2021-03-12T20:07:40.898Z",
"modificationDate": "2021-03-12T20:07:40.938Z",
}
]
}


def _create_py42_response(mocker, text):
response = mocker.MagicMock(spec=Response)
response.text = text
response._content_consumed = mocker.MagicMock()
response.status_code = 200
return Py42Response(response)


def get_all_users_generator():
yield TEST_USERS_RESPONSE


@pytest.fixture
def get_available_roles_response(mocker):
return _create_py42_response(mocker, json.dumps(TEST_ROLE_RETURN_DATA))


@pytest.fixture
def get_all_users_success(cli_state):
cli_state.sdk.users.get_all.return_value = get_all_users_generator()


@pytest.fixture
def get_available_roles_success(cli_state, get_available_roles_response):
cli_state.sdk.users.get_available_roles.return_value = get_available_roles_response


def test_list_outputs_appropriate_columns(runner, cli_state, get_all_users_success):
result = runner.invoke(cli, ["users", "list"], obj=cli_state)
assert "userId" in result.output
assert "userUid" in result.output
assert "status" in result.output
assert "username" in result.output
assert "creationDate" in result.output
assert "modificationDate" in result.output


def test_get_role_id_returns_appropriate_role_id(
cli_state, get_available_roles_success
):
result = _get_role_id(cli_state.sdk, "Customer Cloud Admin")
assert result == "12"


def test_get_users_dataframe_returns_dataframe(cli_state, get_all_users_success):
result = _get_users_dataframe(
cli_state.sdk, org_uid=None, role_id=None, active=None
)
assert isinstance(result, DataFrame)


def test_get_users_dataframe_calls_get_all_users_with_correct_parameters(
Copy link
Contributor

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?

Copy link
Contributor Author

@ceciliastevens ceciliastevens Apr 21, 2021

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@antazoey antazoey Apr 22, 2021

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 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.

Copy link
Contributor Author

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.

cli_state, get_all_users_success
):
org_uid = "TEST_ORG_UID"
role_id = "TEST_ROLE_ID"
active = "TEST_ACTIVE"
_get_users_dataframe(cli_state.sdk, org_uid=org_uid, role_id=role_id, active=active)
cli_state.sdk.users.get_all.assert_called_once_with(
active=active, org_uid=org_uid, role_id=role_id
)