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 legal hold membership to device reporting #192

Merged
merged 23 commits into from
Mar 1, 2021
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
b285a68
Legal Hold work to meet Issue 176
maddie-vargo Dec 28, 2020
ed72879
Fix to Changelog
maddie-vargo Dec 28, 2020
ea4381a
Minor fix to CHANGELOG
maddie-vargo Dec 29, 2020
a0ebe57
Added to legal hold user guide
maddie-vargo Dec 29, 2020
077dea1
Adjusting build parameters to bypass 3.5 for this PR
maddie-vargo Dec 29, 2020
03a3814
Merge branch 'master' into iss176
maddie-vargo Dec 29, 2020
f174992
Fix low hanging fruit for initial PR review
maddie-vargo Jan 4, 2021
205d559
Move development from legal-hold command to devices command, add new …
maddie-vargo Jan 29, 2021
02d6530
remove whitespaces that are coming through as edits
maddie-vargo Feb 2, 2021
e5784cf
fix changes identfied by tox style run
maddie-vargo Feb 2, 2021
9bfcd42
remove duplication in setup.py - file should have no edits
maddie-vargo Feb 2, 2021
2e303de
remove duplication in setup.py - file should have no edits
maddie-vargo Feb 2, 2021
94fe3b9
refactor membership function to use generator and remove NaNs from ou…
maddie-vargo Feb 11, 2021
0b79911
fix tox style run issue
maddie-vargo Feb 11, 2021
1bd1e2f
Fix tox style run x2
maddie-vargo Feb 11, 2021
e4725c7
flipping back to using NaN, awaiting PR #245
maddie-vargo Feb 16, 2021
237ea31
Adding --include-total-storage option, which calculates total number …
maddie-vargo Feb 18, 2021
7212fbf
Remove V2 archives from storage calcuation; rename columns
maddie-vargo Feb 22, 2021
2a917bd
fix small change to the incldue/excluded archive types
maddie-vargo Feb 23, 2021
2d1db8c
reword
maddie-vargo Feb 25, 2021
9a0afcb
conflict reconciliation in changelog, part I
maddie-vargo Feb 26, 2021
a3dd28f
conflict reconciliation in changelog, part II (repulled from upstream…
maddie-vargo Feb 26, 2021
9677f23
fix style run
maddie-vargo Feb 26, 2021
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
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ how a consumer would use the library (e.g. adding unit tests, updating documenta
- `search` to search for audit-logs.
- `send-to` to send audit-logs to server.


Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like our changelog has gotten messed up? The #Unreleased tag should not be there...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@unparalleled-js I re-pulled from master. The #Unreleased tag should be at the top, right? Let me know if I should fix it with this PR

## Unreleased

### Changed
Expand All @@ -71,6 +72,12 @@ how a consumer would use the library (e.g. adding unit tests, updating documenta
- Now, when adding a cloud alias to a detection list user, such as during `departing-employee add`, it will remove the existing cloud alias if one exists.
- Before, it would error and the cloud alias would not get added.

### Added

- `code42 devices list` option:
- `--include-legal-hold-membership` to add legal hold columns to the result output, printing the legal hold matter name and ID for any active device actively on legal hold
Copy link
Contributor

Choose a reason for hiding this comment

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

can we rephrase printing .... to something like prints the legalhold matter name and ID for any active device on legal hold!?

atleast active device actively seems off while reading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that is terrible phrasing. Resolved with latest commit

- `--include-total-storage` to add columns with total number of archives and total sum of archive bytes to each device in the result output

## 1.0.0 - 2020-08-31

### Fixed
Expand Down
81 changes: 80 additions & 1 deletion src/code42cli/cmds/devices.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
from datetime import date

import click
import numpy as np
from pandas import concat
from pandas import DataFrame
from pandas import json_normalize
from pandas import Series
from pandas import to_datetime
from py42 import exceptions
from py42.exceptions import Py42NotFoundError
Expand Down Expand Up @@ -244,6 +247,22 @@ def _get_device_info(sdk, device_guid):
is_flag=True,
help="Include device settings in output.",
)
@click.option(
"--include-legal-hold-membership",
required=False,
type=bool,
default=False,
is_flag=True,
help="Include legal hold membership in output.",
)
@click.option(
"--include-total-storage",
required=False,
type=bool,
default=False,
is_flag=True,
help="Include archive count and total storage in output.",
)
@click.option(
"--exclude-most-recently-connected",
type=int,
Expand Down Expand Up @@ -285,6 +304,8 @@ def list_devices(
include_backup_usage,
include_usernames,
include_settings,
include_legal_hold_membership,
include_total_storage,
exclude_most_recently_connected,
last_connected_after,
last_connected_before,
Expand All @@ -309,7 +330,11 @@ def list_devices(
"userUid",
]
df = _get_device_dataframe(
state.sdk, columns, active, org_uid, include_backup_usage
state.sdk,
columns,
active,
org_uid,
(include_backup_usage or include_total_storage),
)
if last_connected_after:
df = df.loc[to_datetime(df.lastConnected) > last_connected_after]
Expand All @@ -326,17 +351,52 @@ def list_devices(
.head(exclude_most_recently_connected)
)
df = df.drop(most_recent.index)
if include_total_storage:
df = _add_storage_totals_to_dataframe(df, include_backup_usage)
if include_settings:
df = _add_settings_to_dataframe(state.sdk, df)
if include_usernames:
df = _add_usernames_to_device_dataframe(state.sdk, df)
if include_legal_hold_membership:
df = _add_legal_hold_membership_to_device_dataframe(state.sdk, df)
if df.empty:
click.echo("No results found.")
else:
formatter = DataFrameOutputFormatter(format)
formatter.echo_formatted_dataframe(df)


def _add_legal_hold_membership_to_device_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)
)
df = df.merge(
legal_hold_member_dataframe,
how="left",
left_on="userUid",
right_on="user.userUid",
)

df.loc[
df["status"] == "Deactivated", ["legalHold.legalHoldUid", "legalHold.name"],
] = np.nan
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using np.nan for null values here, let's use an empty string, I think NaN would confuse people in the output and think maybe there was a bug. To replace any np.nan values resulting from the .groupby(), you can call .fillna(value="") on the df to replace those as well.

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 chose NaN to match the value assumed by pandas of an empty value. NaN's are generated when the legal hold table is merged with the devices table. If I use the above solution prior to merging with the devices table, the legalHoldUid and legalHold.name columns will contain a mix of None's, actual values, and NaNs.

To confirm: which of the following would you prefer?

  • NaN's in table view
  • None's in table view

(Either will produce null values in the other output formats)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, I meant to say "resulting from the.merge()" instead of .groupby(). Yeah, the .fillna(value="") needs to be called after the merge.

I think we should convert NaN/None values to an empty string for table/CSV outputs, but we'd probably want to keep them as NaN/None when we output to json so it translates to null. But we should push that logic to the DataFrameOutputFormatter so we only need to handle it in one place for any commands that use DataFrames.

I'll start a new PR to rework the formatter to handle that, since I'd like to refactor it a bit for clarity and add some tests around handling dataframe nulls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NaNs removed

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I've opened #245 to add the null handling in the DataFrameOutputFormatter, so we don't need to be concerned with converting them here, since we want the conversion to be conditional on the output format chosen by the end-user.

Sorry to have you flip/flop this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved, and awaiting #245


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
):
yield from _page["legalHoldMemberships"]


def _get_device_dataframe(
sdk, columns, active=None, org_uid=None, include_backup_usage=False
):
Expand Down Expand Up @@ -392,6 +452,25 @@ def _add_usernames_to_device_dataframe(sdk, device_dataframe):
return device_dataframe.merge(users_dataframe, how="left", on="userUid")


def _add_storage_totals_to_dataframe(df, include_backup_usage):
df[["archiveCount", "totalStorageBytes"]] = df["backupUsage"].apply(
_break_backup_usage_into_total_storage
)

if not include_backup_usage:
df = df.drop("backupUsage", axis=1)
return df


def _break_backup_usage_into_total_storage(backup_usage):
total_storage = 0
archive_count = 0
for archive in backup_usage:
maddie-vargo marked this conversation as resolved.
Show resolved Hide resolved
archive_count += 1
total_storage += archive["archiveBytes"]
return Series([archive_count, total_storage])


@devices.command()
@active_option
@inactive_option
Expand Down
183 changes: 182 additions & 1 deletion tests/cmds/test_devices.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
import json
from datetime import date

import numpy as np
import pytest
from pandas import DataFrame
from pandas import Series
from pandas._testing import assert_frame_equal
from pandas._testing import assert_series_equal
from py42.exceptions import Py42BadRequestError
from py42.exceptions import Py42ForbiddenError
from py42.exceptions import Py42NotFoundError
Expand All @@ -11,7 +16,9 @@

from code42cli import PRODUCT_NAME
from code42cli.cmds.devices import _add_backup_set_settings_to_dataframe
from code42cli.cmds.devices import _add_legal_hold_membership_to_device_dataframe
from code42cli.cmds.devices import _add_usernames_to_device_dataframe
from code42cli.cmds.devices import _break_backup_usage_into_total_storage
from code42cli.cmds.devices import _get_device_dataframe
from code42cli.main import cli

Expand Down Expand Up @@ -77,7 +84,19 @@
"836476656572622471","serverName":"cif-sea","serverHostName":"https://cif-sea.crashplan.com",
"isProvider":false,"archiveGuid":"843293524842941560","archiveFormat":"ARCHIVE_V1","activity":
{"connected":false,"backingUp":false,"restoring":false,"timeRemainingInMs":0,
"remainingFiles":0,"remainingBytes":0}}]}}"""
"remainingFiles":0,"remainingBytes":0}},{"targetComputerParentId":null,"targetComputerParentGuid":
null,"targetComputerGuid":"43","targetComputerName":"PROe Cloud, US","targetComputerOsName":null,
"targetComputerType":"SERVER","selectedFiles":1599,"selectedBytes":1529420143,"todoFiles":0,
"todoBytes":0,"archiveBytes":56848550,"billableBytes":1529420143,"sendRateAverage":0,
"completionRateAverage":0,"lastBackup":"2019-12-02T09:34:28.364-06:00","lastCompletedBackup":
"2019-12-02T09:34:28.364-06:00","lastConnected":"2019-12-02T11:02:36.108-06:00","lastMaintenanceDate":
"2021-02-16T07:01:11.697-06:00","lastCompactDate":"2021-02-16T07:01:11.694-06:00","modificationDate":
"2021-02-17T04:57:27.222-06:00","creationDate":"2019-09-26T15:27:38.806-05:00","using":true,
"alertState":16,"alertStates":["CriticalBackupAlert"],"percentComplete":100.0,"storePointId":10989,
"storePointName":"fsa-iad-2","serverId":160024121,"serverGuid":"883282371081742804","serverName":
"fsa-iad","serverHostName":"https://web-fsa-iad.crashplan.com","isProvider":false,"archiveGuid":
"92077743916530001","archiveFormat":"ARCHIVE_V1","activity":{"connected":false,"backingUp":false,
"restoring":false,"timeRemainingInMs":0,"remainingFiles":0,"remainingBytes":0}}]}}"""
TEST_EMPTY_BACKUPUSAGE_RESPONSE = """{"metadata":{"timestamp":"2020-10-13T12:51:28.410Z","params":
{"incBackupUsage":"True","idType":"guid"}},"data":{"computerId":1767,"name":"SNWINTEST1",
"osHostname":"UNKNOWN","guid":"843290890230648046","type":"COMPUTER","status":"Active",
Expand Down Expand Up @@ -222,6 +241,75 @@
},
],
}
MATTER_RESPONSE = {
"legalHolds": [
{
"legalHoldUid": "123456789",
"name": "Test legal hold matter",
"description": "",
"notes": None,
"holdExtRef": None,
"active": True,
"creationDate": "2020-08-05T10:49:58.353-05:00",
"lastModified": "2020-08-05T10:49:58.358-05:00",
"creator": {
"userUid": "12345",
"username": "user@code42.com",
"email": "user@code42.com",
"userExtRef": None,
},
"holdPolicyUid": "966191295667423997",
},
{
"legalHoldUid": "987654321",
"name": "Another Matter",
"description": "",
"notes": None,
"holdExtRef": None,
"active": True,
"creationDate": "2020-05-20T15:58:31.375-05:00",
"lastModified": "2020-05-28T13:49:16.098-05:00",
"creator": {
"userUid": "76543",
"username": "user2@code42.com",
"email": "user2@code42.com",
"userExtRef": None,
},
"holdPolicyUid": "946178665645035826",
},
]
}
ALL_CUSTODIANS_RESPONSE = {
"legalHoldMemberships": [
{
"legalHoldMembershipUid": "99999",
"active": True,
"creationDate": "2020-07-16T08:50:23.405Z",
"legalHold": {
"legalHoldUid": "123456789",
"name": "Test legal hold matter",
},
"user": {
"userUid": "840103986007089121",
"username": "ttranda_deactivated@ttrantest.com",
"email": "ttranda_deactivated@ttrantest.com",
"userExtRef": None,
},
},
{
"legalHoldMembershipUid": "88888",
"active": True,
"creationDate": "2020-07-16T08:50:23.405Z",
"legalHold": {"legalHoldUid": "987654321", "name": "Another Matter"},
"user": {
"userUid": "840103986007089121",
"username": "ttranda_deactivated@ttrantest.com",
"email": "ttranda_deactivated@ttrantest.com",
"userExtRef": None,
},
},
]
}


def _create_py42_response(mocker, text):
Expand Down Expand Up @@ -275,6 +363,14 @@ def users_list_generator():
yield TEST_USERS_LIST_PAGE


def matter_list_generator():
yield MATTER_RESPONSE


def custodian_list_generator():
yield ALL_CUSTODIANS_RESPONSE


@pytest.fixture
def backupusage_response(mocker):
return _create_py42_response(mocker, TEST_BACKUPUSAGE_RESPONSE)
Expand Down Expand Up @@ -357,6 +453,18 @@ def get_all_users_success(cli_state):
cli_state.sdk.users.get_all.return_value = users_list_generator()


@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()
)


def test_deactivate_deactivates_device(
runner, cli_state, deactivate_device_success, get_device_by_guid_success
):
Expand Down Expand Up @@ -559,6 +667,79 @@ def test_add_usernames_to_device_dataframe_adds_usernames_to_dataframe(
assert "username" in result.columns


def test_add_legal_hold_membership_to_device_dataframe_adds_legal_hold_columns_to_dataframe(
cli_state, get_all_matter_success, get_all_custodian_success
):
testdf = DataFrame.from_records(
[
{"userUid": "840103986007089121", "status": "Active"},
{"userUid": "836473273124890369", "status": "Active, Deauthorized"},
]
)
result = _add_legal_hold_membership_to_device_dataframe(cli_state.sdk, testdf)
assert "legalHold.legalHoldUid" in result.columns
assert "legalHold.name" in result.columns


def test_list_include_legal_hold_membership_pops_legal_hold_if_device_deactivated(
cli_state, get_all_matter_success, get_all_custodian_success
):
testdf = DataFrame.from_records(
[
{"userUid": "840103986007089121", "status": "Deactivated"},
{"userUid": "840103986007089121", "status": "Active"},
]
)

testdf_result = DataFrame.from_records(
[
{
"userUid": "840103986007089121",
"status": "Deactivated",
"legalHold.legalHoldUid": np.nan,
"legalHold.name": np.nan,
},
{
"userUid": "840103986007089121",
"status": "Active",
"legalHold.legalHoldUid": "123456789,987654321",
"legalHold.name": "Test legal hold matter,Another Matter",
},
]
)
result = _add_legal_hold_membership_to_device_dataframe(cli_state.sdk, testdf)

assert_frame_equal(result, testdf_result)


def test_list_include_legal_hold_membership_merges_in_and_concats_legal_hold_info(
runner,
cli_state,
get_all_devices_success,
get_all_custodian_success,
get_all_matter_success,
):
result = runner.invoke(
cli, ["devices", "list", "--include-legal-hold-membership"], obj=cli_state
)

assert "Test legal hold matter,Another Matter" in result.output
assert "123456789,987654321" in result.output


def test_break_backup_usage_into_total_storage_correctly_calculates_values():
test_backupusage_cell = json.loads(TEST_BACKUPUSAGE_RESPONSE)["data"]["backupUsage"]
result = _break_backup_usage_into_total_storage(test_backupusage_cell)

test_empty_backupusage_cell = json.loads(TEST_EMPTY_BACKUPUSAGE_RESPONSE)["data"][
"backupUsage"
]
empty_result = _break_backup_usage_into_total_storage(test_empty_backupusage_cell)

assert_series_equal(result, Series([2, 56968051]))
assert_series_equal(empty_result, Series([0, 0]))


def test_last_connected_after_filters_appropriate_results(
cli_state, runner, get_all_devices_success
):
Expand Down