-
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
Update how we Save and Propagate Privacy Preferences [#3013] #3016
Conversation
…ence for storing privacy preferences with respect to the new privacy notices. - Every single time a "privacy preference" is saved, create a PrivacyPreferenceHistory record for consent reporting purposes. Then, upsert the CurrentPrivacyPreference record for a given provided identity and privacy notice. - Create a privacy request each time to propagate preferences where applicable. Link the PrivacyPreferenceHistory records to that PrivacyRequest, not the CurrentPrivacyPreference record, as the latter is subject to change. - Add two new endpoints, to verify a user's identity and then get their CurrentPrivacyPreferences as well as to verify a user's identity and then save privacy preferences. These are similar to the existing Consent endpoints, except they interface with these new tables. - Add some logic for saas connectors to be more discerning as to whether we should propagate a specific preference for a given system. - When running consent requests on a saas connector, create a "skipped" log instead of a "completed" log if we don't fire a request to the given service. - Return a privacy notice history id when getting privacy notices. The privacy preferences should be saved with respect to the historical id.
…ent util should_opt_in_to_service.
…w - saving privacy preferences for privacy notices. - Add some saas fixtures for tests we know will fail before we try to make a connection so we don't need secrets saved. - Test old workflow on whether we will opt into a service.
…acy preferences: - Adjust definition for whether a consent email connector needs to send an email to factor in new workflow (privacyrequest.privacy_preferences), but still support the old workflow (privacyrequest.consent_preferences). Share the same code that we do for the consent email connectors in this calculation. - Adjust batch email send to look for both relevant privacy preferences to the given connector (new workflow) and consent preferences (old workflow) - Adjust the consent email template to include consent preferences (old workflow) and privacy preferences (new workflow) - Add skipped execution logs for consent email connectors if we skip sending. - Add skipped execution logs for erasure email connectors if we skip sending for consistency. - Adjust the email template for consent to be able to work for old consent preferences and new-style privacy preferences.
…urn current privacy preferences and another that saves preferences and returns the current privacy preferences that were changed.
…y preferences - and only allow 50 privacy preferences to be saved at once.
…th respect to data uses) regardless of whether there are executable preferences to propagate.
Passing run #1278 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
…versal analytics configs that don't have secrets.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #3016 +/- ##
==========================================
+ Coverage 86.92% 87.32% +0.40%
==========================================
Files 303 307 +4
Lines 17322 17592 +270
Branches 2230 2260 +30
==========================================
+ Hits 15057 15363 +306
+ Misses 1849 1815 -34
+ Partials 416 414 -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. |
…test verifying that if privacy request creation fails, privacy preferences are still saved.
src/fides/api/ops/email_templates/templates/consent_request_email_fulfillment.html
Show resolved
Hide resolved
@adamsachs here is the rewrite of #2991 based on discussions Friday. Would you have time to review? This PR looks bigger than it is, but a lot of the changes are new tests. @seanpreston I'm tagging you here because this new direction we're going with "privacy preference" consent reporting (new tables created/new endpoints) likely won't solve your issue of surfacing the old-style consent history. @NevilleS tagged you if you were interested in seeing where this evolved since Friday - I did end up adding an additional table for current preferences (similar to Consent) that wasn't specified there. |
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.
i'm not through this fully yet, but wanted to post an incremental review before i end my day here, so that - if you want - you can start to address any comments (none of them major so far!) in the meantime. i haven't done any testing yet, or looked over the automated test updates, so that's one big area i need to get to, and some additional code comments may come from that, as i get a bit more hands-on. i've also not dug into the email connector changes - just because that felt like a somewhat independent chunk that i could wall off for the time being - but i will look to review those ASAP tomorrow.
what i have looked over - which i think gets to the core of the changes in the PR - generally looks really good to me! i don't see any major issues so far.
there are two categories of optimizations that i wanted to entertain: one questioning whether we should be indexing some more db columns, and another about whether it'd be possible to DRY up some of the matching we do between privacy notices and systems. the former is obviously something we should look into now (though i very well may be misguided in my points), while the latter doesn't need to be addressed in this PR by any means. there are a few other minor points throughout, but they should be easily resolved or dismissed.
i'll keep on looking at this as fast as i can, but it's looking very promising!
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
- Dry up getting system data uses. - Remove unnecessary nullable=True from privacy preference models. - Add hashed_email and hashed_phone_number which are indexed. - Index CurrentPrivacyPreference.provided_identity_id and have remove the join to ProvidedIdentity in consent_request_verify_for_privacy_preferences - Overhaul the verify_privacy_notice_and_historical_records docstring - Dry up some of the code checking if a privacy notice history record applies to a system - Add method PrivacyNoticeBase.applies_to_system that can be used for privacy notices or history records - Remove unused schema intended for consent reporting - am detailing a different one in another PR.
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.
ok, went through the latest changes (look great generally!) and wanted to publish another round of comments just so you can get that feedback ASAP.
getting to testing now...
src/fides/api/ops/api/v1/endpoints/privacy_preference_endpoints.py
Outdated
Show resolved
Hide resolved
docs/fides/docs/development/postman/Fides.postman_collection.json
Outdated
Show resolved
Hide resolved
…quirements. - Add missing index to CurrentPrivacyPreference.provided_identity_id - Add new privacy_notice/privacy_notice_history internal description field. Remove obsolete displayed_in_banner and displayed_in_privacy_modal in favor of displayed_in_overlay and displayed_in_api. - Remove bad trailing comma from postman collection - Add new "not_applicable" to enforcement enum - Adjust Request Origin enum to be privacy_center, overlay, and API to match corresponding changes to Privacy Notice - Fix copy/paste error on test name. - Remove docstring with future fields we need to add
OK I think I've addressed your comments @adamsachs and have completed another round of manual testing. A few new fields were added or adjusted based on changing requirements. Test failures seem to match what's on main. |
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.
@pattisdr i think this is ready to go from my POV!
thanks for the diligence in addressing all the comments. and 👏 for turning around a huge enhancement in such quick time, in a clean and easy-to-follow manner!
Likewise @adamsachs, appreciate the detailed, prompt review! ⭐ |
❗ Contains migration; check downrev before merge.
Closes #3013
Code Changes
CurrentPrivacyPreference
(which is similar to theConsent
table to which we will soon stop writing) andPrivacyPreferenceHistory
.PrivacyPreferenceHistory
record for each preference which has a FK to aPrivacyNoticeHistory
record. We link to the historical notice as notices are subject to change over time.CurrentPrivacyPreferences
for the given user in a separate table for the givenPrivacyNotice
PrivacyRequest
is then created, and eachPrivacyPreferenceHistory
record that was just created is given a FK to thatPrivacyRequest
. Request propagation will retrieve these PrivacyPreferenceHistory records, again linking to the historical record here, as theCurrentPrivacyPreference
record is subject to change.PrivacyPreferenceHistory
is largely useful for propagation as well as for consent reporting.CurrentPrivacyPreference
contains the user's latest saved preference for a given notice. `CurrentPrivacyPreference is separated for ease of querying and display of current preferences.consent_request_verify_for_privacy_preferences
which allows you to verify your identity to retrieve your current current preferences (returns records from theCurrentPrivacyPreference
table). This is similar to the existing endpoint that instead returns records from theConsent
table.POST
{{host}}/consent-request/{{consent_request_id}}/verify-for-privacy-preferences
save_privacy_preferences
which allows you to save privacy preferences with respect to privacy notice history records. This createsPrivacyPreferenceHistory
records, upsertsCurrentPrivacyPreferences
records, creates a PrivacyRequest to propagate, then returns theCurrentPrivacyPreferences
that were in the request body.PATCH
{{host}}/consent-request/{{consent_request_id}}/privacy-preferences
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.md
Description Of Changes
Primary changes here: