Skip to content
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] Syncing Privacy Notices and Privacy Experiences #3292

Merged
merged 22 commits into from
May 17, 2023

Conversation

pattisdr
Copy link
Contributor

@pattisdr pattisdr commented May 11, 2023

Closes #3252, #3226, #3241

❗ Contains data migration
❗ Breaks existing privacy experiences
Expected to break Admin UI around retrieving experiences. Admin UI for experiences should call new experience config endpoints instead.
❗ Components should still call existing experience endpoints although the response body has changed.

Description Of Changes

See backend design here

Medium-sized data model changes to ensure there is always a valid Privacy Experience for each region at all times and keep edits to an experience for one region from conflicting with another. In short, adjusts PrivacyExperiences so instead of representing multiple regions, they represent a single region. Create a new PrivacyExperienceConfig table to share common language like banner text and link labels between experiences. PrivacyExperiences are linked to a PrivacyExperienceConfig via FK. There are also historical tables for both experiences and experienceconfig to track how both of these changed over time for consent reporting.

Further, each time Privacy Notices are created or updated, adjust Privacy Experiences behind the scenes so we have an experience that covers all the notices. Additionally, when PrivacyExperienceConfigs are updated, also adjust the PrivacyExperiences to match, provided none of the rules with notices are violated. In other words, PrivacyExperiences are not edited directly, but indirectly by PrivacyNotice or PrivacyExperienceConfig updates.

Removes the PrivacyExperienceTemplate table (this wasn't yet being used). My goal instead is to have a few "PrivacyExperienceConfig" records that are labeled as "default" that we can link to when we need to create or update a PrivacyExperience on the user's behalf.
Screenshot 2023-05-13 at 4 56 58 PM

Code Changes

Model changes
  • Splits the PrivacyExperience table into two halves: the PrivacyExperience and PrivacyExperienceConfig table. Each PrivacyExperience now supports only a single region. Create historical tables for both.
Experience Config
  • Add new endpoints for the admin UI: POST {{host}}/experience-config and PATCH {{host}}/experience-config/{{experience-config-id}} for creating and editing experience configs. Have these endpoints create or update a single resource instead of multiple to reduce complexity due to the fact that several PrivacyExperience updates happen behind the scenes.
    • When an ExperienceConfig is updated, any regions specified in the request have their PrivacyExperience records upserted to match and linked to the ExperienceConfig via FK provided the rules aren't violated.
  • Adds new endpoints to GET Experience Config List and Detail endpoints for the Admin UI: {{host}}/experience-config?component=overlay&show_disabled=False, and {{host}}/experience-config/{{experience-config-id}}
  • Like notices and experiences, if an ExperienceConfig is updated, the version is bumped and a historical record is created
Privacy Experience
  • Update PrivacyExperienceList response body {{host}}/privacy-experience/?region=eu_de- since things like component title and banner title are now stored on a different record, these details are now nested under the experience_config_id field.
  • Removes the endpoints to create and update privacy experiences as these records are no longer updated directly
  • Add query params has_config to PrivacyExperienceList endpoint to surface which experiences have and don't have the "experience config" linked. If there's no experience config linked, there's no text for the banners, overlays, or links
  • Remove region filter on the PrivacyExperienceDetail endpoint. This was previously used to filter an Experience's associated regions, but now that an Experience is associated with a single region, we just use that one.
  • Note that as before, changes to the PrivacyExperience trigger a version to be bumped and a historical record saved. I am storing both the experience_config_id and the experience_config_history_id so if the experience config is updated, this triggers a version bump on the experience.
Privacy Notice
  • Add logic to upsert Privacy Experiences behind the scenes when Privacy Notices are created or updated. For example, if a California Opt In Privacy Notice is created that has to be displayed in an overlay, we need to create a California Overlay Privacy Experience that is delivered by link

Worth noting

  • Privacy Notices drive Privacy Experiences not the reverse. I always check to make sure I have an experience for the given notices not the reverse. If PrivacyNotice changes are made, they should generally succeed, and you adjust the Experiences to match. However, if ExperienceConfig changes are made that would conflict with the notices, throw an error message, so the user knows to delete the offending regions from their request.
  • If I have to create an overlay experience automatically where I have a choice between being delivered by link or banner I choose banner by default to be consistent.
  • Provided privacy notices are valid, I create or update privacy experiences to match. If one already exists, I leave it. If a link overlay needs to be updated to a banner overlay, I do so, and unlink its experience config. If a region is removed from a notice, I leave the related experience untouched. I never delete an experience. It is okay if an experience doesn't have notices. Just leave the experience there for future use if notices for that region are added again. If a PrivacyExperienceConfig adds a region for which there are no notices, also go ahead and create the Experience behind the scenes. Again, it's fine if there are no notices.
  • If a user updates the PrivacyExperienceConfig to be a delivered via link instead of banner or vice versa, I don't remove the other fields behind the scenes that are traditionally associated with link/banner. I just leave them there and they won't be used.
  • When a user creates or updates ExperienceConfig, the regions included in the request body are not stored on that table, they are each separate records in the PrivacyExperience table. I make the ExperienceConfig updates first, and then upsert the PrivacyExperiences. Therefore, require regions to be present in each request to create or update an ExperienceConfig if you want the ExperienceConfig to have those regions. They need to be supplied every time because absence of regions will result in any existing linked PrivacyExperiences to be unlinked
  • Right now if I have to create or update a PrivacyExperience automatically when notices change, they don't have an ExperienceConfig attached, so they're not yet valid for display. I'd like to add the concept of default ExperienceConfigs in the future, so I can link these instead.
  • Further consider adding a flag in the future to PrivacyExperience for when we make a change on the customer's behalf so we can surface this for approval

Steps to Confirm

See postman collection

  • Create Privacy Notices with POST {{host}}/privacy-notice
  • Verify Privacy Experiences were created in the background with GET {{host}}/privacy-experience/ for each region.
    • Verify privacy center experiences created where notice needs to be displayed in privacy center, same for overlay
    • Note that the relevant notices are still nested under each experience in the response
    • Verify no experience_config exists yet
  • Create the language and configuration options for those an overlay experience with POST {{host}}/experience-config.
  • Do a GET {{host}}/privacy-experience/ for the relevant regions to see that ExperienceConfig is linked to the experience
  • Experiment with updating that ExperienceConfig {{host}}/experience-config/{{experience-config-id}} with various types of edits including component, overlay and regions to see that certain regions are blocked if they'll interfere with notices for that region.

Pre-Merge Checklist

pattisdr added 11 commits May 9, 2023 10:58
…ivacyExperience. One table stores the experiences for a given region and component, the other table stores common language shared between experiences.

- Add new endpoints to update ExperienceLanguage.  These can trigger changes to PrivacyExperiences.
- Remove constraint that you can't have both a link overlay and a banner overlay for the same region.
- Add unique constraint on region/component for PrivacyExperience
- Add ability to upsert experiences when privacy notices change.
- Add 50 states
…argeted way. Change reponse to be "linked_regions" instead of "added_regions" to reflect what is important here.
…iences

# Conflicts:
#	src/fides/api/ops/api/v1/endpoints/privacy_notice_endpoints.py
…ienceConfig if regions are not compatible with a given ExperienceConfig.
… just in case so this is not a field we have to check notices for.
…move the obsolete update/create experience endpoints as these are no longer edited directly.
…points into separate files for easier review.
@cypress
Copy link

cypress bot commented May 13, 2023

Passing run #2041 ↗︎

0 4 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge cafa34d into 5680a23...
Project: fides Commit: 7658f85eca ℹ️
Status: Passed Duration: 01:10 💡
Started: May 17, 2023 11:15 PM Ended: May 17, 2023 11:17 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@codecov
Copy link

codecov bot commented May 13, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.20 🎉

Comparison is base (5680a23) 86.86% compared to head (cafa34d) 87.06%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3292      +/-   ##
==========================================
+ Coverage   86.86%   87.06%   +0.20%     
==========================================
  Files         308      309       +1     
  Lines       18540    18821     +281     
  Branches     2423     2461      +38     
==========================================
+ Hits        16104    16386     +282     
  Misses       1995     1995              
+ Partials      441      440       -1     
Impacted Files Coverage Δ
src/fides/api/ops/db/base.py 100.00% <ø> (ø)
src/fides/api/ctl/sql_models.py 97.66% <100.00%> (ø)
src/fides/api/ops/api/v1/api.py 100.00% <100.00%> (ø)
...1/endpoints/privacy_experience_config_endpoints.py 100.00% <100.00%> (ø)
...s/api/v1/endpoints/privacy_experience_endpoints.py 100.00% <100.00%> (ø)
...i/ops/api/v1/endpoints/privacy_notice_endpoints.py 100.00% <100.00%> (ø)
src/fides/api/ops/api/v1/endpoints/utils.py 100.00% <100.00%> (ø)
src/fides/api/ops/api/v1/urn_registry.py 100.00% <100.00%> (ø)
src/fides/api/ops/models/privacy_experience.py 100.00% <100.00%> (+1.38%) ⬆️
src/fides/api/ops/models/privacy_notice.py 100.00% <100.00%> (ø)
... and 2 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pattisdr pattisdr changed the title WIP [Backend] Syncing Privacy Notices and Privacy Experiences [Backend] Syncing Privacy Notices and Privacy Experiences May 13, 2023
@pattisdr pattisdr marked this pull request as ready for review May 13, 2023 22:09
@pattisdr
Copy link
Contributor Author

@allisonking, @eastandwestwind, @NevilleS could you take a look at the changes made here? Playing with the postman collection here is a good way to get up to speed with what's happening.

Copy link
Contributor

@eastandwestwind eastandwestwind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great overall, just a couple Qs for you @pattisdr. Also I haven't manually tested this PR yet, just looked over the code portion.

Copy link
Contributor

@allisonking allisonking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@pattisdr pattisdr merged commit 1290bf1 into main May 17, 2023
@pattisdr pattisdr deleted the fides_3193_syncing_privacy_notices_and_experiences branch May 17, 2023 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Backend] Split out Experience Config from Privacy Experiences
3 participants