-
Notifications
You must be signed in to change notification settings - Fork 13
Users command: option to report on legal hold membership #290
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
Changes from 1 commit
12e9478
2dbc0c4
002c605
f12fe45
51367c0
c9b3987
7d403d3
ac50108
9a2f41d
c5bda56
aaff514
b53ee2b
0d6d108
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
import click | ||
from pandas import DataFrame | ||
from pandas import json_normalize | ||
|
||
from code42cli.click_ext.groups import OrderedGroup | ||
from code42cli.click_ext.options import incompatible_with | ||
|
@@ -46,9 +47,19 @@ def username_option(help): | |
@role_name_option("Limit results to only users having the specified role.") | ||
@active_option | ||
@inactive_option | ||
@click.option( | ||
"--include-legal-hold-membership", | ||
required=False, | ||
maddie-vargo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
type=bool, | ||
maddie-vargo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
default=False, | ||
is_flag=True, | ||
help="Include legal hold membership in output.", | ||
) | ||
@format_option | ||
@sdk_options() | ||
def list_users(state, org_uid, role_name, active, inactive, format): | ||
def list_users( | ||
state, org_uid, role_name, active, inactive, include_legal_hold_membership, format | ||
): | ||
"""List users in your Code42 environment.""" | ||
if inactive: | ||
active = False | ||
|
@@ -59,6 +70,8 @@ def list_users(state, org_uid, role_name, active, inactive, format): | |
else None | ||
) | ||
df = _get_users_dataframe(state.sdk, columns, org_uid, role_id, active) | ||
if include_legal_hold_membership: | ||
df = _add_legal_hold_membership_to_user_dataframe(state.sdk, df) | ||
if df.empty: | ||
click.echo("No results found.") | ||
else: | ||
|
@@ -122,3 +135,37 @@ def _get_users_dataframe(sdk, columns, org_uid, role_id, active): | |
users_list.extend(page["users"]) | ||
|
||
return DataFrame.from_records(users_list, columns=columns) | ||
|
||
|
||
def _add_legal_hold_membership_to_user_dataframe(sdk, df): | ||
columns = ["legalHold.legalHoldUid", "legalHold.name", "user.userUid"] | ||
|
||
legal_hold_member_dataframe = ( | ||
json_normalize(list(_get_all_active_hold_memberships(sdk)))[columns] | ||
.groupby(["user.userUid"]) | ||
.agg(",".join) | ||
.rename( | ||
{ | ||
"legalHold.legalHoldUid": "legalHoldUid", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice to be able to pass in user friendly keys for Table format headers for when using Table format should use label-esque keys while all the other ones should use raw API values. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The strange format of |
||
"legalHold.name": "legalHoldName", | ||
}, | ||
axis=1, | ||
) | ||
) | ||
df = df.merge( | ||
legal_hold_member_dataframe, | ||
how="left", | ||
left_on="userUid", | ||
right_on="user.userUid", | ||
) | ||
|
||
return df | ||
|
||
|
||
def _get_all_active_hold_memberships(sdk): | ||
for page in sdk.legalhold.get_all_matters(active=True): | ||
for matter in page["legalHolds"]: | ||
for _page in sdk.legalhold.get_all_matter_custodians( | ||
legal_hold_uid=matter["legalHoldUid"], active=True | ||
antazoey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
): | ||
yield from _page["legalHoldMemberships"] | ||
maddie-vargo marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,11 @@ | ||
import json | ||
|
||
import pytest | ||
from pandas import DataFrame | ||
from py42.response import Py42Response | ||
from requests import Response | ||
|
||
from code42cli.cmds.users import _add_legal_hold_membership_to_user_dataframe | ||
from code42cli.main import cli | ||
|
||
|
||
|
@@ -32,6 +34,41 @@ | |
} | ||
] | ||
} | ||
TEST_MATTER_RESPONSE = { | ||
"legalHolds": [ | ||
{"legalHoldUid": "123456789", "name": "Legal Hold #1", "active": True}, | ||
{"legalHoldUid": "987654321", "name": "Legal Hold #2", "active": True}, | ||
] | ||
} | ||
TEST_CUSTODIANS_RESPONSE = { | ||
"legalHoldMemberships": [ | ||
{ | ||
"legalHoldMembershipUid": "99999", | ||
"active": True, | ||
"creationDate": "2020-07-16T08:50:23.405Z", | ||
"legalHold": {"legalHoldUid": "123456789", "name": "Legal Hold #1"}, | ||
"user": { | ||
"userUid": "911162111513111325", | ||
"username": "[email protected]", | ||
"email": "[email protected]", | ||
"userExtRef": None, | ||
}, | ||
}, | ||
{ | ||
"legalHoldMembershipUid": "11111", | ||
"active": True, | ||
"creationDate": "2020-07-16T08:50:23.405Z", | ||
"legalHold": {"legalHoldUid": "987654321", "name": "Legal Hold #2"}, | ||
"user": { | ||
"userUid": "911162111513111325", | ||
"username": "[email protected]", | ||
"email": "[email protected]", | ||
"userExtRef": None, | ||
}, | ||
}, | ||
] | ||
} | ||
|
||
TEST_EMPTY_USERS_RESPONSE = {"users": []} | ||
TEST_USERNAME = TEST_USERS_RESPONSE["users"][0]["username"] | ||
TEST_USER_ID = TEST_USERS_RESPONSE["users"][0]["userId"] | ||
|
@@ -50,6 +87,14 @@ def get_all_users_generator(): | |
yield TEST_USERS_RESPONSE | ||
|
||
|
||
def matter_list_generator(): | ||
yield TEST_MATTER_RESPONSE | ||
|
||
|
||
def custodian_list_generator(): | ||
yield TEST_CUSTODIANS_RESPONSE | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These should use the
Anything that is a response from a py42 method should use this method, so all these ones here, it is being used in some of the other fixtures below as an example! Please and thank you! This is looking really good. |
||
|
||
|
||
@pytest.fixture | ||
def get_available_roles_response(mocker): | ||
return _create_py42_response(mocker, json.dumps(TEST_ROLE_RETURN_DATA)) | ||
|
@@ -70,6 +115,18 @@ def get_user_id_failure(cli_state): | |
cli_state.sdk.users.get_by_username.return_value = TEST_EMPTY_USERS_RESPONSE | ||
|
||
|
||
@pytest.fixture | ||
def get_all_matter_success(cli_state): | ||
cli_state.sdk.legalhold.get_all_matters.return_value = matter_list_generator() | ||
|
||
|
||
@pytest.fixture | ||
def get_all_custodian_success(cli_state): | ||
cli_state.sdk.legalhold.get_all_matter_custodians.return_value = ( | ||
custodian_list_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 | ||
|
@@ -169,6 +226,32 @@ def test_list_users_when_given_excluding_active_and_inactive_uses_active_equals_ | |
) | ||
|
||
|
||
def test_add_legal_hold_membership_to_user_dataframe_adds_legal_hold_columns_to_dataframe( | ||
maddie-vargo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
cli_state, get_all_matter_success, get_all_custodian_success | ||
): | ||
testdf = DataFrame.from_records( | ||
[{"userUid": "840103986007089121", "status": "Active"}] | ||
) | ||
result = _add_legal_hold_membership_to_user_dataframe(cli_state.sdk, testdf) | ||
assert "legalHoldUid" in result.columns | ||
assert "legalHoldName" in result.columns | ||
|
||
|
||
def test_list_include_legal_hold_membership_merges_in_and_concats_legal_hold_info( | ||
runner, | ||
cli_state, | ||
get_all_users_success, | ||
get_all_custodian_success, | ||
get_all_matter_success, | ||
): | ||
result = runner.invoke( | ||
cli, ["users", "list", "--include-legal-hold-membership"], obj=cli_state | ||
) | ||
|
||
assert "Legal Hold #1,Legal Hold #2" in result.output | ||
assert "123456789,987654321" in result.output | ||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we get tests for the following two cases: 1.) A user is not on any legal hold but the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added these tests. The code didn't handle cases where 0 custodians were returned. Thanks for suggesting this test case. I also added a test for cases where the flag |
||
def test_add_user_role_adds( | ||
runner, cli_state, get_user_id_success, get_available_roles_success | ||
): | ||
|
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 know this has probably already been said, but what is the use-case for this functionality? I had heard most customers have a single matter for legal hold.
Is this flag general enough or this just for a specific use-case? I am also wondering if this is the right place to include this flag... What if I wanted to filter users by matter ID instead? Or what if I want to know which detection list or alert rules each user belong to as well?
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 primary use case that this addresses is "show me all the users on legal hold."
There are customers that have a lot of legal hold matters. Therefore, the
legal-hold show <matter ID>
command would not meet their needs if they need to see users on all legal holds.Also,
legal-hold show <matter ID>
produces the users in column format. This makes sense for ashow
command, which is meant to be a high-level overview (This was discussed in the PR that added thedevices list --include-legal-hold-membership
). However, for an admin to work with the data and put it in different formats, thelist
command provided some more flexibility (at least for device info)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'm good with this being in the CLI proper. As it could be useful to any customer using legal hold.
I think I'd also be ok with future flags for including if a user is on a detection list as well. Having the CLI
users
command be a kind of one-stop reporting tool to get all users info makes sense. Especially for larger customers who might have multiple teams (or automations) managing who gets put on what list/hold.