-
Notifications
You must be signed in to change notification settings - Fork 13
Add events command to legal-hold #264
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 25 commits
b285a68
ed72879
ea4381a
a0ebe57
077dea1
03a3814
f174992
205d559
02d6530
e5784cf
9bfcd42
2e303de
94fe3b9
0b79911
1bd1e2f
e4725c7
237ea31
7212fbf
2a917bd
2d1db8c
9a0afcb
a3dd28f
9677f23
c328588
2764bf0
537289f
2ab1ae0
73daebb
d8921c5
285a1ec
5e49055
e7d6a5d
8e55a4a
693126f
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 |
---|---|---|
|
@@ -14,6 +14,8 @@ | |
from code42cli.file_readers import read_csv_arg | ||
from code42cli.options import format_option | ||
from code42cli.options import sdk_options | ||
from code42cli.options import set_begin_default_dict | ||
from code42cli.options import set_end_default_dict | ||
from code42cli.output_formats import OutputFormat | ||
from code42cli.output_formats import OutputFormatter | ||
from code42cli.util import format_string_list_to_columns | ||
|
@@ -25,6 +27,18 @@ | |
_MATTER_KEYS_MAP["description"] = "Description" | ||
_MATTER_KEYS_MAP["creator_username"] = "Creator" | ||
_MATTER_KEYS_MAP["creationDate"] = "Creation Date" | ||
_EVENT_KEYS_MAP = OrderedDict() | ||
maddie-vargo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
_EVENT_KEYS_MAP["eventUid"] = "Event ID" | ||
_EVENT_KEYS_MAP["eventType"] = "Event Type" | ||
_EVENT_KEYS_MAP["eventDate"] = "Event Date" | ||
_EVENT_KEYS_MAP["legalHoldUid"] = "Legal Hold ID" | ||
_EVENT_KEYS_MAP["actorUsername"] = "Actor Username" | ||
_EVENT_KEYS_MAP["custodianUsername"] = "Custodian Username" | ||
|
||
|
||
LEGAL_HOLD_KEYWORD = "legal hold events" | ||
BEGIN_DATE_DICT = set_begin_default_dict(LEGAL_HOLD_KEYWORD) | ||
END_DATE_DICT = set_end_default_dict(LEGAL_HOLD_KEYWORD) | ||
|
||
|
||
@click.group(cls=OrderedGroup) | ||
|
@@ -124,6 +138,37 @@ def show(state, matter_id, include_inactive=False, include_policy=False): | |
echo("") | ||
|
||
|
||
@legal_hold.command() | ||
@click.option( | ||
maddie-vargo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"--matter-id", type=str, help="Filter results by legal hold UID", | ||
maddie-vargo marked this conversation as resolved.
Show resolved
Hide resolved
maddie-vargo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
@click.option( | ||
"--event-type", | ||
type=click.Choice( | ||
[ | ||
"MembershipCreated", | ||
"MembershipReactivated", | ||
"MembershipDeactivated", | ||
"HoldCreated", | ||
"HoldDeactivated", | ||
"HoldReactivated", | ||
"Restore", | ||
] | ||
), | ||
help="Filter results by event types", | ||
maddie-vargo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
@click.option("--begin", **BEGIN_DATE_DICT) | ||
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. This should use 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. Same with 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. Sure, I avoided this option initially because the Also, 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 wonder if we should implement 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. Oh! @maddie-vargo Sorry, I did forget about that when I made my initial comments. I do like the idea from @timabrmsn, supporting checkingpointing would be a nice feature. We can always add it later though 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. @unparalleled-js would it be possible to use the I left checkpoint'ing off for now. |
||
@click.option("--end", **END_DATE_DICT) | ||
@format_option | ||
@sdk_options() | ||
def events(state, matter_id, event_type, begin, end, format): | ||
"""Report on legal hold events""" | ||
maddie-vargo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
formatter = OutputFormatter(format, _EVENT_KEYS_MAP) | ||
events = _get_all_events(state.sdk, matter_id, begin, end, event_type) | ||
if events: | ||
formatter.echo_formatted_list(events) | ||
maddie-vargo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
||
@legal_hold.group(cls=OrderedGroup) | ||
@sdk_options(hidden=True) | ||
def bulk(state): | ||
|
@@ -230,6 +275,24 @@ def _get_all_active_matters(sdk): | |
return matters | ||
|
||
|
||
def _get_all_events(sdk, legal_hold_uid, begin_date, end_date, event_type): | ||
maddie-vargo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
events_generator = sdk.legalhold.get_all_events( | ||
legal_hold_uid, begin_date, end_date | ||
) | ||
if event_type: | ||
events = [ | ||
event | ||
for page in events_generator | ||
for event in page["legalHoldEvents"] | ||
if event["eventType"] == event_type | ||
] | ||
else: | ||
events = [ | ||
event for page in events_generator for event in page["legalHoldEvents"] | ||
] | ||
return events | ||
|
||
|
||
def _print_matter_members(username_list, member_type="active"): | ||
if username_list: | ||
echo("\n{} matter members:\n".format(member_type.capitalize())) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,6 @@ | |
from code42cli.cmds.legal_hold import _check_matter_is_accessible | ||
from code42cli.main import cli | ||
|
||
|
||
_NAMESPACE = "{}.cmds.legal_hold".format(PRODUCT_NAME) | ||
TEST_MATTER_ID = "99999" | ||
TEST_LEGAL_HOLD_MEMBERSHIP_UID = "88888" | ||
|
@@ -169,6 +168,41 @@ | |
] | ||
} | ||
""" | ||
EVENTS_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. Something seems off about this response.... It is not valid JSON. Should these objects be under a list with key: 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. Or else I believe line 290 will create a response where this a 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 don't think the fixture that uses this is ever used, if that is the, case can this be removed? 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. Yes, that is no longer needed, since I moved to a generator format for the mocker. I forgot to take it out on my last push along with it's corresponding fixture. |
||
{ | ||
"eventUid":"564564654566", | ||
"eventType":"HoldCreated", | ||
"eventDate":"2015-05-16T15:07:44.820Z", | ||
"legalHoldUid":"88888", | ||
"actorUserUid":"12345", | ||
"actorUsername":"[email protected]", | ||
"actorFirstName":"john", | ||
"actorLastName":"doe", | ||
"actorUserExtRef":null, | ||
"actorEmail":"[email protected]" | ||
}, | ||
{ | ||
"eventUid":"74533457745", | ||
"eventType":"MembershipCreated", | ||
"eventDate":"2019-05-17T15:07:44.820Z", | ||
"legalHoldUid":"88888", | ||
"legalHoldMembershipUid":"645576514441664433", | ||
"custodianUserUid":"12345", | ||
"custodianUsername":"[email protected]", | ||
"custodianFirstName":"kim", | ||
"custodianLastName":"jones", | ||
"custodianUserExtRef":null, | ||
"custodianEmail":"[email protected]", | ||
"actorUserUid":"1234512345", | ||
"actorUsername":"[email protected]", | ||
"actorFirstName":"john", | ||
"actorLastName":"doe", | ||
"actorUserExtRef":null, | ||
"actorEmail":"[email protected]" | ||
} | ||
""" | ||
EMPTY_EVENTS_RESPONSE = """{"legalHoldEvents": []}""" | ||
ALL_EVENTS_RESPONSE = """{{"legalHoldEvents": [{}]}}""".format(EVENTS_RESPONSE) | ||
EMPTY_MATTERS_RESPONSE = """{"legalHolds": []}""" | ||
ALL_MATTERS_RESPONSE = """{{"legalHolds": [{}]}}""".format(MATTER_RESPONSE) | ||
LEGAL_HOLD_COMMAND = "legal-hold" | ||
|
@@ -212,6 +246,21 @@ def active_and_inactive_legal_hold_memberships_response(mocker): | |
return [_create_py42_response(mocker, ALL_ACTIVE_AND_INACTIVE_CUSTODIANS_RESPONSE)] | ||
|
||
|
||
@pytest.fixture | ||
def events_response(mocker): | ||
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. Is this fixture used anywhere? I am not seeing. 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. Yes, this is no longer needed. See my comment on the item above. |
||
return _create_py42_response(mocker, EVENTS_RESPONSE) | ||
|
||
|
||
@pytest.fixture | ||
def empty_events_response(mocker): | ||
return _create_py42_response(mocker, EMPTY_EVENTS_RESPONSE) | ||
|
||
|
||
@pytest.fixture | ||
def all_events_response(mocker): | ||
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 is better to have mock generator methods actually be generators and not just lists. I know it probably works fine, but I have seen bugs show up because the tests were doing this. There are some subtle differences. 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. Let me know if I'm still missing the mark here. 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. This works! |
||
return [_create_py42_response(mocker, ALL_EVENTS_RESPONSE)] | ||
|
||
|
||
@pytest.fixture | ||
def get_user_id_success(cli_state): | ||
cli_state.sdk.users.get_by_username.return_value = { | ||
|
@@ -246,6 +295,11 @@ def check_matter_accessible_failure(cli_state, custom_error): | |
) | ||
|
||
|
||
@pytest.fixture | ||
def get_all_events_success(cli_state): | ||
cli_state.sdk.legalhold.get_all_events.return_value = events_response | ||
|
||
|
||
@pytest.fixture | ||
def user_already_added_response(mocker): | ||
mock_response = mocker.MagicMock(spec=Response) | ||
|
@@ -575,6 +629,28 @@ def test_list_with_csv_format_returns_no_response_when_response_is_empty( | |
assert "Matter ID,Name,Description,Creator,Creation Date" not in result.output | ||
|
||
|
||
def test_events_shows_events_that_respect_type_filters( | ||
runner, cli_state, get_all_events_success, all_events_response | ||
): | ||
|
||
cli_state.sdk.legalhold.get_all_events.return_value = all_events_response | ||
result = runner.invoke( | ||
cli, ["legal-hold", "events", "--event-type", "HoldCreated"], obj=cli_state | ||
) | ||
|
||
assert "564564654566" 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. From the test alone, it is not immediately clear why these values are expected. 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. Defined variables at the top here. I don't love the names
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 names are fine.. but alternatively they could be called Reasons: 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 took your suggestions. Thanks! |
||
assert "74533457745" not in result.output | ||
|
||
|
||
def test_events_with_csv_returns_no_events_when_response_is_empty( | ||
runner, cli_state, get_all_events_success, empty_events_response | ||
): | ||
cli_state.sdk.legalhold.get_all_events.return_value = empty_events_response | ||
result = runner.invoke(cli, ["legal-hold", "events", "-f", "csv"], obj=cli_state) | ||
|
||
assert "Matter ID,Name,Description,Creator,Creation Date" not 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. Could we get test(s) for 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'll work on this. I had tried this initially, but had trouble getting a test to recognize the date inputs in the runner. 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 a test titled |
||
@pytest.mark.parametrize( | ||
"command, error_msg", | ||
[ | ||
|
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.
Should the command be called
search-events
instead?file-events search
commandThere 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.
Changed to
search-events
> let me know if this is not what you had in mind