-
Notifications
You must be signed in to change notification settings - Fork 72
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
Backend Consent Reporting #3095
Conversation
…erences across all users, and another to surface the most current privacy preferences (with a subset of fields for now) - Stop setting secondary_user_ids on privacy request create: instead, dictate to consent connectors to add only relevant identities that we intend to persist to the connector - On both saas and email consent connectors, update Privacy Preference History Records with "affected_system_status" and "secondary_user_ids" where applicable. If the connector isn't relevant to the privacy preference or we don't have the information to send the request, add a "skipped" status, otherwise, update with pending/completed/errored. Only add secondary user ids where our goal is to propagate those ids.
Passing run #1445 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
src/fides/api/ops/api/v1/endpoints/consent_request_endpoints.py
Outdated
Show resolved
Hide resolved
src/fides/api/ctl/migrations/versions/3ac5aef6fe45_index_privacy_preference_created.py
Outdated
Show resolved
Hide resolved
src/fides/api/ops/service/connectors/consent_email_connector.py
Outdated
Show resolved
Hide resolved
src/fides/api/ops/service/connectors/consent_email_connector.py
Outdated
Show resolved
Hide resolved
|
src/fides/api/ops/api/v1/endpoints/privacy_preference_endpoints.py
Outdated
Show resolved
Hide resolved
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.
generally this looks good to me @pattisdr so i'm going to give a tentative approval! i've got some relatively minor nits and questions, but i'm not certain any of them need to be addressed.
maintaining the PrivacyPreferenceHistory.affected_system_status
state definitely feels like the most complex piece here. i think you've done a good job here of keeping things relatively organized and traceable given how complex that requirement is, especially with the different possibilities between consent propagating through saas and email connectors.
on a broader, separate note - not anything to address here - but the fact that you've needed to take so much care in covering those edge cases across different code paths does make me feel like we're getting to the point where we may need a broader refactor to streamline some of the consent propagation processing. i think this will only be more necessary as we work to support consent propagation in the DB connectors codepaths - and maybe that will be a natural point for a bit broader of a refactor.
i'd like to get your POV on this. to me it's starting to feel like with how things are structured now, we need to account for many different edge cases across many different code paths when considering updates/improvements to the consent workflow. i think what you've done on this PR makes sense, but as the use cases and requirements evolve, i'm getting the sense that it may not be sustainable. the consent pieces almost feel sprinkled throughout the DSR execution workflow, in a certain sense.
(i hope this goes without saying, but that's not in any way meant to be a knock on how you've built things here or in previous increments - we (mainly you :)) have rapidly built an impressive amount of support for complex requirements in basically no time. i just want to voice a forward-looking impression i have to see if/how it aligns with your perspective)
src/fides/api/ops/api/v1/endpoints/privacy_preference_endpoints.py
Outdated
Show resolved
Hide resolved
src/fides/api/ops/api/v1/endpoints/privacy_preference_endpoints.py
Outdated
Show resolved
Hide resolved
- Share validate_start_and_end_filters between privacy request, consent request, and privacy preference endpoints. - Index updated_at additionally on CurrentPrivacyPreference and update this report to filter on updated at. - Pass in the session off of task resources through to the PrivacyPreference methods that save affected systems and secondary user ids to follow typical patterns. - Remove comma after catching SkippingConsentPropagation exception
# Conflicts: # CHANGELOG.md
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #3095 +/- ##
==========================================
+ Coverage 87.39% 87.54% +0.15%
==========================================
Files 306 307 +1
Lines 17600 17734 +134
Branches 2271 2288 +17
==========================================
+ Hits 15381 15525 +144
+ Misses 1801 1793 -8
+ Partials 418 416 -2
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
@adamsachs I'm with you, the current path is concerning especially given that we have a consent database connector coming in, where this will surely need additional special handling. Big picture for each consent connector we need to:
The main trickiness here was that the consent email send happens in a completely separate process where we batch all preferences collected throughout the week and send a single email. That process required hooking in to a lot of different locations. For now, I've tried to share methods between the saas and consent email connectors and added multiple levels of tests. |
…mes in saas connector tests as well).
❗ Contains migration; check downrev before merge
Closes #2836
Closes #2837
Code Changes
PrivacyPreferenceHistory.affected_system_status
at the connector level as we execute privacy requests for consent reporting as well assecondary user ids
- see updates in saas connector and consent email connectorSteps to Confirm
privacy_notice_history_id
Privacy Notices > Create Privacy Notice endpointconsent_request_id
PrivacyPreferenceHistory
and `CurrentPrivacyPreference recordGET {{host}}/historical-privacy-preferences
andGET {{host}}/current-privacy-preferences
to see the results of the two endpointsPre-Merge Checklist
CHANGELOG.md
Description Of Changes
Start tracking which systems were affected on each Privacy Preference History record for consent reporting. Start tracking a subset of the relevant secondary user ids (like browser identifiers) on the Privacy Preference History record instead of all identifiers. Only relevant user ides should be in this report. Surface endpoints for consent reporting.
Historical Preferences
GET {{host}}/historical-privacy-preferences
Current Preferences
GET {{{host}}/current-privacy-preferences