Return persisted identities in get_request_status view
#860
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -640,7 +640,7 @@ def test_get_privacy_requests_with_identity( | |
| assert resp["items"][0]["id"] == succeeded_privacy_request.id | ||
| assert ( | ||
| resp["items"][0]["identity"] | ||
| == succeeded_privacy_request.get_persisted_identity() | ||
| ) | ||
|
|
||
| assert resp["items"][0]["policy"]["key"] == privacy_request.policy.key | ||
|
|
@@ -710,22 +710,25 @@ def test_filter_privacy_requests_by_internal_id( | |
| api_client, | ||
| url, | ||
| generate_auth_header, | ||
| policy, | ||
| ): | ||
| data = [ | ||
| { | ||
| "requested_at": "2021-08-30T16:09:37.359Z", | ||
| "policy_key": policy.key, | ||
| "identity": {"email": "[email protected]"}, | ||
| "id": "test_internal_id_1", | ||
| } | ||
| ] | ||
| resp = api_client.post(url, json=data) | ||
| assert resp.status_code == 200 | ||
| response_data = resp.json()["succeeded"] | ||
| assert len(response_data) == 1 | ||
| privacy_request = PrivacyRequest.get(db=db, object_id=response_data[0]["id"]) | ||
| auth_header = generate_auth_header(scopes=[PRIVACY_REQUEST_READ]) | ||
| response = api_client.get( | ||
| url + f"?request_id={privacy_request.id}", | ||
| headers=auth_header, | ||
| ) | ||
| assert response.status_code == status.HTTP_200_OK | ||
| resp = response.json() | ||
|
|
@@ -1080,8 +1083,13 @@ def test_get_privacy_requests_csv_format( | |
| privacy_request.status = PrivacyRequestStatus.approved | ||
| privacy_request.reviewed_by = user.id | ||
| privacy_request.reviewed_at = reviewed_at | ||
| TEST_EMAIL = "[email protected]" | ||
| TEST_PHONE = "+1 234 567 8910" | ||
| privacy_request.cache_identity( | ||
| { | ||
| "email": TEST_EMAIL, | ||
| "phone_number": TEST_PHONE, | ||
| } | ||
| ) | ||
| privacy_request.save(db) | ||
|
|
||
|
|
@@ -1102,8 +1110,8 @@ def test_get_privacy_requests_csv_format( | |
| first_row = next(csv_file) | ||
| assert parse(first_row["Time received"], ignoretz=True) == created_at | ||
| assert ast.literal_eval(first_row["Subject identity"]) == { | ||
| "email": TEST_EMAIL, | ||
| "phone_number": TEST_PHONE, | ||
| } | ||
| assert first_row["Policy key"] == "example_access_request_policy" | ||
| assert first_row["Request status"] == "approved" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,7 @@ | |
| ) | ||
| from fidesops.models.privacy_request import PrivacyRequest, PrivacyRequestStatus | ||
| from fidesops.models.storage import ResponseFormat, StorageConfig | ||
| from fidesops.schemas.redis_cache import PrivacyRequestIdentity | ||
| from fidesops.schemas.storage.storage import ( | ||
| FileNaming, | ||
| StorageDetails, | ||
|
|
@@ -772,10 +773,18 @@ def _create_privacy_request_for_policy( | |
| microsecond=393185, | ||
| tzinfo=timezone.utc, | ||
| ) | ||
| pr = PrivacyRequest.create( | ||
| db=db, | ||
| data=data, | ||
| ) | ||
| pr.persist_identity( | ||
| db=db, | ||
| identity=PrivacyRequestIdentity( | ||
| email="[email protected]", | ||
| phone_number="+1 234 567 8910", | ||
| ), | ||
| ) | ||
| return pr | ||
|
|
||
|
|
||
| @pytest.fixture(scope="function") | ||
|
|
@@ -839,7 +848,12 @@ def succeeded_privacy_request(cache, db: Session, policy: Policy) -> PrivacyRequ | |
| "client_id": policy.client_id, | ||
| }, | ||
| ) | ||
| identity_kwargs = {"email": "[email protected]"} | ||
| pr.cache_identity(identity_kwargs) | ||
| pr.persist_identity( | ||
|
Comment on lines
+852
to
+853
There was a problem hiding this comment. Bug: Policy webhooks can have derived_identities returned. Neither There was a problem hiding this comment. Would it be useful to have a method that both persists the identity in the cache and in the database at the same time? I'd like to avoid these mismatches we have now where they're both being updated in some places and not others. There was a problem hiding this comment.
I'm torn here. On the one hand it's nice to have consistency, on the other, as you rightly suggest above, it means we'll need to be plumbing the DB connection in more places. I'm not sure if it's better to have the execution update the cache with identity data at the very start before the traversal, such that we can guarantee the traversal will always use what was provided by the user on privacy request creation. That way the internals can still use the cache and benefit from the speed, and less refactoring is required. What do you think? There was a problem hiding this comment.
We should separate these concerns for now. The There was a problem hiding this comment.
Thinking about this more, I agree, it's in line with our original design, in that we query everything up front, build the graph, and execute it. We're not regularly querying the database as we execute the traversal which I think is good for performance. There was a problem hiding this comment.
OK, that makes sense |
||
| db=db, | ||
| identity=PrivacyRequestIdentity(**identity_kwargs), | ||
| ) | ||
| yield pr | ||
| pr.delete(db) | ||
|
|
||
|
|
||
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.
Also downloading privacy requests as a CSV above is still using cached identity there, these should both pull from the same source, since they are supposed to be the same data in different formats. Otherwise, I can see the UI showing the identities, and then they go to download a CSV and the identity rows are blank.
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.
Creating the request body for a webhook, creating the requests for saas configs retrieve/update statements, and feeding the initial seed data into the traversal all still use the cache, not the database.
Do we do this because it's easier to access the cache sometimes, we don't always have a readily available session? especially in the traversal? I'm a little worried about having different locations storing what the identity is, some pull from one, others pull from another.
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.
For now, yes. Ideally I'd like everything to use the same source of truth for identity data, but that's a larger refactor for exactly this reason, the DB connection isn't piped into everywhere that would need it yet. I've made this ticket to be actioned as a follow-up.