-
Notifications
You must be signed in to change notification settings - Fork 74
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
HJ-97 - Update systems endpoint to filter vendor deleted systems #5553
Changes from all commits
12a9b13
fdf39e3
208b243
be24649
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 | ||||
---|---|---|---|---|---|---|
|
@@ -2,7 +2,7 @@ | |||||
"""Integration tests for the API module.""" | ||||||
import json | ||||||
import typing | ||||||
from datetime import datetime, timezone | ||||||
from datetime import datetime, timedelta, timezone | ||||||
from json import loads | ||||||
from typing import Dict, List, Tuple | ||||||
from uuid import uuid4 | ||||||
|
@@ -1665,6 +1665,40 @@ def test_list_with_pagination_and_multiple_filters_2( | |||||
|
||||||
assert result_json["items"][0]["fides_key"] == tcf_system.fides_key | ||||||
|
||||||
@pytest.mark.parametrize( | ||||||
"vendor_deleted_date, expected_systems_count, show_deleted", | ||||||
[ | ||||||
(datetime.now() - timedelta(days=1), 1, True), | ||||||
(datetime.now() - timedelta(days=1), 0, None), | ||||||
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. nit - reads a bit weird to me that the value here is
Suggested change
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. More than falsey, I was trying to test the parameter being not present. |
||||||
(datetime.now() + timedelta(days=1), 1, None), | ||||||
(None, 1, None), | ||||||
], | ||||||
) | ||||||
andres-torres-marroquin marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
def test_vendor_deleted_systems( | ||||||
self, | ||||||
db, | ||||||
test_config, | ||||||
system_with_cleanup, | ||||||
vendor_deleted_date, | ||||||
expected_systems_count, | ||||||
show_deleted, | ||||||
): | ||||||
|
||||||
system_with_cleanup.vendor_deleted_date = vendor_deleted_date | ||||||
db.commit() | ||||||
|
||||||
result = _api.ls( | ||||||
url=test_config.cli.server_url, | ||||||
headers=test_config.user.auth_header, | ||||||
resource_type="system", | ||||||
query_params={"show_deleted": True} if show_deleted else {}, | ||||||
) | ||||||
|
||||||
assert result.status_code == 200 | ||||||
result_json = result.json() | ||||||
|
||||||
assert len(result_json) == expected_systems_count | ||||||
|
||||||
|
||||||
@pytest.mark.unit | ||||||
class TestSystemUpdate: | ||||||
|
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.
any particular reason that we're changing the functionality here? if it's just a general optimization, i understand that motivation - but i think i'd lean toward keeping this using the simple
return await list_resource(System, db)
, because i believe this was in place for backward compatibility of this endpoint with uses in the fides CLI, etc - when it specifically did not paginate, filter, or deduplicate (or do anything besides list all the systems, raw).granted, that backward compatibility is not well commented in the code, so if we do keep this as it was, then a nice addition would be a code comment to clarify why it's kept as
list_resource
, i.e. for backward compatibility reasons.i see/know that @erosselli and @galvana worked on refactoring this endpoint a little while back, may be worth getting them to chime in quickly.
also, as more of a stylistic comment - i preferred this conditional coming at the beginning of the endpoint function, as it had been previously. if there's a branch of the function that's a "special case" and returns immediately, i'd prefer to see that at the beginning - i just find that easier to make sense of when reading the function...
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.
Hey guys, yes, that's the reason we kept the non-paginated version of the system retrieval, for backwards compatibility. I also agree with @adamsachs's comment about having this higher up so it's a little more obvious that there's a possible early return.